Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

leave a note to tell user this is not secure #1087

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+1 −1
Open

leave a note to tell user this is not secure #1087

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion utils/common/src/main/java/org/apache/brooklyn/util/crypto/SslTrustUtils.java
View file
Original file line number Diff line number Diff line change
@@ -39,7 +39,7 @@ public static <T extends URLConnection> T trustAll(T connection) {
return connection;
}

/** trusts all SSL certificates */
/** trusts all SSL certificates, it is not secure*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @YYTVicky thanks for the PR! One comment I would make is that if we are going to warn users about something not being secure, we should explain why it is not secure. The additional comment here would probably need to have more explanation in it.

However, I'm not sure that it is right simply to say "it is not secure" here. Even though this policy trusts all certificates, whether or not this is secure depends on where and how you are using it. I can certainly imagine situations in which this would be the wrong class to use. On the other hand there may be situations in which you do want to trust all certificates (e.g. in unit tests where you are not concerned with the trustworthiness of the certificate). Maybe you could reword the comment along the lines of suggesting that users should be aware of any security implications of trusting all certs before using this class?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
		}
	}
};

public static final TrustManager TRUST_ALL = new X509TrustManager() {
@Override
public X509Certificate[] getAcceptedIssuers() {