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

support tls 1.3 #7325

Merged
merged 4 commits into from
Feb 19, 2020
Merged

support tls 1.3 #7325

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5eff39b
support tls 1.3
hanshasselberg Feb 19, 2020
cf216af
compute
hanshasselberg Feb 19, 2020
031c9ee
test
hanshasselberg Feb 19, 2020
9cb4782
better tests
hanshasselberg Feb 19, 2020
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
3 changes: 2 additions & 1 deletion agent/config/runtime.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1423,7 +1423,8 @@ type RuntimeConfig struct {
TLSCipherSuites []uint16

// TLSMinVersion is used to set the minimum TLS version used for TLS
// connections. Should be either "tls10", "tls11", or "tls12".
// connections. Should be either "tls10", "tls11", "tls12" or "tls13".
// Defaults to tls12.
//
// hcl: tls_min_version = string
TLSMinVersion string
Expand Down
6 changes: 5 additions & 1 deletion tlsutil/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,12 @@ var TLSLookup = map[string]uint16{
"tls10": tls.VersionTLS10,
"tls11": tls.VersionTLS11,
"tls12": tls.VersionTLS12,
"tls13": tls.VersionTLS13,
}

// TLSVersions has all the keys from the map above, make sure to keep both in sync
var TLSVersions = "tls10, tls11, tls12, tls13"
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we instead use an init func and programmatically generate this?

Copy link
Member Author

@hanshasselberg hanshasselberg Feb 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Good catch! I went with a normal function instead. But the idea is the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to have a function iterating over the keys again and again. And then I had a var with []string, but given we only ever join, I figured it could be a string as well in the first place.

Copy link
Member Author

Choose a reason for hiding this comment

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

And I think it is better now because both vars are colocated and it is easier to spot the dependency than before.

Copy link
Member

Choose a reason for hiding this comment

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

var TLSVersions string
func init() {
   var versions []string
   for version, _ := range TLSLookup {
      versions = append(versions, version)
   }
   TLSVersions = strings.Join(versions, ", ")
}

Copy link
Member

Choose a reason for hiding this comment

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

That should do a one time generation of the string. But like I said, it doesn't really matter. I doubt there will be a tls 1.4 any time soon. Its sort of like the copyright thing. You might change it once a year so is it worth keeping the code around rather than just updating it very rarely?


// Config used to create tls.Config
type Config struct {
// VerifyIncoming is used to verify the authenticity of incoming
Expand Down Expand Up @@ -323,7 +327,7 @@ func (c *Configurator) check(config Config, pool *x509.CertPool, cert *tls.Certi
// Check if a minimum TLS version was set
if config.TLSMinVersion != "" {
if _, ok := TLSLookup[config.TLSMinVersion]; !ok {
return fmt.Errorf("TLSMinVersion: value %s not supported, please specify one of [tls10,tls11,tls12]", config.TLSMinVersion)
return fmt.Errorf("TLSMinVersion: value %s not supported, please specify one of [%s]", config.TLSMinVersion, TLSVersions)
}
}

Expand Down
3 changes: 2 additions & 1 deletion tlsutil/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,7 @@ func TestConfigurator_ErrorPropagation(t *testing.T) {
{Config{CertFile: "bogus", KeyFile: "bogus"}, true, true}, // 20
{Config{CAFile: "bogus"}, true, true}, // 21
{Config{CAPath: "bogus"}, true, true}, // 22
{Config{TLSMinVersion: "tls13"}, false, false}, // 23
}

c := Configurator{autoEncrypt: &autoEncrypt{}, manual: &manual{}}
Expand Down Expand Up @@ -590,7 +591,7 @@ func TestConfigurator_CommonTLSConfigTLSMinVersion(t *testing.T) {
require.NoError(t, err)
require.Equal(t, c.commonTLSConfig(false).MinVersion, TLSLookup["tls10"])

tlsVersions := []string{"tls10", "tls11", "tls12"}
tlsVersions := []string{"tls10", "tls11", "tls12", "tls13"}
for _, version := range tlsVersions {
require.NoError(t, c.Update(Config{TLSMinVersion: version}))
require.Equal(t, c.commonTLSConfig(false).MinVersion,
Expand Down
4 changes: 2 additions & 2 deletions website/source/docs/agent/options.html.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1830,8 +1830,8 @@ to the old fragment -->
facility messages are sent. By default, `LOCAL0` will be used.

* <a name="tls_min_version"></a><a href="#tls_min_version">`tls_min_version`</a> Added in Consul
0.7.4, this specifies the minimum supported version of TLS. Accepted values are "tls10", "tls11"
or "tls12". This defaults to "tls12". WARNING: TLS 1.1 and lower are generally considered less
0.7.4, this specifies the minimum supported version of TLS. Accepted values are "tls10", "tls11",
"tls12", or "tls13". This defaults to "tls12". WARNING: TLS 1.1 and lower are generally considered less
secure; avoid using these if possible.

* <a name="tls_cipher_suites"></a><a href="#tls_cipher_suites">`tls_cipher_suites`</a> Added in Consul
Expand Down