[GitHub] [netbeans] matthiasblaesing commented on a change in pull request #3207: NETBEANS-5956: Adding default java trust store to xml retriever

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

matthiasblaesing commented on a change in pull request #3207:
URL: https://github.com/apache/netbeans/pull/3207#discussion_r721617337



##########
File path: ide/xml.retriever/src/org/netbeans/modules/xml/retriever/impl/SecureURLResourceRetriever.java
##########
@@ -119,45 +139,48 @@ public void checkServerTrusted(X509Certificate[] certs, String authType)
                 }
             }
         };
+        TrustManager[] combinedTrustManagers = (TrustManager[]) Stream.of(defaultTrustManagers, trustAllCerts)
+                .flatMap(Stream::of)
+                .toArray(size -> new TrustManager[size]);
+
+        KeyManager[] keyManagersFromSystemProperties = null;
+        try {
+            KeyStore keyStoreFromSystemProperties = null;
+            char[] keyStorePassword = System.getProperty("javax.net.ssl.keyStorePassword", "").toCharArray();
+            if (System.getProperty("javax.net.ssl.keyStore") != null) {
+                File keyStoreFile = new File(System.getProperty("javax.net.ssl.keyStore"));
+                if (keyStoreFile.exists()) {
+                    KeyStore keyStore = KeyStore.getInstance(System.getProperty("javax.net.ssl.keyStoreType", KeyStore.getDefaultType()));
+                    try ( InputStream keyStoreStream = new FileInputStream(keyStoreFile)) {
+                        keyStore.load(keyStoreStream, keyStorePassword);
+                    }
 
-        // #208324: proper key managers need to be passed, so let's configure at least the defaults...
-        KeyManager[] mgrs;
-        if (System.getProperty("javax.net.ssl.keyStorePassword") != null &&  // NOI18N
-            System.getProperty("javax.net.ssl.keyStore") != null) { // NOI18N
-            try {
-                KeyStore ks = KeyStore.getInstance("JKS"); // NOI18N
-                    ks.load(new FileInputStream(System.getProperty("javax.net.ssl.keyStore")), //NOI18N
-                    System.getProperty("javax.net.ssl.keyStorePassword").toCharArray() //NOI18N
-                );
-                // Set up key manager factory to use our key store
-                KeyManagerFactory kmf = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
-                kmf.init(ks,System.getProperty("javax.net.ssl.keyStorePassword").toCharArray()); // NOI18N
-                mgrs = kmf.getKeyManagers();
-            } catch (IOException ex) {
-                // this is somewhat expected, i.e. JKS file not present
-                mgrs = null;
-            } catch (java.security.GeneralSecurityException e) {
-                ErrorManager.getDefault().notify(e);
-                return;
+                    keyStoreFromSystemProperties = keyStore;
+                }
             }
-        } else {
-            mgrs = null;
+
+            KeyManagerFactory keyManagerFactory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
+            keyManagerFactory.init(keyStoreFromSystemProperties, keyStorePassword);
+            keyManagersFromSystemProperties = keyManagerFactory.getKeyManagers();
+        } catch (GeneralSecurityException | IOException ex) {
+            keyManagersFromSystemProperties = new KeyManager[0];
         }
+
         try {
             SSLContext sslContext = SSLContext.getInstance("SSL"); //NOI18N
-            sslContext.init(mgrs, trustAllCerts, new java.security.SecureRandom());
+            sslContext.init(keyManagersFromSystemProperties, combinedTrustManagers, new java.security.SecureRandom());
             con.setSSLSocketFactory(sslContext.getSocketFactory());
-            con.setHostnameVerifier(new HostnameVerifier() {
-                public boolean verify(String string, SSLSession sSLSession) {
-                    // accept all hosts
-                    return true;
-                }
-            });
-        } catch (java.security.GeneralSecurityException e) {
+            con.setHostnameVerifier(this::acceptAllHosts);
+        } catch (GeneralSecurityException e) {
             ErrorManager.getDefault().notify(e);
         }
     }
-    
+
+    private boolean acceptAllHosts(String host, SSLSession sslSession) {
+        return true;

Review comment:
       This looks dangerous (and yes I know, that you did not write the code). If I read the new code correctly, it will try to verify the certificate with the system truststore. If that works, we are good. If not the user is prompted and if accepts, the certifcate is stored to be accepted list and user is not asked again.
   
   With the certificate verified, it needs to be tied to the hostname - this is a different issue and can't be done on the TrustManager level. Am I right that with the current implementation just accepts any certificate <-> hostname combination?




-- 
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: notifications-unsubscribe@netbeans.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org

For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists