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

Removes non-FS ciphers to make SSLLabs not warn on weak ciphers #4025

Merged
merged 1 commit into from Jul 31, 2018

Conversation

zwoop
Copy link
Contributor

@zwoop zwoop commented Jul 27, 2018

This is as reported by SSLLabs, and doing this change eliminates
their warnings about weak non-FS ciphers.

@zwoop zwoop added this to the 8.0.0 milestone Jul 27, 2018
@zwoop zwoop self-assigned this Jul 27, 2018
@zwoop
Copy link
Contributor Author

zwoop commented Jul 28, 2018

Before patch: before patch

After patch:
after patch

@maskit
Copy link
Member

maskit commented Jul 28, 2018

It seems fine but probably not. #4007

@zwoop
Copy link
Contributor Author

zwoop commented Jul 28, 2018

Ok, so probably we should remove the TLS v1.3 portion out of this patch then, and just fix the weak ciphers that are in TLS < v1.3 ?

We probably should make sure we can get the TLS v1.3 ciphers in v8.0.x of ATS as well IMO, particularly if we need to add a new configuration (I'm hoping we can avoid a new config).

This is as reported by SSLLabs, and doing this change eliminates
their warnings about weak non-FS ciphers.
@zwoop zwoop changed the title Updates the Ciphers to include TLS1.3 Removes non-FS ciphers to make SSLLabs not warn on weak ciphers Jul 30, 2018
@zwoop
Copy link
Contributor Author

zwoop commented Jul 30, 2018

I removed the TLS13_ portion from this PR. The Weak ciphers change is still as above (same effect on the SSLLabs checks).

Copy link
Contributor

@masaori335 masaori335 left a comment

Choose a reason for hiding this comment

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

The WEAK ciphers are called below in OpenSSL. And I confirmed these are removed.

TLS_RSA_WITH_AES_128_CBC_SHA            AES128-SHA
TLS_RSA_WITH_AES_256_CBC_SHA            AES256-SHA
TLS_RSA_WITH_AES_128_CBC_SHA256           AES128-SHA256
TLS_RSA_WITH_AES_256_CBC_SHA256           AES256-SHA256
TLS_RSA_WITH_AES_128_GCM_SHA256           AES128-GCM-SHA256
TLS_RSA_WITH_AES_256_GCM_SHA384           AES256-GCM-SHA384

https://www.openssl.org/docs/manmaster/man1/openssl-ciphers.html

@masaori335 masaori335 merged commit 2ded013 into apache:master Jul 31, 2018
@zwoop zwoop added this to 8.0.0 backports in 8.x releases Aug 7, 2018
@zwoop
Copy link
Contributor Author

zwoop commented Aug 7, 2018

@masaori335 @maskit Please confirm that you think this is a good change for 8.0.0 or not.

@zwoop zwoop deleted the UpdatedCiphers branch August 7, 2018 23:10
@masaori335
Copy link
Contributor

I'm pretty sure that this is a good change for 8.0.0. According to "Handshake Simulation" on SSLLabs, no client selects these ciphers. It looks no critical side effect of removing them.

@zwoop
Copy link
Contributor Author

zwoop commented Aug 8, 2018

Cherry-picked to 8.0.x

@zwoop zwoop removed this from 8.0.0 backports in 8.x releases Aug 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants