TLS: make cipher suite list and min version configurable #2898
Conversation
docs/sources/operators-guide/configure/reference-configuration-parameters/index.md
Outdated
Show resolved
Hide resolved
Please do so. :) |
Where? The docs in this PR are auto-generated from short descriptions in the code; there isn't really room to put a lengthy explanation in. |
There was a problem hiding this comment.
LGTM, except I'm wondering if these parameters should be advanced. Also wondering if the new parameters should be better documented, as @pstibrany also commented on.
|
What do you think about including version strings and cipher suite names in the help string directly, similar to grafana/dskit#217? |
Should we also mention the new options in |
That list of strings seems way too long for help text, to me. |
In Mimir we can extract it to a single doc block so that it doesn't repeat. Would that help? (I agree it's a bit long, and this is quite low-level option, so not many people will need it.) |
It was more printing it out from |
bboreham
force-pushed
the
tls-cipher-suites
branch
from
8b9183c to
2828bea
Compare
bboreham
force-pushed
the
tls-cipher-suites
branch
2 times, most recently
from
c4716e1 to
d9b6e6d
Compare
There was a problem hiding this comment.
Happy with it! A couple of questions before merging:
- Could you mention the new CLI flags in
docs/sources/operators-guide/secure/securing-communications-with-tls.mdunder the section "Server flags" (there's a list of CLI flags supported)? - Now that we have support for
doc:"description_method=XXX"as struct tag, would you consider adding the full description of supported ciphers as we did in Adding min version and cipher suite variables to tls client configuration dskit#217?
|
The CHANGELOG has just been cut to prepare for the next Mimir release. Please rebase |
bboreham
force-pushed
the
tls-cipher-suites
branch
from
d9b6e6d to
955fc16
Compare
Did you mean I should add this upstream in |
I was wondering if you were willing to add it in upstream, but it's not a blocker to me. |
Note the cipher suite list is configured as a single string, more like Kubernetes than Prometheus. Updates weaveworks/common and prometheus/exporter-toolkit libraries. Fix up reference docs too.
bboreham
force-pushed
the
tls-cipher-suites
branch
from
955fc16 to
7f15867
Compare
|
I added to docs as you suggested, and changed CHANGELOG. |
bboreham
force-pushed
the
tls-cipher-suites
branch
from
7f15867 to
c561de5
Compare
bboreham
force-pushed
the
tls-cipher-suites
branch
from
c561de5 to
e6a83eb
Compare
| @@ -5,6 +5,7 @@ | |||
| ### Grafana Mimir | |||
|
|
|||
| * [CHANGE] Flag `-azure.msi-resource` is now ignored, and will be removed in Mimir 2.7. This setting is now made automatically by Azure. #2682 | |||
| * [ENHANCEMENT] Added `<prefix>.tls-min-version` and `<prefix>.tls-cipher-suites` flags to configure cipher suites and min TLS version supported by servers. #2898 | |||
There was a problem hiding this comment.
What prefixes are available? Diff in help file shows only "server", nothing else.
There was a problem hiding this comment.
You're right. This may be a confusion with the client-side flags.
What this PR does
Allows the admin to configure the list of ciphers supported by Mimir services, and/or the minimum TLS version.
This is revisiting the change rejected in #2309 (comment) on the basis that the documentation was user-hostile.
In this version, the cipher suite list is configured as a single string, patterned after the similar Kubernetes option.
Cipher names are as used by Go, at https://pkg.go.dev/crypto/tls#pkg-constants.
Updates weaveworks/common and prometheus/exporter-toolkit libraries.
NOTE the weaveworks/common PR is not merged yet; I wanted to get consensus on this change before doing that.I didn't add a test specifically for these changes in Mimir; weaveworks/common unit tests check the basic name-decoding functionality.
Documentation change here is pretty minimal; in particular we should link to the list of cipher names.
Checklist
CHANGELOG.mdupdated