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
9 changes: 9 additions & 0 deletions config/http_config.go
Expand Up @@ -779,6 +779,13 @@ 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 cfg.MaxVersion != 0 && cfg.MinVersion != 0 {
if cfg.MaxVersion < cfg.MinVersion {
return nil, fmt.Errorf("tls_config.max_version must be greater than or equal to tls_config.min_version if both are specified")
}
}

// If a CA cert is provided then let's read it in so we can validate the
Expand Down Expand Up @@ -826,6 +833,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
2 changes: 2 additions & 0 deletions config/testdata/tls_config.max_and_min_version.bad.json
@@ -0,0 +1,2 @@
{"max_version": "TLS11",
"min_version": "TLS12"}
2 changes: 2 additions & 0 deletions config/testdata/tls_config.max_and_min_version.bad.yml
@@ -0,0 +1,2 @@
max_version: TLS11
min_version: TLS12
2 changes: 2 additions & 0 deletions config/testdata/tls_config.max_and_min_version.good.json
@@ -0,0 +1,2 @@
{"max_version": "TLS12",
"min_version": "TLS11"}
2 changes: 2 additions & 0 deletions config/testdata/tls_config.max_and_min_version.good.yml
@@ -0,0 +1,2 @@
max_version: TLS12
min_version: TLS11
2 changes: 2 additions & 0 deletions config/testdata/tls_config.max_and_min_version_same.good.json
@@ -0,0 +1,2 @@
{"max_version": "TLS12",
"min_version": "TLS12"}
2 changes: 2 additions & 0 deletions config/testdata/tls_config.max_and_min_version_same.good.yml
@@ -0,0 +1,2 @@
max_version: TLS12
min_version: TLS12
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
36 changes: 36 additions & 0 deletions config/tls_config_test.go
Expand Up @@ -20,6 +20,7 @@ import (
"os"
"path/filepath"
"reflect"
"strings"
"testing"

"encoding/json"
Expand Down Expand Up @@ -64,6 +65,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 +78,15 @@ 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},
}, {
filename: "tls_config.max_and_min_version.good.yml",
config: &tls.Config{MaxVersion: tls.VersionTLS12, MinVersion: tls.VersionTLS11},
}, {
filename: "tls_config.max_and_min_version_same.good.yml",
config: &tls.Config{MaxVersion: tls.VersionTLS12, MinVersion: tls.VersionTLS12},
},
}

Expand All @@ -91,6 +104,29 @@ func TestValidTLSConfig(t *testing.T) {
}
}

var invalidTLSConfigs = []struct {
filename string
errMsg string
}{
{
filename: "tls_config.max_and_min_version.bad.yml",
errMsg: "tls_config.max_version must be greater than or equal to tls_config.min_version if both are specified",
},
}

func TestInvalidTLSConfig(t *testing.T) {
for _, ee := range invalidTLSConfigs {
_, err := LoadTLSConfig("testdata/" + ee.filename)
if err == nil {
t.Error("Expected error with config but got none")
continue
}
if !strings.Contains(err.Error(), ee.errMsg) {
t.Errorf("Expected error for invalid HTTP client configuration to contain %q but got: %s", ee.errMsg, err)
}
}
}

func TestStringer(t *testing.T) {
if s := (TLSVersion)(tls.VersionTLS13); s.String() != "TLS13" {
t.Fatalf("tls.VersionTLS13 string should be TLS13, got %s", s.String())
Expand Down