TLS server options #302
TLS server options #302
Conversation
Adds options (with defaults) for setting tls_min_version (1.2),
tls_cipher_suites (""), and tls_prefer_server_cipher_suites (true).
tvoran
requested review from
tomhjp,
calvn,
benashz,
imthaghost and
mickael-hc
| @@ -160,6 +171,13 @@ func (c *Command) init() { | |||
| c.flagSet.StringVar(&c.flagResourceLimitMem, "memory-limit", agent.DefaultResourceLimitMem, | |||
| fmt.Sprintf("Memory resource limit set in injected containers. Defaults to %s", agent.DefaultResourceLimitMem)) | |||
|
|
|||
| c.flagSet.StringVar(&c.flagTLSMinVersion, "tls-min-version", defaultTLSMinVersion, | |||
| fmt.Sprintf(`Minimum supported version of TLS. Defaults to %s. Accepted values are "tls10", "tls11", "tls12" or "tls13"`, defaultTLSMinVersion)) | |||
| c.flagSet.StringVar(&c.flagTLSCipherSuites, "tls-cipher-suites", "", | |||
There was a problem hiding this comment.
I wonder if we should include the env variable name in the usage?
subcommand/injector/command.go
Outdated
| @@ -206,11 +211,28 @@ func (c *Command) Run(args []string) int { | |||
| } | |||
|
|
|||
| var handler http.Handler = mux | |||
| minTLSVersion, ok := tlsutil.TLSLookup[c.flagTLSMinVersion] | |||
| if !ok { | |||
| c.UI.Error(fmt.Sprintf("Failed to parse minimum TLS version %q", c.flagTLSMinVersion)) | |||
There was a problem hiding this comment.
The error message mentions parsing, but really we received an unsupported TLS version.
There was a problem hiding this comment.
Yep, changed in 05aeaaf to invalid or unsupported TLS version
subcommand/injector/flags.go
Outdated
| DefaultLogLevel = "info" | ||
| DefaultLogFormat = "standard" | ||
| defaultTLSMinVersion = "tls12" | ||
| defaultTLSPreferServerCipherSuites = true |
There was a problem hiding this comment.
Is this flag/option needed? Reading from https://pkg.go.dev/crypto/tls#Config, it looks like this deprecated and is now completely ignored.
There was a problem hiding this comment.
Looks like the deprecation happened recently, in go1.17 so this flag will still be used by the server since we're on 1.16. Having said that, I think that we should consider updating the go version we use to compile rather than introduce a parameter that will be ignored shortly after.
There was a problem hiding this comment.
Right, we can remove the option if we update our builds to go 1.17; they're on 1.16 currently.
There was a problem hiding this comment.
main is now building with go 1.17. I merged that into this branch, and removed the tls_prefer_server_cipher_suites option.
and committed TestTLSConfig()
subcommand/injector/flags.go
Outdated
| c.flagSet.StringVar(&c.flagTLSMinVersion, "tls-min-version", defaultTLSMinVersion, | ||
| fmt.Sprintf(`Minimum supported version of TLS. Defaults to %s. Accepted values are "tls10", "tls11", "tls12" or "tls13"`, defaultTLSMinVersion)) | ||
| c.flagSet.StringVar(&c.flagTLSCipherSuites, "tls-cipher-suites", "", | ||
| "Comma-separated list of supported cipher suites") |
There was a problem hiding this comment.
Is it worth mentioning that the order will not be respected? I think Golang will automatically reorder right? Maybe that's quite an advanced detail to include in quite high level help text though.
There was a problem hiding this comment.
Perhaps, but it is really a matter of which go version was used during the build. I suppose that was one advantage of the previous flag. The server preferred order would be enabled regardless of version of go that was used.
There was a problem hiding this comment.
Yeah, in 1.17 the order is ignored, and it's also only honored for TLS 1.0-1.2 (I added that last part to the usage string in 50c85b8).
| @@ -265,6 +274,28 @@ func (c *Command) handleReady(rw http.ResponseWriter, req *http.Request) { | |||
| rw.WriteHeader(204) | |||
| } | |||
|
|
|||
| func (c *Command) makeTLSConfig() (*tls.Config, error) { | |||
| c.flagSet.StringVar(&c.flagTLSMinVersion, "tls-min-version", defaultTLSMinVersion, | ||
| fmt.Sprintf(`Minimum supported version of TLS. Defaults to %s. Accepted values are %s.`, defaultTLSMinVersion, tlsStr)) | ||
| c.flagSet.StringVar(&c.flagTLSCipherSuites, "tls-cipher-suites", "", | ||
| "Comma-separated list of supported cipher suites for TLS 1.0-1.2") |
There was a problem hiding this comment.
Not sure I follow, does this mean we cannot set the cipher suites for any TLS version above 1.2?
There was a problem hiding this comment.
According to the docs, Note that TLS 1.3 ciphersuites are not configurable.
When testing this, it seems to just ignore that flag when TLS 1.3 is set. No errors thrown or anything.
There was a problem hiding this comment.
Very interesting...
In that case, perhaps rephrasing it a bit to be:
| "Comma-separated list of supported cipher suites for TLS 1.0-1.2") | |
| "Comma-separated list of supported cipher suites (ignored for tls13 and higher)") |
There was a problem hiding this comment.
Thanks, though I think we'll keep this phrasing for now.
Adds options (with defaults) for setting tls_min_version (1.2),
tls_cipher_suites ("").
Adds options for setting the TLS minimum version and supported cipher suites. Each can be set via command-line flag or environment variable.
When deploying with vault-helm, these can be set using the injector.extraEnvironmentVariables chart option.
Requires #303 since this PR assumes tls
Config.PreferServerCipherSuitesis ignored/deprecated as it is in go 1.17.