[PR] MINOR: Support all JVM KeyFactory algorithms for PEM keys [kafka]

["mimaison (via GitHub)" <gi...@apache.org>]

mimaison opened a new pull request, #23914:
URL: https://github.com/apache/kafka/pull/23914

   Remove the hard coded key algorithm list and instead discover which algorithm the JVM supports
   


-- 
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: jira-unsubscribe@kafka.apache.org

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

Re: [PR] MINOR: Support all JVM KeyFactory algorithms for PEM keys [kafka]

["adikou (via GitHub) via jira" <ji...@kafka.apache.org>]

adikou commented on code in PR #23914:
URL: https://github.com/apache/kafka/pull/23914#discussion_r4551677158


##########
clients/src/test/java/org/apache/kafka/common/security/ssl/DefaultSslEngineFactoryTest.java:
##########
@@ -334,8 +337,14 @@ public void testPemKeyStoreFileWithKeyPassword() throws Exception {
 
     @Test
     public void testUnavailableKeyFactoryAlgorithmDoesNotThrow() {
-        List<KeyFactory> factories = DefaultSslEngineFactory.PemStore.keyFactories(List.of("RSA", "DSA", "EC", "BOGUS_FACTORY"));
-        assertFalse(factories.stream().anyMatch(f -> f.getAlgorithm().equals("BOGUS_FACTORY")));
+        List<KeyFactory> factories = DefaultSslEngineFactory.PemStore.keyFactories(Set.of("BOGUS_FACTORY"));
+        assertTrue(factories.isEmpty());
+    }
+
+    @Test
+    public void testAllJvmKeyFactoryAlgorithmsLoaded() {
+        List<KeyFactory> factories = DefaultSslEngineFactory.PemStore.keyFactories(Security.getAlgorithms("KeyFactory"));
+        assertFalse(factories.isEmpty());

Review Comment:
   In addition to checking the internal state, it would be good to assert that's we throw InvalicConfigException when we try to create a private key



##########
clients/src/main/java/org/apache/kafka/common/security/ssl/DefaultSslEngineFactory.java:
##########
@@ -455,7 +456,7 @@ public boolean modified() {
             return false;
         }
 
-        static List<KeyFactory> keyFactories(List<String> algorithms) {
+        static List<KeyFactory> keyFactories(Set<String> algorithms) {

Review Comment:
   Since the only other value comes from a test, you can have this as package-private that defaults to Security.getAlgorithms inside this method. 
   



##########
clients/src/test/java/org/apache/kafka/common/security/ssl/DefaultSslEngineFactoryTest.java:
##########
@@ -334,8 +337,14 @@ public void testPemKeyStoreFileWithKeyPassword() throws Exception {
 
     @Test
     public void testUnavailableKeyFactoryAlgorithmDoesNotThrow() {
-        List<KeyFactory> factories = DefaultSslEngineFactory.PemStore.keyFactories(List.of("RSA", "DSA", "EC", "BOGUS_FACTORY"));
-        assertFalse(factories.stream().anyMatch(f -> f.getAlgorithm().equals("BOGUS_FACTORY")));

Review Comment:
   The assertion has changed here: previously we checked that BOGUS_FACTORY does not exist specifically but now we check nothing exists. 
   
   We need the previous assertion here but imo



##########
clients/src/test/java/org/apache/kafka/common/security/ssl/DefaultSslEngineFactoryTest.java:
##########
@@ -334,8 +337,14 @@ public void testPemKeyStoreFileWithKeyPassword() throws Exception {
 
     @Test
     public void testUnavailableKeyFactoryAlgorithmDoesNotThrow() {
-        List<KeyFactory> factories = DefaultSslEngineFactory.PemStore.keyFactories(List.of("RSA", "DSA", "EC", "BOGUS_FACTORY"));

Review Comment:
   Similar to the previous assertion, maybe we should assert that at least one of RSA/DSA/EC exist to preserve existing test assertion.
   
   I'm wondering if there maybe vendors that satisfy a different set of key algorithms. If it's ok, I'd like to extend the test with generating and writing keypairs for a few other algorithms in a follow-on PR. Currently we only use a single hardcoded pvt key in the test file. 



##########
clients/src/test/java/org/apache/kafka/common/security/ssl/DefaultSslEngineFactoryTest.java:
##########
@@ -334,8 +337,14 @@ public void testPemKeyStoreFileWithKeyPassword() throws Exception {
 
     @Test
     public void testUnavailableKeyFactoryAlgorithmDoesNotThrow() {
-        List<KeyFactory> factories = DefaultSslEngineFactory.PemStore.keyFactories(List.of("RSA", "DSA", "EC", "BOGUS_FACTORY"));
-        assertFalse(factories.stream().anyMatch(f -> f.getAlgorithm().equals("BOGUS_FACTORY")));
+        List<KeyFactory> factories = DefaultSslEngineFactory.PemStore.keyFactories(Set.of("BOGUS_FACTORY"));
+        assertTrue(factories.isEmpty());
+    }
+
+    @Test
+    public void testAllJvmKeyFactoryAlgorithmsLoaded() {
+        List<KeyFactory> factories = DefaultSslEngineFactory.PemStore.keyFactories(Security.getAlgorithms("KeyFactory"));

Review Comment:
   We're releasing internal details here, so the default empty method has a benefit imo



-- 
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: jira-unsubscribe@kafka.apache.org

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