Add the ability to specify the maximum acceptable TLS version #414
Conversation
Signed-off-by: prombot <prometheus-team@googlegroups.com> Signed-off-by: prombot <prometheus-team@googlegroups.com> Signed-off-by: Lyas Spiehler <lspiehler@gmail.com>
Signed-off-by: Lyas Spiehler <lspiehler@gmail.com>
Signed-off-by: Lyas Spiehler <lspiehler@gmail.com>
|
What is the motivation here? We did not enable that because it seems like bad practice to limit the max tls version and we want to encourage the use of TLS 1.3. |
|
I'm trying to monitor certificates using the blackbox_exporter on servers running jetty web servers with support for TLS 1.2 and 1.3. I'm getting the error "remote error: tls: handshake failure" when the go TLS client attempts to connect using TLS 1.3. These are vendor managed servers and I have no access to the server side. I've found I can successfully complete the TLS handshake if I add "MaxVersion: tls.VersionTLS12" to my TLS config. Interestingly, I am able to use TLS 1.3 with OpenSSL using the command A similar issue was reported here https://stackoverflow.com/questions/41250665/go-https-client-issue-remote-error-tls-handshake-failure. Their issue was with TLS 1.2, but they used the same solution. I haven't found another way to get the handshake to succeed without forcing a specific TLS version using "MaxVersion." Thank you! |
|
Okay, I think it's reasonable to add. However, this pull request appears to be a noop I think and does not achieve its goal. Would you add some tests? thanks |
Signed-off-by: Lyas Spiehler <lspiehler@gmail.com>
Signed-off-by: Lyas Spiehler <lspiehler@gmail.com>
config/http_config.go
Outdated
| @@ -21,6 +21,7 @@ import ( | |||
| "crypto/x509" | |||
| "encoding/json" | |||
| "fmt" | |||
| "io/ioutil" | |||
There was a problem hiding this comment.
this was recently fixed, this branch needs to be synced with main
There was a problem hiding this comment.
I resynced and removed the ioutil import in 9d070c0
| @@ -779,6 +780,7 @@ func NewTLSConfig(cfg *TLSConfig) (*tls.Config, error) { | |||
| tlsConfig := &tls.Config{ | |||
| InsecureSkipVerify: cfg.InsecureSkipVerify, | |||
| MinVersion: uint16(cfg.MinVersion), | |||
| MaxVersion: uint16(cfg.MaxVersion), | |||
There was a problem hiding this comment.
This is fine.
What I'm thinking is that we (maybe) should validate the following:
- Both unset is fine
- One set is fine
- Both set should enforce MaxVersion >= MinVersion
I haven't followed all the use locations of the code, but if we don't check for this in NewTLSConfig, I think we might end up with some weird errors later.
There was a problem hiding this comment.
Is there any guarantees that the underlying lib will follow the order ? I would let the underlying tls lib decide what to do.
Signed-off-by: Lyas Spiehler <lspiehler@gmail.com>
… is greater than or equal to minversion Signed-off-by: Lyas Spiehler <lspiehler@gmail.com>
Signed-off-by: Lyas Spiehler <lspiehler@gmail.com>
|
Thanks! |
- Check if TLS certificate and key file have been modified prometheus/common#345 - Add the ability to specify the maximum acceptable TLS version prometheus/common#414 Signed-off-by: Julien Pivotto <roidelapluie@o11y.eu>
…heus#414) Signed-off-by: Lyas Spiehler <lspiehler@gmail.com>
…heus#414) Signed-off-by: Lyas Spiehler <lspiehler@gmail.com>
…heus#414) Signed-off-by: Lyas Spiehler <lspiehler@gmail.com>
…heus#414) Signed-off-by: Lyas Spiehler <lspiehler@gmail.com>
Adding the ability to specify the maximum acceptable TLS version to granularly force a specific version. MaxVersion is documented in the TLS Config struct here https://pkg.go.dev/crypto/tls.