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.

Sign up for GitHub

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

Fixing incorrect default TLS min and TLS max versions #5480

Merged
merged 6 commits into from
Jun 27, 2022
Merged

Fixing incorrect default TLS min and TLS max versions #5480

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1231434
Fixing incorrect TLS min and TLS max Versions by default
bushwhackr Jun 7, 2022
ecb7cb0
Added test cases for TLS min and TLS max versions
bushwhackr Jun 7, 2022
486bdce
Added entry to changelog
bushwhackr Jun 7, 2022
f6b19f5
Added Invalid TLS test cases
bushwhackr Jun 8, 2022
5158851
TLS MinVersion and TLS MaxVersion defaults to be handled by crypto/tls
bushwhackr Jun 9, 2022
d67c723
Removed description that can go out of sync
bushwhackr Jun 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
TLS MinVersion and TLS MaxVersion defaults to be handled by crypto/tls
  • Loading branch information
@bushwhackr
bushwhackr committed Jun 23, 2022
commit 51588515369ba8af28b297423b701f2fdd6baa83
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@

- Fixes the "service.version" label value for internal metrics, always was "latest" in core/contrib distros. (#5449).
- Send correct batch stats when SendBatchMaxSize is set (#5385)
- Sets correct default TLS MinVersion and MaxVersion (#5480)
- TLS `MinVersion` and `MaxVersion` defaults will be handled by `crypto/tls` (#5480)

## v0.52.0 Beta

Expand Down
7 changes: 4 additions & 3 deletions config/configtls/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,11 @@ won't use TLS at all.

Minimum and maximum TLS version can be set:

- `min_version` (default = "1.2"): Minimum acceptable TLS version.
It's recommended to use at least 1.2 as the minimum version.
- `min_version` (default = "" handled by [crypto/tls](https://github.com/golang/go/blob/master/src/crypto/tls/common.go#L694)): Minimum acceptable TLS version.
Copy link
Member

Choose a reason for hiding this comment

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

As I understood it, 1.0 is used as the default version on the server-side. This is not what we want here, on a newer server/library: the recommendation for a few years has been already to default to 1.2 as the minimum version, allowing users to use 1.0 or SSLvX only when absolutely needed.

- options: ["1.0", "1.1", "1.2", "1.3"]

- `max_version` (default = "1.3"): Maximum acceptable TLS version.
- `max_version` (default = "" handled by [crypto/tls](https://github.com/golang/go/blob/master/src/crypto/tls/common.go#L700)): Maximum acceptable TLS version.
- options: ["1.0", "1.1", "1.2", "1.3"]

Additionally certifaces may be reloaded by setting the below configuration.

Expand Down
21 changes: 8 additions & 13 deletions config/configtls/configtls.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,13 @@ type TLSSetting struct {
KeyFile string `mapstructure:"key_file"`

// MinVersion sets the minimum TLS version that is acceptable.
// If not set, TLS 1.0 is used. (optional)
// If not set, refer to crypto/tls for defaults. (optional)
// Which is TLS 1.2 for clients and TLS 1.0 for servers
bushwhackr marked this conversation as resolved.
Show resolved Hide resolved
MinVersion string `mapstructure:"min_version"`

// MaxVersion sets the maximum TLS version that is acceptable.
// If not set, TLS 1.3 is used. (optional)
// If not set, refer to crypto/tls for defaults. (optional)
// Which is currently TLS 1.3.
MaxVersion string `mapstructure:"max_version"`

// ReloadInterval specifies the duration after which the certificate will be reloaded
Expand Down Expand Up @@ -176,14 +178,6 @@ func (c TLSSetting) loadTLSConfig() (*tls.Config, error) {
getClientCertificate = func(cri *tls.CertificateRequestInfo) (*tls.Certificate, error) { return certReloader.GetCertificate() }
}

// Setting default TLS minVersion
if c.MinVersion == "" {
c.MinVersion = "1.0"
}
// Setting default TLS maxVersion
if c.MaxVersion == "" {
c.MaxVersion = "1.3"
}
minTLS, err := convertVersion(c.MinVersion)
if err != nil {
return nil, fmt.Errorf("invalid TLS min_version: %w", err)
Expand All @@ -192,9 +186,6 @@ func (c TLSSetting) loadTLSConfig() (*tls.Config, error) {
if err != nil {
return nil, fmt.Errorf("invalid TLS max_version: %w", err)
}
if minTLS > maxTLS {
return nil, fmt.Errorf("tls min_version is greater than tls max_version: %s > %s", c.MinVersion, c.MaxVersion)
}

return &tls.Config{
RootCAs: certPool,
Expand Down Expand Up @@ -251,6 +242,10 @@ func (c TLSServerSetting) LoadTLSConfig() (*tls.Config, error) {
}

func convertVersion(v string) (uint16, error) {
// Defaults will be handled by go/crypto/tls
if v == "" {
return 0, nil
}
val, ok := tlsVersions[v]
if !ok {
return 0, fmt.Errorf("unsupported TLS version: %q", v)
Expand Down
13 changes: 7 additions & 6 deletions config/configtls/configtls_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,16 +361,17 @@ func TestMinMaxTLSVersions(t *testing.T) {
outMaxVersion uint16
errorTxt string
}{
{name: `TLS Config ["", ""] to give [TLS1.0, TLS1.3]`, minVersion: "", maxVersion: "", outMinVersion: tls.VersionTLS10, outMaxVersion: tls.VersionTLS13},
{name: `TLS Config ["", "1.3"] to give [TLS1.0, TLS1.3]`, minVersion: "", maxVersion: "1.3", outMinVersion: tls.VersionTLS10, outMaxVersion: tls.VersionTLS13},
{name: `TLS Config ["", ""] to give [0, 0]`, minVersion: "", maxVersion: "", outMinVersion: 0, outMaxVersion: 0},
{name: `TLS Config ["", "1.3"] to give [0, TLS1.3]`, minVersion: "", maxVersion: "1.3", outMinVersion: 0, outMaxVersion: tls.VersionTLS13},
{name: `TLS Config ["1.2", ""] to give [TLS1.2, 0]`, minVersion: "1.2", maxVersion: "", outMinVersion: tls.VersionTLS12, outMaxVersion: 0},
{name: `TLS Config ["1.3", "1.3"] to give [TLS1.3, TLS1.3]`, minVersion: "1.3", maxVersion: "1.3", outMinVersion: tls.VersionTLS13, outMaxVersion: tls.VersionTLS13},
{name: `TLS Config ["1.0", "1.0"] to give [TLS1.0, TLS1.0]`, minVersion: "1.0", maxVersion: "1.0", outMinVersion: tls.VersionTLS10, outMaxVersion: tls.VersionTLS10},
{name: `TLS Config ["1.2", ""] to give [TLS1.2, TLS1.3]`, minVersion: "1.2", maxVersion: "", outMinVersion: tls.VersionTLS12, outMaxVersion: tls.VersionTLS13},
{name: `TLS Config ["1.3", "1.2"] to give [Error]`, minVersion: "1.3", maxVersion: "1.2", errorTxt: "tls min_version is greater than tls max_version: 1.3 > 1.2"},
{name: `TLS Config ["1.2", "1.0"] to give [Error]`, minVersion: "1.2", maxVersion: "1.0", errorTxt: "tls min_version is greater than tls max_version: 1.2 > 1.0"},
{name: `TLS Config ["1.0", "1.1"] to give [TLS1.0, TLS1.1]`, minVersion: "1.0", maxVersion: "1.1", outMinVersion: tls.VersionTLS10, outMaxVersion: tls.VersionTLS11},
{name: `TLS Config ["asd", ""] to give [Error]`, minVersion: "asd", maxVersion: "", errorTxt: `invalid TLS min_version: unsupported TLS version: "asd"`},
{name: `TLS Config ["", "asd"] to give [Error]`, minVersion: "", maxVersion: "asd", errorTxt: `invalid TLS max_version: unsupported TLS version: "asd"`},
{name: `TLS Config ["0.4", ""] to give [Error]`, minVersion: "0.4", maxVersion: "", errorTxt: `invalid TLS min_version: unsupported TLS version: "0.4"`},

// Allowing this, however, expecting downstream TLS handshake will throw an error
{name: `TLS Config ["1.2", "1.1"] to give [TLS1.2, TLS1.1]`, minVersion: "1.2", maxVersion: "1.1", outMinVersion: tls.VersionTLS12, outMaxVersion: tls.VersionTLS11},
}

for _, test := range tests {
Expand Down