[GitHub] [brooklyn-server] YYTVicky commented on a change in pull request #1087: leave a note to tell user this is not secure

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

YYTVicky commented on a change in pull request #1087:
URL: https://github.com/apache/brooklyn-server/pull/1087#discussion_r417712318



##########
File path: utils/common/src/main/java/org/apache/brooklyn/util/crypto/SslTrustUtils.java
##########
@@ -39,7 +39,7 @@
         return connection;
     }
     
-    /** trusts all SSL certificates */
+    /** trusts all SSL certificates, it is not secure*/

Review comment:
       Hi,
      Thanks for your kind feedback, our tool detected the checkClientTrusted and checkServerTrusted couldn't be the empty body or simply return true. It might cause Man-in-the-middle attacks. Here is a document to address these issues (https://www.guardrails.io/docs/en/vulnerabilities/java/insecure_network_communication).
   our tool also suggested some code template for such case, simply replace the current implementation may not works, but hope it gives some guideline for implementation!
   new X509TrustManager(){
   @override
   public void checkClientTrusted(X509Certificate[] chain, String authType) throws CertificateException {
   
   			for (final X509TrustManager trustManager : trustManagers) {
   				try {
   					trustManager.checkClientTrusted(chain, authType);
   					return;
   				} catch (final CertificateException e) {
   					//LOGGER.debug(e.getMessage(), e);
   				}
   			}
   			throw new CertificateException("None of the TrustManagers trust this certificate chain");
   
   		}
   
   		@Override
   		public X509Certificate[] getAcceptedIssuers() {
   			for (final X509TrustManager trustManager : trustManagers) {
   				final List<X509Certificate> list = Arrays.asList(trustManager.getAcceptedIssuers());
   				certificates.addAll(list);
   			}
   			return certificates.toArray(new X509Certificate[] {});
   		}
   
   		@Override
   		public void checkServerTrusted(X509Certificate[] chain, String authType) throws CertificateException{
   			if (chain == null) {
   				throw new IllegalArgumentException("checkServerTrusted:x509Certificate array isnull");
   			}
   
   			if (!(chain.length > 0)) {
   				throw new IllegalArgumentException("checkServerTrusted: X509Certificate is empty");
   			}
   
   			if (!(null != authType && authType.equalsIgnoreCase("RSA"))) {
   				throw new CertificateException("checkServerTrusted: AuthType is not RSA");
   			}
   
   
   			try {
   				TrustManagerFactory tmf = TrustManagerFactory.getInstance("X509");
   				tmf.init((KeyStore) null);
   				for (TrustManager trustManager : tmf.getTrustManagers()) {
   					((X509TrustManager) trustManager).checkServerTrusted(chain, authType);
   				}
   			} catch (Exception e) {
   				throw new CertificateException(e);
   			}
   
   
   			RSAPublicKey pubkey = (RSAPublicKey) chain[0].getPublicKey();
   			String encoded = new BigInteger(1 , pubkey.getEncoded()).toString(16);
   			final boolean expected = PUB_KEY.equalsIgnoreCase(encoded);
   
   			if (!expected) {
   				throw new CertificateException("checkServerTrusted: Expected public key: "
   						+ PUB_KEY + ", got public key:" + encoded);
   			}
   		}
   	}; 




----------------------------------------------------------------
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.

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