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

internal/envoy: Disable CHACHA20 ciphers #3347

Closed

Conversation

sunjayBhatia
Copy link
Member

@sunjayBhatia sunjayBhatia commented Feb 10, 2021

Related to an ask that Contour is FIPS compliant by default. Envoy built with
BoringSSL-FIPS removes the CHACHA20 ciphers by default.

Note: Should probably merge after #3292

@sunjayBhatia sunjayBhatia requested a review from a team as a code owner February 10, 2021 18:21
@sunjayBhatia sunjayBhatia requested review from danehans and skriss and removed request for a team February 10, 2021 18:21
@sunjayBhatia sunjayBhatia added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Feb 10, 2021
Related to an ask that Contour is FIPS compliant by default. Envoy built with
BoringSSL-FIPS removes the CHACHA20 cipehrs by default.

Signed-off-by: Sunjay Bhatia <[email protected]>
@sunjayBhatia sunjayBhatia force-pushed the disable-chacha-ciphers branch from e86e707 to 6eba5cc Compare February 10, 2021 18:23
@codecov
Copy link

codecov bot commented Feb 10, 2021

Codecov Report

Merging #3347 (fb974bf) into main (c8c8589) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3347      +/-   ##
==========================================
- Coverage   75.44%   75.42%   -0.02%     
==========================================
  Files          98       98              
  Lines        6283     6283              
==========================================
- Hits         4740     4739       -1     
- Misses       1437     1439       +2     
+ Partials      106      105       -1     
Impacted Files Coverage Δ
internal/k8s/log.go 63.04% <0.00%> (-6.53%) ⬇️
internal/dag/cache.go 91.58% <0.00%> (+0.62%) ⬆️

@sunjayBhatia
Copy link
Member Author

sunjayBhatia commented Feb 10, 2021

Open question, should we do this in 1.13 since we plan for #3292 to be done then too? Or wait for 1.14 to give people some time to adapt? @projectcontour/maintainers

@xaleeks
Copy link

xaleeks commented Feb 10, 2021

I vote we announce this in the 1.13 and perform the deprecation on the following release, so folks have some time to react. We should release this concurrent to or after the FIPS configurability capability is released so people can customize the specific list of ciphers they need.

@jpeach
Copy link
Contributor

jpeach commented Feb 10, 2021

There's a good use case for an easy FIPS-mode, but I don't think that FIPS is an end in itself or necessarily a good default in general. AFAIK CHACHA20 cipher suites are valid, reasonable and secure choices.

https://wiki.mozilla.org/Security/Server_Side_TLS#Modern_compatibility

@youngnick
Copy link
Member

Yes, I agree that FIPS should not be Contour's default. We should make it possible to do FIPS, and document what you have to do, but I don't think that FIPS should be the default.

I agree that we shouldn't turn these ciphers off by default. When you are doing FIPS, you're going to be specifying a cipher set anyway, so it's better to just document what that will be, and make sure it works.

@xaleeks
Copy link

xaleeks commented Feb 11, 2021

FIPS is too much for default for OSS I agree. So how do we provide a mechanism that allows FIPS to be kept off in OSS but the default in TKG, without having to maintain two code bases. So can we leverage the configurable cipher suites capability to not only provide granularity at the cipher suite level but also just turn FIPS on / off as a profile.

I'm kinda just throwing ideas out at this point. Beware that the FIPS requirement is a long term thing for all future releases within TKG. So maintaining two Contours would be very heavy

@youngnick
Copy link
Member

In terms of actual FIPS compliance, it seems to me like there are two requirements:

  • Cipher suite must only have FIPS ciphers (basically, something like this PR)
  • Go crypto libraries must only use FIPS-certified versions. We can actually do this with a separte Dockerfile using FROM goboring/golang:<version> instead of the current Dockerfile. So we may be able to make this relatively easy.

Then, we do not have to maintain two codebases anywhere. But we should talk more about this on #2878.

Thanks for this work, @sunjayBhatia, but I think we should close this PR out.

@sunjayBhatia
Copy link
Member Author

Sounds good to me I mostly wanted to start the conversation and capture opinions etc. I figured there would be some reticence to pulling more ciphers out etc.! I think your comments on this and the linked FIPS issue are spot on

@sunjayBhatia sunjayBhatia removed the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Feb 11, 2021
@sunjayBhatia sunjayBhatia deleted the disable-chacha-ciphers branch February 11, 2021 03:54
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.

4 participants