Enable certificate validation by default, allow all certificates for local environment. #175
Conversation
Update: - Boolean flag validateCertificates is introduced. - createSSLFactory method is modified to handle certificates of local & non-local environment. For now local configuration are set up.
There was a problem hiding this comment.
So for the first pass (where server certificate validate is on by default) we should skip this line if validateCertificates is set to true:
I would also suggest setting validateCertificates to true by default (even though this is a breaking change) and making this a public variable as opposed to protected since we want to enable third party developers to set this themselves (similar to useTLS)
|
@fantavlik, |
Update: - Test case added to bypass certificate validation.
Update: - Skipping certificate validation while executing test cases. - Removed CertificateValidationTest file, as it is no longer required.
There was a problem hiding this comment.
After reviewing our legacy implementation in detail I think some more significant reworks are needed in order to remove some unneeded customization of how the SDK configures SSL and in order to favor the standard Java implementations. I left a number of comments, I also believe this section of code should be re-worked slightly: https://github.com/splunk/splunk-sdk-java/blob/master/splunk/src/main/java/com/splunk/HttpService.java#L538-L546
New logic:
SSLContext context = SSLContext.getDefault();
if (sslSecurityProtocol != null) {
context = SSLContext.getInstance(sslSecurityProtocol.toString());
}Please let me know if these suggestions make sense. I'd also appreciate a review by @ashah-splunk for these suggestions who did some of the original changes a91d966
| @@ -415,7 +423,9 @@ public ResponseMessage send(String path, RequestMessage request) { | |||
| throw new RuntimeException(e.getMessage(), e); | |||
| } | |||
| if (cn instanceof HttpsURLConnection) { | |||
| ((HttpsURLConnection) cn).setSSLSocketFactory(sslSocketFactory); | |||
| if (!validateCertificates) { | |||
There was a problem hiding this comment.
Apologies, I just realized that because we allow users to setSslSecurityProtocol (https://github.com/splunk/splunk-sdk-java/blob/master/splunk/src/main/java/com/splunk/HttpService.java#L210) we need to use the sslSocketFactory and can't skip that step. I believe we need to revert this change and removing the if statement and moving this logic into createSSLFactory (https://github.com/splunk/splunk-sdk-java/blob/master/splunk/src/main/java/com/splunk/HttpService.java#L523)
| public void checkServerTrusted(X509Certificate[] certs, String authType) { | ||
| } | ||
| } | ||
| }; | ||
| context.init(null, trustAll, new java.security.SecureRandom()); |
There was a problem hiding this comment.
I believe we need to move the logic in here like so:
if (validateCertificates) {
context.init(null, null, null);
} else {
context.init(null, trustAll, null);
}I'm also suggesting removing java.security.SecureRandom() in favor of null so that the default implementation can be used: https://docs.oracle.com/javase/8/docs/api/javax/net/ssl/SSLContext.html
There was a problem hiding this comment.
In true block, instead of null, I think, we need to add a logic for validating certificate when environment is not local.
| * For PROD environment, TRUE is strongly recommended, whereas working in localhost OR development environment, FALSE is used. | ||
| * Default Value: TRUE | ||
| */ | ||
| public static boolean validateCertificates = true; |
There was a problem hiding this comment.
Because this logic will now live in createSSLFactory (https://github.com/splunk/splunk-sdk-java/blob/master/splunk/src/main/java/com/splunk/HttpService.java#L523) we need to make this protected and adjust the setValidateCertificates method to re-initialize sslSocketFactory like here: https://github.com/splunk/splunk-sdk-java/blob/master/splunk/src/main/java/com/splunk/HttpService.java#L212-L215
There was a problem hiding this comment.
Yes. This was implemented earlier in createSSLFactory method based on the validateCertificate flag.
I think this would be more appropriate for both scenarios. (w/ certificate & w/o certificate).
| @@ -36,8 +36,17 @@ | |||
| public class HttpService { | |||
| // For debugging purposes | |||
| private static final boolean VERBOSE_REQUESTS = false; | |||
| public static boolean useTLS=false; | |||
| public static boolean useTLS = false; | |||
There was a problem hiding this comment.
I'm going to suggest removing this flag, we introduced this fairly recently (5 months ago) and this should already be possible with HttpService.setSslSecurityProtocol(SSLSecurityProtocol.TLSv1_2) and also we are not properly handling the case when this is changed - the sslSocketFactory also needs to be updated when this flag is changed (it is not currently).
| context.init(null, trustAll, new java.security.SecureRandom()); | ||
|
|
||
|
|
||
| return new SplunkHttpsSocketFactory(context.getSocketFactory()); |
There was a problem hiding this comment.
I suggest removing this SplunkHttpsSocketFactory class entirely, I don't see any aspect that we are customizing where we wouldn't use the standard SSLSocketFactory.
There was a problem hiding this comment.
This is looking great, would it be possible to add a single testcase without this setting
HttpService.setValidateCertificates(false);To test that a failure (presumably self-signed certificate related) is encountered in our dockerized Splunk tests and ensuring that server certificate validation is being performed by default?
|
Verified with the team that the error being thrown when validation is turned on cannot be caught for testing purposes and makes running other tests difficult/impossible but manual testing confirms that validation is being performed/erroring out when validation is turned on in CI (as expected because self signed cert is being used) |
Update: