Marcono1234 commented on code in PR #438: URL: https://github.com/apache/httpcomponents-core/pull/438#discussion_r1344299429
########## httpcore5/src/main/java/org/apache/hc/core5/ssl/TrustStrategy.java: ########## @@ -34,6 +34,19 @@ * configured in the actual SSL context. This interface can be used to override the standard * JSSE certificate verification process. * + * <h2>Security Warning</h2> + * If a trust strategy considers a certificate chain to be trusted, then the default trust manager + * will not be consulted. Trust strategy implementations must therefore properly check the complete + * certificate chain. Checking for example only the subject of a certificate does not protect Review Comment: You are right; it should not be "must". And performing some checks on the certificate it certainly better than using `TrustAllStrategy`. My main concern is that it might not be obvious to users what effect it can have when they define their own custom `TrustStrategy` and what can happen when they don't fully validate the certificate chain ([example 1](https://stackoverflow.com/q/58917168), [example 2](https://stackoverflow.com/q/61056194)). And yes you are right; there are certainly legit use cases where not fully (or at all) validating the certificate chain is fine. But it should be a deliberate decision then and the user doing this should be aware of the consequences. To me it feels this is often not clear to users, for example [here](https://stackoverflow.com/a/36707080) it use suggested to use `TrustSelfSignedStrategy` in combination with a truststore which already contains the self-signed certificate. And similarly Stack Overflow questions asking about trusting self-signed certificates or certificates with incorrect or missing SAN often get answers about completely disabling hostname verification and trusting all certificates (often these answers at least don't have a high score) instead of more secure alternatives, such as adding the self-signed certificate to the truststore or implementing a custom hostname verifier. Do you have other ideas for the wording here? Ideally this should express that if users don't properly implement their custom trust strategy (if they even need it at all), that they make their application vulnerable to man-in-the-middle attacks and that they should be aware of this. -- 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: dev-unsubscr...@hc.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@hc.apache.org For additional commands, e-mail: dev-h...@hc.apache.org
