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

TLS config: Enable selection of min TLS version #375

Merged
merged 2 commits into from Apr 19, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
72 changes: 52 additions & 20 deletions config/http_config.go
Expand Up @@ -36,25 +36,58 @@ import (
"gopkg.in/yaml.v2"
)

// DefaultHTTPClientConfig is the default HTTP client configuration.
var DefaultHTTPClientConfig = HTTPClientConfig{
FollowRedirects: true,
EnableHTTP2: true,
}
var (
// DefaultHTTPClientConfig is the default HTTP client configuration.
DefaultHTTPClientConfig = HTTPClientConfig{
FollowRedirects: true,
EnableHTTP2: true,
}

// defaultHTTPClientOptions holds the default HTTP client options.
var defaultHTTPClientOptions = httpClientOptions{
keepAlivesEnabled: true,
http2Enabled: true,
// 5 minutes is typically above the maximum sane scrape interval. So we can
// use keepalive for all configurations.
idleConnTimeout: 5 * time.Minute,
}
// defaultHTTPClientOptions holds the default HTTP client options.
defaultHTTPClientOptions = httpClientOptions{
keepAlivesEnabled: true,
http2Enabled: true,
// 5 minutes is typically above the maximum sane scrape interval. So we can
// use keepalive for all configurations.
idleConnTimeout: 5 * time.Minute,
}
)

type closeIdler interface {
CloseIdleConnections()
}

type TLSVersion uint16

var TLSVersions = map[string]TLSVersion{
"TLS13": (TLSVersion)(tls.VersionTLS13),
"TLS12": (TLSVersion)(tls.VersionTLS12),
"TLS11": (TLSVersion)(tls.VersionTLS11),
"TLS10": (TLSVersion)(tls.VersionTLS10),
}

func (tv *TLSVersion) UnmarshalYAML(unmarshal func(interface{}) error) error {
Nexucis marked this conversation as resolved.
Show resolved Hide resolved
var s string
err := unmarshal((*string)(&s))
if err != nil {
return err
}
if v, ok := TLSVersions[s]; ok {
*tv = v
return nil
}
return fmt.Errorf("unknown TLS version: %s", s)
}

func (tv *TLSVersion) MarshalYAML() (interface{}, error) {
for s, v := range TLSVersions {
if *tv == v {
return s, nil
}
}
return fmt.Sprintf("%v", tv), nil
}

// BasicAuth contains basic HTTP authentication credentials.
type BasicAuth struct {
Username string `yaml:"username" json:"username"`
Expand Down Expand Up @@ -669,7 +702,10 @@ func cloneRequest(r *http.Request) *http.Request {

// NewTLSConfig creates a new tls.Config from the given TLSConfig.
func NewTLSConfig(cfg *TLSConfig) (*tls.Config, error) {
tlsConfig := &tls.Config{InsecureSkipVerify: cfg.InsecureSkipVerify}
tlsConfig := &tls.Config{
InsecureSkipVerify: cfg.InsecureSkipVerify,
MinVersion: uint16(cfg.MinVersion),
}

// If a CA cert is provided then let's read it in so we can validate the
// scrape target's certificate properly.
Expand Down Expand Up @@ -714,6 +750,8 @@ type TLSConfig struct {
ServerName string `yaml:"server_name,omitempty" json:"server_name,omitempty"`
// Disable target certificate validation.
InsecureSkipVerify bool `yaml:"insecure_skip_verify" json:"insecure_skip_verify"`
// Minimum TLS version.
MinVersion TLSVersion `yaml:"min_version,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

it is missing here the json tag

Suggested change
MinVersion TLSVersion `yaml:"min_version,omitempty"`
MinVersion TLSVersion `yaml:"min_version,omitempty" json:"min_version,omitempty"`

Copy link
Member

Choose a reason for hiding this comment

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

if MinVersion is not mandatory, don't you want to provide a default value ?

You could do it by overriding the Unmarshal's method.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I want to use go default. I think we should let them define the default while providing our users a way to divert if really needed.

Copy link
Member

Choose a reason for hiding this comment

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

mmm ok. And I guess in Prometheus you will define that the default / Minimum version is the previous one used, right ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, the plan is to mark it as a CHANGE in the changelog.

I think out users will benefit from defaults that go runtime provides. We should probably not make the call.

Possibly it can break some users, but they have the option to opt-out this change by explicitly setting tls1.0 as supported version.

Copy link
Member

Choose a reason for hiding this comment

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

I can agree on this assumption and I agree it's better to use the default tls version supported by Go.
I'm just wondering how much it will impact the users.
Let's see if someone answers to your email.

}

// SetDirectory joins any relative file paths with dir.
Expand All @@ -726,12 +764,6 @@ func (c *TLSConfig) SetDirectory(dir string) {
c.KeyFile = JoinDir(dir, c.KeyFile)
}

// UnmarshalYAML implements the yaml.Unmarshaler interface.
func (c *TLSConfig) UnmarshalYAML(unmarshal func(interface{}) error) error {
type plain TLSConfig
return unmarshal((*plain)(c))
}

// getClientCertificate reads the pair of client cert and key from disk and returns a tls.Certificate.
func (c *TLSConfig) getClientCertificate(*tls.CertificateRequestInfo) (*tls.Certificate, error) {
cert, err := tls.LoadX509KeyPair(c.CertFile, c.KeyFile)
Expand Down
6 changes: 4 additions & 2 deletions config/http_config_test.go
Expand Up @@ -627,7 +627,8 @@ func TestTLSConfig(t *testing.T) {
CertFile: ClientCertificatePath,
KeyFile: ClientKeyNoPassPath,
ServerName: "localhost",
InsecureSkipVerify: false}
InsecureSkipVerify: false,
}

tlsCAChain, err := ioutil.ReadFile(TLSCAChainPath)
if err != nil {
Expand All @@ -640,7 +641,8 @@ func TestTLSConfig(t *testing.T) {
expectedTLSConfig := &tls.Config{
RootCAs: rootCAs,
ServerName: configTLSConfig.ServerName,
InsecureSkipVerify: configTLSConfig.InsecureSkipVerify}
InsecureSkipVerify: configTLSConfig.InsecureSkipVerify,
}

tlsConfig, err := NewTLSConfig(&configTLSConfig)
if err != nil {
Expand Down
1 change: 1 addition & 0 deletions config/testdata/tls_config.tlsversion.good.yml
@@ -0,0 +1 @@
min_version: TLS11
3 changes: 3 additions & 0 deletions config/tls_config_test.go
Expand Up @@ -45,6 +45,9 @@ var expectedTLSConfigs = []struct {
}, {
filename: "tls_config.insecure.good.yml",
config: &tls.Config{InsecureSkipVerify: true},
}, {
filename: "tls_config.tlsversion.good.yml",
config: &tls.Config{MinVersion: tls.VersionTLS11},
},
}

Expand Down