[GitHub] [cassandra] maulin-vasavada commented on a diff in pull request #1535: CASSANDRA-17513 Adding new property to server encryption options for creating outbound keystore for internode mTLS

[GitBox <gi...@apache.org>]

maulin-vasavada commented on code in PR #1535:
URL: https://github.com/apache/cassandra/pull/1535#discussion_r877602277


##########
src/java/org/apache/cassandra/security/PEMBasedSslContextFactory.java:
##########
@@ -188,7 +205,11 @@ public synchronized void initHotReloading()
         List<HotReloadableFile> fileList = new ArrayList<>();
         if (maybeFileBasedPrivateKey && hasKeystore())
         {
-            fileList.add(new HotReloadableFile(keystore));
+            fileList.add(new HotReloadableFile(keystoreContext.keystore));
+        }
+        if (maybeFileBasedOutboundPrivateKey && hasKeystore())

Review Comment:
   I have to recollect my memory about this but there was some challenge in having a Test work fine. May be it was not this particular code and something else but if @jyothsnakonisa modifies this as per this suggestion, we will assume all tests are run to ensure it is fine.



##########
test/unit/org/apache/cassandra/security/DummySslContextFactoryImpl.java:
##########
@@ -61,6 +61,12 @@ public boolean shouldReload()
         return false;
     }
 
+    @Override
+    public boolean hasOutboundKeystore()

Review Comment:
   Won't need this if we follow Jon's suggestion of having interface level default.



##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -174,6 +170,30 @@ protected TrustManagerFactory buildTrustManagerFactory() throws SSLException
         }
     }
 
+    private KeyManagerFactory getKeyManagerFactory(final boolean isOutboundKeystore) throws SSLException
+    {
+        final KeyStoreContext context = isOutboundKeystore ? outboundKeystoreContext : keystoreContext;
+        try (InputStream ksf = Files.newInputStream(Paths.get(context.keystore)))
+        {
+            final String algorithm = this.algorithm == null ? KeyManagerFactory.getDefaultAlgorithm() : this.algorithm;
+            KeyManagerFactory kmf = KeyManagerFactory.getInstance(algorithm);
+            KeyStore ks = KeyStore.getInstance(store_type);
+            ks.load(ksf, context.keystorePassword.toCharArray());
+
+            if (!context.checkedExpiry)
+            {
+                checkExpiredCerts(ks);

Review Comment:
   Does it make sense to change this signature to accept KeyStoreContext and then move flipping of the cert expiry check there? 



##########
src/java/org/apache/cassandra/security/ISslContextFactory.java:
##########
@@ -99,6 +99,13 @@ default boolean hasKeystore()
         return true;
     }
 
+    /**
+     * Returns if this factory uses outbound keystore.
+     *
+     * @return {@code true} by default unless the implementation overrides this
+     */
+    boolean hasOutboundKeystore();

Review Comment:
   @jyothsnakonisa I think Jon's comment is in the same line of thought I had when I was asking about backwards compatibility since the older implementation doesn't have a need for outbound key configurations.



##########
src/java/org/apache/cassandra/security/FileBasedSslContextFactory.java:
##########
@@ -174,6 +170,30 @@ protected TrustManagerFactory buildTrustManagerFactory() throws SSLException
         }
     }
 
+    private KeyManagerFactory getKeyManagerFactory(final boolean isOutboundKeystore) throws SSLException
+    {
+        final KeyStoreContext context = isOutboundKeystore ? outboundKeystoreContext : keystoreContext;
+        try (InputStream ksf = Files.newInputStream(Paths.get(context.keystore)))
+        {
+            final String algorithm = this.algorithm == null ? KeyManagerFactory.getDefaultAlgorithm() : this.algorithm;
+            KeyManagerFactory kmf = KeyManagerFactory.getInstance(algorithm);
+            KeyStore ks = KeyStore.getInstance(store_type);
+            ks.load(ksf, context.keystorePassword.toCharArray());
+
+            if (!context.checkedExpiry)
+            {
+                checkExpiredCerts(ks);
+                context.checkedExpiry = true;

Review Comment:
   Why wouldn't you have a setter on the KeyStoreContext for this instead of a direct member manipulation? Anyway the context is mutable due to that expiry check flag. However, I feel this could be subjective (given this object is protected and shared only in the subclasses) and I am used to doing it in a specific way :)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org