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.

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

fix: support JSSE system properties #175

Closed
wants to merge 1 commit into from
Closed

fix: support JSSE system properties #175

wants to merge 1 commit into from

Conversation

padamstx
Copy link
Member

@padamstx padamstx commented Apr 5, 2022

This commit makes a small change to the
HttpClientSingleton.setupTLSProtocol() method
to allow for the use of mutual TLS (i.e. client
authentication).
Specifically, the code changes will allow the JSSE-related
system properties to be used to specify a client truststore
and/or keystore:

  • javax.net.ssl.trustStore
  • javax.net.ssl.trustStorePassword
  • javax.net.ssl.keyStore
  • javax.net.ssl.keyStorePassword

This commit makes a small change to the
HttpClientSingleton.setupTLSProtocol() method
to allow for the use of mutual TLS (i.e. client
authentication).
Specifically, the code changes will allow the JSSE-related
system properties to be used to specify a client truststore
and/or keystore:
- javax.net.ssl.trustStore
- javax.net.ssl.trustStorePassword
- javax.net.ssl.keyStore
- javax.net.ssl.keyStorePassword
@padamstx padamstx self-assigned this Apr 5, 2022
@ricellis
Copy link
Member

ricellis commented Apr 6, 2022

Isn't this a breaking change?
Consider a case where the default SSLContext for whatever reason doesn't enable the TLS protocol. The existing code using getInstance("TLS") will go through the list of configured providers looking for one that does support TLS, but this new code will not and will fail instead; ulimately forcing a user configuration change to add TLS support in their default context (which is presumably not desirable if it hasn't been configured as such) or to otherwise customize the SSLSocketFactory.

I'm not sure just how significant that risk is, but nonetheless I feel it is a breaking behaviour change and it probably disproportionately impacts server environments with different frontend/backend contexts.

I also have concerns about disableSslVerification (beyond my usual ones about how I dislike that option 😄) - it won't delegate to the default SSLContext instead creating one in the current style, so there will be mismatch in behaviour depending if that toggle is on (beyond the existing one I've just noticed where SSL and TLS may not be necessarily be provided by the same provider anyway) .

Looking at this area of code in general I think we should probably consider the balance of the most common use case vs where customization is required by the user before making a breaking change. For example using cacerts by default is more likely to succeed when connecting to the IBM Cloud services with public certs than using any other configured trust store. Further when running the SDKs, in e.g. an app in a WAS Liberty runtime, then there are some specific caveats around what is honoured and what isn't and expectations for third-party (i.e. non-WAS) code to call SSLSocketFactory.getDefault() rather than SSLContext.getDefault().

In the meantime #174 provides an option for customizing right? via something like:

getHttpClient().newBuilder().sslSocketFactory(...).build()

@padamstx
Copy link
Member Author

padamstx commented Apr 6, 2022

@ricellis and I discussed this via slack and decided not to go with these changes due to the potential change in default behavior for users. It's not clear how popular mTLS is among our users, so we need to be careful not to potentially break existing (non-mTLS) users in order to solve a problem that has unknown scope/prevalence. I'll be looking at alternatives and will submit a new PR if/when appropriate.

@padamstx padamstx closed this Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants