Enable certificate validation by default, allow all certificates for local environment.

[bparmar-splunk]

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.

[fantavlik]

So for the first pass (where server certificate validate is on by default) we should skip this line if validateCertificates is set to true:

splunk-sdk-java/splunk/src/main/java/com/splunk/HttpService.java

Line 418 in 636b3c1

((HttpsURLConnection) cn).setSSLSocketFactory(sslSocketFactory);

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)

[bparmar-splunk]

@fantavlik,
I will make scope change from protected to public. But if we are thinking of removing above line of setting default SSLSockerFactory then it will give an error (unable to find valid certification path to requested target) if we are using https connection.
For now, changes are pushed.

[fantavlik]

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

[fantavlik]

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)

[fantavlik]

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

[bparmar-splunk]

In true block, instead of null, I think, we need to add a logic for validating certificate when environment is not local.

[fantavlik]

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

[bparmar-splunk]

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

[fantavlik]

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

[fantavlik]

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.

[bparmar-splunk]

Okay

[fantavlik]

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?

[fantavlik]

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)