Make SSL cipher suite configurable #17440
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,191 @@ | ||
| // Copyright 2021 The Gitea Authors. All rights reserved. | ||
| // Use of this source code is governed by a MIT-style | ||
| // license that can be found in the LICENSE file. | ||
|
|
||
| package cmd | ||
|
|
||
| import ( | ||
| "crypto/tls" | ||
| "net/http" | ||
| "os" | ||
| "strings" | ||
|
|
||
| "code.gitea.io/gitea/modules/graceful" | ||
| "code.gitea.io/gitea/modules/log" | ||
| "code.gitea.io/gitea/modules/setting" | ||
| "github.com/klauspost/cpuid/v2" | ||
| ) | ||
|
|
||
| var tlsVersionStringMap = map[string]uint16{ | ||
| "": tls.VersionTLS12, // Default to tls.VersionTLS12 | ||
| "tlsv1.0": tls.VersionTLS10, | ||
| "tlsv1.1": tls.VersionTLS11, | ||
| "tlsv1.2": tls.VersionTLS12, | ||
| "tlsv1.3": tls.VersionTLS13, | ||
| } | ||
|
|
||
| func toTLSVersion(version string) uint16 { | ||
| tlsVersion, ok := tlsVersionStringMap[strings.TrimSpace(strings.ToLower(version))] | ||
| if !ok { | ||
| log.Warn("Unknown tls version: %s", version) | ||
| return 0 | ||
| } | ||
| return tlsVersion | ||
| } | ||
|
|
||
| var curveStringMap = map[string]tls.CurveID{ | ||
| "x25519": tls.X25519, | ||
| "p256": tls.CurveP256, | ||
| "p384": tls.CurveP384, | ||
| "p521": tls.CurveP521, | ||
| } | ||
|
|
||
| func toCurvePreferences(preferences []string) []tls.CurveID { | ||
| ids := make([]tls.CurveID, 0, len(preferences)) | ||
| for _, pref := range preferences { | ||
| id, ok := curveStringMap[strings.TrimSpace(strings.ToLower(pref))] | ||
| if !ok { | ||
| log.Warn("Unknown curve: %s", pref) | ||
| } | ||
| if id != 0 { | ||
| ids = append(ids, id) | ||
| } | ||
| } | ||
| return ids | ||
| } | ||
|
|
||
| var cipherStringMap = map[string]uint16{ | ||
| "rsa_with_rc4_128_sha": tls.TLS_RSA_WITH_RC4_128_SHA, | ||
|
There was a problem hiding this comment. As above, we should not list all possible cipher here but only those proved secure currently. There was a problem hiding this comment. I do agree on this one - we should only provide a option to those we can somewhat guarantee security. The best thing to do is to re-use the list that golang uses(they have a awesome crypto packages and will hold them up to date as time goes on) the There was a problem hiding this comment. As above, the insecure protocols are disabled by default. If a user insist to use it, then that's their choice. There was a problem hiding this comment. Please look at the documentation for nginx, apache or tomcat. They say nothing. Honestly I think saying nothing is the only correct way. |
||
| "rsa_with_3des_ede_cbc_sha": tls.TLS_RSA_WITH_3DES_EDE_CBC_SHA, | ||
| "rsa_with_aes_128_cbc_sha": tls.TLS_RSA_WITH_AES_128_CBC_SHA, | ||
| "rsa_with_aes_256_cbc_sha": tls.TLS_RSA_WITH_AES_256_CBC_SHA, | ||
| "rsa_with_aes_128_cbc_sha256": tls.TLS_RSA_WITH_AES_128_CBC_SHA256, | ||
| "rsa_with_aes_128_gcm_sha256": tls.TLS_RSA_WITH_AES_128_GCM_SHA256, | ||
| "rsa_with_aes_256_gcm_sha384": tls.TLS_RSA_WITH_AES_256_GCM_SHA384, | ||
| "ecdhe_ecdsa_with_rc4_128_sha": tls.TLS_ECDHE_ECDSA_WITH_RC4_128_SHA, | ||
| "ecdhe_ecdsa_with_aes_128_cbc_sha": tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, | ||
| "ecdhe_ecdsa_with_aes_256_cbc_sha": tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, | ||
| "ecdhe_rsa_with_rc4_128_sha": tls.TLS_ECDHE_RSA_WITH_RC4_128_SHA, | ||
| "ecdhe_rsa_with_3des_ede_cbc_sha": tls.TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA, | ||
| "ecdhe_rsa_with_aes_128_cbc_sha": tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, | ||
| "ecdhe_rsa_with_aes_256_cbc_sha": tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, | ||
| "ecdhe_ecdsa_with_aes_128_cbc_sha256": tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256, | ||
| "ecdhe_rsa_with_aes_128_cbc_sha256": tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256, | ||
| "ecdhe_rsa_with_aes_128_gcm_sha256": tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, | ||
| "ecdhe_ecdsa_with_aes_128_gcm_sha256": tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, | ||
| "ecdhe_rsa_with_aes_256_gcm_sha384": tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, | ||
| "ecdhe_ecdsa_with_aes_256_gcm_sha384": tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, | ||
| "ecdhe_rsa_with_chacha20_poly1305_sha256": tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, | ||
| "ecdhe_ecdsa_with_chacha20_poly1305_sha256": tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, | ||
| "ecdhe_rsa_with_chacha20_poly1305": tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, | ||
| "ecdhe_ecdsa_with_chacha20_poly1305": tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, | ||
| "aes_128_gcm_sha256": tls.TLS_AES_128_GCM_SHA256, | ||
| "aes_256_gcm_sha384": tls.TLS_AES_256_GCM_SHA384, | ||
| "chacha20_poly1305_sha256": tls.TLS_CHACHA20_POLY1305_SHA256, | ||
| } | ||
|
|
||
| func toTLSCiphers(cipherStrings []string) []uint16 { | ||
| ciphers := make([]uint16, 0, len(cipherStrings)) | ||
| for _, cipherString := range cipherStrings { | ||
| cipher, ok := cipherStringMap[strings.TrimSpace(strings.ToLower(cipherString))] | ||
| if !ok { | ||
| log.Warn("Unknown cipher: %s", cipherString) | ||
| } | ||
| if cipher != 0 { | ||
| ciphers = append(ciphers, cipher) | ||
| } | ||
| } | ||
|
|
||
| return ciphers | ||
| } | ||
|
|
||
| // defaultCiphers uses hardware support to check if AES is specifically | ||
| // supported by the CPU. | ||
| // | ||
| // If AES is supported AES ciphers will be preferred over ChaCha based ciphers | ||
| // (This code is directly inspired by the certmagic code.) | ||
| func defaultCiphers() []uint16 { | ||
| if cpuid.CPU.Supports(cpuid.AESNI) { | ||
| return defaultCiphersAESfirst | ||
| } | ||
| return defaultCiphersChaChaFirst | ||
| } | ||
|
|
||
| var ( | ||
| defaultCiphersAES = []uint16{ | ||
|
There was a problem hiding this comment. Just to note that these default ciphers are recommended for TLS v1.0-v.1.2 TLS v1.3 has another default cipher recommendation IIRC. IIRC(please check it up) AES support: No-AES support: There was a problem hiding this comment. To give another note on TLS v1.3 - all chiper suites are secure enough that TLS v1.3 supports, thus golang currently will disregard custom cipher suites when TLS v1.3 is used - Might be worth noting somewhere to avoid confusion. There was a problem hiding this comment. I have copied these defaults from They represent a reasonable choice of default of ciphers when we default to MinTLSVersion 1.2. If users want to change their TLS version they need to know how to set things themselves. |
||
| tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, | ||
| tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, | ||
| tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, | ||
| tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, | ||
| } | ||
|
|
||
| defaultCiphersChaCha = []uint16{ | ||
| tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, | ||
| tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, | ||
| } | ||
|
|
||
| defaultCiphersAESfirst = append(defaultCiphersAES, defaultCiphersChaCha...) | ||
| defaultCiphersChaChaFirst = append(defaultCiphersChaCha, defaultCiphersAES...) | ||
|
Comment on lines
+127
to
+128
There was a problem hiding this comment. We might want to add the whole list as well which golang uses as well. https://github.com/golang/go/blob/master/src/crypto/tls/cipher_suites.go#L271 (Skip the first 6 as they are "defaultCiphersChaCha" and "defaultCiphersAES") There was a problem hiding this comment. I have copied these defaults from They represent a reasonable choice of default of ciphers when we default to MinTLSVersion 1.2. |
||
| ) | ||
|
|
||
| // runHTTPs listens on the provided network address and then calls | ||
| // Serve to handle requests on incoming TLS connections. | ||
| // | ||
| // Filenames containing a certificate and matching private key for the server must | ||
| // be provided. If the certificate is signed by a certificate authority, the | ||
| // certFile should be the concatenation of the server's certificate followed by the | ||
| // CA's certificate. | ||
| func runHTTPS(network, listenAddr, name, certFile, keyFile string, m http.Handler) error { | ||
| tlsConfig := &tls.Config{} | ||
| if tlsConfig.NextProtos == nil { | ||
| tlsConfig.NextProtos = []string{"h2", "http/1.1"} | ||
| } | ||
|
|
||
| if version := toTLSVersion(setting.SSLMinimumVersion); version != 0 { | ||
| tlsConfig.MinVersion = version | ||
| } | ||
zeripath marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if version := toTLSVersion(setting.SSLMaximumVersion); version != 0 { | ||
| tlsConfig.MaxVersion = version | ||
| } | ||
|
|
||
| // Set curve preferences | ||
| tlsConfig.CurvePreferences = []tls.CurveID{ | ||
|
There was a problem hiding this comment. Golang will by default set a good curvePreference which is bit less restrictive than this one, not sure if it would make sense to set our own then: There was a problem hiding this comment. I have copied these defaults from They represent a reasonable choice of default of ciphers when we default to MinTLSVersion 1.2. If users want to change their TLS version they need to know how to set things themselves. |
||
| tls.X25519, | ||
| tls.CurveP256, | ||
| } | ||
| if curves := toCurvePreferences(setting.SSLCurvePreferences); len(curves) > 0 { | ||
| tlsConfig.CurvePreferences = curves | ||
| } | ||
|
|
||
| // Set cipher suites | ||
| tlsConfig.CipherSuites = defaultCiphers() | ||
|
There was a problem hiding this comment. The less we say the better. |
||
| if ciphers := toTLSCiphers(setting.SSLCipherSuites); len(ciphers) > 0 { | ||
| tlsConfig.CipherSuites = ciphers | ||
| } | ||
|
|
||
| tlsConfig.Certificates = make([]tls.Certificate, 1) | ||
|
|
||
| certPEMBlock, err := os.ReadFile(certFile) | ||
| if err != nil { | ||
| log.Error("Failed to load https cert file %s for %s:%s: %v", certFile, network, listenAddr, err) | ||
| return err | ||
| } | ||
|
|
||
| keyPEMBlock, err := os.ReadFile(keyFile) | ||
| if err != nil { | ||
| log.Error("Failed to load https key file %s for %s:%s: %v", keyFile, network, listenAddr, err) | ||
| return err | ||
| } | ||
|
|
||
| tlsConfig.Certificates[0], err = tls.X509KeyPair(certPEMBlock, keyPEMBlock) | ||
| if err != nil { | ||
| log.Error("Failed to create certificate from cert file %s and key file %s for %s:%s: %v", certFile, keyFile, network, listenAddr, err) | ||
| return err | ||
| } | ||
|
|
||
| return graceful.HTTPListenAndServeTLSConfig(network, listenAddr, name, tlsConfig, m) | ||
| } | ||
|
|
||
| func runHTTPSWithTLSConfig(network, listenAddr, name string, tlsConfig *tls.Config, m http.Handler) error { | ||
| return graceful.HTTPListenAndServeTLSConfig(network, listenAddr, name, tlsConfig, m) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not allow v1.0 and v1.1 since they are not secure any more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In real world, some users still need these legacy protocols.
In my company, one of our customers are still using TLSv1.1 😭
So, I think we just provide the options, whatever is used is the end user's choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we at least advise on not recommending using the TLSv1.0 and TLSv1.1? Having as a option is fine, but it shouldn't be prominent that a user could think that's a good choice to choose it if they don't have a specific reason for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, we can write something about "not recommended to use xxx"
However, the default values of setting options are generally fine tuned, so if a user have to edit the setting to change the TLS protocols, they must be familiar the TLS system already and know what they are doing, our warning doesn't seem to help much, because everyone on internet will tell you that "only TLS>=1.2 is secure"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least we did warn, better safe than sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess a good example would be to look at the documentation for nginx, Apache and Tomcat
nginx's documentation:
http://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_protocols
Apache's documentation:
https://httpd.apache.org/docs/2.4/mod/mod_ssl.html#sslprotocol
Tomcat appears to be completely silent too.
The point of this PR is to make Gitea configurable and provide the guns for people to shoot themselves should they want to do so or rather need to do so. It also updates our defaults to match a better standard.
It very deliberately does not provide security advice. We do not have the expertise, time and patience to keep this up-to-date and if we start doing so we're absolutely going to have to keep it up-to-date.
Are we really going to get into the habit of providing this level of security advice in Gitea? Do you want to be responsible for keeping this advice up-to-date and dealing with the negative comments from people who determine that we've done this wrong or not to their standards?
Feel free to write the documentation for this and add the warnings you feel necessary but honestly I think we would be better to be silent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well - then I will say the magic words.
"Let it be", I do personally have a hard time with providing options to the user that seriously weaken the security. I do get that their are legitimate reasons for using TLSv1.0 and TLSv1.1 - Given that the option is only be done after you explicitly typed the version, I think that should be fine (but it's for me personally on the edge).
The security advice that can be given possibly is widely accepted within the security community - In theoretical sense something like this could be given: "Changing this setting can have impact on the security of gitea, please read about what the TLS versions means over at: link". Making the user aware shouldn't do any harm(while like you say, people can and will disagree).
If in the future such issue or discussion come up, feel free, better I will insist that someone ping's me. I gladly will take part into such discussion.
The configuring of the SSL chipher suite is good - The question if we should warn users can possibly be done on another PR. Let's get this PR rolled into master.