fix(tls): Update TLS strict cipher suite to actually work #20921
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
danxmoran
commented
Mar 10, 2021
| @@ -616,10 +619,6 @@ func (m *Launcher) run(ctx context.Context, opts *InfluxdOpts) (err error) { | |||
| log.Info("Stopping") | |||
| }(m.log) | |||
|
|
|||
| m.httpServer = &nethttp.Server{ | |||
There was a problem hiding this comment.
Took the opportunity to factor some code out of the huge run method here.
danxmoran
force-pushed
the
dm-tls-strict-ciphers-20762
branch
2 times, most recently
from
a3b6948
to
a2f8ae4
Compare
danxmoran
force-pushed
the
dm-tls-strict-ciphers-20762
branch
from
7339573
to
dd68d00
Compare
lesam
reviewed
Mar 16, 2021
| } | ||
| tlsMinVersion = tls.VersionTLS13 | ||
| default: | ||
| return fmt.Errorf("unsupported TLS version: %s", opts.HttpTLSMinVersion) |
lesam
reviewed
Mar 16, 2021
| if tlsMinVersion != tls.VersionTLS13 && opts.HttpTLSStrictCiphers { | ||
| cipherConfig = strictCiphers | ||
| var tlsMinVersion uint16 | ||
| var useStrictCiphers = opts.HttpTLSStrictCiphers |
There was a problem hiding this comment.
Does this / should this default true for 1.2?
There was a problem hiding this comment.
It currently defaults to false for all versions (maybe because setting it to true made everything break). I'm wary of changing the default at this point since I don't know what impact to expect on existing users, but if somebody from product / somebody with more TLS knowledge gave the 👍 then I'd be ok with it. Would want to remove the Warn log in the 1.3 case to avoid noise.
lesam
reviewed
Mar 16, 2021
|
|
||
| return nil | ||
| } | ||
|
|
lesam
reviewed
Mar 16, 2021
|
Self review: it looks like the description of the strict-cipher flag hard-codes the accepted cipher list. Will need to update that. |
lesam
approved these changes
Mar 16, 2021
gwossum
added a commit
to influxdata/docs-v2
that referenced
this pull request
Jan 25, 2024
Update cipher suite list when `tls-strict-ciphers` is enabled to match changes made in influxdata/influxdb#20921
2 tasks
sanderson
pushed a commit
to influxdata/docs-v2
that referenced
this pull request
Jan 26, 2024
Update cipher suite list when `tls-strict-ciphers` is enabled to match changes made in influxdata/influxdb#20921
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #20762
The cipher suite we had hard-coded was invalid, and would cause the
influxdprocess to hang on startup. The user who reported the problem referenced a Mozilla page that showed the proper suite to use, so I swapped to using it. I also fixed the code so a failure to serve HTTP/HTTPS will causeinfluxdto exit instead of hang.I've added a regression suite to check that enabling TLS works at all. Hopefully that'll catch problems like this in the future...