Skip to content

HTTPCLIENT-1969: Filter out weak cipher suites#140

Merged
ok2c merged 1 commit intoapache:4.5.xfrom
artem-smotrakov:filter-weak-ciphers
Mar 5, 2019
Merged

HTTPCLIENT-1969: Filter out weak cipher suites#140
ok2c merged 1 commit intoapache:4.5.xfrom
artem-smotrakov:filter-weak-ciphers

Conversation

@artem-smotrakov
Copy link
Contributor

Please consider a patch for HTTPCLIENT-1969:

  • Defined a list of weak algorithms which may be used in a TLS connection. The list is based on the latest settings in modern OpenJDK, see java.security file (EXPORT ciphers are also disabled in modern OpenJDK by default)
  • Updated SSLConnectionSocketFactory to filter out weak ciphers if cipher suites are not explicitly set.

Please note that the test passes with latest Java versions even without patching SSLConnectionSocketFactory because latest Java versions disable weak ciphers by default. The filtering mechanism blocks weak ciphers in case older Java versions are used.

Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

I like the idea of this patch!

Copy link
Member

@ok2c ok2c left a comment

Choose a reason for hiding this comment

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

@artem-smotrakov Please normalize cipher names prior to doing the weak algo matching.

@artem-smotrakov
Copy link
Contributor Author

Addressed comments from @ok2c

  • Use regexp to catch weak cipher suites
  • Normalize cipher suites names prior matching
  • Added more weak algorithms even them they are not supported by the default JSSE implementation
  • Updated tests

Please take a look.

Copy link
Member

@ok2c ok2c left a comment

Choose a reason for hiding this comment

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

@artem-smotrakov Almost there. See my comment. Please also squash commits into a single one.

@artem-smotrakov
Copy link
Contributor Author

Updated the patch:

  • Use non-case sensitive regex matching
  • Squash commits into a single one

@ok2c Please take a look.

Copy link
Member

@ok2c ok2c left a comment

Choose a reason for hiding this comment

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

@artem-smotrakov So much nicer, is it not?

@ok2c ok2c merged commit ba1f444 into apache:4.5.x Mar 5, 2019
@ok2c
Copy link
Member

ok2c commented Mar 5, 2019

@artem-smotrakov Would you also be willing to port this change-set to master?

@artem-smotrakov
Copy link
Contributor Author

To be honest, I am not convinced that regexp is better here, but it's not a big deal to me :) Let's agree to disagree, you have more experience with this project :) Sure, I'll port it to the master branch. Thanks for the review @ok2c !

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.

3 participants