Added ability to connect to cassandra over ssl using configurable key… #7326
Conversation
… and trust store locations.
|
@meungblut thank you for your contribution! It seems like your first commit's message is too long. Please follow these guidelines when writing a commit message: http://chris.beams.io/posts/git-commit/. You should also squash the two commits titled "Document SSL config". |
There was a problem hiding this comment.
Yes, thanks for your contribution! We should have a test for this, though security stuff is always a bit tricky to test. Best case scenario would be if the in-memory Cassandra supports SSL, in which case you could do this in TestCassandraIntegrationSmokeTest (or maybe a new test file in the same directory) and run your tests on a new Cassandra query runner that has SSL enabled. Otherwise, we have a product test framework that spins up docker containers, and we could add the test there, but that would be a bit more involved.
| @@ -111,6 +114,13 @@ public static CassandraSession createCassandraSession( | |||
|
|
|||
| LoadBalancingPolicy loadPolicy = new RoundRobinPolicy(); | |||
|
|
|||
| if (config.getUseTls()) { | |||
There was a problem hiding this comment.
This should work with the default truststore and keystore, to include the functionality from this PR: #6745. Would that be achieved by not specifying the truststore/keystore in the config (which would pass null values into getSSLContext)?
There was a problem hiding this comment.
new property? cassandra.ssl.use-default-stores ?
There was a problem hiding this comment.
Hmm, I think it's reasonable to use the default stores if you don't specify a custom keystore/truststore, we don't need a new property
| @@ -73,6 +73,71 @@ | |||
| private int noHostAvailableRetryCount = 1; | |||
| private int speculativeExecutionLimit = 1; | |||
| private Duration speculativeExecutionDelay = new Duration(500, MILLISECONDS); | |||
| private boolean isUseTls; | |||
| private String sslKeyStore; | ||
| private String sslKeyStorePassword; | ||
|
|
||
| public boolean getUseTls() |
| } | ||
|
|
||
| @Config("cassandra.ssl.enabled") | ||
| public CassandraClientConfig setUseTls(boolean useTls) |
| return sslTrustStore; | ||
| } | ||
|
|
||
| @Config("cassandra.ssl.trust-store") |
There was a problem hiding this comment.
For the CLI, Presto treats truststore as one word:
. It'd be good to have consistency.| SSLContext ctx = SSLContext.getInstance("SSL"); | ||
|
|
||
| KeyStore ts = KeyStore.getInstance("JKS"); | ||
| ts.load(tsf, truststorePassword.toCharArray()); |
There was a problem hiding this comment.
This will cause a NPE if truststorePassword is null. And we want to allow that case, so that you can use the default keystore/truststore -- as in this PR #6745.
| KeyManagerFactory kmf = | ||
| KeyManagerFactory.getInstance( | ||
| KeyManagerFactory.getDefaultAlgorithm()); | ||
| kmf.init(ks, keystorePassword.toCharArray()); |
| tmf.init(ts); | ||
|
|
||
| KeyStore ks = KeyStore.getInstance("JKS"); | ||
| ks.load(ksf, keystorePassword.toCharArray()); |
| { | ||
| FileInputStream tsf = new FileInputStream(truststorePath); | ||
| FileInputStream ksf = new FileInputStream(keystorePath); | ||
| SSLContext ctx = SSLContext.getInstance("SSL"); |
| String keystorePassword) | ||
| throws Exception | ||
| { | ||
| FileInputStream tsf = new FileInputStream(truststorePath); |
There was a problem hiding this comment.
in the Presto codebase, we don't use abbreviations -- please use full words (e.g. trustStoreInputStream)
| @@ -196,3 +207,8 @@ This table can be described in Presto:: | |||
| This table can then be queried in Presto:: | |||
|
|
|||
| SELECT * FROM cassandra.mykeyspace.users; | |||
|
|
|||
| When using SSL, use both the cassandra.ssl properties outlined above, as well as the followinf thrift port config: | |||
There was a problem hiding this comment.
Oh, also, we just removed Thrift from the Cassandra connector, since they were deprecated and don't work by default with the newest version of Cassandra: #7013
So you can remove this section in the docs.
|
will work out how to write test - thanks for feedback! |
|
any progress on this pr? |
|
Hi @cawallin do you need any help on this? I am happy to address any comments. Please let me know! Thanks! |
|
This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the task, make sure you've addressed reviewer comments, and rebase on the latest master. Thank you for your contributions! |
… and trust store locations.
Sample config:
cassandra.ssl.enabled=true
cassandra.ssl.trust-store=/etc/cassandra/conf/truststore
cassandra.ssl.trust-store-password=somepassword
cassandra.ssl.key-store=/etc/cassandra/conf/keystore
cassandra.ssl.key-store-password=somepassword
cassandra.thrift-connection-factory-class=org.apache.cassandra.thrift.SSLTransportFactory
cassandra.transport-factory-options=enc.keystore=/etc/cassandra/conf/keystore,enc.truststore=/etc/cassandra/conf/truststore,enc.truststore.password=somepassword,enc.keystore.password=somepassword