Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the ability to specify the maximum acceptable TLS version #414

Merged
merged 9 commits into from Dec 8, 2022
1 change: 1 addition & 0 deletions .yamllint
Expand Up @@ -14,6 +14,7 @@ rules:
document-start: disable
indentation:
spaces: consistent
indent-sequences: consistent
key-duplicates:
ignore: |
config/testdata/section_key_dup.bad.yml
Expand Down
4 changes: 2 additions & 2 deletions CODE_OF_CONDUCT.md
@@ -1,3 +1,3 @@
## Prometheus Community Code of Conduct
# Prometheus Community Code of Conduct

Prometheus follows the [CNCF Code of Conduct](https://github.com/cncf/foundation/blob/master/code-of-conduct.md).
Prometheus follows the [CNCF Code of Conduct](https://github.com/cncf/foundation/blob/main/code-of-conduct.md).
2 changes: 1 addition & 1 deletion SECURITY.md
Expand Up @@ -3,4 +3,4 @@
The Prometheus security policy, including how to report vulnerabilities, can be
found here:

https://prometheus.io/docs/operating/security/
<https://prometheus.io/docs/operating/security/>
4 changes: 4 additions & 0 deletions config/http_config.go
Expand Up @@ -21,6 +21,7 @@ import (
"crypto/x509"
"encoding/json"
"fmt"
"io/ioutil"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was recently fixed, this branch needs to be synced with main

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I resynced and removed the ioutil import in 9d070c0

"net"
"net/http"
"net/url"
Expand Down Expand Up @@ -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),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a9a674f will generate an error if max and min version are both set and max is less than min. I added a bunch of tests in 5977368. Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any guarantees that the underlying lib will follow the order ? I would let the underlying tls lib decide what to do.

}

// If a CA cert is provided then let's read it in so we can validate the
Expand Down Expand Up @@ -826,6 +828,8 @@ type TLSConfig struct {
InsecureSkipVerify bool `yaml:"insecure_skip_verify" json:"insecure_skip_verify"`
// Minimum TLS version.
MinVersion TLSVersion `yaml:"min_version,omitempty" json:"min_version,omitempty"`
// Maximum TLS version.
MaxVersion TLSVersion `yaml:"max_version,omitempty" json:"max_version,omitempty"`
}

// SetDirectory joins any relative file paths with dir.
Expand Down
1 change: 1 addition & 0 deletions config/http_config_test.go
Expand Up @@ -21,6 +21,7 @@ import (
"errors"
"fmt"
"io"
"io/ioutil"
"net"
"net/http"
"net/http/httptest"
Expand Down
1 change: 1 addition & 0 deletions config/testdata/tls_config.max_version.good.json
@@ -0,0 +1 @@
{"max_version": "TLS12"}
1 change: 1 addition & 0 deletions config/testdata/tls_config.max_version.good.yml
@@ -0,0 +1 @@
max_version: TLS12
6 changes: 6 additions & 0 deletions config/tls_config_test.go
Expand Up @@ -64,6 +64,9 @@ var expectedTLSConfigs = []struct {
}, {
filename: "tls_config.tlsversion.good.json",
config: &tls.Config{MinVersion: tls.VersionTLS11},
}, {
filename: "tls_config.max_version.good.json",
config: &tls.Config{MaxVersion: tls.VersionTLS12},
},
{
filename: "tls_config.empty.good.yml",
Expand All @@ -74,6 +77,9 @@ var expectedTLSConfigs = []struct {
}, {
filename: "tls_config.tlsversion.good.yml",
config: &tls.Config{MinVersion: tls.VersionTLS11},
}, {
filename: "tls_config.max_version.good.yml",
config: &tls.Config{MaxVersion: tls.VersionTLS12},
},
}

Expand Down