Skip to content

fix(http): enable TLSv1.3; disable TLSv1.0 TLSv1.1#123

Merged
padamstx merged 3 commits intomainfrom
2656-enable-tlsv1.3
May 19, 2021
Merged

fix(http): enable TLSv1.3; disable TLSv1.0 TLSv1.1#123
padamstx merged 3 commits intomainfrom
2656-enable-tlsv1.3

Conversation

@ricellis
Copy link
Copy Markdown
Member

The setupTLSProtocol function in HttpClientSingleton was hard-coded to TLS_1_0 TLS_1_1 TLS_1_2 thus preventing negotiation to TLSv1.3 even if it was available in the supported protocols for the socket and on the server.

Since configureHttpClient is using the MODERN_TLS connection spec anyway this change determines the set of protocols to enable from the union of the protocols listed in MODERN_TLS and the protocols supported by the socket.

It should be noted that:

  • The protocols allowed by MODERN_TLS gets updated with OkHttp updates (as such updating in future may disable additional protocols or enable new ones if they are also supported by the runtime environment).
  • MODERN_TLS currently allows only TLSv1.2 and TLSv1.3.

So this change will also disable TLSv1.0 and TLSv1.1 connections, but that should not pose a problem as we should be using TLSv1.2 minimum since 1.0 and 1.1 have known insecurities and are considered deprecated.

I added a test to validate that each enabled protocol on a socket created from the configured client was valid against both the MODERN_TLS list and the supported protocols list from the socket.

I also noticed that when using disableSslVerification the [existing] protocol filtering was not applied. I did not see a reason that the same protocol set should not be used even if the certificates and hostnames were going to be universally trusted. I've made that change and added a test for it in a separate commit so if others disagree it can be easily removed.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 18, 2021

CLA assistant check
All committers have signed the CLA.

@ricellis ricellis force-pushed the 2656-enable-tlsv1.3 branch from 990817a to fb510e6 Compare May 18, 2021 10:55
@padamstx padamstx self-requested a review May 18, 2021 17:39
Copy link
Copy Markdown
Contributor

@padamstx padamstx left a comment

Choose a reason for hiding this comment

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

Looks great, @ricellis !
Is this ready to merge?

@ricellis
Copy link
Copy Markdown
Member Author

Is this ready to merge?

I marked it as fix because it should be transparent given using TLSv1.2 min in services anyway, but I suppose there is a risk that some test environment somewhere that uses disableSSLVerification and doesn't provide TLSv1.2 might see a break. Personally I think that's an acceptable risk and assuming others agree then yes, this is ready to merge.

@padamstx
Copy link
Copy Markdown
Contributor

padamstx commented May 19, 2021

I think that's an acceptable risk and assuming others agree then yes, this is ready to merge.

agreed

I've tested this version of the java core with the platform-services java sdk (ran integration tests for several of the services) and all seems to be working well.

@ricellis Thank you for contributing this change!

@padamstx padamstx merged commit 3b05fd5 into main May 19, 2021
@padamstx padamstx deleted the 2656-enable-tlsv1.3 branch May 19, 2021 16:59
ibm-devx-sdk pushed a commit that referenced this pull request May 19, 2021
## [9.10.1](9.10.0...9.10.1) (2021-05-19)

### Bug Fixes

* **http:** enable TLSv1.3; disable TLSv1.0 TLSv1.1 ([#123](#123)) ([3b05fd5](3b05fd5))
@ibm-devx-sdk
Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 9.10.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants