[GitHub] [ratis] szetszwo commented on a diff in pull request #785: RATIS-1747. Support keyManager and trustManager in tlsConfig.

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

szetszwo commented on code in PR #785:
URL: https://github.com/apache/ratis/pull/785#discussion_r1024349594


##########
ratis-common/pom.xml:
##########
@@ -49,6 +49,17 @@
       <scope>test</scope>
     </dependency>
 
+    <dependency>
+      <groupId>org.bouncycastle</groupId>
+      <artifactId>bcprov-jdk15on</artifactId>
+      <version>${bouncycastle.version}</version>
+    </dependency>
+    <dependency>
+      <groupId>org.bouncycastle</groupId>
+      <artifactId>bcpkix-jdk15on</artifactId>
+      <version>${bouncycastle.version}</version>
+    </dependency>

Review Comment:
   Move org.bouncycastle dependencies to `ratis-test` and add 
   ```xml
         <scope>test</scope>
   ```
   



##########
ratis-common/src/main/java/org/apache/ratis/security/TlsConf.java:
##########
@@ -96,24 +98,34 @@ public PrivateKeyConf(File privateKeyFile) {
   /** Configurations for a trust manager. */
   public static final class TrustManagerConf {
     /** Trust certificates. */
-    private final CertificatesConf trustCertificates;
+    private CertificatesConf trustCertificates;
+    private TrustManager trustManager;

Review Comment:
   Keep both fields `final`.



##########
ratis-common/src/test/java/org/apache/ratis/security/SecurityTestUtils.java:
##########
@@ -20,12 +20,33 @@
 import org.apache.ratis.security.TlsConf.Builder;
 import org.apache.ratis.security.TlsConf.CertificatesConf;
 import org.apache.ratis.security.TlsConf.PrivateKeyConf;
+import org.bouncycastle.util.io.pem.PemObject;
+import org.bouncycastle.util.io.pem.PemReader;

Review Comment:
   Let's move `SecurityTestUtils` to `ratis-test`.  Then, we don't have to add org.bouncycastle dependencies to `ratis-common`.



##########
pom.xml:
##########
@@ -218,6 +218,8 @@
     <test.exclude.pattern>_</test.exclude.pattern>
     <!-- number of threads/forks to use when running tests in parallel, see parallel-tests profile -->
     <testsThreadCount>4</testsThreadCount>
+
+    <bouncycastle.version>1.67</bouncycastle.version>

Review Comment:
   Use 1.70.  IntelliJ reported some security vulnerabilities on 1.67.



##########
ratis-common/src/main/java/org/apache/ratis/security/TlsConf.java:
##########
@@ -96,24 +98,34 @@ public PrivateKeyConf(File privateKeyFile) {
   /** Configurations for a trust manager. */
   public static final class TrustManagerConf {
     /** Trust certificates. */
-    private final CertificatesConf trustCertificates;
+    private CertificatesConf trustCertificates;
+    private TrustManager trustManager;
 
     private TrustManagerConf(CertificatesConf trustCertificates) {
       this.trustCertificates = trustCertificates;
     }
 
+    private TrustManagerConf(TrustManager trustManager) {
+      this.trustManager = trustManager;
+    }
+
     /** @return the trust certificates. */
     public CertificatesConf getTrustCertificates() {
       return trustCertificates;
     }
+
+    public TrustManager getTrustManager() {
+      return trustManager;
+    }
   }
 
   /** Configurations for a key manager. */
   public static final class KeyManagerConf {
     /** A {@link PrivateKey}. */
-    private final PrivateKeyConf privateKey;
+    private PrivateKeyConf privateKey;
     /** Certificates for the private key. */
-    private final CertificatesConf keyCertificates;
+    private CertificatesConf keyCertificates;
+    private KeyManager keyManager;

Review Comment:
   Keep all the fields `final`.



##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/GrpcTlsConfig.java:
##########
@@ -81,6 +88,18 @@ public File getTrustStoreFile() {
         .orElse(null);
   }
 
+  public TrustManager getSslTrustManager() {
+    return Optional.ofNullable(getTrustManager())
+        .map(TrustManagerConf::getTrustManager)
+        .orElse(null);
+  }
+
+  public KeyManager getSslKeyManager() {
+    return Optional.ofNullable(getKeyManager())
+        .map(KeyManagerConf::getKeyManager)
+        .orElse(null);
+  }

Review Comment:
   Let's don't add new API to `GrpcTlsConfig`, which will be deprecated soon.  We should use the API from `TlsConf`.



-- 
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: issues-unsubscribe@ratis.apache.org

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