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

Remove old ciphers #7282

Merged
merged 7 commits into from
Mar 10, 2020
Merged

Remove old ciphers #7282

merged 7 commits into from
Mar 10, 2020

Conversation

hanshasselberg
Copy link
Member

@hanshasselberg hanshasselberg commented Feb 12, 2020

3DES ciphers are officially retired because their are vulnerable to the
https://sweet32.info/ attack. You can read more about the reasoning
here:
https://www.cryptomathic.com/news-events/blog/3des-is-officially-being-retired.

After reading more about ciphers and recommended ciphers here: https://github.com/ssllabs/research/wiki/SSL-and-TLS-Deployment-Best-Practices I cut down the list even more.

3DES ciphers are officially retired because their are vulnerable to the
https://sweet32.info/ attack. You can read more about the reasoning
here:
https://www.cryptomathic.com/news-events/blog/3des-is-officially-being-retired.
@hanshasselberg hanshasselberg self-assigned this Feb 12, 2020
@hanshasselberg hanshasselberg requested a review from a team February 12, 2020 21:46
@hanshasselberg hanshasselberg added the waiting-reply Waiting on response from Original Poster or another individual in the thread label Feb 12, 2020
@banks
Copy link
Member

banks commented Feb 13, 2020

I'm all for this change in general, but I'm wondering about timing.

  1. This is a breaking change. My gut feeling is that probably zero users are relying on 3DES TLS mode at this point so it's low impact, but it is still a breaking change and if we're wrong and some user's workload can't Connect to Consul anymore then it would be unpleasant to get that in a patch release. So We probably need to hold this for 1.8.0

  2. While we're at it, can we trim the list right down to the bare minimum NIST recommended ciphers in 1.8.0 - I don't think that will cause problems for many folks and leaves us in a better place going forward. I suggest we also require TLS 1.2 if we don't already.

Alternatively, if we don't want to wait, or if we think it's a nicer deprecation cycle, we could trim the list now but with an option to "allow deprecated TLS algos" (I'd avoid letting users specify specific ones just for simplicity). I'd be more OK with putting that in a patch release although deserves some thought.

It would be great if we could first emit a warning when using a deprecated cipher before we remove support - I think that would be possible in our TLS handling code on servers/clients but it's more involved.

What do you think? Am I too paranoid? Can we argue that although it's a breaking change its warranted in a patch release on security grounds? That would be true for a CVE response or similar but this isn't quite the same thing - it's been considered insecure for years.

@ghost ghost removed waiting-reply Waiting on response from Original Poster or another individual in the thread labels Feb 13, 2020
@banks
Copy link
Member

banks commented Feb 13, 2020

For context:

NIST on July 19, 2018, the Triple Data Encryption Algorithm (TDEA or 3DES) is officially being retired. The guidelines propose that, after a period of public consultation, 3DES is deprecated for all new applications and usage is disallowed after 2023.

(Emphasis mine). So it's been deprecated for well over a year already but is not disallowed for another 3 years.

I'm super up for killing it as soon as possible though, just want to think what a responsible deprecation cycle would be for us.

@banks banks closed this Feb 13, 2020
@banks
Copy link
Member

banks commented Feb 13, 2020

Sorry, mashed wrong button.

@banks banks reopened this Feb 13, 2020
@hanshasselberg
Copy link
Member Author

We default to tls12

tls_min_version = "tls12"
but allow configuring tls10, tls11, tls12.

@hanshasselberg hanshasselberg changed the title Remove 3DES ciphers Remove old ciphers Feb 19, 2020
@hanshasselberg
Copy link
Member Author

This is going into 1.8 because it is a breaking change, that we couldn't justify putting into a minor release because there might be customers out there depending on this. This is also why we don't add a way to opt into deprecated ciphers since we feel strongly that we don't want to enable our customers to used known bad ciphers.

Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

:shipit:

@hanshasselberg hanshasselberg merged commit 7777891 into master Mar 10, 2020
@hanshasselberg hanshasselberg deleted the sweet_cipher branch March 10, 2020 20:44
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