CASSANDRA-17559: FQLTool Replay should support full set of login options - #5064
CASSANDRA-17559: FQLTool Replay should support full set of login options#5064arvindKandpal-ksolves wants to merge 2 commits into
Conversation
* Added support for SSL encryption (--ssl, --ssl-truststore, --ssl-keystore, passwords) * Added support for custom Auth Providers (--auth-provider) * Implemented proper password masking for target hosts in logs and result paths * Added fail-fast validation for SSL and AuthProvider configurations on startup * Added unit tests for invalid configurations and edge cases in password masking
| { | ||
| try | ||
| { | ||
| return (AuthProvider) Class.forName(authProviderClass).getDeclaredConstructor().newInstance(); |
There was a problem hiding this comment.
We can't always return the no-arg constructor, we need to do something like https://github.com/apache/cassandra/blob/trunk/tools/sstableloader/src/org/apache/cassandra/tools/LoaderOptions.java#L703
There was a problem hiding this comment.
Fixed, now follows the same pattern as LoaderOptions#constructAuthProvider — tries a (String, String) constructor when credentials are present, errors out clearly if not supported.
| throw new IllegalArgumentException("The results path (" + basePath + ") should be an existing directory"); | ||
| } | ||
| resultPaths = targetHosts.stream().map(target -> new File(basePath, target)).collect(Collectors.toList()); | ||
| resultPaths = targetHosts.stream().map(target -> new File(basePath, ResultHandler.maskPassword(target))).collect(Collectors.toList()); |
There was a problem hiding this comment.
I think it would be better to do the password redaction in ParsedTargetHost where we've already done the parsing.
There was a problem hiding this comment.
Moved maskPassword into ParsedTargetHost as suggested.
…d fix AuthProvider initialization
9064070 to
627087c
Compare
|
|
||
| static ParsedTargetHost fromString(String s) | ||
| { | ||
| String [] userInfoHostPort = s.split("@"); |
There was a problem hiding this comment.
A password like 'user:p@ssword@host:9042' is going to break this, we need to split on lastIndexOf and adjust.
There was a problem hiding this comment.
we would not be solving this if it was in a file
|
|
||
| // Fail fast on bad SSL config or an unloadable auth provider class. The AuthProvider itself | ||
| // is built per-connection in connect(), using credentials parsed from the target host. | ||
| this.sslOptions = ssl ? buildSSLOptions() : null; |
There was a problem hiding this comment.
I think we need to improve the validation, truststore/keystore options do nothing unless --ssl is also supplied and passwords without their corresponding store paths are also accepted and ignored. We can again look at the loader for inspiration https://github.com/apache/cassandra/blob/trunk/tools/sstableloader/src/org/apache/cassandra/tools/LoaderOptions.java#L630
I think here we should do:
- Any SSL-related option implicitly enables SSL.
- Reject a truststore password without a truststore path.
- Reject a keystore password without a keystore path.
- Build through EncryptionOptions.ClientEncryptionOptions.Builder.
There was a problem hiding this comment.
+1 for using something like LoaderOptions which would encapsulate validation / parsing and similar.
| @Option(paramLabel = "replay_ddl_statements", names = { "--replay-ddl-statements" }, description = "If specified, replays DDL statements as well, they are excluded from replaying by default.") | ||
| private boolean replayDDLStatements; | ||
|
|
||
| @Option(paramLabel = "ssl", names = { "--ssl" }, description = "Use SSL for connecting to the target hosts.") |
There was a problem hiding this comment.
all of this stuff will be visible in the command line history, same with target, what if we specified a new parameter which would read all of that from a file? nothing can leak anymore ...
There was a problem hiding this comment.
I don't think the loader handles this either and it used often in comparison, so I don't think we should block this on it.
| } | ||
|
|
||
| public static void replay(String keyspace, List<String> arguments, List<String> targetHosts, List<File> resultPaths, String queryStorePath, boolean replayDDLStatements) | ||
| public static void replay(String keyspace, List<String> arguments, List<String> targetHosts, List<File> resultPaths, String queryStorePath, boolean replayDDLStatements, |
There was a problem hiding this comment.
this is becoming too complex, maybe having one helper class holding it all would be better (security related options). We would populate it from a file, for example. We can have individual options on the command line too.
3 participants
What this PR does
This PR implements the missing connection options for
fqltool replay. Previously, the tool lacked support for SSL and custom authentication providers, which prevented it from connecting to secured Cassandra clusters. It also fixes a security issue where passwords in the connection string were exposed in plain text.Changes Included
--ssl, keystore, and truststore options).--auth-providerflag to allow custom authentication via the DataStax Driver.user:password@host) are safely masked and never written to logs or result directories.patch by Arvind Kandpal; reviewed by TBD for CASSANDRA-17559