Hardened tls cipher suits and added option for tls min version #315
Conversation
rest-server/main.go: Added parameter handling for TLS min version rest-server/main.go: Added crypto.tls, implemented and configured tlsConfig object
|
I actually have no idea how the tests work. From what I can guess out of main_test.go I assume it tests the program response to certain conditions to ensure it will throw errors when parameters are missing or misleading. If that's the case I think it is not necessary to add tests for what I did, because if |
There was a problem hiding this comment.
#322 will bump Go to 1.22 which disables TLS before 1.2 by default. TLS 1.2 is older than Go 1.0. So I don't think anyone will ever need an older TLS version (if they do then they should fix their setup instead).
That would sort of make the --tls-min-version somewhat obsolete. WDYT?
Removing the 3DES ciphers is fine. The cipher suite subset is already added by https://datatracker.ietf.org/doc/html/rfc5289 which is from 2008. That also implicitly means that clients that only support TLS < 1.2 won't be able to use any of those ciphers, which is one reason more to not even consider offering TLS 1.0 and 1.1.
| @@ -49,6 +49,7 @@ Flags: | |||
| --tls turn on TLS support | |||
| --tls-cert string TLS certificate path | |||
| --tls-key string TLS key path | |||
| --tls-min-ver string TLS min version (default: 1.2) (default "1.2") | |||
There was a problem hiding this comment.
The default is mentioned twice.
| case "1.3": | ||
| tlscfg.MinVersion = tls.VersionTLS13 | ||
| default: | ||
| tlscfg.MinVersion = tls.VersionTLS12 |
There was a problem hiding this comment.
invalid settings should result in a startup error. Otherwise behavior can change when new values become supported.
| @@ -162,8 +165,36 @@ func (app *restServerApp) runRoot(cmd *cobra.Command, args []string) error { | |||
| app.listenerAddress = listener.Addr() | |||
| app.listenerAddressMu.Unlock() | |||
|
|
|||
| tlscfg := &tls.Config{ | |||
| MinVersion: tls.VersionTLS12, | |||
| CurvePreferences: []tls.CurveID{tls.CurveP521, tls.CurveP384, tls.CurveP256}, | |||
There was a problem hiding this comment.
Go 1.23 defaults to []CurveID{X25519, CurveP256, CurveP384, CurveP521}. Why is that list different?
There was a problem hiding this comment.
I'd prefer to stick to the Go default here.
| }, | ||
| } | ||
| switch app.Server.TLSMinVer { | ||
| case "1.0": |
There was a problem hiding this comment.
I don't think anyone ever needs TLS 1.0 and 1.1.
What is the purpose of this change? What does it change?
With tls activated, the rest-server will provide insecure tls versions and insecure or broken tls cipher suits. This change configures the default settings in tls mode, sets a secure set of cipher suits (according to CIS NGINX Benchmark v2.1.0) and sets up TLSv1.2 as min version. It also adds a command line parameter to select a different version as min version.
The current version of the crypto.tls library also sets a small, secure set of cipher suits as default and limits the available TLS versions to 1.2 and 1.3. Earlier versions are also available but have to be explicitly enabled during compilation.
Was the change discussed in an issue or in the forum before?
Closes #251
Checklist
changelog/unreleased/that describes the changes for our users (template here)gofmton the code in all commits