From d881d63b4bfb6c7a470a5c293866e5f451710c5f Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Wed, 27 Feb 2019 20:43:43 +0100 Subject: [PATCH 01/31] copy config --- agent/config/runtime.go | 4 +- agent/consul/config.go | 4 +- tlsutil/config.go | 12 ++-- tlsutil/config_test.go | 138 +++++++++++++++++++--------------------- 4 files changed, 74 insertions(+), 84 deletions(-) diff --git a/agent/config/runtime.go b/agent/config/runtime.go index 0596034f648b..c0ea6ce8f0bb 100644 --- a/agent/config/runtime.go +++ b/agent/config/runtime.go @@ -1580,8 +1580,8 @@ func (c *RuntimeConfig) Sanitized() map[string]interface{} { return sanitize("rt", reflect.ValueOf(c)).Interface().(map[string]interface{}) } -func (c *RuntimeConfig) ToTLSUtilConfig() *tlsutil.Config { - return &tlsutil.Config{ +func (c *RuntimeConfig) ToTLSUtilConfig() tlsutil.Config { + return tlsutil.Config{ VerifyIncoming: c.VerifyIncoming, VerifyIncomingRPC: c.VerifyIncomingRPC, VerifyIncomingHTTPS: c.VerifyIncomingHTTPS, diff --git a/agent/consul/config.go b/agent/consul/config.go index 4f8146e44db8..2a7066443452 100644 --- a/agent/consul/config.go +++ b/agent/consul/config.go @@ -379,8 +379,8 @@ type Config struct { CAConfig *structs.CAConfiguration } -func (c *Config) ToTLSUtilConfig() *tlsutil.Config { - return &tlsutil.Config{ +func (c *Config) ToTLSUtilConfig() tlsutil.Config { + return tlsutil.Config{ VerifyIncoming: c.VerifyIncoming, VerifyOutgoing: c.VerifyOutgoing, CAFile: c.CAFile, diff --git a/tlsutil/config.go b/tlsutil/config.go index 19d08aad3e5d..57f02674168f 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -205,26 +205,22 @@ type Configurator struct { // configuration. // Todo (Hans): should config be a value instead a pointer to avoid side // effects? -func NewConfigurator(config *Config) *Configurator { - return &Configurator{base: config, checks: map[string]bool{}} +func NewConfigurator(config Config) *Configurator { + return &Configurator{base: &config, checks: map[string]bool{}} } // Update updates the internal configuration which is used to generate // *tls.Config. -func (c *Configurator) Update(config *Config) { +func (c *Configurator) Update(config Config) { c.Lock() defer c.Unlock() - c.base = config + c.base = &config } // commonTLSConfig generates a *tls.Config from the base configuration the // Configurator has. It accepts an additional flag in case a config is needed // for incoming TLS connections. func (c *Configurator) commonTLSConfig(additionalVerifyIncomingFlag bool) (*tls.Config, error) { - if c.base == nil { - return nil, fmt.Errorf("No base config") - } - tlsConfig := &tls.Config{ InsecureSkipVerify: !c.base.VerifyServerHostname, } diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index de213c0def47..c0a970f7238b 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -40,7 +40,7 @@ func TestConfig_KeyPair_Valid(t *testing.T) { } func TestConfigurator_OutgoingTLS_MissingCA(t *testing.T) { - conf := &Config{ + conf := Config{ VerifyOutgoing: true, } c := NewConfigurator(conf) @@ -50,7 +50,7 @@ func TestConfigurator_OutgoingTLS_MissingCA(t *testing.T) { } func TestConfigurator_OutgoingTLS_OnlyCA(t *testing.T) { - conf := &Config{ + conf := Config{ CAFile: "../test/ca/root.cer", } c := NewConfigurator(conf) @@ -60,7 +60,7 @@ func TestConfigurator_OutgoingTLS_OnlyCA(t *testing.T) { } func TestConfigurator_OutgoingTLS_VerifyOutgoing(t *testing.T) { - conf := &Config{ + conf := Config{ VerifyOutgoing: true, CAFile: "../test/ca/root.cer", } @@ -73,8 +73,8 @@ func TestConfigurator_OutgoingTLS_VerifyOutgoing(t *testing.T) { require.True(t, tlsConf.InsecureSkipVerify) } -func TestConfigurator_OutgoingRPC_ServerName(t *testing.T) { - conf := &Config{ +func TestConfigurator_OutgoingTLS_ServerName(t *testing.T) { + conf := Config{ VerifyOutgoing: true, CAFile: "../test/ca/root.cer", ServerName: "consul.example.com", @@ -89,7 +89,7 @@ func TestConfigurator_OutgoingRPC_ServerName(t *testing.T) { } func TestConfigurator_OutgoingTLS_VerifyHostname(t *testing.T) { - conf := &Config{ + conf := Config{ VerifyOutgoing: true, VerifyServerHostname: true, CAFile: "../test/ca/root.cer", @@ -103,7 +103,7 @@ func TestConfigurator_OutgoingTLS_VerifyHostname(t *testing.T) { } func TestConfigurator_OutgoingTLS_WithKeyPair(t *testing.T) { - conf := &Config{ + conf := Config{ VerifyOutgoing: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", @@ -120,7 +120,7 @@ func TestConfigurator_OutgoingTLS_WithKeyPair(t *testing.T) { func TestConfigurator_OutgoingTLS_TLSMinVersion(t *testing.T) { tlsVersions := []string{"tls10", "tls11", "tls12"} for _, version := range tlsVersions { - conf := &Config{ + conf := Config{ VerifyOutgoing: true, CAFile: "../test/ca/root.cer", TLSMinVersion: version, @@ -136,7 +136,7 @@ func TestConfigurator_OutgoingTLS_TLSMinVersion(t *testing.T) { func startTLSServer(config *Config) (net.Conn, chan error) { errc := make(chan error, 1) - c := NewConfigurator(config) + c := NewConfigurator(*config) tlsConfigServer, err := c.IncomingRPCConfig() if err != nil { errc <- err @@ -172,7 +172,7 @@ func startTLSServer(config *Config) (net.Conn, chan error) { } func TestConfigurator_outgoingWrapper_OK(t *testing.T) { - config := &Config{ + config := Config{ CAFile: "../test/hostname/CertAuth.crt", CertFile: "../test/hostname/Alice.crt", KeyFile: "../test/hostname/Alice.key", @@ -181,7 +181,7 @@ func TestConfigurator_outgoingWrapper_OK(t *testing.T) { Domain: "consul", } - client, errc := startTLSServer(config) + client, errc := startTLSServer(&config) if client == nil { t.Fatalf("startTLSServer err: %v", <-errc) } @@ -202,7 +202,7 @@ func TestConfigurator_outgoingWrapper_OK(t *testing.T) { } func TestConfigurator_outgoingWrapper_BadDC(t *testing.T) { - config := &Config{ + config := Config{ CAFile: "../test/hostname/CertAuth.crt", CertFile: "../test/hostname/Alice.crt", KeyFile: "../test/hostname/Alice.key", @@ -211,7 +211,7 @@ func TestConfigurator_outgoingWrapper_BadDC(t *testing.T) { Domain: "consul", } - client, errc := startTLSServer(config) + client, errc := startTLSServer(&config) if client == nil { t.Fatalf("startTLSServer err: %v", <-errc) } @@ -232,7 +232,7 @@ func TestConfigurator_outgoingWrapper_BadDC(t *testing.T) { } func TestConfigurator_outgoingWrapper_BadCert(t *testing.T) { - config := &Config{ + config := Config{ CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key", @@ -241,7 +241,7 @@ func TestConfigurator_outgoingWrapper_BadCert(t *testing.T) { Domain: "consul", } - client, errc := startTLSServer(config) + client, errc := startTLSServer(&config) if client == nil { t.Fatalf("startTLSServer err: %v", <-errc) } @@ -263,14 +263,14 @@ func TestConfigurator_outgoingWrapper_BadCert(t *testing.T) { } func TestConfigurator_wrapTLS_OK(t *testing.T) { - config := &Config{ + config := Config{ CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key", VerifyOutgoing: true, } - client, errc := startTLSServer(config) + client, errc := startTLSServer(&config) if client == nil { t.Fatalf("startTLSServer err: %v", <-errc) } @@ -298,7 +298,7 @@ func TestConfigurator_wrapTLS_BadCert(t *testing.T) { t.Fatalf("startTLSServer err: %v", <-errc) } - clientConfig := &Config{ + clientConfig := Config{ CAFile: "../test/ca/root.cer", VerifyOutgoing: true, } @@ -379,7 +379,7 @@ func TestConfig_ParseCiphers(t *testing.T) { } func TestConfigurator_IncomingHTTPSConfig_CA_PATH(t *testing.T) { - conf := &Config{CAPath: "../test/ca_path"} + conf := Config{CAPath: "../test/ca_path"} c := NewConfigurator(conf) tlsConf, err := c.IncomingHTTPSConfig() @@ -388,7 +388,7 @@ func TestConfigurator_IncomingHTTPSConfig_CA_PATH(t *testing.T) { } func TestConfigurator_IncomingHTTPS(t *testing.T) { - conf := &Config{ + conf := Config{ VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", @@ -404,7 +404,7 @@ func TestConfigurator_IncomingHTTPS(t *testing.T) { } func TestConfigurator_IncomingHTTPS_MissingCA(t *testing.T) { - conf := &Config{ + conf := Config{ VerifyIncoming: true, CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key", @@ -415,7 +415,7 @@ func TestConfigurator_IncomingHTTPS_MissingCA(t *testing.T) { } func TestConfigurator_IncomingHTTPS_MissingKey(t *testing.T) { - conf := &Config{ + conf := Config{ VerifyIncoming: true, CAFile: "../test/ca/root.cer", } @@ -425,7 +425,7 @@ func TestConfigurator_IncomingHTTPS_MissingKey(t *testing.T) { } func TestConfigurator_IncomingHTTPS_NoVerify(t *testing.T) { - conf := &Config{} + conf := Config{} c := NewConfigurator(conf) tlsConf, err := c.IncomingHTTPSConfig() require.NoError(t, err) @@ -438,7 +438,7 @@ func TestConfigurator_IncomingHTTPS_NoVerify(t *testing.T) { func TestConfigurator_IncomingHTTPS_TLSMinVersion(t *testing.T) { tlsVersions := []string{"tls10", "tls11", "tls12"} for _, version := range tlsVersions { - conf := &Config{ + conf := Config{ VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", @@ -455,29 +455,23 @@ func TestConfigurator_IncomingHTTPS_TLSMinVersion(t *testing.T) { func TestConfigurator_IncomingHTTPSCAPath_Valid(t *testing.T) { - c := NewConfigurator(&Config{CAPath: "../test/ca_path"}) + c := NewConfigurator(Config{CAPath: "../test/ca_path"}) tlsConf, err := c.IncomingHTTPSConfig() require.NoError(t, err) require.Len(t, tlsConf.ClientCAs.Subjects(), 2) } -func TestConfigurator_CommonTLSConfigNoBaseConfig(t *testing.T) { - c := NewConfigurator(nil) - _, err := c.commonTLSConfig(false) - require.Error(t, err) -} - func TestConfigurator_CommonTLSConfigServerNameNodeName(t *testing.T) { type variant struct { - config *Config + config Config result string } variants := []variant{ - {config: &Config{NodeName: "node", ServerName: "server"}, + {config: Config{NodeName: "node", ServerName: "server"}, result: "server"}, - {config: &Config{ServerName: "server"}, + {config: Config{ServerName: "server"}, result: "server"}, - {config: &Config{NodeName: "node"}, + {config: Config{NodeName: "node"}, result: "node"}, } for _, v := range variants { @@ -489,12 +483,12 @@ func TestConfigurator_CommonTLSConfigServerNameNodeName(t *testing.T) { } func TestConfigurator_CommonTLSConfigCipherSuites(t *testing.T) { - c := NewConfigurator(&Config{}) + c := NewConfigurator(Config{}) tlsConfig, err := c.commonTLSConfig(false) require.NoError(t, err) require.Empty(t, tlsConfig.CipherSuites) - conf := &Config{CipherSuites: []uint16{tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305}} + conf := Config{CipherSuites: []uint16{tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305}} c.Update(conf) tlsConfig, err = c.commonTLSConfig(false) require.NoError(t, err) @@ -502,16 +496,16 @@ func TestConfigurator_CommonTLSConfigCipherSuites(t *testing.T) { } func TestConfigurator_CommonTLSConfigCertKey(t *testing.T) { - c := NewConfigurator(&Config{}) + c := NewConfigurator(Config{}) tlsConf, err := c.commonTLSConfig(false) require.NoError(t, err) require.Empty(t, tlsConf.Certificates) - c.Update(&Config{CertFile: "/something/bogus", KeyFile: "/more/bogus"}) + c.Update(Config{CertFile: "/something/bogus", KeyFile: "/more/bogus"}) tlsConf, err = c.commonTLSConfig(false) require.Error(t, err) - c.Update(&Config{CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) + c.Update(Config{CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) tlsConf, err = c.commonTLSConfig(false) require.NoError(t, err) require.Len(t, tlsConf.Certificates, 1) @@ -520,51 +514,51 @@ func TestConfigurator_CommonTLSConfigCertKey(t *testing.T) { func TestConfigurator_CommonTLSConfigTLSMinVersion(t *testing.T) { tlsVersions := []string{"tls10", "tls11", "tls12"} for _, version := range tlsVersions { - c := NewConfigurator(&Config{TLSMinVersion: version}) + c := NewConfigurator(Config{TLSMinVersion: version}) tlsConf, err := c.commonTLSConfig(false) require.NoError(t, err) require.Equal(t, tlsConf.MinVersion, TLSLookup[version]) } - c := NewConfigurator(&Config{TLSMinVersion: "tlsBOGUS"}) + c := NewConfigurator(Config{TLSMinVersion: "tlsBOGUS"}) _, err := c.commonTLSConfig(false) require.Error(t, err) } func TestConfigurator_CommonTLSConfigValidateVerifyOutgoingCA(t *testing.T) { - c := NewConfigurator(&Config{VerifyOutgoing: true}) + c := NewConfigurator(Config{VerifyOutgoing: true}) _, err := c.commonTLSConfig(false) require.Error(t, err) } func TestConfigurator_CommonTLSConfigLoadCA(t *testing.T) { - c := NewConfigurator(&Config{}) + c := NewConfigurator(Config{}) tlsConf, err := c.commonTLSConfig(false) require.NoError(t, err) require.Nil(t, tlsConf.RootCAs) require.Nil(t, tlsConf.ClientCAs) - c.Update(&Config{CAFile: "/something/bogus"}) + c.Update(Config{CAFile: "/something/bogus"}) _, err = c.commonTLSConfig(false) require.Error(t, err) - c.Update(&Config{CAPath: "/something/bogus/"}) + c.Update(Config{CAPath: "/something/bogus/"}) _, err = c.commonTLSConfig(false) require.Error(t, err) - c.Update(&Config{CAFile: "../test/ca/root.cer"}) + c.Update(Config{CAFile: "../test/ca/root.cer"}) tlsConf, err = c.commonTLSConfig(false) require.NoError(t, err) require.Len(t, tlsConf.RootCAs.Subjects(), 1) require.Len(t, tlsConf.ClientCAs.Subjects(), 1) - c.Update(&Config{CAPath: "../test/ca_path"}) + c.Update(Config{CAPath: "../test/ca_path"}) tlsConf, err = c.commonTLSConfig(false) require.NoError(t, err) require.Len(t, tlsConf.RootCAs.Subjects(), 2) require.Len(t, tlsConf.ClientCAs.Subjects(), 2) - c.Update(&Config{CAFile: "../test/ca/root.cer", CAPath: "../test/ca_path"}) + c.Update(Config{CAFile: "../test/ca/root.cer", CAPath: "../test/ca_path"}) tlsConf, err = c.commonTLSConfig(false) require.NoError(t, err) require.Len(t, tlsConf.RootCAs.Subjects(), 1) @@ -572,29 +566,29 @@ func TestConfigurator_CommonTLSConfigLoadCA(t *testing.T) { } func TestConfigurator_CommonTLSConfigVerifyIncoming(t *testing.T) { - c := NewConfigurator(&Config{}) + c := NewConfigurator(Config{}) tlsConf, err := c.commonTLSConfig(false) require.NoError(t, err) require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) - c.Update(&Config{VerifyIncoming: true}) + c.Update(Config{VerifyIncoming: true}) tlsConf, err = c.commonTLSConfig(false) require.Error(t, err) - c.Update(&Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer"}) + c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer"}) tlsConf, err = c.commonTLSConfig(false) require.Error(t, err) - c.Update(&Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/cert/ourdomain.cer"}) + c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/cert/ourdomain.cer"}) tlsConf, err = c.commonTLSConfig(false) require.Error(t, err) - c.Update(&Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) + c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) tlsConf, err = c.commonTLSConfig(false) require.NoError(t, err) require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) - c.Update(&Config{VerifyIncoming: false, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) + c.Update(Config{VerifyIncoming: false, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) tlsConf, err = c.commonTLSConfig(true) require.NoError(t, err) require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) @@ -611,93 +605,93 @@ func TestConfigurator_CommonTLSConfigVerifyIncoming(t *testing.T) { } func TestConfigurator_IncomingRPCConfig(t *testing.T) { - c := NewConfigurator(&Config{}) + c := NewConfigurator(Config{}) tlsConf, err := c.IncomingRPCConfig() require.NoError(t, err) require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) - c.Update(&Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) + c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) tlsConf, err = c.IncomingRPCConfig() require.NoError(t, err) require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) - c.Update(&Config{VerifyIncomingRPC: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) + c.Update(Config{VerifyIncomingRPC: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) tlsConf, err = c.IncomingRPCConfig() require.NoError(t, err) require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) - c.Update(&Config{VerifyIncomingHTTPS: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) + c.Update(Config{VerifyIncomingHTTPS: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) tlsConf, err = c.IncomingRPCConfig() require.NoError(t, err) require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) } func TestConfigurator_IncomingHTTPSConfig(t *testing.T) { - c := NewConfigurator(&Config{}) + c := NewConfigurator(Config{}) tlsConf, err := c.IncomingHTTPSConfig() require.NoError(t, err) require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) - c.Update(&Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) + c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) tlsConf, err = c.IncomingHTTPSConfig() require.NoError(t, err) require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) - c.Update(&Config{VerifyIncomingHTTPS: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) + c.Update(Config{VerifyIncomingHTTPS: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) tlsConf, err = c.IncomingHTTPSConfig() require.NoError(t, err) require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) - c.Update(&Config{VerifyIncomingRPC: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) + c.Update(Config{VerifyIncomingRPC: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) tlsConf, err = c.IncomingHTTPSConfig() require.NoError(t, err) require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) } func TestConfigurator_OutgoingRPCConfig(t *testing.T) { - c := NewConfigurator(&Config{}) + c := NewConfigurator(Config{}) tlsConf, err := c.OutgoingRPCConfig() require.NoError(t, err) require.Nil(t, tlsConf) - c.Update(&Config{VerifyOutgoing: true}) + c.Update(Config{VerifyOutgoing: true}) tlsConf, err = c.OutgoingRPCConfig() require.Error(t, err) - c.Update(&Config{VerifyOutgoing: true, CAFile: "../test/ca/root.cer"}) + c.Update(Config{VerifyOutgoing: true, CAFile: "../test/ca/root.cer"}) tlsConf, err = c.OutgoingRPCConfig() require.NoError(t, err) - c.Update(&Config{VerifyOutgoing: true, CAPath: "../test/ca_path"}) + c.Update(Config{VerifyOutgoing: true, CAPath: "../test/ca_path"}) tlsConf, err = c.OutgoingRPCConfig() require.NoError(t, err) } func TestConfigurator_OutgoingTLSConfigForChecks(t *testing.T) { - c := NewConfigurator(&Config{}) + c := NewConfigurator(Config{}) tlsConf, err := c.OutgoingTLSConfigForCheck("") require.NoError(t, err) require.False(t, tlsConf.InsecureSkipVerify) - c.Update(&Config{EnableAgentTLSForChecks: true}) + c.Update(Config{EnableAgentTLSForChecks: true}) tlsConf, err = c.OutgoingTLSConfigForCheck("") require.NoError(t, err) require.False(t, tlsConf.InsecureSkipVerify) c.AddCheck("c1", true) - c.Update(&Config{EnableAgentTLSForChecks: true}) + c.Update(Config{EnableAgentTLSForChecks: true}) tlsConf, err = c.OutgoingTLSConfigForCheck("c1") require.NoError(t, err) require.True(t, tlsConf.InsecureSkipVerify) c.AddCheck("c1", false) - c.Update(&Config{EnableAgentTLSForChecks: true}) + c.Update(Config{EnableAgentTLSForChecks: true}) tlsConf, err = c.OutgoingTLSConfigForCheck("c1") require.NoError(t, err) require.False(t, tlsConf.InsecureSkipVerify) c.AddCheck("c1", false) - c.Update(&Config{EnableAgentTLSForChecks: true}) + c.Update(Config{EnableAgentTLSForChecks: true}) tlsConf, err = c.OutgoingTLSConfigForCheck("c1") require.NoError(t, err) require.False(t, tlsConf.InsecureSkipVerify) From 8bcab53b80488f302cea8437c975234c49e7bbb4 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Thu, 28 Feb 2019 14:40:15 +0100 Subject: [PATCH 02/31] wip --- agent/agent_test.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/agent/agent_test.go b/agent/agent_test.go index 1046dc10a08c..97842071ece3 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -17,9 +17,12 @@ import ( "github.com/hashicorp/consul/testrpc" + "github.com/hashicorp/consul/agent/ae" "github.com/hashicorp/consul/agent/checks" "github.com/hashicorp/consul/agent/config" "github.com/hashicorp/consul/agent/connect" + "github.com/hashicorp/consul/agent/consul" + "github.com/hashicorp/consul/agent/local" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/testutil" @@ -3543,3 +3546,26 @@ func TestAgent_loadTokens(t *testing.T) { require.Equal("foxtrot", a.tokens.ReplicationToken()) }) } + +func TestHansAgent_ReloadConfigTLS(t *testing.T) { + t.Parallel() + dataDir := testutil.TempDir(t, "agent") // we manage the data dir + defer os.RemoveAll(dataDir) + hcl := ` + data_dir = "` + dataDir + `" + verify_outgoing = true + ca_file = "../test/ca/root.cer" + cert_file = "../test/key/ourdomain.cer" + key_file = "../test/key/ourdomain.key" + ` + c := TestConfig(config.Source{Name: t.Name(), Format: "hcl", Data: hcl}) + a, err := New(c) + a.State = local.NewState(LocalConfig(c), a.logger, a.tokens) + a.sync = ae.NewStateSyncer(a.State, c.AEInterval, a.shutdownCh, a.logger) + a.delegate = &consul.Client{} + a.State.TriggerSyncChanges = a.sync.SyncChanges.Trigger + require.NoError(t, err) + + err = a.ReloadConfig(c) + require.NoError(t, err) +} From 76249dcc4332924f9e49d49eae177805d1d4d546 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Thu, 28 Feb 2019 15:20:48 +0100 Subject: [PATCH 03/31] GetConfigForClient --- agent/agent_test.go | 78 ++++++++++++++++++++++++++++++++++----------- agent/testagent.go | 16 ++++++++++ tlsutil/config.go | 18 +++++++++-- 3 files changed, 91 insertions(+), 21 deletions(-) diff --git a/agent/agent_test.go b/agent/agent_test.go index 97842071ece3..bef64e997312 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -17,12 +17,9 @@ import ( "github.com/hashicorp/consul/testrpc" - "github.com/hashicorp/consul/agent/ae" "github.com/hashicorp/consul/agent/checks" "github.com/hashicorp/consul/agent/config" "github.com/hashicorp/consul/agent/connect" - "github.com/hashicorp/consul/agent/consul" - "github.com/hashicorp/consul/agent/local" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/testutil" @@ -3369,11 +3366,7 @@ func TestAgent_SetupProxyManager(t *testing.T) { ports { http = -1 } data_dir = "` + dataDir + `" ` - c := TestConfig( - // randomPortsSource(false), - config.Source{Name: t.Name(), Format: "hcl", Data: hcl}, - ) - a, err := New(c) + a, err := NewUnstartedAgent(t, t.Name(), hcl) require.NoError(t, err) require.Error(t, a.setupProxyManager(), "setupProxyManager should fail with invalid HTTP API config") @@ -3381,11 +3374,7 @@ func TestAgent_SetupProxyManager(t *testing.T) { ports { http = 8001 } data_dir = "` + dataDir + `" ` - c = TestConfig( - // randomPortsSource(false), - config.Source{Name: t.Name(), Format: "hcl", Data: hcl}, - ) - a, err = New(c) + a, err = NewUnstartedAgent(t, t.Name(), hcl) require.NoError(t, err) require.NoError(t, a.setupProxyManager()) } @@ -3547,7 +3536,7 @@ func TestAgent_loadTokens(t *testing.T) { }) } -func TestHansAgent_ReloadConfigTLS(t *testing.T) { +func TestAgent_ReloadConfigOutgoingRPCConfig(t *testing.T) { t.Parallel() dataDir := testutil.TempDir(t, "agent") // we manage the data dir defer os.RemoveAll(dataDir) @@ -3557,15 +3546,66 @@ func TestHansAgent_ReloadConfigTLS(t *testing.T) { ca_file = "../test/ca/root.cer" cert_file = "../test/key/ourdomain.cer" key_file = "../test/key/ourdomain.key" + verify_server_hostname = false + ` + a, err := NewUnstartedAgent(t, t.Name(), hcl) + require.NoError(t, err) + tlsConf, err := a.tlsConfigurator.OutgoingRPCConfig() + require.NoError(t, err) + require.True(t, tlsConf.InsecureSkipVerify) + + hcl = ` + data_dir = "` + dataDir + `" + verify_outgoing = true + ca_file = "../test/ca/root.cer" + cert_file = "../test/key/ourdomain.cer" + key_file = "../test/key/ourdomain.key" + verify_server_hostname = true ` c := TestConfig(config.Source{Name: t.Name(), Format: "hcl", Data: hcl}) - a, err := New(c) - a.State = local.NewState(LocalConfig(c), a.logger, a.tokens) - a.sync = ae.NewStateSyncer(a.State, c.AEInterval, a.shutdownCh, a.logger) - a.delegate = &consul.Client{} - a.State.TriggerSyncChanges = a.sync.SyncChanges.Trigger + err = a.ReloadConfig(c) + require.NoError(t, err) + tlsConf, err = a.tlsConfigurator.OutgoingRPCConfig() + require.NoError(t, err) + require.True(t, tlsConf.InsecureSkipVerify) +} + +func TestAgent_ReloadConfigIncomingRPCConfig(t *testing.T) { + t.Parallel() + dataDir := testutil.TempDir(t, "agent") // we manage the data dir + defer os.RemoveAll(dataDir) + hcl := ` + data_dir = "` + dataDir + `" + verify_outgoing = true + ca_file = "../test/ca/root.cer" + cert_file = "../test/key/ourdomain.cer" + key_file = "../test/key/ourdomain.key" + verify_server_hostname = false + ` + a, err := NewUnstartedAgent(t, t.Name(), hcl) + require.NoError(t, err) + tlsConf, err := a.tlsConfigurator.IncomingRPCConfig() + require.NoError(t, err) + require.NotNil(t, tlsConf.GetConfigForClient) + tlsConf, err = tlsConf.GetConfigForClient(nil) require.NoError(t, err) + require.NotNil(t, tlsConf) + require.True(t, tlsConf.InsecureSkipVerify) + hcl = ` + data_dir = "` + dataDir + `" + verify_outgoing = true + ca_file = "../test/ca/root.cer" + cert_file = "../test/key/ourdomain.cer" + key_file = "../test/key/ourdomain.key" + verify_server_hostname = true + ` + c := TestConfig(config.Source{Name: t.Name(), Format: "hcl", Data: hcl}) err = a.ReloadConfig(c) require.NoError(t, err) + tlsConf, err = a.tlsConfigurator.IncomingRPCConfig() + require.NoError(t, err) + require.NotNil(t, tlsConf.GetConfigForClient) + tlsConf, err = tlsConf.GetConfigForClient(nil) + require.True(t, tlsConf.InsecureSkipVerify) } diff --git a/agent/testagent.go b/agent/testagent.go index e9749611dbf4..5bd1f4d2fb04 100644 --- a/agent/testagent.go +++ b/agent/testagent.go @@ -18,9 +18,11 @@ import ( metrics "github.com/armon/go-metrics" uuid "github.com/hashicorp/go-uuid" + "github.com/hashicorp/consul/agent/ae" "github.com/hashicorp/consul/agent/config" "github.com/hashicorp/consul/agent/connect" "github.com/hashicorp/consul/agent/consul" + "github.com/hashicorp/consul/agent/local" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/lib/freeport" @@ -103,6 +105,20 @@ func NewTestAgent(t *testing.T, name string, hcl string) *TestAgent { return a } +func NewUnstartedAgent(t *testing.T, name string, hcl string) (*Agent, error) { + c := TestConfig(config.Source{Name: name, Format: "hcl", Data: hcl}) + a, err := New(c) + if err != nil { + return nil, err + } + a.State = local.NewState(LocalConfig(c), a.logger, a.tokens) + a.sync = ae.NewStateSyncer(a.State, c.AEInterval, a.shutdownCh, a.logger) + a.delegate = &consul.Client{} + a.State.TriggerSyncChanges = a.sync.SyncChanges.Trigger + a.tlsConfigurator = tlsutil.NewConfigurator(c.ToTLSUtilConfig()) + return a, nil +} + // Start starts a test agent. It fails the test if the agent could not be started. func (a *TestAgent) Start(t *testing.T) *TestAgent { require := require.New(t) diff --git a/tlsutil/config.go b/tlsutil/config.go index 57f02674168f..d4b3a64e80e8 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -289,12 +289,26 @@ func (c *Configurator) commonTLSConfig(additionalVerifyIncomingFlag bool) (*tls. // IncomingRPCConfig generates a *tls.Config for incoming RPC connections. func (c *Configurator) IncomingRPCConfig() (*tls.Config, error) { - return c.commonTLSConfig(c.base.VerifyIncomingRPC) + config, err := c.commonTLSConfig(c.base.VerifyIncomingRPC) + if err != nil { + return nil, err + } + config.GetConfigForClient = func(*tls.ClientHelloInfo) (*tls.Config, error) { + return c.IncomingRPCConfig() + } + return config, nil } // IncomingHTTPSConfig generates a *tls.Config for incoming HTTPS connections. func (c *Configurator) IncomingHTTPSConfig() (*tls.Config, error) { - return c.commonTLSConfig(c.base.VerifyIncomingHTTPS) + config, err := c.commonTLSConfig(c.base.VerifyIncomingHTTPS) + if err != nil { + return nil, err + } + config.GetConfigForClient = func(*tls.ClientHelloInfo) (*tls.Config, error) { + return c.IncomingHTTPSConfig() + } + return config, nil } // IncomingTLSConfig generates a *tls.Config for outgoing TLS connections for From 12076fa546211992586e718b753e2ab7ce0546b5 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Thu, 28 Feb 2019 15:57:51 +0100 Subject: [PATCH 04/31] better test --- agent/agent_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/agent/agent_test.go b/agent/agent_test.go index bef64e997312..e3d3c5f55597 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -3603,9 +3603,6 @@ func TestAgent_ReloadConfigIncomingRPCConfig(t *testing.T) { c := TestConfig(config.Source{Name: t.Name(), Format: "hcl", Data: hcl}) err = a.ReloadConfig(c) require.NoError(t, err) - tlsConf, err = a.tlsConfigurator.IncomingRPCConfig() - require.NoError(t, err) - require.NotNil(t, tlsConf.GetConfigForClient) tlsConf, err = tlsConf.GetConfigForClient(nil) require.True(t, tlsConf.InsecureSkipVerify) } From f76c13e52ba34bfb0db7ebec410957d1fb7e36a2 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Thu, 28 Feb 2019 16:10:42 +0100 Subject: [PATCH 05/31] guard config reading --- agent/agent.go | 13 ++++++++----- agent/agent_test.go | 4 ++-- tlsutil/config.go | 44 ++++++++++++++++---------------------------- 3 files changed, 26 insertions(+), 35 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 58e374e72a80..69ad1c11ca28 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -2232,8 +2232,7 @@ func (a *Agent) addCheck(check *structs.HealthCheck, chkType *structs.CheckType, chkType.Interval = checks.MinInterval } - a.tlsConfigurator.AddCheck(string(check.CheckID), chkType.TLSSkipVerify) - tlsClientConfig, err := a.tlsConfigurator.OutgoingTLSConfigForCheck(string(check.CheckID)) + tlsClientConfig, err := a.tlsConfigurator.OutgoingTLSConfigForCheck(chkType.TLSSkipVerify) if err != nil { return fmt.Errorf("Failed to set up TLS: %v", err) } @@ -2288,8 +2287,7 @@ func (a *Agent) addCheck(check *structs.HealthCheck, chkType *structs.CheckType, var tlsClientConfig *tls.Config if chkType.GRPCUseTLS { var err error - a.tlsConfigurator.AddCheck(string(check.CheckID), chkType.TLSSkipVerify) - tlsClientConfig, err = a.tlsConfigurator.OutgoingTLSConfigForCheck(string(check.CheckID)) + tlsClientConfig, err = a.tlsConfigurator.OutgoingTLSConfigForCheck(chkType.TLSSkipVerify) if err != nil { return fmt.Errorf("Failed to set up TLS: %v", err) } @@ -2431,7 +2429,12 @@ func (a *Agent) removeCheckLocked(checkID types.CheckID, persist bool) error { return fmt.Errorf("CheckID missing") } - a.tlsConfigurator.RemoveCheck(string(checkID)) + // Add to the local state for anti-entropy + a.State.RemoveCheck(checkID) + + a.checkLock.Lock() + defer a.checkLock.Unlock() + a.cancelCheckMonitors(checkID) a.State.RemoveCheck(checkID) diff --git a/agent/agent_test.go b/agent/agent_test.go index e3d3c5f55597..58c757c1a370 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -3536,7 +3536,7 @@ func TestAgent_loadTokens(t *testing.T) { }) } -func TestAgent_ReloadConfigOutgoingRPCConfig(t *testing.T) { +func TestHansAgent_ReloadConfigOutgoingRPCConfig(t *testing.T) { t.Parallel() dataDir := testutil.TempDir(t, "agent") // we manage the data dir defer os.RemoveAll(dataDir) @@ -3570,7 +3570,7 @@ func TestAgent_ReloadConfigOutgoingRPCConfig(t *testing.T) { require.True(t, tlsConf.InsecureSkipVerify) } -func TestAgent_ReloadConfigIncomingRPCConfig(t *testing.T) { +func TestHansAgent_ReloadConfigIncomingRPCConfig(t *testing.T) { t.Parallel() dataDir := testutil.TempDir(t, "agent") // we manage the data dir defer os.RemoveAll(dataDir) diff --git a/tlsutil/config.go b/tlsutil/config.go index d4b3a64e80e8..a5884e47b527 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -196,9 +196,8 @@ func (c *Config) wrapTLSClient(conn net.Conn, tlsConfig *tls.Config) (net.Conn, // Configurator holds a Config and is responsible for generating all the // *tls.Config necessary for Consul. Except the one in the api package. type Configurator struct { - sync.Mutex - base *Config - checks map[string]bool + sync.RWMutex + base *Config } // NewConfigurator creates a new Configurator and sets the provided @@ -206,7 +205,7 @@ type Configurator struct { // Todo (Hans): should config be a value instead a pointer to avoid side // effects? func NewConfigurator(config Config) *Configurator { - return &Configurator{base: &config, checks: map[string]bool{}} + return &Configurator{base: &config} } // Update updates the internal configuration which is used to generate @@ -289,6 +288,8 @@ func (c *Configurator) commonTLSConfig(additionalVerifyIncomingFlag bool) (*tls. // IncomingRPCConfig generates a *tls.Config for incoming RPC connections. func (c *Configurator) IncomingRPCConfig() (*tls.Config, error) { + c.RLock() + defer c.RUnlock() config, err := c.commonTLSConfig(c.base.VerifyIncomingRPC) if err != nil { return nil, err @@ -301,6 +302,8 @@ func (c *Configurator) IncomingRPCConfig() (*tls.Config, error) { // IncomingHTTPSConfig generates a *tls.Config for incoming HTTPS connections. func (c *Configurator) IncomingHTTPSConfig() (*tls.Config, error) { + c.RLock() + defer c.RUnlock() config, err := c.commonTLSConfig(c.base.VerifyIncomingHTTPS) if err != nil { return nil, err @@ -315,10 +318,12 @@ func (c *Configurator) IncomingHTTPSConfig() (*tls.Config, error) { // checks. This function is seperated because there is an extra flag to // consider for checks. EnableAgentTLSForChecks and InsecureSkipVerify has to // be checked for checks. -func (c *Configurator) OutgoingTLSConfigForCheck(id string) (*tls.Config, error) { +func (c *Configurator) OutgoingTLSConfigForCheck(skipVerify bool) (*tls.Config, error) { + c.RLock() + defer c.RUnlock() if !c.base.EnableAgentTLSForChecks { return &tls.Config{ - InsecureSkipVerify: c.getSkipVerifyForCheck(id), + InsecureSkipVerify: skipVerify, }, nil } @@ -326,7 +331,7 @@ func (c *Configurator) OutgoingTLSConfigForCheck(id string) (*tls.Config, error) if err != nil { return nil, err } - tlsConfig.InsecureSkipVerify = c.getSkipVerifyForCheck(id) + tlsConfig.InsecureSkipVerify = skipVerify tlsConfig.ServerName = c.base.ServerName if tlsConfig.ServerName == "" { tlsConfig.ServerName = c.base.NodeName @@ -339,6 +344,8 @@ func (c *Configurator) OutgoingTLSConfigForCheck(id string) (*tls.Config, error) // there is a CA or VerifyOutgoing is set, a *tls.Config will be provided, // otherwise we assume that no TLS should be used. func (c *Configurator) OutgoingRPCConfig() (*tls.Config, error) { + c.RLock() + defer c.RUnlock() useTLS := c.base.CAFile != "" || c.base.CAPath != "" || c.base.VerifyOutgoing if !useTLS { return nil, nil @@ -362,6 +369,8 @@ func (c *Configurator) OutgoingRPCWrapper() (DCWrapper, error) { // Generate the wrapper based on hostname verification wrapper := func(dc string, conn net.Conn) (net.Conn, error) { + c.RLock() + defer c.RUnlock() if c.base.VerifyServerHostname { // Strip the trailing '.' from the domain if any domain := strings.TrimSuffix(c.base.Domain, ".") @@ -374,27 +383,6 @@ func (c *Configurator) OutgoingRPCWrapper() (DCWrapper, error) { return wrapper, nil } -// AddCheck adds a check to the internal check map together with the skipVerify -// value, which is used when generating a *tls.Config for this check. -func (c *Configurator) AddCheck(id string, skipVerify bool) { - c.Lock() - defer c.Unlock() - c.checks[id] = skipVerify -} - -// RemoveCheck removes a check from the internal check map. -func (c *Configurator) RemoveCheck(id string) { - c.Lock() - defer c.Unlock() - delete(c.checks, id) -} - -func (c *Configurator) getSkipVerifyForCheck(id string) bool { - c.Lock() - defer c.Unlock() - return c.checks[id] -} - // ParseCiphers parse ciphersuites from the comma-separated string into // recognized slice func ParseCiphers(cipherStr string) ([]uint16, error) { From a064f142b777ef7ead4f2dd0e4a2f6709a193051 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Thu, 28 Feb 2019 16:51:05 +0100 Subject: [PATCH 06/31] getclientconfigfor --- agent/agent.go | 2 ++ agent/agent_test.go | 16 ++++++++++++---- agent/config/runtime.go | 1 + agent/config/runtime_test.go | 2 ++ tlsutil/config.go | 1 + 5 files changed, 18 insertions(+), 4 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 69ad1c11ca28..e84d64630685 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -3562,6 +3562,8 @@ func (a *Agent) ReloadConfig(newCfg *config.RuntimeConfig) error { // the checks and service registrations. a.loadTokens(newCfg) + a.tlsConfigurator.Update(newCfg.ToTLSUtilConfig()) + // Reload service/check definitions and metadata. if err := a.loadServices(newCfg); err != nil { return fmt.Errorf("Failed reloading services: %s", err) diff --git a/agent/agent_test.go b/agent/agent_test.go index 58c757c1a370..906870dd9400 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -3553,11 +3553,13 @@ func TestHansAgent_ReloadConfigOutgoingRPCConfig(t *testing.T) { tlsConf, err := a.tlsConfigurator.OutgoingRPCConfig() require.NoError(t, err) require.True(t, tlsConf.InsecureSkipVerify) + require.Len(t, tlsConf.ClientCAs.Subjects(), 1) + require.Len(t, tlsConf.RootCAs.Subjects(), 1) hcl = ` data_dir = "` + dataDir + `" verify_outgoing = true - ca_file = "../test/ca/root.cer" + ca_path = "../test/ca_path" cert_file = "../test/key/ourdomain.cer" key_file = "../test/key/ourdomain.key" verify_server_hostname = true @@ -3567,7 +3569,9 @@ func TestHansAgent_ReloadConfigOutgoingRPCConfig(t *testing.T) { require.NoError(t, err) tlsConf, err = a.tlsConfigurator.OutgoingRPCConfig() require.NoError(t, err) - require.True(t, tlsConf.InsecureSkipVerify) + require.False(t, tlsConf.InsecureSkipVerify) + require.Len(t, tlsConf.RootCAs.Subjects(), 2) + require.Len(t, tlsConf.ClientCAs.Subjects(), 2) } func TestHansAgent_ReloadConfigIncomingRPCConfig(t *testing.T) { @@ -3591,11 +3595,13 @@ func TestHansAgent_ReloadConfigIncomingRPCConfig(t *testing.T) { require.NoError(t, err) require.NotNil(t, tlsConf) require.True(t, tlsConf.InsecureSkipVerify) + require.Len(t, tlsConf.ClientCAs.Subjects(), 1) + require.Len(t, tlsConf.RootCAs.Subjects(), 1) hcl = ` data_dir = "` + dataDir + `" verify_outgoing = true - ca_file = "../test/ca/root.cer" + ca_path = "../test/ca_path" cert_file = "../test/key/ourdomain.cer" key_file = "../test/key/ourdomain.key" verify_server_hostname = true @@ -3604,5 +3610,7 @@ func TestHansAgent_ReloadConfigIncomingRPCConfig(t *testing.T) { err = a.ReloadConfig(c) require.NoError(t, err) tlsConf, err = tlsConf.GetConfigForClient(nil) - require.True(t, tlsConf.InsecureSkipVerify) + require.False(t, tlsConf.InsecureSkipVerify) + require.Len(t, tlsConf.ClientCAs.Subjects(), 2) + require.Len(t, tlsConf.RootCAs.Subjects(), 2) } diff --git a/agent/config/runtime.go b/agent/config/runtime.go index c0ea6ce8f0bb..9aefadc0bd31 100644 --- a/agent/config/runtime.go +++ b/agent/config/runtime.go @@ -1586,6 +1586,7 @@ func (c *RuntimeConfig) ToTLSUtilConfig() tlsutil.Config { VerifyIncomingRPC: c.VerifyIncomingRPC, VerifyIncomingHTTPS: c.VerifyIncomingHTTPS, VerifyOutgoing: c.VerifyOutgoing, + VerifyServerHostname: c.VerifyServerHostname, CAFile: c.CAFile, CAPath: c.CAPath, CertFile: c.CertFile, diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index e9889f541171..6de42304e137 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -5434,6 +5434,7 @@ func TestRuntime_ToTLSUtilConfig(t *testing.T) { VerifyIncomingRPC: true, VerifyIncomingHTTPS: true, VerifyOutgoing: true, + VerifyServerHostname: true, CAFile: "a", CAPath: "b", CertFile: "c", @@ -5450,6 +5451,7 @@ func TestRuntime_ToTLSUtilConfig(t *testing.T) { require.Equal(t, c.VerifyIncomingRPC, r.VerifyIncomingRPC) require.Equal(t, c.VerifyIncomingHTTPS, r.VerifyIncomingHTTPS) require.Equal(t, c.VerifyOutgoing, r.VerifyOutgoing) + require.Equal(t, c.VerifyServerHostname, r.VerifyServerHostname) require.Equal(t, c.CAFile, r.CAFile) require.Equal(t, c.CAPath, r.CAPath) require.Equal(t, c.CertFile, r.CertFile) diff --git a/tlsutil/config.go b/tlsutil/config.go index a5884e47b527..568e618d29bd 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -214,6 +214,7 @@ func (c *Configurator) Update(config Config) { c.Lock() defer c.Unlock() c.base = &config + } // commonTLSConfig generates a *tls.Config from the base configuration the From c996dd685a389d960da6d66d5662854728789a99 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Mon, 4 Mar 2019 14:10:59 +0100 Subject: [PATCH 07/31] remove comment --- tlsutil/config.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/tlsutil/config.go b/tlsutil/config.go index 568e618d29bd..56a05b3c789b 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -202,8 +202,6 @@ type Configurator struct { // NewConfigurator creates a new Configurator and sets the provided // configuration. -// Todo (Hans): should config be a value instead a pointer to avoid side -// effects? func NewConfigurator(config Config) *Configurator { return &Configurator{base: &config} } From 6017229a1e0827b1d362c629bd5534991bb18ea5 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Mon, 4 Mar 2019 14:21:43 +0100 Subject: [PATCH 08/31] fix tests --- tlsutil/config_test.go | 46 +++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index c0a970f7238b..ae1d2fc7a383 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -593,12 +593,12 @@ func TestConfigurator_CommonTLSConfigVerifyIncoming(t *testing.T) { require.NoError(t, err) require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) - c.Update(&Config{VerifyServerHostname: false, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) + c.Update(Config{VerifyServerHostname: false, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) tlsConf, err = c.commonTLSConfig(false) require.NoError(t, err) require.True(t, tlsConf.InsecureSkipVerify) - c.Update(&Config{VerifyServerHostname: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) + c.Update(Config{VerifyServerHostname: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) tlsConf, err = c.commonTLSConfig(false) require.NoError(t, err) require.False(t, tlsConf.InsecureSkipVerify) @@ -609,21 +609,25 @@ func TestConfigurator_IncomingRPCConfig(t *testing.T) { tlsConf, err := c.IncomingRPCConfig() require.NoError(t, err) require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) + require.NotNil(t, tlsConf.GetConfigForClient) c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) tlsConf, err = c.IncomingRPCConfig() require.NoError(t, err) require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) + require.NotNil(t, tlsConf.GetConfigForClient) c.Update(Config{VerifyIncomingRPC: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) tlsConf, err = c.IncomingRPCConfig() require.NoError(t, err) require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) + require.NotNil(t, tlsConf.GetConfigForClient) c.Update(Config{VerifyIncomingHTTPS: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) tlsConf, err = c.IncomingRPCConfig() require.NoError(t, err) require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) + require.NotNil(t, tlsConf.GetConfigForClient) } func TestConfigurator_IncomingHTTPSConfig(t *testing.T) { @@ -631,21 +635,25 @@ func TestConfigurator_IncomingHTTPSConfig(t *testing.T) { tlsConf, err := c.IncomingHTTPSConfig() require.NoError(t, err) require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) + require.NotNil(t, tlsConf.GetConfigForClient) c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) tlsConf, err = c.IncomingHTTPSConfig() require.NoError(t, err) require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) + require.NotNil(t, tlsConf.GetConfigForClient) c.Update(Config{VerifyIncomingHTTPS: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) tlsConf, err = c.IncomingHTTPSConfig() require.NoError(t, err) require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) + require.NotNil(t, tlsConf.GetConfigForClient) c.Update(Config{VerifyIncomingRPC: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) tlsConf, err = c.IncomingHTTPSConfig() require.NoError(t, err) require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) + require.NotNil(t, tlsConf.GetConfigForClient) } func TestConfigurator_OutgoingRPCConfig(t *testing.T) { @@ -668,46 +676,38 @@ func TestConfigurator_OutgoingRPCConfig(t *testing.T) { } func TestConfigurator_OutgoingTLSConfigForChecks(t *testing.T) { - c := NewConfigurator(Config{}) - tlsConf, err := c.OutgoingTLSConfigForCheck("") - require.NoError(t, err) - require.False(t, tlsConf.InsecureSkipVerify) - - c.Update(Config{EnableAgentTLSForChecks: true}) - tlsConf, err = c.OutgoingTLSConfigForCheck("") + c := NewConfigurator(Config{EnableAgentTLSForChecks: false}) + tlsConf, err := c.OutgoingTLSConfigForCheck(false) require.NoError(t, err) require.False(t, tlsConf.InsecureSkipVerify) - c.AddCheck("c1", true) - c.Update(Config{EnableAgentTLSForChecks: true}) - tlsConf, err = c.OutgoingTLSConfigForCheck("c1") + c.Update(Config{EnableAgentTLSForChecks: false}) + tlsConf, err = c.OutgoingTLSConfigForCheck(true) require.NoError(t, err) require.True(t, tlsConf.InsecureSkipVerify) - c.AddCheck("c1", false) c.Update(Config{EnableAgentTLSForChecks: true}) - tlsConf, err = c.OutgoingTLSConfigForCheck("c1") + tlsConf, err = c.OutgoingTLSConfigForCheck(false) require.NoError(t, err) require.False(t, tlsConf.InsecureSkipVerify) - c.AddCheck("c1", false) c.Update(Config{EnableAgentTLSForChecks: true}) - tlsConf, err = c.OutgoingTLSConfigForCheck("c1") + tlsConf, err = c.OutgoingTLSConfigForCheck(true) require.NoError(t, err) - require.False(t, tlsConf.InsecureSkipVerify) + require.True(t, tlsConf.InsecureSkipVerify) - c.Update(&Config{EnableAgentTLSForChecks: true, NodeName: "node", ServerName: "server"}) - tlsConf, err = c.OutgoingTLSConfigForCheck("") + c.Update(Config{EnableAgentTLSForChecks: true, NodeName: "node", ServerName: "server"}) + tlsConf, err = c.OutgoingTLSConfigForCheck(false) require.NoError(t, err) require.Equal(t, "server", tlsConf.ServerName) - c.Update(&Config{EnableAgentTLSForChecks: true, ServerName: "server"}) - tlsConf, err = c.OutgoingTLSConfigForCheck("") + c.Update(Config{EnableAgentTLSForChecks: true, ServerName: "server"}) + tlsConf, err = c.OutgoingTLSConfigForCheck(false) require.NoError(t, err) require.Equal(t, "server", tlsConf.ServerName) - c.Update(&Config{EnableAgentTLSForChecks: true, NodeName: "node"}) - tlsConf, err = c.OutgoingTLSConfigForCheck("") + c.Update(Config{EnableAgentTLSForChecks: true, NodeName: "node"}) + tlsConf, err = c.OutgoingTLSConfigForCheck(false) require.NoError(t, err) require.Equal(t, "node", tlsConf.ServerName) } From 066acf6fea72a51fb4b4b64ecb2a4b0366166899 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Mon, 4 Mar 2019 14:57:10 +0100 Subject: [PATCH 09/31] check config --- agent/agent.go | 6 +++++- tlsutil/config.go | 12 ++++++++++++ tlsutil/config_test.go | 9 +++++++++ 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/agent/agent.go b/agent/agent.go index e84d64630685..c7d9e920eec1 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -3562,7 +3562,11 @@ func (a *Agent) ReloadConfig(newCfg *config.RuntimeConfig) error { // the checks and service registrations. a.loadTokens(newCfg) - a.tlsConfigurator.Update(newCfg.ToTLSUtilConfig()) + tlsConfig := newCfg.ToTLSUtilConfig() + if err := a.tlsConfigurator.Check(tlsConfig); err != nil { + return fmt.Errorf("Failed reloading tls configuration: %s", err) + } + a.tlsConfigurator.Update(tlsConfig) // Reload service/check definitions and metadata. if err := a.loadServices(newCfg); err != nil { diff --git a/tlsutil/config.go b/tlsutil/config.go index 56a05b3c789b..3e2c02f12456 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -215,6 +215,18 @@ func (c *Configurator) Update(config Config) { } +// Check is verifying the provided configuration and returns an error if the +// config has problems. +func (c *Configurator) Check(config Config) error { + c.Lock() + defer c.Unlock() + orig := c.base + c.base = &config + defer func() { c.base = orig }() + _, err := c.commonTLSConfig(c.base.VerifyIncomingRPC || c.base.VerifyIncomingHTTPS) + return err +} + // commonTLSConfig generates a *tls.Config from the base configuration the // Configurator has. It accepts an additional flag in case a config is needed // for incoming TLS connections. diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index ae1d2fc7a383..d657ca49539e 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -711,3 +711,12 @@ func TestConfigurator_OutgoingTLSConfigForChecks(t *testing.T) { require.NoError(t, err) require.Equal(t, "node", tlsConf.ServerName) } + +func TestConfigurator_Check(t *testing.T) { + c := NewConfigurator(Config{}) + require.NoError(t, c.Check(Config{})) + require.Error(t, c.Check(Config{VerifyOutgoing: true})) + require.Error(t, c.Check(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer"})) + require.False(t, c.base.VerifyIncoming) + require.False(t, c.base.VerifyOutgoing) +} From 61b6c50da88e1a7d23d1d9d2bc547dd1df9dc0c4 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Mon, 4 Mar 2019 15:03:59 +0100 Subject: [PATCH 10/31] add test for failing tlsconfig reload --- agent/agent.go | 6 +++--- agent/agent_test.go | 15 +++++++++++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index c7d9e920eec1..1fe3df56aa9e 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -3562,11 +3562,11 @@ func (a *Agent) ReloadConfig(newCfg *config.RuntimeConfig) error { // the checks and service registrations. a.loadTokens(newCfg) - tlsConfig := newCfg.ToTLSUtilConfig() - if err := a.tlsConfigurator.Check(tlsConfig); err != nil { + newTLSConfig := newCfg.ToTLSUtilConfig() + if err := a.tlsConfigurator.Check(newTLSConfig); err != nil { return fmt.Errorf("Failed reloading tls configuration: %s", err) } - a.tlsConfigurator.Update(tlsConfig) + a.tlsConfigurator.Update(newTLSConfig) // Reload service/check definitions and metadata. if err := a.loadServices(newCfg); err != nil { diff --git a/agent/agent_test.go b/agent/agent_test.go index 906870dd9400..a7daa7dcfd27 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -2,6 +2,7 @@ package agent import ( "bytes" + "crypto/tls" "encoding/json" "fmt" "io/ioutil" @@ -3610,7 +3611,21 @@ func TestHansAgent_ReloadConfigIncomingRPCConfig(t *testing.T) { err = a.ReloadConfig(c) require.NoError(t, err) tlsConf, err = tlsConf.GetConfigForClient(nil) + require.NoError(t, err) require.False(t, tlsConf.InsecureSkipVerify) require.Len(t, tlsConf.ClientCAs.Subjects(), 2) require.Len(t, tlsConf.RootCAs.Subjects(), 2) + + hcl = ` + data_dir = "` + dataDir + `" + verify_incoming = true + ` + c = TestConfig(config.Source{Name: t.Name(), Format: "hcl", Data: hcl}) + err = a.ReloadConfig(c) + require.Error(t, err) + tlsConf, err = tlsConf.GetConfigForClient(nil) + require.NoError(t, err) + require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) + require.Len(t, tlsConf.ClientCAs.Subjects(), 2) + require.Len(t, tlsConf.RootCAs.Subjects(), 2) } From cd353316c36bd8e22b65e22e9966748f8e62688d Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Mon, 4 Mar 2019 15:11:29 +0100 Subject: [PATCH 11/31] extra test for test --- agent/agent_test.go | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/agent/agent_test.go b/agent/agent_test.go index a7daa7dcfd27..9d5dacb30b3d 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -3615,17 +3615,35 @@ func TestHansAgent_ReloadConfigIncomingRPCConfig(t *testing.T) { require.False(t, tlsConf.InsecureSkipVerify) require.Len(t, tlsConf.ClientCAs.Subjects(), 2) require.Len(t, tlsConf.RootCAs.Subjects(), 2) +} + +func TestHansAgent_ReloadConfigTLSConfigFailure(t *testing.T) { + t.Parallel() + dataDir := testutil.TempDir(t, "agent") // we manage the data dir + defer os.RemoveAll(dataDir) + hcl := ` + data_dir = "` + dataDir + `" + verify_outgoing = true + ca_file = "../test/ca/root.cer" + cert_file = "../test/key/ourdomain.cer" + key_file = "../test/key/ourdomain.key" + verify_server_hostname = false + ` + a, err := NewUnstartedAgent(t, t.Name(), hcl) + require.NoError(t, err) + tlsConf, err := a.tlsConfigurator.IncomingRPCConfig() + require.NoError(t, err) hcl = ` data_dir = "` + dataDir + `" verify_incoming = true ` - c = TestConfig(config.Source{Name: t.Name(), Format: "hcl", Data: hcl}) + c := TestConfig(config.Source{Name: t.Name(), Format: "hcl", Data: hcl}) err = a.ReloadConfig(c) require.Error(t, err) tlsConf, err = tlsConf.GetConfigForClient(nil) require.NoError(t, err) require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) - require.Len(t, tlsConf.ClientCAs.Subjects(), 2) - require.Len(t, tlsConf.RootCAs.Subjects(), 2) + require.Len(t, tlsConf.ClientCAs.Subjects(), 1) + require.Len(t, tlsConf.RootCAs.Subjects(), 1) } From dd93d735af0d05abee1830c26dde885be1e594e8 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Tue, 5 Mar 2019 15:41:18 +0100 Subject: [PATCH 12/31] add logger so that we can debug log --- agent/agent.go | 2 +- agent/consul/client.go | 2 +- agent/consul/server.go | 2 +- agent/testagent.go | 4 +-- tlsutil/config.go | 21 ++++++++++-- tlsutil/config_test.go | 73 +++++++++++++++++++++++------------------- 6 files changed, 63 insertions(+), 41 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 1fe3df56aa9e..960fb495448d 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -393,7 +393,7 @@ func (a *Agent) Start() error { // waiting to discover a consul server consulCfg.ServerUp = a.sync.SyncFull.Trigger - a.tlsConfigurator = tlsutil.NewConfigurator(c.ToTLSUtilConfig()) + a.tlsConfigurator = tlsutil.NewConfigurator(c.ToTLSUtilConfig(), a.logger) // Setup either the client or the server. if c.ServerMode { diff --git a/agent/consul/client.go b/agent/consul/client.go index 48e279a12bae..af092d4fc7ff 100644 --- a/agent/consul/client.go +++ b/agent/consul/client.go @@ -89,7 +89,7 @@ type Client struct { // NewClient is used to construct a new Consul client from the // configuration, potentially returning an error func NewClient(config *Config) (*Client, error) { - return NewClientLogger(config, nil, tlsutil.NewConfigurator(config.ToTLSUtilConfig())) + return NewClientLogger(config, nil, tlsutil.NewConfigurator(config.ToTLSUtilConfig(), nil)) } func NewClientLogger(config *Config, logger *log.Logger, tlsConfigurator *tlsutil.Configurator) (*Client, error) { diff --git a/agent/consul/server.go b/agent/consul/server.go index a81c8310bed5..e0ddd38dbfc0 100644 --- a/agent/consul/server.go +++ b/agent/consul/server.go @@ -253,7 +253,7 @@ type Server struct { } func NewServer(config *Config) (*Server, error) { - return NewServerLogger(config, nil, new(token.Store), tlsutil.NewConfigurator(config.ToTLSUtilConfig())) + return NewServerLogger(config, nil, new(token.Store), tlsutil.NewConfigurator(config.ToTLSUtilConfig(), nil)) } // NewServer is used to construct a new Consul server from the diff --git a/agent/testagent.go b/agent/testagent.go index 5bd1f4d2fb04..77e16e5e6f7f 100644 --- a/agent/testagent.go +++ b/agent/testagent.go @@ -115,7 +115,7 @@ func NewUnstartedAgent(t *testing.T, name string, hcl string) (*Agent, error) { a.sync = ae.NewStateSyncer(a.State, c.AEInterval, a.shutdownCh, a.logger) a.delegate = &consul.Client{} a.State.TriggerSyncChanges = a.sync.SyncChanges.Trigger - a.tlsConfigurator = tlsutil.NewConfigurator(c.ToTLSUtilConfig()) + a.tlsConfigurator = tlsutil.NewConfigurator(c.ToTLSUtilConfig(), nil) return a, nil } @@ -165,7 +165,7 @@ func (a *TestAgent) Start(t *testing.T) *TestAgent { agent.LogWriter = a.LogWriter agent.logger = log.New(logOutput, a.Name+" - ", log.LstdFlags|log.Lmicroseconds) agent.MemSink = metrics.NewInmemSink(1*time.Second, time.Minute) - agent.tlsConfigurator = tlsutil.NewConfigurator(a.Config.ToTLSUtilConfig()) + agent.tlsConfigurator = tlsutil.NewConfigurator(a.Config.ToTLSUtilConfig(), nil) // we need the err var in the next exit condition if err := agent.Start(); err == nil { diff --git a/tlsutil/config.go b/tlsutil/config.go index 3e2c02f12456..c2de56645719 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -4,6 +4,7 @@ import ( "crypto/tls" "crypto/x509" "fmt" + "log" "net" "strings" "sync" @@ -197,13 +198,15 @@ func (c *Config) wrapTLSClient(conn net.Conn, tlsConfig *tls.Config) (net.Conn, // *tls.Config necessary for Consul. Except the one in the api package. type Configurator struct { sync.RWMutex - base *Config + base *Config + logger *log.Logger + version int } // NewConfigurator creates a new Configurator and sets the provided // configuration. -func NewConfigurator(config Config) *Configurator { - return &Configurator{base: &config} +func NewConfigurator(config Config, logger *log.Logger) *Configurator { + return &Configurator{base: &config, logger: logger, version: 1} } // Update updates the internal configuration which is used to generate @@ -212,6 +215,7 @@ func (c *Configurator) Update(config Config) { c.Lock() defer c.Unlock() c.base = &config + c.version++ } @@ -299,6 +303,7 @@ func (c *Configurator) commonTLSConfig(additionalVerifyIncomingFlag bool) (*tls. // IncomingRPCConfig generates a *tls.Config for incoming RPC connections. func (c *Configurator) IncomingRPCConfig() (*tls.Config, error) { + c.log("IncomingRPCConfig") c.RLock() defer c.RUnlock() config, err := c.commonTLSConfig(c.base.VerifyIncomingRPC) @@ -313,6 +318,7 @@ func (c *Configurator) IncomingRPCConfig() (*tls.Config, error) { // IncomingHTTPSConfig generates a *tls.Config for incoming HTTPS connections. func (c *Configurator) IncomingHTTPSConfig() (*tls.Config, error) { + c.log("IncomingHTTPSConfig") c.RLock() defer c.RUnlock() config, err := c.commonTLSConfig(c.base.VerifyIncomingHTTPS) @@ -330,6 +336,7 @@ func (c *Configurator) IncomingHTTPSConfig() (*tls.Config, error) { // consider for checks. EnableAgentTLSForChecks and InsecureSkipVerify has to // be checked for checks. func (c *Configurator) OutgoingTLSConfigForCheck(skipVerify bool) (*tls.Config, error) { + c.log("OutgoingTLSConfigForCheck") c.RLock() defer c.RUnlock() if !c.base.EnableAgentTLSForChecks { @@ -355,6 +362,7 @@ func (c *Configurator) OutgoingTLSConfigForCheck(skipVerify bool) (*tls.Config, // there is a CA or VerifyOutgoing is set, a *tls.Config will be provided, // otherwise we assume that no TLS should be used. func (c *Configurator) OutgoingRPCConfig() (*tls.Config, error) { + c.log("OutgoingRPCConfig") c.RLock() defer c.RUnlock() useTLS := c.base.CAFile != "" || c.base.CAPath != "" || c.base.VerifyOutgoing @@ -367,6 +375,7 @@ func (c *Configurator) OutgoingRPCConfig() (*tls.Config, error) { // OutgoingRPCWrapper wraps the result of OutgoingRPCConfig in a DCWrapper. It // decides if verify server hostname should be used. func (c *Configurator) OutgoingRPCWrapper() (DCWrapper, error) { + c.log("OutgoingRPCWrapper") // Get the TLS config tlsConfig, err := c.OutgoingRPCConfig() if err != nil { @@ -394,6 +403,12 @@ func (c *Configurator) OutgoingRPCWrapper() (DCWrapper, error) { return wrapper, nil } +func (c *Configurator) log(name string) { + if c.logger != nil { + c.logger.Printf("[DEBUG] tlsutil: %s with version %d", name, c.version) + } +} + // ParseCiphers parse ciphersuites from the comma-separated string into // recognized slice func ParseCiphers(cipherStr string) ([]uint16, error) { diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index d657ca49539e..b613c22792c4 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -43,7 +43,7 @@ func TestConfigurator_OutgoingTLS_MissingCA(t *testing.T) { conf := Config{ VerifyOutgoing: true, } - c := NewConfigurator(conf) + c := NewConfigurator(conf, nil) tlsConf, err := c.OutgoingRPCConfig() require.Error(t, err) require.Nil(t, tlsConf) @@ -53,7 +53,7 @@ func TestConfigurator_OutgoingTLS_OnlyCA(t *testing.T) { conf := Config{ CAFile: "../test/ca/root.cer", } - c := NewConfigurator(conf) + c := NewConfigurator(conf, nil) tlsConf, err := c.OutgoingRPCConfig() require.NoError(t, err) require.NotNil(t, tlsConf) @@ -64,7 +64,7 @@ func TestConfigurator_OutgoingTLS_VerifyOutgoing(t *testing.T) { VerifyOutgoing: true, CAFile: "../test/ca/root.cer", } - c := NewConfigurator(conf) + c := NewConfigurator(conf, nil) tlsConf, err := c.OutgoingRPCConfig() require.NoError(t, err) require.NotNil(t, tlsConf) @@ -79,7 +79,7 @@ func TestConfigurator_OutgoingTLS_ServerName(t *testing.T) { CAFile: "../test/ca/root.cer", ServerName: "consul.example.com", } - c := NewConfigurator(conf) + c := NewConfigurator(conf, nil) tlsConf, err := c.OutgoingRPCConfig() require.NoError(t, err) require.NotNil(t, tlsConf) @@ -94,7 +94,7 @@ func TestConfigurator_OutgoingTLS_VerifyHostname(t *testing.T) { VerifyServerHostname: true, CAFile: "../test/ca/root.cer", } - c := NewConfigurator(conf) + c := NewConfigurator(conf, nil) tlsConf, err := c.OutgoingRPCConfig() require.NoError(t, err) require.NotNil(t, tlsConf) @@ -109,7 +109,7 @@ func TestConfigurator_OutgoingTLS_WithKeyPair(t *testing.T) { CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key", } - c := NewConfigurator(conf) + c := NewConfigurator(conf, nil) tlsConf, err := c.OutgoingRPCConfig() require.NoError(t, err) require.NotNil(t, tlsConf) @@ -125,7 +125,7 @@ func TestConfigurator_OutgoingTLS_TLSMinVersion(t *testing.T) { CAFile: "../test/ca/root.cer", TLSMinVersion: version, } - c := NewConfigurator(conf) + c := NewConfigurator(conf, nil) tlsConf, err := c.OutgoingRPCConfig() require.NoError(t, err) require.NotNil(t, tlsConf) @@ -136,7 +136,7 @@ func TestConfigurator_OutgoingTLS_TLSMinVersion(t *testing.T) { func startTLSServer(config *Config) (net.Conn, chan error) { errc := make(chan error, 1) - c := NewConfigurator(*config) + c := NewConfigurator(*config, nil) tlsConfigServer, err := c.IncomingRPCConfig() if err != nil { errc <- err @@ -186,7 +186,7 @@ func TestConfigurator_outgoingWrapper_OK(t *testing.T) { t.Fatalf("startTLSServer err: %v", <-errc) } - c := NewConfigurator(config) + c := NewConfigurator(config, nil) wrap, err := c.OutgoingRPCWrapper() require.NoError(t, err) @@ -216,7 +216,7 @@ func TestConfigurator_outgoingWrapper_BadDC(t *testing.T) { t.Fatalf("startTLSServer err: %v", <-errc) } - c := NewConfigurator(config) + c := NewConfigurator(config, nil) wrap, err := c.OutgoingRPCWrapper() require.NoError(t, err) @@ -246,7 +246,7 @@ func TestConfigurator_outgoingWrapper_BadCert(t *testing.T) { t.Fatalf("startTLSServer err: %v", <-errc) } - c := NewConfigurator(config) + c := NewConfigurator(config, nil) wrap, err := c.OutgoingRPCWrapper() require.NoError(t, err) @@ -275,7 +275,7 @@ func TestConfigurator_wrapTLS_OK(t *testing.T) { t.Fatalf("startTLSServer err: %v", <-errc) } - c := NewConfigurator(config) + c := NewConfigurator(config, nil) clientConfig, err := c.OutgoingRPCConfig() require.NoError(t, err) @@ -303,7 +303,7 @@ func TestConfigurator_wrapTLS_BadCert(t *testing.T) { VerifyOutgoing: true, } - c := NewConfigurator(clientConfig) + c := NewConfigurator(clientConfig, nil) clientTLSConfig, err := c.OutgoingRPCConfig() require.NoError(t, err) @@ -381,7 +381,7 @@ func TestConfig_ParseCiphers(t *testing.T) { func TestConfigurator_IncomingHTTPSConfig_CA_PATH(t *testing.T) { conf := Config{CAPath: "../test/ca_path"} - c := NewConfigurator(conf) + c := NewConfigurator(conf, nil) tlsConf, err := c.IncomingHTTPSConfig() require.NoError(t, err) require.Len(t, tlsConf.ClientCAs.Subjects(), 2) @@ -394,7 +394,7 @@ func TestConfigurator_IncomingHTTPS(t *testing.T) { CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key", } - c := NewConfigurator(conf) + c := NewConfigurator(conf, nil) tlsConf, err := c.IncomingHTTPSConfig() require.NoError(t, err) require.NotNil(t, tlsConf) @@ -409,7 +409,7 @@ func TestConfigurator_IncomingHTTPS_MissingCA(t *testing.T) { CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key", } - c := NewConfigurator(conf) + c := NewConfigurator(conf, nil) _, err := c.IncomingHTTPSConfig() require.Error(t, err) } @@ -419,14 +419,14 @@ func TestConfigurator_IncomingHTTPS_MissingKey(t *testing.T) { VerifyIncoming: true, CAFile: "../test/ca/root.cer", } - c := NewConfigurator(conf) + c := NewConfigurator(conf, nil) _, err := c.IncomingHTTPSConfig() require.Error(t, err) } func TestConfigurator_IncomingHTTPS_NoVerify(t *testing.T) { conf := Config{} - c := NewConfigurator(conf) + c := NewConfigurator(conf, nil) tlsConf, err := c.IncomingHTTPSConfig() require.NoError(t, err) require.NotNil(t, tlsConf) @@ -445,7 +445,7 @@ func TestConfigurator_IncomingHTTPS_TLSMinVersion(t *testing.T) { KeyFile: "../test/key/ourdomain.key", TLSMinVersion: version, } - c := NewConfigurator(conf) + c := NewConfigurator(conf, nil) tlsConf, err := c.IncomingHTTPSConfig() require.NoError(t, err) require.NotNil(t, tlsConf) @@ -455,7 +455,7 @@ func TestConfigurator_IncomingHTTPS_TLSMinVersion(t *testing.T) { func TestConfigurator_IncomingHTTPSCAPath_Valid(t *testing.T) { - c := NewConfigurator(Config{CAPath: "../test/ca_path"}) + c := NewConfigurator(Config{CAPath: "../test/ca_path"}, nil) tlsConf, err := c.IncomingHTTPSConfig() require.NoError(t, err) require.Len(t, tlsConf.ClientCAs.Subjects(), 2) @@ -475,7 +475,7 @@ func TestConfigurator_CommonTLSConfigServerNameNodeName(t *testing.T) { result: "node"}, } for _, v := range variants { - c := NewConfigurator(v.config) + c := NewConfigurator(v.config, nil) tlsConf, err := c.commonTLSConfig(false) require.NoError(t, err) require.Empty(t, tlsConf.ServerName) @@ -483,7 +483,7 @@ func TestConfigurator_CommonTLSConfigServerNameNodeName(t *testing.T) { } func TestConfigurator_CommonTLSConfigCipherSuites(t *testing.T) { - c := NewConfigurator(Config{}) + c := NewConfigurator(Config{}, nil) tlsConfig, err := c.commonTLSConfig(false) require.NoError(t, err) require.Empty(t, tlsConfig.CipherSuites) @@ -496,7 +496,7 @@ func TestConfigurator_CommonTLSConfigCipherSuites(t *testing.T) { } func TestConfigurator_CommonTLSConfigCertKey(t *testing.T) { - c := NewConfigurator(Config{}) + c := NewConfigurator(Config{}, nil) tlsConf, err := c.commonTLSConfig(false) require.NoError(t, err) require.Empty(t, tlsConf.Certificates) @@ -514,25 +514,25 @@ func TestConfigurator_CommonTLSConfigCertKey(t *testing.T) { func TestConfigurator_CommonTLSConfigTLSMinVersion(t *testing.T) { tlsVersions := []string{"tls10", "tls11", "tls12"} for _, version := range tlsVersions { - c := NewConfigurator(Config{TLSMinVersion: version}) + c := NewConfigurator(Config{TLSMinVersion: version}, nil) tlsConf, err := c.commonTLSConfig(false) require.NoError(t, err) require.Equal(t, tlsConf.MinVersion, TLSLookup[version]) } - c := NewConfigurator(Config{TLSMinVersion: "tlsBOGUS"}) + c := NewConfigurator(Config{TLSMinVersion: "tlsBOGUS"}, nil) _, err := c.commonTLSConfig(false) require.Error(t, err) } func TestConfigurator_CommonTLSConfigValidateVerifyOutgoingCA(t *testing.T) { - c := NewConfigurator(Config{VerifyOutgoing: true}) + c := NewConfigurator(Config{VerifyOutgoing: true}, nil) _, err := c.commonTLSConfig(false) require.Error(t, err) } func TestConfigurator_CommonTLSConfigLoadCA(t *testing.T) { - c := NewConfigurator(Config{}) + c := NewConfigurator(Config{}, nil) tlsConf, err := c.commonTLSConfig(false) require.NoError(t, err) require.Nil(t, tlsConf.RootCAs) @@ -566,7 +566,7 @@ func TestConfigurator_CommonTLSConfigLoadCA(t *testing.T) { } func TestConfigurator_CommonTLSConfigVerifyIncoming(t *testing.T) { - c := NewConfigurator(Config{}) + c := NewConfigurator(Config{}, nil) tlsConf, err := c.commonTLSConfig(false) require.NoError(t, err) require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) @@ -605,7 +605,7 @@ func TestConfigurator_CommonTLSConfigVerifyIncoming(t *testing.T) { } func TestConfigurator_IncomingRPCConfig(t *testing.T) { - c := NewConfigurator(Config{}) + c := NewConfigurator(Config{}, nil) tlsConf, err := c.IncomingRPCConfig() require.NoError(t, err) require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) @@ -631,7 +631,7 @@ func TestConfigurator_IncomingRPCConfig(t *testing.T) { } func TestConfigurator_IncomingHTTPSConfig(t *testing.T) { - c := NewConfigurator(Config{}) + c := NewConfigurator(Config{}, nil) tlsConf, err := c.IncomingHTTPSConfig() require.NoError(t, err) require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) @@ -657,7 +657,7 @@ func TestConfigurator_IncomingHTTPSConfig(t *testing.T) { } func TestConfigurator_OutgoingRPCConfig(t *testing.T) { - c := NewConfigurator(Config{}) + c := NewConfigurator(Config{}, nil) tlsConf, err := c.OutgoingRPCConfig() require.NoError(t, err) require.Nil(t, tlsConf) @@ -676,7 +676,7 @@ func TestConfigurator_OutgoingRPCConfig(t *testing.T) { } func TestConfigurator_OutgoingTLSConfigForChecks(t *testing.T) { - c := NewConfigurator(Config{EnableAgentTLSForChecks: false}) + c := NewConfigurator(Config{EnableAgentTLSForChecks: false}, nil) tlsConf, err := c.OutgoingTLSConfigForCheck(false) require.NoError(t, err) require.False(t, tlsConf.InsecureSkipVerify) @@ -713,10 +713,17 @@ func TestConfigurator_OutgoingTLSConfigForChecks(t *testing.T) { } func TestConfigurator_Check(t *testing.T) { - c := NewConfigurator(Config{}) + c := NewConfigurator(Config{}, nil) require.NoError(t, c.Check(Config{})) require.Error(t, c.Check(Config{VerifyOutgoing: true})) require.Error(t, c.Check(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer"})) require.False(t, c.base.VerifyIncoming) require.False(t, c.base.VerifyOutgoing) } + +func TestConfigurator_Version(t *testing.T) { + c := NewConfigurator(Config{}, nil) + require.Equal(t, c.version, 1) + c.Update(Config{VerifyOutgoing: true}) + require.Equal(t, c.version, 2) +} From b111993cd2930f8cfdf0351a8b269428d8cde878 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Tue, 5 Mar 2019 21:24:58 +0100 Subject: [PATCH 13/31] that refactor.... --- agent/agent.go | 9 +- agent/consul/client.go | 6 +- agent/consul/server.go | 6 +- agent/testagent.go | 10 ++- tlsutil/config.go | 89 +++++++++++------- tlsutil/config_test.go | 200 +++++++++++++++++++++-------------------- 6 files changed, 184 insertions(+), 136 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 960fb495448d..e366a72eee24 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -393,7 +393,11 @@ func (a *Agent) Start() error { // waiting to discover a consul server consulCfg.ServerUp = a.sync.SyncFull.Trigger - a.tlsConfigurator = tlsutil.NewConfigurator(c.ToTLSUtilConfig(), a.logger) + tlsConfigurator, err := tlsutil.NewConfigurator(c.ToTLSUtilConfig(), a.logger) + if err != nil { + return err + } + a.tlsConfigurator = tlsConfigurator // Setup either the client or the server. if c.ServerMode { @@ -3563,10 +3567,9 @@ func (a *Agent) ReloadConfig(newCfg *config.RuntimeConfig) error { a.loadTokens(newCfg) newTLSConfig := newCfg.ToTLSUtilConfig() - if err := a.tlsConfigurator.Check(newTLSConfig); err != nil { + if err := a.tlsConfigurator.Update(newTLSConfig); err != nil { return fmt.Errorf("Failed reloading tls configuration: %s", err) } - a.tlsConfigurator.Update(newTLSConfig) // Reload service/check definitions and metadata. if err := a.loadServices(newCfg); err != nil { diff --git a/agent/consul/client.go b/agent/consul/client.go index af092d4fc7ff..db35544ac404 100644 --- a/agent/consul/client.go +++ b/agent/consul/client.go @@ -89,7 +89,11 @@ type Client struct { // NewClient is used to construct a new Consul client from the // configuration, potentially returning an error func NewClient(config *Config) (*Client, error) { - return NewClientLogger(config, nil, tlsutil.NewConfigurator(config.ToTLSUtilConfig(), nil)) + c, err := tlsutil.NewConfigurator(config.ToTLSUtilConfig(), nil) + if err != nil { + return nil, err + } + return NewClientLogger(config, nil, c) } func NewClientLogger(config *Config, logger *log.Logger, tlsConfigurator *tlsutil.Configurator) (*Client, error) { diff --git a/agent/consul/server.go b/agent/consul/server.go index e0ddd38dbfc0..e6ea325daf29 100644 --- a/agent/consul/server.go +++ b/agent/consul/server.go @@ -253,7 +253,11 @@ type Server struct { } func NewServer(config *Config) (*Server, error) { - return NewServerLogger(config, nil, new(token.Store), tlsutil.NewConfigurator(config.ToTLSUtilConfig(), nil)) + c, err := tlsutil.NewConfigurator(config.ToTLSUtilConfig(), nil) + if err != nil { + return nil, err + } + return NewServerLogger(config, nil, new(token.Store), c) } // NewServer is used to construct a new Consul server from the diff --git a/agent/testagent.go b/agent/testagent.go index 77e16e5e6f7f..64343f071a15 100644 --- a/agent/testagent.go +++ b/agent/testagent.go @@ -115,7 +115,11 @@ func NewUnstartedAgent(t *testing.T, name string, hcl string) (*Agent, error) { a.sync = ae.NewStateSyncer(a.State, c.AEInterval, a.shutdownCh, a.logger) a.delegate = &consul.Client{} a.State.TriggerSyncChanges = a.sync.SyncChanges.Trigger - a.tlsConfigurator = tlsutil.NewConfigurator(c.ToTLSUtilConfig(), nil) + tlsConfigurator, err := tlsutil.NewConfigurator(c.ToTLSUtilConfig(), nil) + if err != nil { + return nil, err + } + a.tlsConfigurator = tlsConfigurator return a, nil } @@ -165,7 +169,9 @@ func (a *TestAgent) Start(t *testing.T) *TestAgent { agent.LogWriter = a.LogWriter agent.logger = log.New(logOutput, a.Name+" - ", log.LstdFlags|log.Lmicroseconds) agent.MemSink = metrics.NewInmemSink(1*time.Second, time.Minute) - agent.tlsConfigurator = tlsutil.NewConfigurator(a.Config.ToTLSUtilConfig(), nil) + tlsConfigurator, err := tlsutil.NewConfigurator(a.Config.ToTLSUtilConfig(), nil) + require.NoError(err) + agent.tlsConfigurator = tlsConfigurator // we need the err var in the next exit condition if err := agent.Start(); err == nil { diff --git a/tlsutil/config.go b/tlsutil/config.go index c2de56645719..1801e71ad4d1 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -199,36 +199,74 @@ func (c *Config) wrapTLSClient(conn net.Conn, tlsConfig *tls.Config) (net.Conn, type Configurator struct { sync.RWMutex base *Config + cert *tls.Certificate + cas *x509.CertPool logger *log.Logger version int } // NewConfigurator creates a new Configurator and sets the provided // configuration. -func NewConfigurator(config Config, logger *log.Logger) *Configurator { - return &Configurator{base: &config, logger: logger, version: 1} +func NewConfigurator(config Config, logger *log.Logger) (*Configurator, error) { + c := &Configurator{logger: logger} + err := c.Update(config) + if err != nil { + return nil, err + } + return c, nil } // Update updates the internal configuration which is used to generate // *tls.Config. -func (c *Configurator) Update(config Config) { +func (c *Configurator) Update(config Config) error { c.Lock() defer c.Unlock() + cert, err := c.loadKeyPair(config.CertFile, config.KeyFile) + if err != nil { + return err + } + cas, err := c.loadCAs(config.CAFile, config.CAPath) + if err != nil { + return err + } + + origConfig := c.base + origCert := c.cert + origCAs := c.cas + c.base = &config + c.cert = cert + c.cas = cas + _, err = c.commonTLSConfig(c.base.VerifyIncomingRPC || c.base.VerifyIncomingHTTPS) + if err != nil { + c.base = origConfig + c.cert = origCert + c.cas = origCAs + return err + } c.version++ + return nil +} +func (c *Configurator) loadKeyPair(certFile, keyFile string) (*tls.Certificate, error) { + if certFile == "" || keyFile == "" { + return nil, nil + } + cert, err := tls.LoadX509KeyPair(certFile, keyFile) + if err != nil { + return nil, fmt.Errorf("Failed to load cert/key pair: %v", err) + } + return &cert, nil } -// Check is verifying the provided configuration and returns an error if the -// config has problems. -func (c *Configurator) Check(config Config) error { - c.Lock() - defer c.Unlock() - orig := c.base - c.base = &config - defer func() { c.base = orig }() - _, err := c.commonTLSConfig(c.base.VerifyIncomingRPC || c.base.VerifyIncomingHTTPS) - return err +func (c *Configurator) loadCAs(caFile, caPath string) (*x509.CertPool, error) { + if caFile != "" { + return rootcerts.LoadCAFile(caFile) + } else if caPath != "" { + return rootcerts.LoadCAPath(caPath) + } + return nil, nil + } // commonTLSConfig generates a *tls.Config from the base configuration the @@ -248,11 +286,8 @@ func (c *Configurator) commonTLSConfig(additionalVerifyIncomingFlag bool) (*tls. } // Add cert/key - cert, err := c.base.KeyPair() - if err != nil { - return nil, err - } else if cert != nil { - tlsConfig.Certificates = []tls.Certificate{*cert} + if c.cert != nil { + tlsConfig.Certificates = []tls.Certificate{*c.cert} } // Check if a minimum TLS version was set @@ -269,21 +304,9 @@ func (c *Configurator) commonTLSConfig(additionalVerifyIncomingFlag bool) (*tls. return nil, fmt.Errorf("VerifyOutgoing set, and no CA certificate provided!") } - // Parse the CA certs if any - if c.base.CAFile != "" { - pool, err := rootcerts.LoadCAFile(c.base.CAFile) - if err != nil { - return nil, err - } - tlsConfig.ClientCAs = pool - tlsConfig.RootCAs = pool - } else if c.base.CAPath != "" { - pool, err := rootcerts.LoadCAPath(c.base.CAPath) - if err != nil { - return nil, err - } - tlsConfig.ClientCAs = pool - tlsConfig.RootCAs = pool + if c.cas != nil { + tlsConfig.ClientCAs = c.cas + tlsConfig.RootCAs = c.cas } // Set ClientAuth if necessary diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index b613c22792c4..bb529b01be9f 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -43,17 +43,17 @@ func TestConfigurator_OutgoingTLS_MissingCA(t *testing.T) { conf := Config{ VerifyOutgoing: true, } - c := NewConfigurator(conf, nil) - tlsConf, err := c.OutgoingRPCConfig() + c, err := NewConfigurator(conf, nil) require.Error(t, err) - require.Nil(t, tlsConf) + require.Nil(t, c) } func TestConfigurator_OutgoingTLS_OnlyCA(t *testing.T) { conf := Config{ CAFile: "../test/ca/root.cer", } - c := NewConfigurator(conf, nil) + c, err := NewConfigurator(conf, nil) + require.NoError(t, err) tlsConf, err := c.OutgoingRPCConfig() require.NoError(t, err) require.NotNil(t, tlsConf) @@ -64,7 +64,8 @@ func TestConfigurator_OutgoingTLS_VerifyOutgoing(t *testing.T) { VerifyOutgoing: true, CAFile: "../test/ca/root.cer", } - c := NewConfigurator(conf, nil) + c, err := NewConfigurator(conf, nil) + require.NoError(t, err) tlsConf, err := c.OutgoingRPCConfig() require.NoError(t, err) require.NotNil(t, tlsConf) @@ -79,7 +80,8 @@ func TestConfigurator_OutgoingTLS_ServerName(t *testing.T) { CAFile: "../test/ca/root.cer", ServerName: "consul.example.com", } - c := NewConfigurator(conf, nil) + c, err := NewConfigurator(conf, nil) + require.NoError(t, err) tlsConf, err := c.OutgoingRPCConfig() require.NoError(t, err) require.NotNil(t, tlsConf) @@ -94,7 +96,8 @@ func TestConfigurator_OutgoingTLS_VerifyHostname(t *testing.T) { VerifyServerHostname: true, CAFile: "../test/ca/root.cer", } - c := NewConfigurator(conf, nil) + c, err := NewConfigurator(conf, nil) + require.NoError(t, err) tlsConf, err := c.OutgoingRPCConfig() require.NoError(t, err) require.NotNil(t, tlsConf) @@ -109,7 +112,8 @@ func TestConfigurator_OutgoingTLS_WithKeyPair(t *testing.T) { CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key", } - c := NewConfigurator(conf, nil) + c, err := NewConfigurator(conf, nil) + require.NoError(t, err) tlsConf, err := c.OutgoingRPCConfig() require.NoError(t, err) require.NotNil(t, tlsConf) @@ -125,7 +129,8 @@ func TestConfigurator_OutgoingTLS_TLSMinVersion(t *testing.T) { CAFile: "../test/ca/root.cer", TLSMinVersion: version, } - c := NewConfigurator(conf, nil) + c, err := NewConfigurator(conf, nil) + require.NoError(t, err) tlsConf, err := c.OutgoingRPCConfig() require.NoError(t, err) require.NotNil(t, tlsConf) @@ -136,7 +141,11 @@ func TestConfigurator_OutgoingTLS_TLSMinVersion(t *testing.T) { func startTLSServer(config *Config) (net.Conn, chan error) { errc := make(chan error, 1) - c := NewConfigurator(*config, nil) + c, err := NewConfigurator(*config, nil) + if err != nil { + errc <- err + return nil, errc + } tlsConfigServer, err := c.IncomingRPCConfig() if err != nil { errc <- err @@ -186,7 +195,8 @@ func TestConfigurator_outgoingWrapper_OK(t *testing.T) { t.Fatalf("startTLSServer err: %v", <-errc) } - c := NewConfigurator(config, nil) + c, err := NewConfigurator(config, nil) + require.NoError(t, err) wrap, err := c.OutgoingRPCWrapper() require.NoError(t, err) @@ -216,7 +226,8 @@ func TestConfigurator_outgoingWrapper_BadDC(t *testing.T) { t.Fatalf("startTLSServer err: %v", <-errc) } - c := NewConfigurator(config, nil) + c, err := NewConfigurator(config, nil) + require.NoError(t, err) wrap, err := c.OutgoingRPCWrapper() require.NoError(t, err) @@ -246,7 +257,8 @@ func TestConfigurator_outgoingWrapper_BadCert(t *testing.T) { t.Fatalf("startTLSServer err: %v", <-errc) } - c := NewConfigurator(config, nil) + c, err := NewConfigurator(config, nil) + require.NoError(t, err) wrap, err := c.OutgoingRPCWrapper() require.NoError(t, err) @@ -275,7 +287,8 @@ func TestConfigurator_wrapTLS_OK(t *testing.T) { t.Fatalf("startTLSServer err: %v", <-errc) } - c := NewConfigurator(config, nil) + c, err := NewConfigurator(config, nil) + require.NoError(t, err) clientConfig, err := c.OutgoingRPCConfig() require.NoError(t, err) @@ -303,7 +316,8 @@ func TestConfigurator_wrapTLS_BadCert(t *testing.T) { VerifyOutgoing: true, } - c := NewConfigurator(clientConfig, nil) + c, err := NewConfigurator(clientConfig, nil) + require.NoError(t, err) clientTLSConfig, err := c.OutgoingRPCConfig() require.NoError(t, err) @@ -381,7 +395,8 @@ func TestConfig_ParseCiphers(t *testing.T) { func TestConfigurator_IncomingHTTPSConfig_CA_PATH(t *testing.T) { conf := Config{CAPath: "../test/ca_path"} - c := NewConfigurator(conf, nil) + c, err := NewConfigurator(conf, nil) + require.NoError(t, err) tlsConf, err := c.IncomingHTTPSConfig() require.NoError(t, err) require.Len(t, tlsConf.ClientCAs.Subjects(), 2) @@ -394,7 +409,8 @@ func TestConfigurator_IncomingHTTPS(t *testing.T) { CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key", } - c := NewConfigurator(conf, nil) + c, err := NewConfigurator(conf, nil) + require.NoError(t, err) tlsConf, err := c.IncomingHTTPSConfig() require.NoError(t, err) require.NotNil(t, tlsConf) @@ -409,8 +425,7 @@ func TestConfigurator_IncomingHTTPS_MissingCA(t *testing.T) { CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key", } - c := NewConfigurator(conf, nil) - _, err := c.IncomingHTTPSConfig() + _, err := NewConfigurator(conf, nil) require.Error(t, err) } @@ -419,14 +434,14 @@ func TestConfigurator_IncomingHTTPS_MissingKey(t *testing.T) { VerifyIncoming: true, CAFile: "../test/ca/root.cer", } - c := NewConfigurator(conf, nil) - _, err := c.IncomingHTTPSConfig() + _, err := NewConfigurator(conf, nil) require.Error(t, err) } func TestConfigurator_IncomingHTTPS_NoVerify(t *testing.T) { conf := Config{} - c := NewConfigurator(conf, nil) + c, err := NewConfigurator(conf, nil) + require.NoError(t, err) tlsConf, err := c.IncomingHTTPSConfig() require.NoError(t, err) require.NotNil(t, tlsConf) @@ -445,7 +460,8 @@ func TestConfigurator_IncomingHTTPS_TLSMinVersion(t *testing.T) { KeyFile: "../test/key/ourdomain.key", TLSMinVersion: version, } - c := NewConfigurator(conf, nil) + c, err := NewConfigurator(conf, nil) + require.NoError(t, err) tlsConf, err := c.IncomingHTTPSConfig() require.NoError(t, err) require.NotNil(t, tlsConf) @@ -455,7 +471,8 @@ func TestConfigurator_IncomingHTTPS_TLSMinVersion(t *testing.T) { func TestConfigurator_IncomingHTTPSCAPath_Valid(t *testing.T) { - c := NewConfigurator(Config{CAPath: "../test/ca_path"}, nil) + c, err := NewConfigurator(Config{CAPath: "../test/ca_path"}, nil) + require.NoError(t, err) tlsConf, err := c.IncomingHTTPSConfig() require.NoError(t, err) require.Len(t, tlsConf.ClientCAs.Subjects(), 2) @@ -475,7 +492,8 @@ func TestConfigurator_CommonTLSConfigServerNameNodeName(t *testing.T) { result: "node"}, } for _, v := range variants { - c := NewConfigurator(v.config, nil) + c, err := NewConfigurator(v.config, nil) + require.NoError(t, err) tlsConf, err := c.commonTLSConfig(false) require.NoError(t, err) require.Empty(t, tlsConf.ServerName) @@ -483,29 +501,29 @@ func TestConfigurator_CommonTLSConfigServerNameNodeName(t *testing.T) { } func TestConfigurator_CommonTLSConfigCipherSuites(t *testing.T) { - c := NewConfigurator(Config{}, nil) + c, err := NewConfigurator(Config{}, nil) + require.NoError(t, err) tlsConfig, err := c.commonTLSConfig(false) require.NoError(t, err) require.Empty(t, tlsConfig.CipherSuites) conf := Config{CipherSuites: []uint16{tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305}} - c.Update(conf) + require.NoError(t, c.Update(conf)) tlsConfig, err = c.commonTLSConfig(false) require.NoError(t, err) require.Equal(t, conf.CipherSuites, tlsConfig.CipherSuites) } func TestConfigurator_CommonTLSConfigCertKey(t *testing.T) { - c := NewConfigurator(Config{}, nil) + c, err := NewConfigurator(Config{}, nil) + require.NoError(t, err) tlsConf, err := c.commonTLSConfig(false) require.NoError(t, err) require.Empty(t, tlsConf.Certificates) - c.Update(Config{CertFile: "/something/bogus", KeyFile: "/more/bogus"}) - tlsConf, err = c.commonTLSConfig(false) - require.Error(t, err) + require.Error(t, c.Update(Config{CertFile: "/something/bogus", KeyFile: "/more/bogus"})) - c.Update(Config{CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) + require.NoError(t, c.Update(Config{CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) tlsConf, err = c.commonTLSConfig(false) require.NoError(t, err) require.Len(t, tlsConf.Certificates, 1) @@ -514,51 +532,45 @@ func TestConfigurator_CommonTLSConfigCertKey(t *testing.T) { func TestConfigurator_CommonTLSConfigTLSMinVersion(t *testing.T) { tlsVersions := []string{"tls10", "tls11", "tls12"} for _, version := range tlsVersions { - c := NewConfigurator(Config{TLSMinVersion: version}, nil) + c, err := NewConfigurator(Config{TLSMinVersion: version}, nil) + require.NoError(t, err) tlsConf, err := c.commonTLSConfig(false) require.NoError(t, err) require.Equal(t, tlsConf.MinVersion, TLSLookup[version]) } - c := NewConfigurator(Config{TLSMinVersion: "tlsBOGUS"}, nil) - _, err := c.commonTLSConfig(false) + _, err := NewConfigurator(Config{TLSMinVersion: "tlsBOGUS"}, nil) require.Error(t, err) } func TestConfigurator_CommonTLSConfigValidateVerifyOutgoingCA(t *testing.T) { - c := NewConfigurator(Config{VerifyOutgoing: true}, nil) - _, err := c.commonTLSConfig(false) + _, err := NewConfigurator(Config{VerifyOutgoing: true}, nil) require.Error(t, err) } func TestConfigurator_CommonTLSConfigLoadCA(t *testing.T) { - c := NewConfigurator(Config{}, nil) + c, err := NewConfigurator(Config{}, nil) + require.NoError(t, err) tlsConf, err := c.commonTLSConfig(false) require.NoError(t, err) require.Nil(t, tlsConf.RootCAs) require.Nil(t, tlsConf.ClientCAs) - c.Update(Config{CAFile: "/something/bogus"}) - _, err = c.commonTLSConfig(false) - require.Error(t, err) - - c.Update(Config{CAPath: "/something/bogus/"}) - _, err = c.commonTLSConfig(false) - require.Error(t, err) - - c.Update(Config{CAFile: "../test/ca/root.cer"}) + require.Error(t, c.Update(Config{CAFile: "/something/bogus"})) + require.Error(t, c.Update(Config{CAPath: "/something/bogus/"})) + require.NoError(t, c.Update(Config{CAFile: "../test/ca/root.cer"})) tlsConf, err = c.commonTLSConfig(false) require.NoError(t, err) require.Len(t, tlsConf.RootCAs.Subjects(), 1) require.Len(t, tlsConf.ClientCAs.Subjects(), 1) - c.Update(Config{CAPath: "../test/ca_path"}) + require.NoError(t, c.Update(Config{CAPath: "../test/ca_path"})) tlsConf, err = c.commonTLSConfig(false) require.NoError(t, err) require.Len(t, tlsConf.RootCAs.Subjects(), 2) require.Len(t, tlsConf.ClientCAs.Subjects(), 2) - c.Update(Config{CAFile: "../test/ca/root.cer", CAPath: "../test/ca_path"}) + require.NoError(t, c.Update(Config{CAFile: "../test/ca/root.cer", CAPath: "../test/ca_path"})) tlsConf, err = c.commonTLSConfig(false) require.NoError(t, err) require.Len(t, tlsConf.RootCAs.Subjects(), 1) @@ -566,64 +578,57 @@ func TestConfigurator_CommonTLSConfigLoadCA(t *testing.T) { } func TestConfigurator_CommonTLSConfigVerifyIncoming(t *testing.T) { - c := NewConfigurator(Config{}, nil) + c, err := NewConfigurator(Config{}, nil) + require.NoError(t, err) tlsConf, err := c.commonTLSConfig(false) require.NoError(t, err) require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) - c.Update(Config{VerifyIncoming: true}) - tlsConf, err = c.commonTLSConfig(false) - require.Error(t, err) - - c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer"}) - tlsConf, err = c.commonTLSConfig(false) - require.Error(t, err) - - c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/cert/ourdomain.cer"}) - tlsConf, err = c.commonTLSConfig(false) - require.Error(t, err) - - c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) + require.Error(t, c.Update(Config{VerifyIncoming: true})) + require.Error(t, c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer"})) + require.Error(t, c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/cert/ourdomain.cer"})) + require.NoError(t, c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) tlsConf, err = c.commonTLSConfig(false) require.NoError(t, err) require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) - c.Update(Config{VerifyIncoming: false, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) + require.NoError(t, c.Update(Config{VerifyIncoming: false, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) tlsConf, err = c.commonTLSConfig(true) require.NoError(t, err) require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) - c.Update(Config{VerifyServerHostname: false, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) + require.NoError(t, c.Update(Config{VerifyServerHostname: false, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) tlsConf, err = c.commonTLSConfig(false) require.NoError(t, err) require.True(t, tlsConf.InsecureSkipVerify) - c.Update(Config{VerifyServerHostname: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) + require.NoError(t, c.Update(Config{VerifyServerHostname: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) tlsConf, err = c.commonTLSConfig(false) require.NoError(t, err) require.False(t, tlsConf.InsecureSkipVerify) } func TestConfigurator_IncomingRPCConfig(t *testing.T) { - c := NewConfigurator(Config{}, nil) + c, err := NewConfigurator(Config{}, nil) + require.NoError(t, err) tlsConf, err := c.IncomingRPCConfig() require.NoError(t, err) require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) require.NotNil(t, tlsConf.GetConfigForClient) - c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) + require.NoError(t, c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) tlsConf, err = c.IncomingRPCConfig() require.NoError(t, err) require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) require.NotNil(t, tlsConf.GetConfigForClient) - c.Update(Config{VerifyIncomingRPC: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) + require.NoError(t, c.Update(Config{VerifyIncomingRPC: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) tlsConf, err = c.IncomingRPCConfig() require.NoError(t, err) require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) require.NotNil(t, tlsConf.GetConfigForClient) - c.Update(Config{VerifyIncomingHTTPS: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) + require.NoError(t, c.Update(Config{VerifyIncomingHTTPS: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) tlsConf, err = c.IncomingRPCConfig() require.NoError(t, err) require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) @@ -631,25 +636,26 @@ func TestConfigurator_IncomingRPCConfig(t *testing.T) { } func TestConfigurator_IncomingHTTPSConfig(t *testing.T) { - c := NewConfigurator(Config{}, nil) + c, err := NewConfigurator(Config{}, nil) + require.NoError(t, err) tlsConf, err := c.IncomingHTTPSConfig() require.NoError(t, err) require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) require.NotNil(t, tlsConf.GetConfigForClient) - c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) + require.NoError(t, c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) tlsConf, err = c.IncomingHTTPSConfig() require.NoError(t, err) require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) require.NotNil(t, tlsConf.GetConfigForClient) - c.Update(Config{VerifyIncomingHTTPS: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) + require.NoError(t, c.Update(Config{VerifyIncomingHTTPS: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) tlsConf, err = c.IncomingHTTPSConfig() require.NoError(t, err) require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) require.NotNil(t, tlsConf.GetConfigForClient) - c.Update(Config{VerifyIncomingRPC: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"}) + require.NoError(t, c.Update(Config{VerifyIncomingRPC: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) tlsConf, err = c.IncomingHTTPSConfig() require.NoError(t, err) require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) @@ -657,73 +663,75 @@ func TestConfigurator_IncomingHTTPSConfig(t *testing.T) { } func TestConfigurator_OutgoingRPCConfig(t *testing.T) { - c := NewConfigurator(Config{}, nil) + c, err := NewConfigurator(Config{}, nil) + require.NoError(t, err) tlsConf, err := c.OutgoingRPCConfig() require.NoError(t, err) require.Nil(t, tlsConf) - c.Update(Config{VerifyOutgoing: true}) - tlsConf, err = c.OutgoingRPCConfig() - require.Error(t, err) - - c.Update(Config{VerifyOutgoing: true, CAFile: "../test/ca/root.cer"}) + require.Error(t, c.Update(Config{VerifyOutgoing: true})) + require.NoError(t, c.Update(Config{VerifyOutgoing: true, CAFile: "../test/ca/root.cer"})) tlsConf, err = c.OutgoingRPCConfig() require.NoError(t, err) - c.Update(Config{VerifyOutgoing: true, CAPath: "../test/ca_path"}) + require.NoError(t, c.Update(Config{VerifyOutgoing: true, CAPath: "../test/ca_path"})) tlsConf, err = c.OutgoingRPCConfig() require.NoError(t, err) } func TestConfigurator_OutgoingTLSConfigForChecks(t *testing.T) { - c := NewConfigurator(Config{EnableAgentTLSForChecks: false}, nil) + c, err := NewConfigurator(Config{EnableAgentTLSForChecks: false}, nil) + require.NoError(t, err) tlsConf, err := c.OutgoingTLSConfigForCheck(false) require.NoError(t, err) require.False(t, tlsConf.InsecureSkipVerify) - c.Update(Config{EnableAgentTLSForChecks: false}) + require.NoError(t, c.Update(Config{EnableAgentTLSForChecks: false})) tlsConf, err = c.OutgoingTLSConfigForCheck(true) require.NoError(t, err) require.True(t, tlsConf.InsecureSkipVerify) - c.Update(Config{EnableAgentTLSForChecks: true}) + require.NoError(t, c.Update(Config{EnableAgentTLSForChecks: true})) tlsConf, err = c.OutgoingTLSConfigForCheck(false) require.NoError(t, err) require.False(t, tlsConf.InsecureSkipVerify) - c.Update(Config{EnableAgentTLSForChecks: true}) + require.NoError(t, c.Update(Config{EnableAgentTLSForChecks: true})) tlsConf, err = c.OutgoingTLSConfigForCheck(true) require.NoError(t, err) require.True(t, tlsConf.InsecureSkipVerify) - c.Update(Config{EnableAgentTLSForChecks: true, NodeName: "node", ServerName: "server"}) + require.NoError(t, c.Update(Config{EnableAgentTLSForChecks: true, NodeName: "node", ServerName: "server"})) tlsConf, err = c.OutgoingTLSConfigForCheck(false) require.NoError(t, err) require.Equal(t, "server", tlsConf.ServerName) - c.Update(Config{EnableAgentTLSForChecks: true, ServerName: "server"}) + require.NoError(t, c.Update(Config{EnableAgentTLSForChecks: true, ServerName: "server"})) tlsConf, err = c.OutgoingTLSConfigForCheck(false) require.NoError(t, err) require.Equal(t, "server", tlsConf.ServerName) - c.Update(Config{EnableAgentTLSForChecks: true, NodeName: "node"}) + require.NoError(t, c.Update(Config{EnableAgentTLSForChecks: true, NodeName: "node"})) tlsConf, err = c.OutgoingTLSConfigForCheck(false) require.NoError(t, err) require.Equal(t, "node", tlsConf.ServerName) } -func TestConfigurator_Check(t *testing.T) { - c := NewConfigurator(Config{}, nil) - require.NoError(t, c.Check(Config{})) - require.Error(t, c.Check(Config{VerifyOutgoing: true})) - require.Error(t, c.Check(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer"})) +func TestConfigurator_UpdateChecks(t *testing.T) { + c, err := NewConfigurator(Config{}, nil) + require.NoError(t, err) + require.NoError(t, c.Update(Config{})) + require.Error(t, c.Update(Config{VerifyOutgoing: true})) + require.Error(t, c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer"})) require.False(t, c.base.VerifyIncoming) require.False(t, c.base.VerifyOutgoing) + require.Equal(t, c.version, 2) } func TestConfigurator_Version(t *testing.T) { - c := NewConfigurator(Config{}, nil) + c, err := NewConfigurator(Config{}, nil) + require.NoError(t, err) + require.Equal(t, c.version, 1) + require.Error(t, c.Update(Config{VerifyOutgoing: true})) require.Equal(t, c.version, 1) - c.Update(Config{VerifyOutgoing: true}) - require.Equal(t, c.version, 2) } From 10aef9cc80ed23c863df5472d3820710bbd37667 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Wed, 6 Mar 2019 10:12:08 +0100 Subject: [PATCH 14/31] woot --- agent/agent.go | 16 +-- agent/consul/server.go | 8 +- tlsutil/config.go | 265 ++++++++++++++++++++--------------------- tlsutil/config_test.go | 161 +++++++++---------------- 4 files changed, 185 insertions(+), 265 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index e366a72eee24..d9b06763a5a8 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -666,10 +666,7 @@ func (a *Agent) listenHTTP() ([]*HTTPServer, error) { var tlscfg *tls.Config _, isTCP := l.(*tcpKeepAliveListener) if isTCP && proto == "https" { - tlscfg, err = a.tlsConfigurator.IncomingHTTPSConfig() - if err != nil { - return err - } + tlscfg = a.tlsConfigurator.IncomingHTTPSConfig() l = tls.NewListener(l, tlscfg) } srv := &HTTPServer{ @@ -2236,10 +2233,7 @@ func (a *Agent) addCheck(check *structs.HealthCheck, chkType *structs.CheckType, chkType.Interval = checks.MinInterval } - tlsClientConfig, err := a.tlsConfigurator.OutgoingTLSConfigForCheck(chkType.TLSSkipVerify) - if err != nil { - return fmt.Errorf("Failed to set up TLS: %v", err) - } + tlsClientConfig := a.tlsConfigurator.OutgoingTLSConfigForCheck(chkType.TLSSkipVerify) http := &checks.CheckHTTP{ Notify: a.State, @@ -2290,11 +2284,7 @@ func (a *Agent) addCheck(check *structs.HealthCheck, chkType *structs.CheckType, var tlsClientConfig *tls.Config if chkType.GRPCUseTLS { - var err error - tlsClientConfig, err = a.tlsConfigurator.OutgoingTLSConfigForCheck(chkType.TLSSkipVerify) - if err != nil { - return fmt.Errorf("Failed to set up TLS: %v", err) - } + tlsClientConfig = a.tlsConfigurator.OutgoingTLSConfigForCheck(chkType.TLSSkipVerify) } grpc := &checks.CheckGRPC{ diff --git a/agent/consul/server.go b/agent/consul/server.go index e6ea325daf29..1b1c79122f8a 100644 --- a/agent/consul/server.go +++ b/agent/consul/server.go @@ -306,12 +306,6 @@ func NewServerLogger(config *Config, logger *log.Logger, tokens *token.Store, tl return nil, err } - // Get the incoming TLS config. - incomingTLS, err := tlsConfigurator.IncomingRPCConfig() - if err != nil { - return nil, err - } - // Create the tombstone GC. gc, err := state.NewTombstoneGC(config.TombstoneTTL, config.TombstoneTTLGranularity) if err != nil { @@ -342,7 +336,7 @@ func NewServerLogger(config *Config, logger *log.Logger, tokens *token.Store, tl reconcileCh: make(chan serf.Member, reconcileChSize), router: router.NewRouter(logger, config.Datacenter), rpcServer: rpc.NewServer(), - rpcTLS: incomingTLS, + rpcTLS: tlsConfigurator.IncomingRPCConfig(), reassertLeaderCh: make(chan chan error), segmentLAN: make(map[string]*serf.Serf, len(config.Segments)), sessionTimers: NewSessionTimers(), diff --git a/tlsutil/config.go b/tlsutil/config.go index 1801e71ad4d1..5b1a7372b8c3 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -136,64 +136,6 @@ func SpecificDC(dc string, tlsWrap DCWrapper) Wrapper { } } -// Wrap a net.Conn into a client tls connection, performing any -// additional verification as needed. -// -// As of go 1.3, crypto/tls only supports either doing no certificate -// verification, or doing full verification including of the peer's -// DNS name. For consul, we want to validate that the certificate is -// signed by a known CA, but because consul doesn't use DNS names for -// node names, we don't verify the certificate DNS names. Since go 1.3 -// no longer supports this mode of operation, we have to do it -// manually. -func (c *Config) wrapTLSClient(conn net.Conn, tlsConfig *tls.Config) (net.Conn, error) { - var err error - var tlsConn *tls.Conn - - tlsConn = tls.Client(conn, tlsConfig) - - // If crypto/tls is doing verification, there's no need to do - // our own. - if tlsConfig.InsecureSkipVerify == false { - return tlsConn, nil - } - - // If verification is not turned on, don't do it. - if !c.VerifyOutgoing { - return tlsConn, nil - } - - if err = tlsConn.Handshake(); err != nil { - tlsConn.Close() - return nil, err - } - - // The following is lightly-modified from the doFullHandshake - // method in crypto/tls's handshake_client.go. - opts := x509.VerifyOptions{ - Roots: tlsConfig.RootCAs, - CurrentTime: time.Now(), - DNSName: "", - Intermediates: x509.NewCertPool(), - } - - certs := tlsConn.ConnectionState().PeerCertificates - for i, cert := range certs { - if i == 0 { - continue - } - opts.Intermediates.AddCert(cert) - } - - _, err = certs[0].Verify(opts) - if err != nil { - tlsConn.Close() - return nil, err - } - - return tlsConn, err -} - // Configurator holds a Config and is responsible for generating all the // *tls.Config necessary for Consul. Except the one in the api package. type Configurator struct { @@ -230,21 +172,39 @@ func (c *Configurator) Update(config Config) error { return err } - origConfig := c.base - origCert := c.cert - origCAs := c.cas - + if err = c.check(config, cert); err != nil { + return err + } c.base = &config c.cert = cert c.cas = cas - _, err = c.commonTLSConfig(c.base.VerifyIncomingRPC || c.base.VerifyIncomingHTTPS) - if err != nil { - c.base = origConfig - c.cert = origCert - c.cas = origCAs - return err - } c.version++ + c.log("Update") + return nil +} + +func (c *Configurator) check(config Config, cert *tls.Certificate) error { + // 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) + } + } + + // Ensure we have a CA if VerifyOutgoing is set + if config.VerifyOutgoing && config.CAFile == "" && config.CAPath == "" { + return fmt.Errorf("VerifyOutgoing set, and no CA certificate provided!") + } + + // Ensure we have a CA and cert if VerifyIncoming is set + if config.VerifyIncoming || config.VerifyIncomingRPC || config.VerifyIncomingHTTPS { + if config.CAFile == "" && config.CAPath == "" { + return fmt.Errorf("VerifyIncoming set, and no CA certificate provided!") + } + if cert == nil { + return fmt.Errorf("VerifyIncoming set, and no Cert/Key pair provided!") + } + } return nil } @@ -266,13 +226,12 @@ func (c *Configurator) loadCAs(caFile, caPath string) (*x509.CertPool, error) { return rootcerts.LoadCAPath(caPath) } return nil, nil - } // commonTLSConfig generates a *tls.Config from the base configuration the // Configurator has. It accepts an additional flag in case a config is needed // for incoming TLS connections. -func (c *Configurator) commonTLSConfig(additionalVerifyIncomingFlag bool) (*tls.Config, error) { +func (c *Configurator) commonTLSConfig(additionalVerifyIncomingFlag bool) *tls.Config { tlsConfig := &tls.Config{ InsecureSkipVerify: !c.base.VerifyServerHostname, } @@ -285,23 +244,11 @@ func (c *Configurator) commonTLSConfig(additionalVerifyIncomingFlag bool) (*tls. tlsConfig.PreferServerCipherSuites = true } - // Add cert/key - if c.cert != nil { - tlsConfig.Certificates = []tls.Certificate{*c.cert} + tlsConfig.GetCertificate = func(*tls.ClientHelloInfo) (*tls.Certificate, error) { + return c.cert, nil } - - // Check if a minimum TLS version was set - if c.base.TLSMinVersion != "" { - tlsvers, ok := TLSLookup[c.base.TLSMinVersion] - if !ok { - return nil, fmt.Errorf("TLSMinVersion: value %s not supported, please specify one of [tls10,tls11,tls12]", c.base.TLSMinVersion) - } - tlsConfig.MinVersion = tlsvers - } - - // Ensure we have a CA if VerifyOutgoing is set - if c.base.VerifyOutgoing && c.base.CAFile == "" && c.base.CAPath == "" { - return nil, fmt.Errorf("VerifyOutgoing set, and no CA certificate provided!") + tlsConfig.GetClientCertificate = func(*tls.CertificateRequestInfo) (*tls.Certificate, error) { + return c.cert, nil } if c.cas != nil { @@ -309,104 +256,86 @@ func (c *Configurator) commonTLSConfig(additionalVerifyIncomingFlag bool) (*tls. tlsConfig.RootCAs = c.cas } + tlsConfig.MinVersion = TLSLookup[c.base.TLSMinVersion] + // Set ClientAuth if necessary if c.base.VerifyIncoming || additionalVerifyIncomingFlag { - if c.base.CAFile == "" && c.base.CAPath == "" { - return nil, fmt.Errorf("VerifyIncoming set, and no CA certificate provided!") - } - if len(tlsConfig.Certificates) == 0 { - return nil, fmt.Errorf("VerifyIncoming set, and no Cert/Key pair provided!") - } - tlsConfig.ClientAuth = tls.RequireAndVerifyClientCert } - return tlsConfig, nil + return tlsConfig } // IncomingRPCConfig generates a *tls.Config for incoming RPC connections. -func (c *Configurator) IncomingRPCConfig() (*tls.Config, error) { +func (c *Configurator) IncomingRPCConfig() *tls.Config { c.log("IncomingRPCConfig") c.RLock() defer c.RUnlock() - config, err := c.commonTLSConfig(c.base.VerifyIncomingRPC) - if err != nil { - return nil, err - } + config := c.commonTLSConfig(c.base.VerifyIncomingRPC) config.GetConfigForClient = func(*tls.ClientHelloInfo) (*tls.Config, error) { - return c.IncomingRPCConfig() + return c.IncomingRPCConfig(), nil } - return config, nil + return config } // IncomingHTTPSConfig generates a *tls.Config for incoming HTTPS connections. -func (c *Configurator) IncomingHTTPSConfig() (*tls.Config, error) { +func (c *Configurator) IncomingHTTPSConfig() *tls.Config { c.log("IncomingHTTPSConfig") c.RLock() defer c.RUnlock() - config, err := c.commonTLSConfig(c.base.VerifyIncomingHTTPS) - if err != nil { - return nil, err - } + config := c.commonTLSConfig(c.base.VerifyIncomingHTTPS) config.GetConfigForClient = func(*tls.ClientHelloInfo) (*tls.Config, error) { - return c.IncomingHTTPSConfig() + return c.IncomingHTTPSConfig(), nil } - return config, nil + return config } // IncomingTLSConfig generates a *tls.Config for outgoing TLS connections for // checks. This function is seperated because there is an extra flag to // consider for checks. EnableAgentTLSForChecks and InsecureSkipVerify has to // be checked for checks. -func (c *Configurator) OutgoingTLSConfigForCheck(skipVerify bool) (*tls.Config, error) { +func (c *Configurator) OutgoingTLSConfigForCheck(skipVerify bool) *tls.Config { c.log("OutgoingTLSConfigForCheck") c.RLock() defer c.RUnlock() if !c.base.EnableAgentTLSForChecks { return &tls.Config{ InsecureSkipVerify: skipVerify, - }, nil + } } - tlsConfig, err := c.commonTLSConfig(false) - if err != nil { - return nil, err - } - tlsConfig.InsecureSkipVerify = skipVerify - tlsConfig.ServerName = c.base.ServerName - if tlsConfig.ServerName == "" { - tlsConfig.ServerName = c.base.NodeName + config := c.commonTLSConfig(false) + config.InsecureSkipVerify = skipVerify + config.ServerName = c.base.ServerName + if config.ServerName == "" { + config.ServerName = c.base.NodeName } - return tlsConfig, nil + return config } // OutgoingRPCConfig generates a *tls.Config for outgoing RPC connections. If // there is a CA or VerifyOutgoing is set, a *tls.Config will be provided, // otherwise we assume that no TLS should be used. -func (c *Configurator) OutgoingRPCConfig() (*tls.Config, error) { +func (c *Configurator) OutgoingRPCConfig() *tls.Config { c.log("OutgoingRPCConfig") c.RLock() defer c.RUnlock() - useTLS := c.base.CAFile != "" || c.base.CAPath != "" || c.base.VerifyOutgoing - if !useTLS { - return nil, nil + if c.outgoingRPCTLSDisabled() { + return nil } return c.commonTLSConfig(false) } +func (c *Configurator) outgoingRPCTLSDisabled() bool { + return c.base.CAFile == "" && c.base.CAPath == "" && !c.base.VerifyOutgoing +} + // OutgoingRPCWrapper wraps the result of OutgoingRPCConfig in a DCWrapper. It // decides if verify server hostname should be used. func (c *Configurator) OutgoingRPCWrapper() (DCWrapper, error) { c.log("OutgoingRPCWrapper") - // Get the TLS config - tlsConfig, err := c.OutgoingRPCConfig() - if err != nil { - return nil, err - } - - // Check if TLS is not enabled - if tlsConfig == nil { + if c.outgoingRPCTLSDisabled() { return nil, nil } @@ -414,13 +343,7 @@ func (c *Configurator) OutgoingRPCWrapper() (DCWrapper, error) { wrapper := func(dc string, conn net.Conn) (net.Conn, error) { c.RLock() defer c.RUnlock() - if c.base.VerifyServerHostname { - // Strip the trailing '.' from the domain if any - domain := strings.TrimSuffix(c.base.Domain, ".") - tlsConfig = tlsConfig.Clone() - tlsConfig.ServerName = "server." + dc + "." + domain - } - return c.base.wrapTLSClient(conn, tlsConfig) + return c.wrapTLSClient(conn, dc) } return wrapper, nil @@ -432,6 +355,70 @@ func (c *Configurator) log(name string) { } } +// Wrap a net.Conn into a client tls connection, performing any +// additional verification as needed. +// +// As of go 1.3, crypto/tls only supports either doing no certificate +// verification, or doing full verification including of the peer's +// DNS name. For consul, we want to validate that the certificate is +// signed by a known CA, but because consul doesn't use DNS names for +// node names, we don't verify the certificate DNS names. Since go 1.3 +// no longer supports this mode of operation, we have to do it +// manually. +func (c *Configurator) wrapTLSClient(conn net.Conn, dc string) (net.Conn, error) { + var err error + var tlsConn *tls.Conn + + config := c.OutgoingRPCConfig() + if c.base.VerifyServerHostname { + // Strip the trailing '.' from the domain if any + domain := strings.TrimSuffix(c.base.Domain, ".") + config.ServerName = "server." + dc + "." + domain + } + tlsConn = tls.Client(conn, config) + + // If crypto/tls is doing verification, there's no need to do + // our own. + if config.InsecureSkipVerify == false { + return tlsConn, nil + } + + // If verification is not turned on, don't do it. + if !c.base.VerifyOutgoing { + return tlsConn, nil + } + + if err = tlsConn.Handshake(); err != nil { + tlsConn.Close() + return nil, err + } + + // The following is lightly-modified from the doFullHandshake + // method in crypto/tls's handshake_client.go. + opts := x509.VerifyOptions{ + Roots: config.RootCAs, + CurrentTime: time.Now(), + DNSName: "", + Intermediates: x509.NewCertPool(), + } + + certs := tlsConn.ConnectionState().PeerCertificates + for i, cert := range certs { + if i == 0 { + continue + } + opts.Intermediates.AddCert(cert) + } + + _, err = certs[0].Verify(opts) + if err != nil { + tlsConn.Close() + return nil, err + } + + return tlsConn, err +} + // ParseCiphers parse ciphersuites from the comma-separated string into // recognized slice func ParseCiphers(cipherStr string) ([]uint16, error) { diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index bb529b01be9f..b50ea1687be4 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -54,8 +54,7 @@ func TestConfigurator_OutgoingTLS_OnlyCA(t *testing.T) { } c, err := NewConfigurator(conf, nil) require.NoError(t, err) - tlsConf, err := c.OutgoingRPCConfig() - require.NoError(t, err) + tlsConf := c.OutgoingRPCConfig() require.NotNil(t, tlsConf) } @@ -66,8 +65,7 @@ func TestConfigurator_OutgoingTLS_VerifyOutgoing(t *testing.T) { } c, err := NewConfigurator(conf, nil) require.NoError(t, err) - tlsConf, err := c.OutgoingRPCConfig() - require.NoError(t, err) + tlsConf := c.OutgoingRPCConfig() require.NotNil(t, tlsConf) require.Len(t, tlsConf.RootCAs.Subjects(), 1) require.Empty(t, tlsConf.ServerName) @@ -82,8 +80,7 @@ func TestConfigurator_OutgoingTLS_ServerName(t *testing.T) { } c, err := NewConfigurator(conf, nil) require.NoError(t, err) - tlsConf, err := c.OutgoingRPCConfig() - require.NoError(t, err) + tlsConf := c.OutgoingRPCConfig() require.NotNil(t, tlsConf) require.Len(t, tlsConf.RootCAs.Subjects(), 1) require.Empty(t, tlsConf.ServerName) @@ -98,8 +95,7 @@ func TestConfigurator_OutgoingTLS_VerifyHostname(t *testing.T) { } c, err := NewConfigurator(conf, nil) require.NoError(t, err) - tlsConf, err := c.OutgoingRPCConfig() - require.NoError(t, err) + tlsConf := c.OutgoingRPCConfig() require.NotNil(t, tlsConf) require.Len(t, tlsConf.RootCAs.Subjects(), 1) require.False(t, tlsConf.InsecureSkipVerify) @@ -114,11 +110,12 @@ func TestConfigurator_OutgoingTLS_WithKeyPair(t *testing.T) { } c, err := NewConfigurator(conf, nil) require.NoError(t, err) - tlsConf, err := c.OutgoingRPCConfig() - require.NoError(t, err) + tlsConf := c.OutgoingRPCConfig() require.NotNil(t, tlsConf) require.True(t, tlsConf.InsecureSkipVerify) - require.Len(t, tlsConf.Certificates, 1) + cert, err := tlsConf.GetCertificate(nil) + require.NoError(t, err) + require.NotNil(t, cert) } func TestConfigurator_OutgoingTLS_TLSMinVersion(t *testing.T) { @@ -131,8 +128,7 @@ func TestConfigurator_OutgoingTLS_TLSMinVersion(t *testing.T) { } c, err := NewConfigurator(conf, nil) require.NoError(t, err) - tlsConf, err := c.OutgoingRPCConfig() - require.NoError(t, err) + tlsConf := c.OutgoingRPCConfig() require.NotNil(t, tlsConf) require.Equal(t, tlsConf.MinVersion, TLSLookup[version]) } @@ -146,12 +142,7 @@ func startTLSServer(config *Config) (net.Conn, chan error) { errc <- err return nil, errc } - tlsConfigServer, err := c.IncomingRPCConfig() - if err != nil { - errc <- err - return nil, errc - } - + tlsConfigServer := c.IncomingRPCConfig() client, server := net.Pipe() // Use yamux to buffer the reads, otherwise it's easy to deadlock @@ -289,10 +280,8 @@ func TestConfigurator_wrapTLS_OK(t *testing.T) { c, err := NewConfigurator(config, nil) require.NoError(t, err) - clientConfig, err := c.OutgoingRPCConfig() - require.NoError(t, err) - tlsClient, err := config.wrapTLSClient(client, clientConfig) + tlsClient, err := c.wrapTLSClient(client, "dc1") require.NoError(t, err) tlsClient.Close() @@ -318,10 +307,7 @@ func TestConfigurator_wrapTLS_BadCert(t *testing.T) { c, err := NewConfigurator(clientConfig, nil) require.NoError(t, err) - clientTLSConfig, err := c.OutgoingRPCConfig() - require.NoError(t, err) - - tlsClient, err := clientConfig.wrapTLSClient(client, clientTLSConfig) + tlsClient, err := c.wrapTLSClient(client, "dc1") require.Error(t, err) require.Nil(t, tlsClient) @@ -397,8 +383,7 @@ func TestConfigurator_IncomingHTTPSConfig_CA_PATH(t *testing.T) { c, err := NewConfigurator(conf, nil) require.NoError(t, err) - tlsConf, err := c.IncomingHTTPSConfig() - require.NoError(t, err) + tlsConf := c.IncomingHTTPSConfig() require.Len(t, tlsConf.ClientCAs.Subjects(), 2) } @@ -411,12 +396,13 @@ func TestConfigurator_IncomingHTTPS(t *testing.T) { } c, err := NewConfigurator(conf, nil) require.NoError(t, err) - tlsConf, err := c.IncomingHTTPSConfig() - require.NoError(t, err) + tlsConf := c.IncomingHTTPSConfig() require.NotNil(t, tlsConf) require.Len(t, tlsConf.ClientCAs.Subjects(), 1) require.Equal(t, tlsConf.ClientAuth, tls.RequireAndVerifyClientCert) - require.Len(t, tlsConf.Certificates, 1) + cert, err := tlsConf.GetCertificate(nil) + require.NoError(t, err) + require.NotNil(t, cert) } func TestConfigurator_IncomingHTTPS_MissingCA(t *testing.T) { @@ -442,8 +428,7 @@ func TestConfigurator_IncomingHTTPS_NoVerify(t *testing.T) { conf := Config{} c, err := NewConfigurator(conf, nil) require.NoError(t, err) - tlsConf, err := c.IncomingHTTPSConfig() - require.NoError(t, err) + tlsConf := c.IncomingHTTPSConfig() require.NotNil(t, tlsConf) require.Nil(t, tlsConf.ClientCAs) require.Equal(t, tlsConf.ClientAuth, tls.NoClientCert) @@ -462,8 +447,7 @@ func TestConfigurator_IncomingHTTPS_TLSMinVersion(t *testing.T) { } c, err := NewConfigurator(conf, nil) require.NoError(t, err) - tlsConf, err := c.IncomingHTTPSConfig() - require.NoError(t, err) + tlsConf := c.IncomingHTTPSConfig() require.NotNil(t, tlsConf) require.Equal(t, tlsConf.MinVersion, TLSLookup[version]) } @@ -473,8 +457,7 @@ func TestConfigurator_IncomingHTTPSCAPath_Valid(t *testing.T) { c, err := NewConfigurator(Config{CAPath: "../test/ca_path"}, nil) require.NoError(t, err) - tlsConf, err := c.IncomingHTTPSConfig() - require.NoError(t, err) + tlsConf := c.IncomingHTTPSConfig() require.Len(t, tlsConf.ClientCAs.Subjects(), 2) } @@ -494,8 +477,7 @@ func TestConfigurator_CommonTLSConfigServerNameNodeName(t *testing.T) { for _, v := range variants { c, err := NewConfigurator(v.config, nil) require.NoError(t, err) - tlsConf, err := c.commonTLSConfig(false) - require.NoError(t, err) + tlsConf := c.commonTLSConfig(false) require.Empty(t, tlsConf.ServerName) } } @@ -503,30 +485,28 @@ func TestConfigurator_CommonTLSConfigServerNameNodeName(t *testing.T) { func TestConfigurator_CommonTLSConfigCipherSuites(t *testing.T) { c, err := NewConfigurator(Config{}, nil) require.NoError(t, err) - tlsConfig, err := c.commonTLSConfig(false) - require.NoError(t, err) - require.Empty(t, tlsConfig.CipherSuites) + tlsConf := c.commonTLSConfig(false) + require.Empty(t, tlsConf.CipherSuites) conf := Config{CipherSuites: []uint16{tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305}} require.NoError(t, c.Update(conf)) - tlsConfig, err = c.commonTLSConfig(false) - require.NoError(t, err) - require.Equal(t, conf.CipherSuites, tlsConfig.CipherSuites) + tlsConf = c.commonTLSConfig(false) + require.Equal(t, conf.CipherSuites, tlsConf.CipherSuites) } func TestConfigurator_CommonTLSConfigCertKey(t *testing.T) { c, err := NewConfigurator(Config{}, nil) require.NoError(t, err) - tlsConf, err := c.commonTLSConfig(false) - require.NoError(t, err) + tlsConf := c.commonTLSConfig(false) require.Empty(t, tlsConf.Certificates) require.Error(t, c.Update(Config{CertFile: "/something/bogus", KeyFile: "/more/bogus"})) require.NoError(t, c.Update(Config{CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) - tlsConf, err = c.commonTLSConfig(false) + tlsConf = c.commonTLSConfig(false) + cert, err := tlsConf.GetCertificate(nil) require.NoError(t, err) - require.Len(t, tlsConf.Certificates, 1) + require.NotNil(t, cert) } func TestConfigurator_CommonTLSConfigTLSMinVersion(t *testing.T) { @@ -534,8 +514,7 @@ func TestConfigurator_CommonTLSConfigTLSMinVersion(t *testing.T) { for _, version := range tlsVersions { c, err := NewConfigurator(Config{TLSMinVersion: version}, nil) require.NoError(t, err) - tlsConf, err := c.commonTLSConfig(false) - require.NoError(t, err) + tlsConf := c.commonTLSConfig(false) require.Equal(t, tlsConf.MinVersion, TLSLookup[version]) } @@ -551,28 +530,24 @@ func TestConfigurator_CommonTLSConfigValidateVerifyOutgoingCA(t *testing.T) { func TestConfigurator_CommonTLSConfigLoadCA(t *testing.T) { c, err := NewConfigurator(Config{}, nil) require.NoError(t, err) - tlsConf, err := c.commonTLSConfig(false) - require.NoError(t, err) + tlsConf := c.commonTLSConfig(false) require.Nil(t, tlsConf.RootCAs) require.Nil(t, tlsConf.ClientCAs) require.Error(t, c.Update(Config{CAFile: "/something/bogus"})) require.Error(t, c.Update(Config{CAPath: "/something/bogus/"})) require.NoError(t, c.Update(Config{CAFile: "../test/ca/root.cer"})) - tlsConf, err = c.commonTLSConfig(false) - require.NoError(t, err) + tlsConf = c.commonTLSConfig(false) require.Len(t, tlsConf.RootCAs.Subjects(), 1) require.Len(t, tlsConf.ClientCAs.Subjects(), 1) require.NoError(t, c.Update(Config{CAPath: "../test/ca_path"})) - tlsConf, err = c.commonTLSConfig(false) - require.NoError(t, err) + tlsConf = c.commonTLSConfig(false) require.Len(t, tlsConf.RootCAs.Subjects(), 2) require.Len(t, tlsConf.ClientCAs.Subjects(), 2) require.NoError(t, c.Update(Config{CAFile: "../test/ca/root.cer", CAPath: "../test/ca_path"})) - tlsConf, err = c.commonTLSConfig(false) - require.NoError(t, err) + tlsConf = c.commonTLSConfig(false) require.Len(t, tlsConf.RootCAs.Subjects(), 1) require.Len(t, tlsConf.ClientCAs.Subjects(), 1) } @@ -580,57 +555,48 @@ func TestConfigurator_CommonTLSConfigLoadCA(t *testing.T) { func TestConfigurator_CommonTLSConfigVerifyIncoming(t *testing.T) { c, err := NewConfigurator(Config{}, nil) require.NoError(t, err) - tlsConf, err := c.commonTLSConfig(false) - require.NoError(t, err) + tlsConf := c.commonTLSConfig(false) require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) require.Error(t, c.Update(Config{VerifyIncoming: true})) require.Error(t, c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer"})) require.Error(t, c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/cert/ourdomain.cer"})) require.NoError(t, c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) - tlsConf, err = c.commonTLSConfig(false) - require.NoError(t, err) + tlsConf = c.commonTLSConfig(false) require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) require.NoError(t, c.Update(Config{VerifyIncoming: false, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) - tlsConf, err = c.commonTLSConfig(true) - require.NoError(t, err) + tlsConf = c.commonTLSConfig(true) require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) require.NoError(t, c.Update(Config{VerifyServerHostname: false, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) - tlsConf, err = c.commonTLSConfig(false) - require.NoError(t, err) + tlsConf = c.commonTLSConfig(false) require.True(t, tlsConf.InsecureSkipVerify) require.NoError(t, c.Update(Config{VerifyServerHostname: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) - tlsConf, err = c.commonTLSConfig(false) - require.NoError(t, err) + tlsConf = c.commonTLSConfig(false) require.False(t, tlsConf.InsecureSkipVerify) } func TestConfigurator_IncomingRPCConfig(t *testing.T) { c, err := NewConfigurator(Config{}, nil) require.NoError(t, err) - tlsConf, err := c.IncomingRPCConfig() - require.NoError(t, err) + tlsConf := c.IncomingRPCConfig() require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) require.NotNil(t, tlsConf.GetConfigForClient) require.NoError(t, c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) - tlsConf, err = c.IncomingRPCConfig() - require.NoError(t, err) + tlsConf = c.IncomingRPCConfig() require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) require.NotNil(t, tlsConf.GetConfigForClient) require.NoError(t, c.Update(Config{VerifyIncomingRPC: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) - tlsConf, err = c.IncomingRPCConfig() - require.NoError(t, err) + tlsConf = c.IncomingRPCConfig() require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) require.NotNil(t, tlsConf.GetConfigForClient) require.NoError(t, c.Update(Config{VerifyIncomingHTTPS: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) - tlsConf, err = c.IncomingRPCConfig() - require.NoError(t, err) + tlsConf = c.IncomingRPCConfig() require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) require.NotNil(t, tlsConf.GetConfigForClient) } @@ -638,26 +604,22 @@ func TestConfigurator_IncomingRPCConfig(t *testing.T) { func TestConfigurator_IncomingHTTPSConfig(t *testing.T) { c, err := NewConfigurator(Config{}, nil) require.NoError(t, err) - tlsConf, err := c.IncomingHTTPSConfig() - require.NoError(t, err) + tlsConf := c.IncomingHTTPSConfig() require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) require.NotNil(t, tlsConf.GetConfigForClient) require.NoError(t, c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) - tlsConf, err = c.IncomingHTTPSConfig() - require.NoError(t, err) + tlsConf = c.IncomingHTTPSConfig() require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) require.NotNil(t, tlsConf.GetConfigForClient) require.NoError(t, c.Update(Config{VerifyIncomingHTTPS: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) - tlsConf, err = c.IncomingHTTPSConfig() - require.NoError(t, err) + tlsConf = c.IncomingHTTPSConfig() require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) require.NotNil(t, tlsConf.GetConfigForClient) require.NoError(t, c.Update(Config{VerifyIncomingRPC: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) - tlsConf, err = c.IncomingHTTPSConfig() - require.NoError(t, err) + tlsConf = c.IncomingHTTPSConfig() require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) require.NotNil(t, tlsConf.GetConfigForClient) } @@ -665,55 +627,42 @@ func TestConfigurator_IncomingHTTPSConfig(t *testing.T) { func TestConfigurator_OutgoingRPCConfig(t *testing.T) { c, err := NewConfigurator(Config{}, nil) require.NoError(t, err) - tlsConf, err := c.OutgoingRPCConfig() - require.NoError(t, err) + tlsConf := c.OutgoingRPCConfig() require.Nil(t, tlsConf) require.Error(t, c.Update(Config{VerifyOutgoing: true})) require.NoError(t, c.Update(Config{VerifyOutgoing: true, CAFile: "../test/ca/root.cer"})) - tlsConf, err = c.OutgoingRPCConfig() - require.NoError(t, err) - require.NoError(t, c.Update(Config{VerifyOutgoing: true, CAPath: "../test/ca_path"})) - tlsConf, err = c.OutgoingRPCConfig() - require.NoError(t, err) } func TestConfigurator_OutgoingTLSConfigForChecks(t *testing.T) { c, err := NewConfigurator(Config{EnableAgentTLSForChecks: false}, nil) require.NoError(t, err) - tlsConf, err := c.OutgoingTLSConfigForCheck(false) - require.NoError(t, err) + tlsConf := c.OutgoingTLSConfigForCheck(false) require.False(t, tlsConf.InsecureSkipVerify) require.NoError(t, c.Update(Config{EnableAgentTLSForChecks: false})) - tlsConf, err = c.OutgoingTLSConfigForCheck(true) - require.NoError(t, err) + tlsConf = c.OutgoingTLSConfigForCheck(true) require.True(t, tlsConf.InsecureSkipVerify) require.NoError(t, c.Update(Config{EnableAgentTLSForChecks: true})) - tlsConf, err = c.OutgoingTLSConfigForCheck(false) - require.NoError(t, err) + tlsConf = c.OutgoingTLSConfigForCheck(false) require.False(t, tlsConf.InsecureSkipVerify) require.NoError(t, c.Update(Config{EnableAgentTLSForChecks: true})) - tlsConf, err = c.OutgoingTLSConfigForCheck(true) - require.NoError(t, err) + tlsConf = c.OutgoingTLSConfigForCheck(true) require.True(t, tlsConf.InsecureSkipVerify) require.NoError(t, c.Update(Config{EnableAgentTLSForChecks: true, NodeName: "node", ServerName: "server"})) - tlsConf, err = c.OutgoingTLSConfigForCheck(false) - require.NoError(t, err) + tlsConf = c.OutgoingTLSConfigForCheck(false) require.Equal(t, "server", tlsConf.ServerName) require.NoError(t, c.Update(Config{EnableAgentTLSForChecks: true, ServerName: "server"})) - tlsConf, err = c.OutgoingTLSConfigForCheck(false) - require.NoError(t, err) + tlsConf = c.OutgoingTLSConfigForCheck(false) require.Equal(t, "server", tlsConf.ServerName) require.NoError(t, c.Update(Config{EnableAgentTLSForChecks: true, NodeName: "node"})) - tlsConf, err = c.OutgoingTLSConfigForCheck(false) - require.NoError(t, err) + tlsConf = c.OutgoingTLSConfigForCheck(false) require.Equal(t, "node", tlsConf.ServerName) } From fccacc7c5fc1f34d4816b099e04707a6d2868a88 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Wed, 6 Mar 2019 10:18:49 +0100 Subject: [PATCH 15/31] fix small things --- agent/agent_test.go | 12 ++++-------- agent/consul/server_test.go | 6 +++++- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/agent/agent_test.go b/agent/agent_test.go index 9d5dacb30b3d..c33b2de66fb7 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -3551,8 +3551,7 @@ func TestHansAgent_ReloadConfigOutgoingRPCConfig(t *testing.T) { ` a, err := NewUnstartedAgent(t, t.Name(), hcl) require.NoError(t, err) - tlsConf, err := a.tlsConfigurator.OutgoingRPCConfig() - require.NoError(t, err) + tlsConf := a.tlsConfigurator.OutgoingRPCConfig() require.True(t, tlsConf.InsecureSkipVerify) require.Len(t, tlsConf.ClientCAs.Subjects(), 1) require.Len(t, tlsConf.RootCAs.Subjects(), 1) @@ -3568,8 +3567,7 @@ func TestHansAgent_ReloadConfigOutgoingRPCConfig(t *testing.T) { c := TestConfig(config.Source{Name: t.Name(), Format: "hcl", Data: hcl}) err = a.ReloadConfig(c) require.NoError(t, err) - tlsConf, err = a.tlsConfigurator.OutgoingRPCConfig() - require.NoError(t, err) + tlsConf = a.tlsConfigurator.OutgoingRPCConfig() require.False(t, tlsConf.InsecureSkipVerify) require.Len(t, tlsConf.RootCAs.Subjects(), 2) require.Len(t, tlsConf.ClientCAs.Subjects(), 2) @@ -3589,8 +3587,7 @@ func TestHansAgent_ReloadConfigIncomingRPCConfig(t *testing.T) { ` a, err := NewUnstartedAgent(t, t.Name(), hcl) require.NoError(t, err) - tlsConf, err := a.tlsConfigurator.IncomingRPCConfig() - require.NoError(t, err) + tlsConf := a.tlsConfigurator.IncomingRPCConfig() require.NotNil(t, tlsConf.GetConfigForClient) tlsConf, err = tlsConf.GetConfigForClient(nil) require.NoError(t, err) @@ -3631,8 +3628,7 @@ func TestHansAgent_ReloadConfigTLSConfigFailure(t *testing.T) { ` a, err := NewUnstartedAgent(t, t.Name(), hcl) require.NoError(t, err) - tlsConf, err := a.tlsConfigurator.IncomingRPCConfig() - require.NoError(t, err) + tlsConf := a.tlsConfigurator.IncomingRPCConfig() hcl = ` data_dir = "` + dataDir + `" diff --git a/agent/consul/server_test.go b/agent/consul/server_test.go index ba53c5517002..f33616a01cd3 100644 --- a/agent/consul/server_test.go +++ b/agent/consul/server_test.go @@ -179,7 +179,11 @@ func newServer(c *Config) (*Server, error) { w = os.Stderr } logger := log.New(w, c.NodeName+" - ", log.LstdFlags|log.Lmicroseconds) - srv, err := NewServerLogger(c, logger, new(token.Store), tlsutil.NewConfigurator(c.ToTLSUtilConfig())) + tlsConf, err := tlsutil.NewConfigurator(c.ToTLSUtilConfig(), logger) + if err != nil { + return nil, err + } + srv, err := NewServerLogger(c, logger, new(token.Store), tlsConf) if err != nil { return nil, err } From a8c3f057e258cb1bcd2c821f1a0ac7d6654fc6f4 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Wed, 6 Mar 2019 10:22:02 +0100 Subject: [PATCH 16/31] improvements --- agent/agent_test.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/agent/agent_test.go b/agent/agent_test.go index c33b2de66fb7..6e5a579e174d 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -3565,8 +3565,7 @@ func TestHansAgent_ReloadConfigOutgoingRPCConfig(t *testing.T) { verify_server_hostname = true ` c := TestConfig(config.Source{Name: t.Name(), Format: "hcl", Data: hcl}) - err = a.ReloadConfig(c) - require.NoError(t, err) + require.NoError(t, a.ReloadConfig(c)) tlsConf = a.tlsConfigurator.OutgoingRPCConfig() require.False(t, tlsConf.InsecureSkipVerify) require.Len(t, tlsConf.RootCAs.Subjects(), 2) @@ -3605,8 +3604,7 @@ func TestHansAgent_ReloadConfigIncomingRPCConfig(t *testing.T) { verify_server_hostname = true ` c := TestConfig(config.Source{Name: t.Name(), Format: "hcl", Data: hcl}) - err = a.ReloadConfig(c) - require.NoError(t, err) + require.NoError(t, a.ReloadConfig(c)) tlsConf, err = tlsConf.GetConfigForClient(nil) require.NoError(t, err) require.False(t, tlsConf.InsecureSkipVerify) @@ -3635,8 +3633,7 @@ func TestHansAgent_ReloadConfigTLSConfigFailure(t *testing.T) { verify_incoming = true ` c := TestConfig(config.Source{Name: t.Name(), Format: "hcl", Data: hcl}) - err = a.ReloadConfig(c) - require.Error(t, err) + require.Error(t, a.ReloadConfig(c)) tlsConf, err = tlsConf.GetConfigForClient(nil) require.NoError(t, err) require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) From cb3c407f9399484333084c83b5f1ea485cf993ae Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Wed, 6 Mar 2019 10:30:33 +0100 Subject: [PATCH 17/31] small things --- tlsutil/config.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/tlsutil/config.go b/tlsutil/config.go index 5b1a7372b8c3..114435d564bd 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -24,6 +24,7 @@ type Wrapper func(conn net.Conn) (net.Conn, error) // TLSLookup maps the tls_min_version configuration to the internal value var TLSLookup = map[string]uint16{ + "": tls.VersionTLS10, // default in golang "tls10": tls.VersionTLS10, "tls11": tls.VersionTLS11, "tls12": tls.VersionTLS12, @@ -240,9 +241,8 @@ func (c *Configurator) commonTLSConfig(additionalVerifyIncomingFlag bool) *tls.C if len(c.base.CipherSuites) != 0 { tlsConfig.CipherSuites = c.base.CipherSuites } - if c.base.PreferServerCipherSuites { - tlsConfig.PreferServerCipherSuites = true - } + + tlsConfig.PreferServerCipherSuites = c.base.PreferServerCipherSuites tlsConfig.GetCertificate = func(*tls.ClientHelloInfo) (*tls.Certificate, error) { return c.cert, nil @@ -251,10 +251,8 @@ func (c *Configurator) commonTLSConfig(additionalVerifyIncomingFlag bool) *tls.C return c.cert, nil } - if c.cas != nil { - tlsConfig.ClientCAs = c.cas - tlsConfig.RootCAs = c.cas - } + tlsConfig.ClientCAs = c.cas + tlsConfig.RootCAs = c.cas tlsConfig.MinVersion = TLSLookup[c.base.TLSMinVersion] From ae321dbcf126552e4b358fbad43ffc778968cb02 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Wed, 6 Mar 2019 10:46:55 +0100 Subject: [PATCH 18/31] correct locking in wrap --- tlsutil/config.go | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/tlsutil/config.go b/tlsutil/config.go index 114435d564bd..c2fa5cff7385 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -337,11 +337,9 @@ func (c *Configurator) OutgoingRPCWrapper() (DCWrapper, error) { return nil, nil } - // Generate the wrapper based on hostname verification + // Generate the wrapper based on dc wrapper := func(dc string, conn net.Conn) (net.Conn, error) { - c.RLock() - defer c.RUnlock() - return c.wrapTLSClient(conn, dc) + return c.wrapTLSClient(dc, conn) } return wrapper, nil @@ -363,26 +361,32 @@ func (c *Configurator) log(name string) { // node names, we don't verify the certificate DNS names. Since go 1.3 // no longer supports this mode of operation, we have to do it // manually. -func (c *Configurator) wrapTLSClient(conn net.Conn, dc string) (net.Conn, error) { +func (c *Configurator) wrapTLSClient(dc string, conn net.Conn) (net.Conn, error) { var err error var tlsConn *tls.Conn + c.RLock() config := c.OutgoingRPCConfig() - if c.base.VerifyServerHostname { + verifyServerHostname := c.base.VerifyServerHostname + verifyOutgoing := c.base.VerifyOutgoing + domain := c.base.Domain + c.RUnlock() + + if verifyServerHostname { // Strip the trailing '.' from the domain if any - domain := strings.TrimSuffix(c.base.Domain, ".") + domain = strings.TrimSuffix(domain, ".") config.ServerName = "server." + dc + "." + domain } tlsConn = tls.Client(conn, config) // If crypto/tls is doing verification, there's no need to do // our own. - if config.InsecureSkipVerify == false { + if !config.InsecureSkipVerify { return tlsConn, nil } // If verification is not turned on, don't do it. - if !c.base.VerifyOutgoing { + if !verifyOutgoing { return tlsConn, nil } From b789253ffedd87aee75c28bb8ff52e7bd908334f Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Wed, 6 Mar 2019 10:56:12 +0100 Subject: [PATCH 19/31] small things --- tlsutil/config.go | 17 +++++------------ tlsutil/config_test.go | 4 ++-- 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/tlsutil/config.go b/tlsutil/config.go index c2fa5cff7385..6079ba9c21e6 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -116,14 +116,7 @@ type Config struct { // KeyPair is used to open and parse a certificate and key file func (c *Config) KeyPair() (*tls.Certificate, error) { - if c.CertFile == "" || c.KeyFile == "" { - return nil, nil - } - cert, err := tls.LoadX509KeyPair(c.CertFile, c.KeyFile) - if err != nil { - return nil, fmt.Errorf("Failed to load cert/key pair: %v", err) - } - return &cert, err + return loadKeyPair(c.CertFile, c.KeyFile) } // SpecificDC is used to invoke a static datacenter @@ -164,11 +157,11 @@ func NewConfigurator(config Config, logger *log.Logger) (*Configurator, error) { func (c *Configurator) Update(config Config) error { c.Lock() defer c.Unlock() - cert, err := c.loadKeyPair(config.CertFile, config.KeyFile) + cert, err := loadKeyPair(config.CertFile, config.KeyFile) if err != nil { return err } - cas, err := c.loadCAs(config.CAFile, config.CAPath) + cas, err := loadCAs(config.CAFile, config.CAPath) if err != nil { return err } @@ -209,7 +202,7 @@ func (c *Configurator) check(config Config, cert *tls.Certificate) error { return nil } -func (c *Configurator) loadKeyPair(certFile, keyFile string) (*tls.Certificate, error) { +func loadKeyPair(certFile, keyFile string) (*tls.Certificate, error) { if certFile == "" || keyFile == "" { return nil, nil } @@ -220,7 +213,7 @@ func (c *Configurator) loadKeyPair(certFile, keyFile string) (*tls.Certificate, return &cert, nil } -func (c *Configurator) loadCAs(caFile, caPath string) (*x509.CertPool, error) { +func loadCAs(caFile, caPath string) (*x509.CertPool, error) { if caFile != "" { return rootcerts.LoadCAFile(caFile) } else if caPath != "" { diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index b50ea1687be4..231d49047719 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -281,7 +281,7 @@ func TestConfigurator_wrapTLS_OK(t *testing.T) { c, err := NewConfigurator(config, nil) require.NoError(t, err) - tlsClient, err := c.wrapTLSClient(client, "dc1") + tlsClient, err := c.wrapTLSClient("dc1", client) require.NoError(t, err) tlsClient.Close() @@ -307,7 +307,7 @@ func TestConfigurator_wrapTLS_BadCert(t *testing.T) { c, err := NewConfigurator(clientConfig, nil) require.NoError(t, err) - tlsClient, err := c.wrapTLSClient(client, "dc1") + tlsClient, err := c.wrapTLSClient("dc1", client) require.Error(t, err) require.Nil(t, tlsClient) From 93f9adcbaf1db5d31724515f57308de189de9fb4 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Wed, 6 Mar 2019 12:36:36 +0100 Subject: [PATCH 20/31] fix that test :tada: --- tlsutil/config.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tlsutil/config.go b/tlsutil/config.go index 6079ba9c21e6..f040b7b0fc80 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -275,8 +275,10 @@ func (c *Configurator) IncomingHTTPSConfig() *tls.Config { c.RLock() defer c.RUnlock() config := c.commonTLSConfig(c.base.VerifyIncomingHTTPS) - config.GetConfigForClient = func(*tls.ClientHelloInfo) (*tls.Config, error) { - return c.IncomingHTTPSConfig(), nil + config.GetConfigForClient = func(hello *tls.ClientHelloInfo) (*tls.Config, error) { + config := c.IncomingHTTPSConfig() + config.NextProtos = hello.SupportedProtos + return config, nil } return config } From b2f7f1985df736e9262b9a31ddd899b98022a553 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Wed, 6 Mar 2019 12:46:42 +0100 Subject: [PATCH 21/31] remove that name --- agent/agent_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/agent/agent_test.go b/agent/agent_test.go index 6e5a579e174d..ee364d90aa94 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -3537,7 +3537,7 @@ func TestAgent_loadTokens(t *testing.T) { }) } -func TestHansAgent_ReloadConfigOutgoingRPCConfig(t *testing.T) { +func TestAgent_ReloadConfigOutgoingRPCConfig(t *testing.T) { t.Parallel() dataDir := testutil.TempDir(t, "agent") // we manage the data dir defer os.RemoveAll(dataDir) @@ -3572,7 +3572,7 @@ func TestHansAgent_ReloadConfigOutgoingRPCConfig(t *testing.T) { require.Len(t, tlsConf.ClientCAs.Subjects(), 2) } -func TestHansAgent_ReloadConfigIncomingRPCConfig(t *testing.T) { +func TestAgent_ReloadConfigIncomingRPCConfig(t *testing.T) { t.Parallel() dataDir := testutil.TempDir(t, "agent") // we manage the data dir defer os.RemoveAll(dataDir) @@ -3612,7 +3612,7 @@ func TestHansAgent_ReloadConfigIncomingRPCConfig(t *testing.T) { require.Len(t, tlsConf.RootCAs.Subjects(), 2) } -func TestHansAgent_ReloadConfigTLSConfigFailure(t *testing.T) { +func TestAgent_ReloadConfigTLSConfigFailure(t *testing.T) { t.Parallel() dataDir := testutil.TempDir(t, "agent") // we manage the data dir defer os.RemoveAll(dataDir) From 16c1c80d1e9f8fe03643de222def339a1d53f942 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Wed, 6 Mar 2019 13:01:20 +0100 Subject: [PATCH 22/31] merge conflict. --- agent/agent.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index d9b06763a5a8..9b3a33cc52f0 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -2426,9 +2426,6 @@ func (a *Agent) removeCheckLocked(checkID types.CheckID, persist bool) error { // Add to the local state for anti-entropy a.State.RemoveCheck(checkID) - a.checkLock.Lock() - defer a.checkLock.Unlock() - a.cancelCheckMonitors(checkID) a.State.RemoveCheck(checkID) From 792889ad03034429dfd63206160adcb0e08937ab Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Wed, 6 Mar 2019 13:06:12 +0100 Subject: [PATCH 23/31] another bad merge --- agent/agent.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 9b3a33cc52f0..dec221bff59b 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -2423,9 +2423,6 @@ func (a *Agent) removeCheckLocked(checkID types.CheckID, persist bool) error { return fmt.Errorf("CheckID missing") } - // Add to the local state for anti-entropy - a.State.RemoveCheck(checkID) - a.cancelCheckMonitors(checkID) a.State.RemoveCheck(checkID) @@ -3553,8 +3550,7 @@ func (a *Agent) ReloadConfig(newCfg *config.RuntimeConfig) error { // the checks and service registrations. a.loadTokens(newCfg) - newTLSConfig := newCfg.ToTLSUtilConfig() - if err := a.tlsConfigurator.Update(newTLSConfig); err != nil { + if err := a.tlsConfigurator.Update(newCfg.ToTLSUtilConfig()); err != nil { return fmt.Errorf("Failed reloading tls configuration: %s", err) } From 967fb9049904d563876a43cbdc8a012a2e0be717 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Wed, 6 Mar 2019 17:03:03 +0100 Subject: [PATCH 24/31] improve locking --- tlsutil/config.go | 88 +++++++++++++++++++++++++++++++++-------------- 1 file changed, 62 insertions(+), 26 deletions(-) diff --git a/tlsutil/config.go b/tlsutil/config.go index f040b7b0fc80..907e927ce5b2 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -154,9 +154,8 @@ func NewConfigurator(config Config, logger *log.Logger) (*Configurator, error) { // Update updates the internal configuration which is used to generate // *tls.Config. +// This function acquires a write lock because it writes the new config. func (c *Configurator) Update(config Config) error { - c.Lock() - defer c.Unlock() cert, err := loadKeyPair(config.CertFile, config.KeyFile) if err != nil { return err @@ -169,10 +168,12 @@ func (c *Configurator) Update(config Config) error { if err = c.check(config, cert); err != nil { return err } + c.Lock() c.base = &config c.cert = cert c.cas = cas c.version++ + c.Unlock() c.log("Update") return nil } @@ -225,7 +226,10 @@ func loadCAs(caFile, caPath string) (*x509.CertPool, error) { // commonTLSConfig generates a *tls.Config from the base configuration the // Configurator has. It accepts an additional flag in case a config is needed // for incoming TLS connections. +// This function acquires a read lock because it reads from the config. func (c *Configurator) commonTLSConfig(additionalVerifyIncomingFlag bool) *tls.Config { + c.RLock() + defer c.RUnlock() tlsConfig := &tls.Config{ InsecureSkipVerify: !c.base.VerifyServerHostname, } @@ -247,6 +251,9 @@ func (c *Configurator) commonTLSConfig(additionalVerifyIncomingFlag bool) *tls.C tlsConfig.ClientCAs = c.cas tlsConfig.RootCAs = c.cas + // This is possible because TLSLookup also contains "" with golang's + // default (tls10). And because the initial check makes sure the + // version correctly matches. tlsConfig.MinVersion = TLSLookup[c.base.TLSMinVersion] // Set ClientAuth if necessary @@ -257,12 +264,55 @@ func (c *Configurator) commonTLSConfig(additionalVerifyIncomingFlag bool) *tls.C return tlsConfig } +// This function acquires a read lock because it reads from the config. +func (c *Configurator) outgoingRPCTLSDisabled() bool { + c.RLock() + defer c.RUnlock() + return c.base.CAFile == "" && c.base.CAPath == "" && !c.base.VerifyOutgoing +} + +// This function acquires a read lock because it reads from the config. +func (c *Configurator) someValuesFromConfig() (bool, bool, string) { + c.RLock() + defer c.RUnlock() + return c.base.VerifyServerHostname, c.base.VerifyOutgoing, c.base.Domain +} + +// This function acquires a read lock because it reads from the config. +func (c *Configurator) verifyIncomingRPC() bool { + c.RLock() + defer c.RUnlock() + return c.base.VerifyIncomingRPC +} + +// This function acquires a read lock because it reads from the config. +func (c *Configurator) verifyIncomingHTTPS() bool { + c.RLock() + defer c.RUnlock() + return c.base.VerifyIncomingHTTPS +} + +// This function acquires a read lock because it reads from the config. +func (c *Configurator) enableAgentTLSForChecks() bool { + c.RLock() + defer c.RUnlock() + return c.base.EnableAgentTLSForChecks +} + +// This function acquires a read lock because it reads from the config. +func (c *Configurator) serverNameOrNodeName() string { + c.RLock() + defer c.RUnlock() + if c.base.ServerName != "" { + return c.base.ServerName + } + return c.base.NodeName +} + // IncomingRPCConfig generates a *tls.Config for incoming RPC connections. func (c *Configurator) IncomingRPCConfig() *tls.Config { c.log("IncomingRPCConfig") - c.RLock() - defer c.RUnlock() - config := c.commonTLSConfig(c.base.VerifyIncomingRPC) + config := c.commonTLSConfig(c.verifyIncomingRPC()) config.GetConfigForClient = func(*tls.ClientHelloInfo) (*tls.Config, error) { return c.IncomingRPCConfig(), nil } @@ -272,9 +322,7 @@ func (c *Configurator) IncomingRPCConfig() *tls.Config { // IncomingHTTPSConfig generates a *tls.Config for incoming HTTPS connections. func (c *Configurator) IncomingHTTPSConfig() *tls.Config { c.log("IncomingHTTPSConfig") - c.RLock() - defer c.RUnlock() - config := c.commonTLSConfig(c.base.VerifyIncomingHTTPS) + config := c.commonTLSConfig(c.verifyIncomingHTTPS()) config.GetConfigForClient = func(hello *tls.ClientHelloInfo) (*tls.Config, error) { config := c.IncomingHTTPSConfig() config.NextProtos = hello.SupportedProtos @@ -289,9 +337,7 @@ func (c *Configurator) IncomingHTTPSConfig() *tls.Config { // be checked for checks. func (c *Configurator) OutgoingTLSConfigForCheck(skipVerify bool) *tls.Config { c.log("OutgoingTLSConfigForCheck") - c.RLock() - defer c.RUnlock() - if !c.base.EnableAgentTLSForChecks { + if !c.enableAgentTLSForChecks() { return &tls.Config{ InsecureSkipVerify: skipVerify, } @@ -299,10 +345,7 @@ func (c *Configurator) OutgoingTLSConfigForCheck(skipVerify bool) *tls.Config { config := c.commonTLSConfig(false) config.InsecureSkipVerify = skipVerify - config.ServerName = c.base.ServerName - if config.ServerName == "" { - config.ServerName = c.base.NodeName - } + config.ServerName = c.serverNameOrNodeName() return config } @@ -312,18 +355,12 @@ func (c *Configurator) OutgoingTLSConfigForCheck(skipVerify bool) *tls.Config { // otherwise we assume that no TLS should be used. func (c *Configurator) OutgoingRPCConfig() *tls.Config { c.log("OutgoingRPCConfig") - c.RLock() - defer c.RUnlock() if c.outgoingRPCTLSDisabled() { return nil } return c.commonTLSConfig(false) } -func (c *Configurator) outgoingRPCTLSDisabled() bool { - return c.base.CAFile == "" && c.base.CAPath == "" && !c.base.VerifyOutgoing -} - // OutgoingRPCWrapper wraps the result of OutgoingRPCConfig in a DCWrapper. It // decides if verify server hostname should be used. func (c *Configurator) OutgoingRPCWrapper() (DCWrapper, error) { @@ -340,8 +377,11 @@ func (c *Configurator) OutgoingRPCWrapper() (DCWrapper, error) { return wrapper, nil } +// This function acquires a read lock because it reads from the config. func (c *Configurator) log(name string) { if c.logger != nil { + c.RLock() + defer c.RUnlock() c.logger.Printf("[DEBUG] tlsutil: %s with version %d", name, c.version) } } @@ -360,12 +400,8 @@ func (c *Configurator) wrapTLSClient(dc string, conn net.Conn) (net.Conn, error) var err error var tlsConn *tls.Conn - c.RLock() config := c.OutgoingRPCConfig() - verifyServerHostname := c.base.VerifyServerHostname - verifyOutgoing := c.base.VerifyOutgoing - domain := c.base.Domain - c.RUnlock() + verifyServerHostname, verifyOutgoing, domain := c.someValuesFromConfig() if verifyServerHostname { // Strip the trailing '.' from the domain if any From b25c9df5d0112f8528f9bbcbc2d38d17af6aa627 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Fri, 8 Mar 2019 10:16:48 +0100 Subject: [PATCH 25/31] more tests --- agent/consul/client.go | 9 +- agent/consul/server.go | 10 +- tlsutil/config.go | 8 +- tlsutil/config_test.go | 462 +++++++++++++++++++++++------------------ 4 files changed, 269 insertions(+), 220 deletions(-) diff --git a/agent/consul/client.go b/agent/consul/client.go index db35544ac404..85eb34f0a028 100644 --- a/agent/consul/client.go +++ b/agent/consul/client.go @@ -117,12 +117,6 @@ func NewClientLogger(config *Config, logger *log.Logger, tlsConfigurator *tlsuti config.LogOutput = os.Stderr } - // Create the tls Wrapper - tlsWrap, err := tlsConfigurator.OutgoingRPCWrapper() - if err != nil { - return nil, err - } - // Create a logger if logger == nil { logger = log.New(config.LogOutput, "", log.LstdFlags) @@ -133,7 +127,7 @@ func NewClientLogger(config *Config, logger *log.Logger, tlsConfigurator *tlsuti LogOutput: config.LogOutput, MaxTime: clientRPCConnMaxIdle, MaxStreams: clientMaxStreams, - TLSWrapper: tlsWrap, + TLSWrapper: tlsConfigurator.OutgoingRPCWrapper(), ForceTLS: config.VerifyOutgoing, } @@ -162,6 +156,7 @@ func NewClientLogger(config *Config, logger *log.Logger, tlsConfigurator *tlsuti CacheConfig: clientACLCacheConfig, Sentinel: nil, } + var err error if c.acls, err = NewACLResolver(&aclConfig); err != nil { c.Shutdown() return nil, fmt.Errorf("Failed to create ACL resolver: %v", err) diff --git a/agent/consul/server.go b/agent/consul/server.go index 1b1c79122f8a..1e19dcbd236d 100644 --- a/agent/consul/server.go +++ b/agent/consul/server.go @@ -300,12 +300,6 @@ func NewServerLogger(config *Config, logger *log.Logger, tokens *token.Store, tl } } - // Create the TLS wrapper for outgoing connections. - tlsWrap, err := tlsConfigurator.OutgoingRPCWrapper() - if err != nil { - return nil, err - } - // Create the tombstone GC. gc, err := state.NewTombstoneGC(config.TombstoneTTL, config.TombstoneTTLGranularity) if err != nil { @@ -320,7 +314,7 @@ func NewServerLogger(config *Config, logger *log.Logger, tokens *token.Store, tl LogOutput: config.LogOutput, MaxTime: serverRPCCache, MaxStreams: serverMaxStreams, - TLSWrapper: tlsWrap, + TLSWrapper: tlsConfigurator.OutgoingRPCWrapper(), ForceTLS: config.VerifyOutgoing, } @@ -371,7 +365,7 @@ func NewServerLogger(config *Config, logger *log.Logger, tokens *token.Store, tl } // Initialize the RPC layer. - if err := s.setupRPC(tlsWrap); err != nil { + if err := s.setupRPC(tlsConfigurator.OutgoingRPCWrapper()); err != nil { s.Shutdown() return nil, fmt.Errorf("Failed to start RPC layer: %v", err) } diff --git a/tlsutil/config.go b/tlsutil/config.go index 907e927ce5b2..48972ba91266 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -363,18 +363,16 @@ func (c *Configurator) OutgoingRPCConfig() *tls.Config { // OutgoingRPCWrapper wraps the result of OutgoingRPCConfig in a DCWrapper. It // decides if verify server hostname should be used. -func (c *Configurator) OutgoingRPCWrapper() (DCWrapper, error) { +func (c *Configurator) OutgoingRPCWrapper() DCWrapper { c.log("OutgoingRPCWrapper") if c.outgoingRPCTLSDisabled() { - return nil, nil + return nil } // Generate the wrapper based on dc - wrapper := func(dc string, conn net.Conn) (net.Conn, error) { + return func(dc string, conn net.Conn) (net.Conn, error) { return c.wrapTLSClient(dc, conn) } - - return wrapper, nil } // This function acquires a read lock because it reads from the config. diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index 231d49047719..a2c3e6decff2 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -14,40 +14,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestConfig_KeyPair_None(t *testing.T) { - conf := &Config{} - cert, err := conf.KeyPair() - if err != nil { - t.Fatalf("err: %v", err) - } - if cert != nil { - t.Fatalf("bad: %v", cert) - } -} - -func TestConfig_KeyPair_Valid(t *testing.T) { - conf := &Config{ - CertFile: "../test/key/ourdomain.cer", - KeyFile: "../test/key/ourdomain.key", - } - cert, err := conf.KeyPair() - if err != nil { - t.Fatalf("err: %v", err) - } - if cert == nil { - t.Fatalf("expected cert") - } -} - -func TestConfigurator_OutgoingTLS_MissingCA(t *testing.T) { - conf := Config{ - VerifyOutgoing: true, - } - c, err := NewConfigurator(conf, nil) - require.Error(t, err) - require.Nil(t, c) -} - func TestConfigurator_OutgoingTLS_OnlyCA(t *testing.T) { conf := Config{ CAFile: "../test/ca/root.cer", @@ -118,22 +84,6 @@ func TestConfigurator_OutgoingTLS_WithKeyPair(t *testing.T) { require.NotNil(t, cert) } -func TestConfigurator_OutgoingTLS_TLSMinVersion(t *testing.T) { - tlsVersions := []string{"tls10", "tls11", "tls12"} - for _, version := range tlsVersions { - conf := Config{ - VerifyOutgoing: true, - CAFile: "../test/ca/root.cer", - TLSMinVersion: version, - } - c, err := NewConfigurator(conf, nil) - require.NoError(t, err) - tlsConf := c.OutgoingRPCConfig() - require.NotNil(t, tlsConf) - require.Equal(t, tlsConf.MinVersion, TLSLookup[version]) - } -} - func startTLSServer(config *Config) (net.Conn, chan error) { errc := make(chan error, 1) @@ -188,8 +138,8 @@ func TestConfigurator_outgoingWrapper_OK(t *testing.T) { c, err := NewConfigurator(config, nil) require.NoError(t, err) - wrap, err := c.OutgoingRPCWrapper() - require.NoError(t, err) + wrap := c.OutgoingRPCWrapper() + require.NotNil(t, wrap) tlsClient, err := wrap("dc1", client) require.NoError(t, err) @@ -219,8 +169,7 @@ func TestConfigurator_outgoingWrapper_BadDC(t *testing.T) { c, err := NewConfigurator(config, nil) require.NoError(t, err) - wrap, err := c.OutgoingRPCWrapper() - require.NoError(t, err) + wrap := c.OutgoingRPCWrapper() tlsClient, err := wrap("dc2", client) require.NoError(t, err) @@ -250,8 +199,7 @@ func TestConfigurator_outgoingWrapper_BadCert(t *testing.T) { c, err := NewConfigurator(config, nil) require.NoError(t, err) - wrap, err := c.OutgoingRPCWrapper() - require.NoError(t, err) + wrap := c.OutgoingRPCWrapper() tlsClient, err := wrap("dc1", client) require.NoError(t, err) @@ -435,24 +383,6 @@ func TestConfigurator_IncomingHTTPS_NoVerify(t *testing.T) { require.Empty(t, tlsConf.Certificates) } -func TestConfigurator_IncomingHTTPS_TLSMinVersion(t *testing.T) { - tlsVersions := []string{"tls10", "tls11", "tls12"} - for _, version := range tlsVersions { - conf := Config{ - VerifyIncoming: true, - CAFile: "../test/ca/root.cer", - CertFile: "../test/key/ourdomain.cer", - KeyFile: "../test/key/ourdomain.key", - TLSMinVersion: version, - } - c, err := NewConfigurator(conf, nil) - require.NoError(t, err) - tlsConf := c.IncomingHTTPSConfig() - require.NotNil(t, tlsConf) - require.Equal(t, tlsConf.MinVersion, TLSLookup[version]) - } -} - func TestConfigurator_IncomingHTTPSCAPath_Valid(t *testing.T) { c, err := NewConfigurator(Config{CAPath: "../test/ca_path"}, nil) @@ -461,6 +391,8 @@ func TestConfigurator_IncomingHTTPSCAPath_Valid(t *testing.T) { require.Len(t, tlsConf.ClientCAs.Subjects(), 2) } +////////////////////////////////////////////////////////////// + func TestConfigurator_CommonTLSConfigServerNameNodeName(t *testing.T) { type variant struct { config Config @@ -482,188 +414,302 @@ func TestConfigurator_CommonTLSConfigServerNameNodeName(t *testing.T) { } } -func TestConfigurator_CommonTLSConfigCipherSuites(t *testing.T) { - c, err := NewConfigurator(Config{}, nil) - require.NoError(t, err) - tlsConf := c.commonTLSConfig(false) - require.Empty(t, tlsConf.CipherSuites) +func TestConfigurator_check(t *testing.T) { + c := &Configurator{} + require.NoError(t, c.check(Config{}, nil)) - conf := Config{CipherSuites: []uint16{tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305}} - require.NoError(t, c.Update(conf)) - tlsConf = c.commonTLSConfig(false) - require.Equal(t, conf.CipherSuites, tlsConf.CipherSuites) + // test tls min version + require.Error(t, c.check(Config{TLSMinVersion: "tls9"}, nil)) + require.NoError(t, c.check(Config{TLSMinVersion: ""}, nil)) + require.NoError(t, c.check(Config{TLSMinVersion: "tls10"}, nil)) + require.NoError(t, c.check(Config{TLSMinVersion: "tls11"}, nil)) + require.NoError(t, c.check(Config{TLSMinVersion: "tls12"}, nil)) + + // test ca and verifyoutgoing + require.Error(t, c.check(Config{VerifyOutgoing: true, CAFile: "", CAPath: ""}, nil)) + require.NoError(t, c.check(Config{VerifyOutgoing: false, CAFile: "", CAPath: ""}, nil)) + require.NoError(t, c.check(Config{VerifyOutgoing: false, CAFile: "a", CAPath: ""}, nil)) + require.NoError(t, c.check(Config{VerifyOutgoing: false, CAFile: "", CAPath: "a"}, nil)) + require.NoError(t, c.check(Config{VerifyOutgoing: false, CAFile: "a", CAPath: "a"}, nil)) + require.NoError(t, c.check(Config{VerifyOutgoing: true, CAFile: "a", CAPath: ""}, nil)) + require.NoError(t, c.check(Config{VerifyOutgoing: true, CAFile: "", CAPath: "a"}, nil)) + require.NoError(t, c.check(Config{VerifyOutgoing: true, CAFile: "a", CAPath: "a"}, nil)) + + // test ca, cert and verifyIncoming + require.Error(t, c.check(Config{VerifyIncoming: true, CAFile: "", CAPath: ""}, nil)) + require.Error(t, c.check(Config{VerifyIncomingRPC: true, CAFile: "", CAPath: ""}, nil)) + require.Error(t, c.check(Config{VerifyIncomingHTTPS: true, CAFile: "", CAPath: ""}, nil)) + require.Error(t, c.check(Config{VerifyIncoming: true, CAFile: "a", CAPath: ""}, nil)) + require.Error(t, c.check(Config{VerifyIncoming: true, CAFile: "", CAPath: "a"}, nil)) + require.NoError(t, c.check(Config{VerifyIncoming: true, CAFile: "", CAPath: "a"}, &tls.Certificate{})) } -func TestConfigurator_CommonTLSConfigCertKey(t *testing.T) { - c, err := NewConfigurator(Config{}, nil) +func TestConfigurator_loadKeyPair(t *testing.T) { + cert, err := loadKeyPair("", "") require.NoError(t, err) - tlsConf := c.commonTLSConfig(false) - require.Empty(t, tlsConf.Certificates) + require.Nil(t, cert) - require.Error(t, c.Update(Config{CertFile: "/something/bogus", KeyFile: "/more/bogus"})) + cert, err = loadKeyPair("bogus", "") + require.NoError(t, err) + require.Nil(t, cert) - require.NoError(t, c.Update(Config{CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) - tlsConf = c.commonTLSConfig(false) - cert, err := tlsConf.GetCertificate(nil) + cert, err = loadKeyPair("", "bogus") + require.NoError(t, err) + require.Nil(t, cert) + + cert, err = loadKeyPair("bogus", "bogus") + require.Error(t, err) + require.Nil(t, cert) + + cert, err = loadKeyPair("../test/key/ourdomain.cer", "../test/key/ourdomain.key") require.NoError(t, err) require.NotNil(t, cert) } -func TestConfigurator_CommonTLSConfigTLSMinVersion(t *testing.T) { - tlsVersions := []string{"tls10", "tls11", "tls12"} - for _, version := range tlsVersions { - c, err := NewConfigurator(Config{TLSMinVersion: version}, nil) - require.NoError(t, err) - tlsConf := c.commonTLSConfig(false) - require.Equal(t, tlsConf.MinVersion, TLSLookup[version]) - } +func TestConfigurator_loadCAs(t *testing.T) { + cas, err := loadCAs("", "") + require.NoError(t, err) + require.Nil(t, cas) - _, err := NewConfigurator(Config{TLSMinVersion: "tlsBOGUS"}, nil) + cas, err = loadCAs("bogus", "") require.Error(t, err) -} + require.Nil(t, cas) -func TestConfigurator_CommonTLSConfigValidateVerifyOutgoingCA(t *testing.T) { - _, err := NewConfigurator(Config{VerifyOutgoing: true}, nil) - require.Error(t, err) + cas, err = loadCAs("../test/ca/root.cer", "") + require.NoError(t, err) + require.NotNil(t, cas) + + cas, err = loadCAs("", "../test/ca_path") + require.NoError(t, err) + require.NotNil(t, cas) + + cas, err = loadCAs("../test/ca/root.cer", "../test/ca_path") + require.NoError(t, err) + require.NotNil(t, cas) + require.Len(t, cas.Subjects(), 1) } -func TestConfigurator_CommonTLSConfigLoadCA(t *testing.T) { +func TestConfigurator_CommonTLSConfigInsecureSkipVerify(t *testing.T) { c, err := NewConfigurator(Config{}, nil) require.NoError(t, err) tlsConf := c.commonTLSConfig(false) - require.Nil(t, tlsConf.RootCAs) - require.Nil(t, tlsConf.ClientCAs) - - require.Error(t, c.Update(Config{CAFile: "/something/bogus"})) - require.Error(t, c.Update(Config{CAPath: "/something/bogus/"})) - require.NoError(t, c.Update(Config{CAFile: "../test/ca/root.cer"})) - tlsConf = c.commonTLSConfig(false) - require.Len(t, tlsConf.RootCAs.Subjects(), 1) - require.Len(t, tlsConf.ClientCAs.Subjects(), 1) + require.True(t, tlsConf.InsecureSkipVerify) - require.NoError(t, c.Update(Config{CAPath: "../test/ca_path"})) + require.NoError(t, c.Update(Config{VerifyServerHostname: false})) tlsConf = c.commonTLSConfig(false) - require.Len(t, tlsConf.RootCAs.Subjects(), 2) - require.Len(t, tlsConf.ClientCAs.Subjects(), 2) + require.True(t, tlsConf.InsecureSkipVerify) - require.NoError(t, c.Update(Config{CAFile: "../test/ca/root.cer", CAPath: "../test/ca_path"})) + require.NoError(t, c.Update(Config{VerifyServerHostname: true})) tlsConf = c.commonTLSConfig(false) - require.Len(t, tlsConf.RootCAs.Subjects(), 1) - require.Len(t, tlsConf.ClientCAs.Subjects(), 1) + require.False(t, tlsConf.InsecureSkipVerify) } -func TestConfigurator_CommonTLSConfigVerifyIncoming(t *testing.T) { +func TestConfigurator_CommonTLSConfigPreferServerCipherSuites(t *testing.T) { c, err := NewConfigurator(Config{}, nil) require.NoError(t, err) tlsConf := c.commonTLSConfig(false) - require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) + require.False(t, tlsConf.PreferServerCipherSuites) - require.Error(t, c.Update(Config{VerifyIncoming: true})) - require.Error(t, c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer"})) - require.Error(t, c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/cert/ourdomain.cer"})) - require.NoError(t, c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) + require.NoError(t, c.Update(Config{PreferServerCipherSuites: false})) tlsConf = c.commonTLSConfig(false) - require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) + require.False(t, tlsConf.PreferServerCipherSuites) - require.NoError(t, c.Update(Config{VerifyIncoming: false, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) - tlsConf = c.commonTLSConfig(true) - require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) - - require.NoError(t, c.Update(Config{VerifyServerHostname: false, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) + require.NoError(t, c.Update(Config{PreferServerCipherSuites: true})) tlsConf = c.commonTLSConfig(false) - require.True(t, tlsConf.InsecureSkipVerify) + require.True(t, tlsConf.PreferServerCipherSuites) +} + +func TestConfigurator_CommonTLSConfigCipherSuites(t *testing.T) { + c, err := NewConfigurator(Config{}, nil) + require.NoError(t, err) + tlsConf := c.commonTLSConfig(false) + require.Empty(t, tlsConf.CipherSuites) - require.NoError(t, c.Update(Config{VerifyServerHostname: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) + conf := Config{CipherSuites: []uint16{tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305}} + require.NoError(t, c.Update(conf)) tlsConf = c.commonTLSConfig(false) - require.False(t, tlsConf.InsecureSkipVerify) + require.Equal(t, conf.CipherSuites, tlsConf.CipherSuites) } -func TestConfigurator_IncomingRPCConfig(t *testing.T) { +func TestConfigurator_CommonTLSConfigGetClientCertificate(t *testing.T) { c, err := NewConfigurator(Config{}, nil) require.NoError(t, err) - tlsConf := c.IncomingRPCConfig() - require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) - require.NotNil(t, tlsConf.GetConfigForClient) - require.NoError(t, c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) - tlsConf = c.IncomingRPCConfig() - require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) - require.NotNil(t, tlsConf.GetConfigForClient) + cert, err := c.commonTLSConfig(false).GetCertificate(nil) + require.NoError(t, err) + require.Nil(t, cert) - require.NoError(t, c.Update(Config{VerifyIncomingRPC: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) - tlsConf = c.IncomingRPCConfig() - require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) - require.NotNil(t, tlsConf.GetConfigForClient) + c.cert = &tls.Certificate{} + cert, err = c.commonTLSConfig(false).GetCertificate(nil) + require.NoError(t, err) + require.Equal(t, c.cert, cert) - require.NoError(t, c.Update(Config{VerifyIncomingHTTPS: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) - tlsConf = c.IncomingRPCConfig() - require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) - require.NotNil(t, tlsConf.GetConfigForClient) + cert, err = c.commonTLSConfig(false).GetClientCertificate(nil) + require.NoError(t, err) + require.Equal(t, c.cert, cert) } -func TestConfigurator_IncomingHTTPSConfig(t *testing.T) { +func TestConfigurator_CommonTLSConfigCAs(t *testing.T) { c, err := NewConfigurator(Config{}, nil) require.NoError(t, err) - tlsConf := c.IncomingHTTPSConfig() - require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) - require.NotNil(t, tlsConf.GetConfigForClient) + require.Nil(t, c.commonTLSConfig(false).ClientCAs) + require.Nil(t, c.commonTLSConfig(false).RootCAs) - require.NoError(t, c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) - tlsConf = c.IncomingHTTPSConfig() - require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) - require.NotNil(t, tlsConf.GetConfigForClient) + c.cas = &x509.CertPool{} + require.Equal(t, c.cas, c.commonTLSConfig(false).ClientCAs) + require.Equal(t, c.cas, c.commonTLSConfig(false).RootCAs) +} - require.NoError(t, c.Update(Config{VerifyIncomingHTTPS: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) - tlsConf = c.IncomingHTTPSConfig() - require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) - require.NotNil(t, tlsConf.GetConfigForClient) +func TestConfigurator_CommonTLSConfigTLSMinVersion(t *testing.T) { + c, err := NewConfigurator(Config{TLSMinVersion: ""}, nil) + require.NoError(t, err) + require.Equal(t, c.commonTLSConfig(false).MinVersion, TLSLookup["tls10"]) - require.NoError(t, c.Update(Config{VerifyIncomingRPC: true, CAFile: "../test/ca/root.cer", CertFile: "../test/key/ourdomain.cer", KeyFile: "../test/key/ourdomain.key"})) - tlsConf = c.IncomingHTTPSConfig() - require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth) - require.NotNil(t, tlsConf.GetConfigForClient) + tlsVersions := []string{"tls10", "tls11", "tls12"} + for _, version := range tlsVersions { + require.NoError(t, c.Update(Config{TLSMinVersion: version})) + require.Equal(t, c.commonTLSConfig(false).MinVersion, + TLSLookup[version]) + } + + require.Error(t, c.Update(Config{TLSMinVersion: "tlsBOGUS"})) } -func TestConfigurator_OutgoingRPCConfig(t *testing.T) { - c, err := NewConfigurator(Config{}, nil) - require.NoError(t, err) - tlsConf := c.OutgoingRPCConfig() - require.Nil(t, tlsConf) +func TestConfigurator_CommonTLSConfigVerifyIncoming(t *testing.T) { + c := Configurator{base: &Config{}} + require.Equal(t, tls.NoClientCert, c.commonTLSConfig(false).ClientAuth) + require.Equal(t, tls.RequireAndVerifyClientCert, + c.commonTLSConfig(true).ClientAuth) +} - require.Error(t, c.Update(Config{VerifyOutgoing: true})) - require.NoError(t, c.Update(Config{VerifyOutgoing: true, CAFile: "../test/ca/root.cer"})) - require.NoError(t, c.Update(Config{VerifyOutgoing: true, CAPath: "../test/ca_path"})) +func TestConfigurator_OutgoingRPCTLSDisabled(t *testing.T) { + c := Configurator{base: &Config{}} + type variant struct { + verify bool + file string + path string + expected bool + } + variants := []variant{ + {false, "", "", true}, + {false, "a", "", false}, + {false, "", "a", false}, + {false, "a", "a", false}, + {true, "", "", false}, + {true, "a", "", false}, + {true, "", "a", false}, + {true, "a", "a", false}, + } + for _, v := range variants { + c.base.VerifyOutgoing = v.verify + c.base.CAFile = v.file + c.base.CAPath = v.path + require.Equal(t, v.expected, c.outgoingRPCTLSDisabled()) + } } -func TestConfigurator_OutgoingTLSConfigForChecks(t *testing.T) { - c, err := NewConfigurator(Config{EnableAgentTLSForChecks: false}, nil) - require.NoError(t, err) - tlsConf := c.OutgoingTLSConfigForCheck(false) - require.False(t, tlsConf.InsecureSkipVerify) +func TestConfigurator_SomeValuesFromConfig(t *testing.T) { + c := Configurator{base: &Config{ + VerifyServerHostname: true, + VerifyOutgoing: true, + Domain: "abc.de", + }} + one, two, three := c.someValuesFromConfig() + require.Equal(t, c.base.VerifyServerHostname, one) + require.Equal(t, c.base.VerifyOutgoing, two) + require.Equal(t, c.base.Domain, three) +} - require.NoError(t, c.Update(Config{EnableAgentTLSForChecks: false})) - tlsConf = c.OutgoingTLSConfigForCheck(true) - require.True(t, tlsConf.InsecureSkipVerify) +func TestConfigurator_VerifyIncomingRPC(t *testing.T) { + c := Configurator{base: &Config{ + VerifyIncomingRPC: true, + }} + verify := c.verifyIncomingRPC() + require.Equal(t, c.base.VerifyIncomingRPC, verify) +} - require.NoError(t, c.Update(Config{EnableAgentTLSForChecks: true})) - tlsConf = c.OutgoingTLSConfigForCheck(false) - require.False(t, tlsConf.InsecureSkipVerify) +func TestConfigurator_VerifyIncomingHTTPS(t *testing.T) { + c := Configurator{base: &Config{ + VerifyIncomingHTTPS: true, + }} + verify := c.verifyIncomingHTTPS() + require.Equal(t, c.base.VerifyIncomingHTTPS, verify) +} - require.NoError(t, c.Update(Config{EnableAgentTLSForChecks: true})) - tlsConf = c.OutgoingTLSConfigForCheck(true) - require.True(t, tlsConf.InsecureSkipVerify) +func TestConfigurator_EnableAgentTLSForChecks(t *testing.T) { + c := Configurator{base: &Config{ + EnableAgentTLSForChecks: true, + }} + enabled := c.enableAgentTLSForChecks() + require.Equal(t, c.base.EnableAgentTLSForChecks, enabled) +} + +func TestConfigurator_IncomingRPCConfig(t *testing.T) { + c, err := NewConfigurator(Config{ + VerifyIncomingRPC: true, + CAFile: "../test/ca/root.cer", + CertFile: "../test/key/ourdomain.cer", + KeyFile: "../test/key/ourdomain.key", + }, nil) + require.NoError(t, err) + tlsConf := c.IncomingRPCConfig() + require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) + require.NotNil(t, tlsConf.GetConfigForClient) + tlsConf, err = tlsConf.GetConfigForClient(nil) + require.NoError(t, err) + require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) +} - require.NoError(t, c.Update(Config{EnableAgentTLSForChecks: true, NodeName: "node", ServerName: "server"})) - tlsConf = c.OutgoingTLSConfigForCheck(false) - require.Equal(t, "server", tlsConf.ServerName) +func TestConfigurator_IncomingHTTPSConfig(t *testing.T) { + c, err := NewConfigurator(Config{ + VerifyIncomingHTTPS: true, + CAFile: "../test/ca/root.cer", + CertFile: "../test/key/ourdomain.cer", + KeyFile: "../test/key/ourdomain.key", + }, nil) + require.NoError(t, err) + tlsConf := c.IncomingHTTPSConfig() + require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) + require.NotNil(t, tlsConf.GetConfigForClient) + tlsConf, err = tlsConf.GetConfigForClient( + &tls.ClientHelloInfo{SupportedProtos: []string{"h2"}}, + ) + require.NoError(t, err) + require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) + require.Equal(t, []string{"h2"}, tlsConf.NextProtos) +} - require.NoError(t, c.Update(Config{EnableAgentTLSForChecks: true, ServerName: "server"})) - tlsConf = c.OutgoingTLSConfigForCheck(false) - require.Equal(t, "server", tlsConf.ServerName) +func TestConfigurator_OutgoingTLSConfigForChecks(t *testing.T) { + c := Configurator{base: &Config{ + TLSMinVersion: "tls12", + EnableAgentTLSForChecks: false, + }} + tlsConf := c.OutgoingTLSConfigForCheck(true) + require.Equal(t, true, tlsConf.InsecureSkipVerify) + require.Equal(t, uint16(0), tlsConf.MinVersion) + + c.base.EnableAgentTLSForChecks = true + c.base.ServerName = "servername" + tlsConf = c.OutgoingTLSConfigForCheck(true) + require.Equal(t, true, tlsConf.InsecureSkipVerify) + require.Equal(t, TLSLookup[c.base.TLSMinVersion], tlsConf.MinVersion) + require.Equal(t, c.base.ServerName, tlsConf.ServerName) +} + +func TestConfigurator_OutgoingRPCConfig(t *testing.T) { + c := Configurator{base: &Config{}} + require.Nil(t, c.OutgoingRPCConfig()) + c.base.VerifyOutgoing = true + require.NotNil(t, c.OutgoingRPCConfig()) +} - require.NoError(t, c.Update(Config{EnableAgentTLSForChecks: true, NodeName: "node"})) - tlsConf = c.OutgoingTLSConfigForCheck(false) - require.Equal(t, "node", tlsConf.ServerName) +func TestConfigurator_OutgoingRPCWrapper(t *testing.T) { + c := Configurator{base: &Config{}} + require.Nil(t, c.OutgoingRPCWrapper()) + c.base.VerifyOutgoing = true + wrap := c.OutgoingRPCWrapper() + require.NotNil(t, wrap) + t.Log("TODO: actually call wrap here eventually") } func TestConfigurator_UpdateChecks(t *testing.T) { @@ -677,10 +723,26 @@ func TestConfigurator_UpdateChecks(t *testing.T) { require.Equal(t, c.version, 2) } -func TestConfigurator_Version(t *testing.T) { +func TestConfigurator_UpdateSetsStuff(t *testing.T) { c, err := NewConfigurator(Config{}, nil) require.NoError(t, err) - require.Equal(t, c.version, 1) + require.Nil(t, c.cas) + require.Nil(t, c.cert) + require.Equal(t, c.base, &Config{}) + require.Equal(t, 1, c.version) + require.Error(t, c.Update(Config{VerifyOutgoing: true})) require.Equal(t, c.version, 1) + + config := Config{ + CAFile: "../test/ca/root.cer", + CertFile: "../test/key/ourdomain.cer", + KeyFile: "../test/key/ourdomain.key", + } + require.NoError(t, c.Update(config)) + require.NotNil(t, c.cas) + require.Len(t, c.cas.Subjects(), 1) + require.NotNil(t, c.cert) + require.Equal(t, c.base, &config) + require.Equal(t, 2, c.version) } From cab155a653fe8ee131bc6e61cabb683c2e740de8 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Fri, 8 Mar 2019 15:33:29 +0100 Subject: [PATCH 26/31] more tests --- tlsutil/config_test.go | 432 +++++++++++++++++++++-------------------- 1 file changed, 225 insertions(+), 207 deletions(-) diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index a2c3e6decff2..2e837e94ee52 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -1,10 +1,13 @@ package tlsutil import ( + "bytes" "crypto/tls" "crypto/x509" + "fmt" "io" "io/ioutil" + "log" "net" "reflect" "strings" @@ -14,76 +17,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestConfigurator_OutgoingTLS_OnlyCA(t *testing.T) { - conf := Config{ - CAFile: "../test/ca/root.cer", - } - c, err := NewConfigurator(conf, nil) - require.NoError(t, err) - tlsConf := c.OutgoingRPCConfig() - require.NotNil(t, tlsConf) -} - -func TestConfigurator_OutgoingTLS_VerifyOutgoing(t *testing.T) { - conf := Config{ - VerifyOutgoing: true, - CAFile: "../test/ca/root.cer", - } - c, err := NewConfigurator(conf, nil) - require.NoError(t, err) - tlsConf := c.OutgoingRPCConfig() - require.NotNil(t, tlsConf) - require.Len(t, tlsConf.RootCAs.Subjects(), 1) - require.Empty(t, tlsConf.ServerName) - require.True(t, tlsConf.InsecureSkipVerify) -} - -func TestConfigurator_OutgoingTLS_ServerName(t *testing.T) { - conf := Config{ - VerifyOutgoing: true, - CAFile: "../test/ca/root.cer", - ServerName: "consul.example.com", - } - c, err := NewConfigurator(conf, nil) - require.NoError(t, err) - tlsConf := c.OutgoingRPCConfig() - require.NotNil(t, tlsConf) - require.Len(t, tlsConf.RootCAs.Subjects(), 1) - require.Empty(t, tlsConf.ServerName) - require.True(t, tlsConf.InsecureSkipVerify) -} - -func TestConfigurator_OutgoingTLS_VerifyHostname(t *testing.T) { - conf := Config{ - VerifyOutgoing: true, - VerifyServerHostname: true, - CAFile: "../test/ca/root.cer", - } - c, err := NewConfigurator(conf, nil) - require.NoError(t, err) - tlsConf := c.OutgoingRPCConfig() - require.NotNil(t, tlsConf) - require.Len(t, tlsConf.RootCAs.Subjects(), 1) - require.False(t, tlsConf.InsecureSkipVerify) -} - -func TestConfigurator_OutgoingTLS_WithKeyPair(t *testing.T) { - conf := Config{ - VerifyOutgoing: true, - CAFile: "../test/ca/root.cer", - CertFile: "../test/key/ourdomain.cer", - KeyFile: "../test/key/ourdomain.key", - } - c, err := NewConfigurator(conf, nil) - require.NoError(t, err) - tlsConf := c.OutgoingRPCConfig() - require.NotNil(t, tlsConf) - require.True(t, tlsConf.InsecureSkipVerify) - cert, err := tlsConf.GetCertificate(nil) - require.NoError(t, err) - require.NotNil(t, cert) -} - func startTLSServer(config *Config) (net.Conn, chan error) { errc := make(chan error, 1) @@ -152,6 +85,35 @@ func TestConfigurator_outgoingWrapper_OK(t *testing.T) { require.NoError(t, err) } +func TestConfigurator_outgoingWrapper_noverify_OK(t *testing.T) { + config := Config{ + CAFile: "../test/hostname/CertAuth.crt", + CertFile: "../test/hostname/Alice.crt", + KeyFile: "../test/hostname/Alice.key", + Domain: "consul", + } + + client, errc := startTLSServer(&config) + if client == nil { + t.Fatalf("startTLSServer err: %v", <-errc) + } + + c, err := NewConfigurator(config, nil) + require.NoError(t, err) + wrap := c.OutgoingRPCWrapper() + require.NotNil(t, wrap) + + tlsClient, err := wrap("dc1", client) + require.NoError(t, err) + + defer tlsClient.Close() + err = tlsClient.(*tls.Conn).Handshake() + require.NoError(t, err) + + err = <-errc + require.NoError(t, err) +} + func TestConfigurator_outgoingWrapper_BadDC(t *testing.T) { config := Config{ CAFile: "../test/hostname/CertAuth.crt", @@ -313,85 +275,151 @@ func TestConfig_ParseCiphers(t *testing.T) { tls.TLS_ECDHE_ECDSA_WITH_RC4_128_SHA, } v, err := ParseCiphers(testOk) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) if got, want := v, ciphers; !reflect.DeepEqual(got, want) { t.Fatalf("got ciphers %#v want %#v", got, want) } - testBad := "TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,cipherX" - if _, err := ParseCiphers(testBad); err == nil { - t.Fatal("should fail on unsupported cipherX") - } -} - -func TestConfigurator_IncomingHTTPSConfig_CA_PATH(t *testing.T) { - conf := Config{CAPath: "../test/ca_path"} + _, err = ParseCiphers("TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA,cipherX") + require.Error(t, err) - c, err := NewConfigurator(conf, nil) + v, err = ParseCiphers("") require.NoError(t, err) - tlsConf := c.IncomingHTTPSConfig() - require.Len(t, tlsConf.ClientCAs.Subjects(), 2) + require.Equal(t, []uint16{}, v) } -func TestConfigurator_IncomingHTTPS(t *testing.T) { - conf := Config{ - VerifyIncoming: true, - CAFile: "../test/ca/root.cer", - CertFile: "../test/key/ourdomain.cer", - KeyFile: "../test/key/ourdomain.key", +func TestConfigurator_loadKeyPair(t *testing.T) { + type variant struct { + cert, key string + shoulderr bool + isnil bool } - c, err := NewConfigurator(conf, nil) - require.NoError(t, err) - tlsConf := c.IncomingHTTPSConfig() - require.NotNil(t, tlsConf) - require.Len(t, tlsConf.ClientCAs.Subjects(), 1) - require.Equal(t, tlsConf.ClientAuth, tls.RequireAndVerifyClientCert) - cert, err := tlsConf.GetCertificate(nil) - require.NoError(t, err) - require.NotNil(t, cert) -} - -func TestConfigurator_IncomingHTTPS_MissingCA(t *testing.T) { - conf := Config{ - VerifyIncoming: true, - CertFile: "../test/key/ourdomain.cer", - KeyFile: "../test/key/ourdomain.key", + variants := []variant{ + {"", "", false, true}, + {"bogus", "", false, true}, + {"", "bogus", false, true}, + {"../test/key/ourdomain.cer", "", false, true}, + {"", "../test/key/ourdomain.key", false, true}, + {"bogus", "bogus", true, true}, + {"../test/key/ourdomain.cer", "../test/key/ourdomain.key", + false, false}, } - _, err := NewConfigurator(conf, nil) - require.Error(t, err) -} - -func TestConfigurator_IncomingHTTPS_MissingKey(t *testing.T) { - conf := Config{ - VerifyIncoming: true, - CAFile: "../test/ca/root.cer", + for _, v := range variants { + cert1, err1 := loadKeyPair(v.cert, v.key) + config := &Config{CertFile: v.cert, KeyFile: v.key} + cert2, err2 := config.KeyPair() + if v.shoulderr { + require.Error(t, err1) + require.Error(t, err2) + } else { + require.NoError(t, err1) + require.NoError(t, err2) + } + if v.isnil { + require.Nil(t, cert1) + require.Nil(t, cert2) + } else { + require.NotNil(t, cert1) + require.NotNil(t, cert2) + } } - _, err := NewConfigurator(conf, nil) - require.Error(t, err) } -func TestConfigurator_IncomingHTTPS_NoVerify(t *testing.T) { - conf := Config{} - c, err := NewConfigurator(conf, nil) +func TestConfig_SpecifyDC(t *testing.T) { + require.Nil(t, SpecificDC("", nil)) + dcwrap := func(dc string, conn net.Conn) (net.Conn, error) { return nil, nil } + wrap := SpecificDC("", dcwrap) + require.NotNil(t, wrap) + conn, err := wrap(nil) require.NoError(t, err) - tlsConf := c.IncomingHTTPSConfig() - require.NotNil(t, tlsConf) - require.Nil(t, tlsConf.ClientCAs) - require.Equal(t, tlsConf.ClientAuth, tls.NoClientCert) - require.Empty(t, tlsConf.Certificates) + require.Nil(t, conn) } -func TestConfigurator_IncomingHTTPSCAPath_Valid(t *testing.T) { - - c, err := NewConfigurator(Config{CAPath: "../test/ca_path"}, nil) +func TestConfigurator_NewConfigurator(t *testing.T) { + buf := bytes.Buffer{} + logger := log.New(&buf, "logger: ", log.Lshortfile) + c, err := NewConfigurator(Config{}, logger) require.NoError(t, err) - tlsConf := c.IncomingHTTPSConfig() - require.Len(t, tlsConf.ClientCAs.Subjects(), 2) + require.NotNil(t, c) + require.Equal(t, logger, c.logger) + + c, err = NewConfigurator(Config{VerifyOutgoing: true}, nil) + require.Error(t, err) + require.Nil(t, c) } -////////////////////////////////////////////////////////////// +func TestConfigurator_ErrorPropagation(t *testing.T) { + type variant struct { + config Config + shouldErr bool + excludeCheck bool + } + cafile := "../test/ca/root.cer" + capath := "../test/ca_path" + certfile := "../test/key/ourdomain.cer" + keyfile := "../test/key/ourdomain.key" + variants := []variant{ + {Config{}, false, false}, + {Config{TLSMinVersion: "tls9"}, true, false}, + {Config{TLSMinVersion: ""}, false, false}, + {Config{TLSMinVersion: "tls10"}, false, false}, + {Config{TLSMinVersion: "tls11"}, false, false}, + {Config{TLSMinVersion: "tls12"}, false, false}, + {Config{VerifyOutgoing: true, CAFile: "", CAPath: ""}, true, false}, + {Config{VerifyOutgoing: false, CAFile: "", CAPath: ""}, false, false}, + {Config{VerifyOutgoing: false, CAFile: cafile, CAPath: ""}, + false, false}, + {Config{VerifyOutgoing: false, CAFile: "", CAPath: capath}, + false, false}, + {Config{VerifyOutgoing: false, CAFile: cafile, CAPath: capath}, + false, false}, + {Config{VerifyOutgoing: true, CAFile: cafile, CAPath: ""}, + false, false}, + {Config{VerifyOutgoing: true, CAFile: "", CAPath: capath}, + false, false}, + {Config{VerifyOutgoing: true, CAFile: cafile, CAPath: capath}, + false, false}, + {Config{VerifyIncoming: true, CAFile: "", CAPath: ""}, true, false}, + {Config{VerifyIncomingRPC: true, CAFile: "", CAPath: ""}, + true, false}, + {Config{VerifyIncomingHTTPS: true, CAFile: "", CAPath: ""}, + true, false}, + {Config{VerifyIncoming: true, CAFile: "a", CAPath: ""}, true, false}, + {Config{VerifyIncoming: true, CAFile: "", CAPath: "a"}, true, false}, + {Config{VerifyIncoming: true, CAFile: "", CAPath: capath, + CertFile: certfile, KeyFile: keyfile}, false, false}, + {Config{CertFile: "bogus", KeyFile: "bogus"}, true, true}, + {Config{CAFile: "bogus"}, true, true}, + {Config{CAPath: "bogus"}, true, true}, + } + + c := &Configurator{} + for i, v := range variants { + info := fmt.Sprintf("case %d", i) + _, err1 := NewConfigurator(v.config, nil) + err2 := c.Update(v.config) + + var err3 error + if !v.excludeCheck { + cert, err := v.config.KeyPair() + require.NoError(t, err, info) + err3 = c.check(v.config, cert) + } + if v.shouldErr { + require.Error(t, err1, info) + require.Error(t, err2, info) + if !v.excludeCheck { + require.Error(t, err3, info) + } + } else { + require.NoError(t, err1, info) + require.NoError(t, err2, info) + if !v.excludeCheck { + require.NoError(t, err3, info) + } + } + } +} func TestConfigurator_CommonTLSConfigServerNameNodeName(t *testing.T) { type variant struct { @@ -414,79 +442,36 @@ func TestConfigurator_CommonTLSConfigServerNameNodeName(t *testing.T) { } } -func TestConfigurator_check(t *testing.T) { - c := &Configurator{} - require.NoError(t, c.check(Config{}, nil)) - - // test tls min version - require.Error(t, c.check(Config{TLSMinVersion: "tls9"}, nil)) - require.NoError(t, c.check(Config{TLSMinVersion: ""}, nil)) - require.NoError(t, c.check(Config{TLSMinVersion: "tls10"}, nil)) - require.NoError(t, c.check(Config{TLSMinVersion: "tls11"}, nil)) - require.NoError(t, c.check(Config{TLSMinVersion: "tls12"}, nil)) - - // test ca and verifyoutgoing - require.Error(t, c.check(Config{VerifyOutgoing: true, CAFile: "", CAPath: ""}, nil)) - require.NoError(t, c.check(Config{VerifyOutgoing: false, CAFile: "", CAPath: ""}, nil)) - require.NoError(t, c.check(Config{VerifyOutgoing: false, CAFile: "a", CAPath: ""}, nil)) - require.NoError(t, c.check(Config{VerifyOutgoing: false, CAFile: "", CAPath: "a"}, nil)) - require.NoError(t, c.check(Config{VerifyOutgoing: false, CAFile: "a", CAPath: "a"}, nil)) - require.NoError(t, c.check(Config{VerifyOutgoing: true, CAFile: "a", CAPath: ""}, nil)) - require.NoError(t, c.check(Config{VerifyOutgoing: true, CAFile: "", CAPath: "a"}, nil)) - require.NoError(t, c.check(Config{VerifyOutgoing: true, CAFile: "a", CAPath: "a"}, nil)) - - // test ca, cert and verifyIncoming - require.Error(t, c.check(Config{VerifyIncoming: true, CAFile: "", CAPath: ""}, nil)) - require.Error(t, c.check(Config{VerifyIncomingRPC: true, CAFile: "", CAPath: ""}, nil)) - require.Error(t, c.check(Config{VerifyIncomingHTTPS: true, CAFile: "", CAPath: ""}, nil)) - require.Error(t, c.check(Config{VerifyIncoming: true, CAFile: "a", CAPath: ""}, nil)) - require.Error(t, c.check(Config{VerifyIncoming: true, CAFile: "", CAPath: "a"}, nil)) - require.NoError(t, c.check(Config{VerifyIncoming: true, CAFile: "", CAPath: "a"}, &tls.Certificate{})) -} - -func TestConfigurator_loadKeyPair(t *testing.T) { - cert, err := loadKeyPair("", "") - require.NoError(t, err) - require.Nil(t, cert) - - cert, err = loadKeyPair("bogus", "") - require.NoError(t, err) - require.Nil(t, cert) - - cert, err = loadKeyPair("", "bogus") - require.NoError(t, err) - require.Nil(t, cert) - - cert, err = loadKeyPair("bogus", "bogus") - require.Error(t, err) - require.Nil(t, cert) - - cert, err = loadKeyPair("../test/key/ourdomain.cer", "../test/key/ourdomain.key") - require.NoError(t, err) - require.NotNil(t, cert) -} - func TestConfigurator_loadCAs(t *testing.T) { - cas, err := loadCAs("", "") - require.NoError(t, err) - require.Nil(t, cas) - - cas, err = loadCAs("bogus", "") - require.Error(t, err) - require.Nil(t, cas) - - cas, err = loadCAs("../test/ca/root.cer", "") - require.NoError(t, err) - require.NotNil(t, cas) - - cas, err = loadCAs("", "../test/ca_path") - require.NoError(t, err) - require.NotNil(t, cas) - - cas, err = loadCAs("../test/ca/root.cer", "../test/ca_path") - require.NoError(t, err) - require.NotNil(t, cas) - require.Len(t, cas.Subjects(), 1) + type variant struct { + cafile, capath string + shouldErr bool + isNil bool + count int + } + variants := []variant{ + {"", "", false, true, 0}, + {"bogus", "", true, true, 0}, + {"", "bogus", true, true, 0}, + {"../test/ca/root.cer", "", false, false, 1}, + {"", "../test/ca_path", false, false, 2}, + {"../test/ca/root.cer", "../test/ca_path", false, false, 1}, + } + for i, v := range variants { + cas, err := loadCAs(v.cafile, v.capath) + info := fmt.Sprintf("case %d", i) + if v.shouldErr { + require.Error(t, err, info) + } else { + require.NoError(t, err, info) + } + if v.isNil { + require.Nil(t, cas, info) + } else { + require.NotNil(t, cas, info) + require.Len(t, cas.Subjects(), v.count, info) + } + } } func TestConfigurator_CommonTLSConfigInsecureSkipVerify(t *testing.T) { @@ -525,7 +510,8 @@ func TestConfigurator_CommonTLSConfigCipherSuites(t *testing.T) { tlsConf := c.commonTLSConfig(false) require.Empty(t, tlsConf.CipherSuites) - conf := Config{CipherSuites: []uint16{tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305}} + conf := Config{CipherSuites: []uint16{ + tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305}} require.NoError(t, c.Update(conf)) tlsConf = c.commonTLSConfig(false) require.Equal(t, conf.CipherSuites, tlsConf.CipherSuites) @@ -577,9 +563,22 @@ func TestConfigurator_CommonTLSConfigTLSMinVersion(t *testing.T) { func TestConfigurator_CommonTLSConfigVerifyIncoming(t *testing.T) { c := Configurator{base: &Config{}} - require.Equal(t, tls.NoClientCert, c.commonTLSConfig(false).ClientAuth) - require.Equal(t, tls.RequireAndVerifyClientCert, - c.commonTLSConfig(true).ClientAuth) + type variant struct { + verify bool + additional bool + expected tls.ClientAuthType + } + variants := []variant{ + {false, false, tls.NoClientCert}, + {true, false, tls.RequireAndVerifyClientCert}, + {false, true, tls.RequireAndVerifyClientCert}, + {true, true, tls.RequireAndVerifyClientCert}, + } + for _, v := range variants { + c.base.VerifyIncoming = v.verify + require.Equal(t, v.expected, + c.commonTLSConfig(v.additional).ClientAuth) + } } func TestConfigurator_OutgoingRPCTLSDisabled(t *testing.T) { @@ -717,7 +716,8 @@ func TestConfigurator_UpdateChecks(t *testing.T) { require.NoError(t, err) require.NoError(t, c.Update(Config{})) require.Error(t, c.Update(Config{VerifyOutgoing: true})) - require.Error(t, c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer"})) + require.Error(t, c.Update(Config{VerifyIncoming: true, + CAFile: "../test/ca/root.cer"})) require.False(t, c.base.VerifyIncoming) require.False(t, c.base.VerifyOutgoing) require.Equal(t, c.version, 2) @@ -746,3 +746,21 @@ func TestConfigurator_UpdateSetsStuff(t *testing.T) { require.Equal(t, c.base, &config) require.Equal(t, 2, c.version) } + +func TestConfigurator_ServerNameOrNodeName(t *testing.T) { + c := Configurator{base: &Config{}} + type variant struct { + server, node, expected string + } + variants := []variant{ + {"", "", ""}, + {"a", "", "a"}, + {"", "b", "b"}, + {"a", "b", "a"}, + } + for _, v := range variants { + c.base.ServerName = v.server + c.base.NodeName = v.node + require.Equal(t, v.expected, c.serverNameOrNodeName()) + } +} From 3d7fc9eaab1ec24b0a6e2fe49bde8b8afe7fba72 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Fri, 8 Mar 2019 15:44:08 +0100 Subject: [PATCH 27/31] add note about tls reloading to the docs. --- website/source/docs/agent/options.html.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/website/source/docs/agent/options.html.md b/website/source/docs/agent/options.html.md index c3a360bade3b..e0cdfd7ffcc8 100644 --- a/website/source/docs/agent/options.html.md +++ b/website/source/docs/agent/options.html.md @@ -1731,6 +1731,8 @@ items which are reloaded include: * Services * Watches * HTTP Client Address +* TLS Configuration + * Please be aware that this is currently limited to reload a configuration that is already TLS enabled. You cannot enable or disable TLS only with reloading. * Node Metadata * Metric Prefix Filter * Discard Check Output From 7f78eb8979a47123a323c8262a0f933ae3bfea73 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Mon, 11 Mar 2019 20:53:27 +0100 Subject: [PATCH 28/31] add comments to explain these functions are only used for testing. --- agent/consul/client.go | 6 ++++-- agent/consul/server.go | 4 +++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/agent/consul/client.go b/agent/consul/client.go index 85eb34f0a028..d43fbf08d461 100644 --- a/agent/consul/client.go +++ b/agent/consul/client.go @@ -86,8 +86,10 @@ type Client struct { EnterpriseClient } -// NewClient is used to construct a new Consul client from the -// configuration, potentially returning an error +// NewClient is used to construct a new Consul client from the configuration, +// potentially returning an error. +// NewClient only used to help setting up a client for testing. Normal code +// exercises NewClientLogger. func NewClient(config *Config) (*Client, error) { c, err := tlsutil.NewConfigurator(config.ToTLSUtilConfig(), nil) if err != nil { diff --git a/agent/consul/server.go b/agent/consul/server.go index 1e19dcbd236d..330321f08a90 100644 --- a/agent/consul/server.go +++ b/agent/consul/server.go @@ -252,6 +252,8 @@ type Server struct { EnterpriseServer } +// NewServer is only used to help setting up a server for testing. Normal code +// exercises NewServerLogger. func NewServer(config *Config) (*Server, error) { c, err := tlsutil.NewConfigurator(config.ToTLSUtilConfig(), nil) if err != nil { @@ -260,7 +262,7 @@ func NewServer(config *Config) (*Server, error) { return NewServerLogger(config, nil, new(token.Store), c) } -// NewServer is used to construct a new Consul server from the +// NewServerLogger is used to construct a new Consul server from the // configuration, potentially returning an error func NewServerLogger(config *Config, logger *log.Logger, tokens *token.Store, tlsConfigurator *tlsutil.Configurator) (*Server, error) { // Check the protocol version. From 2efe7d21b333fb0a51e937d0e7ffd11316240506 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Mon, 11 Mar 2019 22:07:35 +0100 Subject: [PATCH 29/31] actually check the cas and not only for values in the configuration --- tlsutil/config.go | 21 +++++++++++++++------ tlsutil/config_test.go | 33 ++++++++++++++++++++------------- 2 files changed, 35 insertions(+), 19 deletions(-) diff --git a/tlsutil/config.go b/tlsutil/config.go index 48972ba91266..38d4ded945cc 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -165,7 +165,7 @@ func (c *Configurator) Update(config Config) error { return err } - if err = c.check(config, cert); err != nil { + if err = c.check(config, cas, cert); err != nil { return err } c.Lock() @@ -178,7 +178,7 @@ func (c *Configurator) Update(config Config) error { return nil } -func (c *Configurator) check(config Config, cert *tls.Certificate) error { +func (c *Configurator) check(config Config, cas *x509.CertPool, cert *tls.Certificate) error { // Check if a minimum TLS version was set if config.TLSMinVersion != "" { if _, ok := TLSLookup[config.TLSMinVersion]; !ok { @@ -187,13 +187,13 @@ func (c *Configurator) check(config Config, cert *tls.Certificate) error { } // Ensure we have a CA if VerifyOutgoing is set - if config.VerifyOutgoing && config.CAFile == "" && config.CAPath == "" { + if config.VerifyOutgoing && cas == nil { return fmt.Errorf("VerifyOutgoing set, and no CA certificate provided!") } // Ensure we have a CA and cert if VerifyIncoming is set if config.VerifyIncoming || config.VerifyIncomingRPC || config.VerifyIncomingHTTPS { - if config.CAFile == "" && config.CAPath == "" { + if cas == nil { return fmt.Errorf("VerifyIncoming set, and no CA certificate provided!") } if cert == nil { @@ -218,7 +218,16 @@ func loadCAs(caFile, caPath string) (*x509.CertPool, error) { if caFile != "" { return rootcerts.LoadCAFile(caFile) } else if caPath != "" { - return rootcerts.LoadCAPath(caPath) + pool, err := rootcerts.LoadCAPath(caPath) + if err != nil { + return nil, err + } + // make sure to not return an empty pool because this is not + // the users intention when providing a path for CAs. + if len(pool.Subjects()) == 0 { + return nil, fmt.Errorf("Error loading CA: path %q has no CAs", caPath) + } + return pool, nil } return nil, nil } @@ -268,7 +277,7 @@ func (c *Configurator) commonTLSConfig(additionalVerifyIncomingFlag bool) *tls.C func (c *Configurator) outgoingRPCTLSDisabled() bool { c.RLock() defer c.RUnlock() - return c.base.CAFile == "" && c.base.CAPath == "" && !c.base.VerifyOutgoing + return c.cas == nil && !c.base.VerifyOutgoing } // This function acquires a read lock because it reads from the config. diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index 2e837e94ee52..04fe648a48aa 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -384,8 +384,8 @@ func TestConfigurator_ErrorPropagation(t *testing.T) { true, false}, {Config{VerifyIncomingHTTPS: true, CAFile: "", CAPath: ""}, true, false}, - {Config{VerifyIncoming: true, CAFile: "a", CAPath: ""}, true, false}, - {Config{VerifyIncoming: true, CAFile: "", CAPath: "a"}, true, false}, + {Config{VerifyIncoming: true, CAFile: cafile, CAPath: ""}, true, false}, + {Config{VerifyIncoming: true, CAFile: "", CAPath: capath}, true, false}, {Config{VerifyIncoming: true, CAFile: "", CAPath: capath, CertFile: certfile, KeyFile: keyfile}, false, false}, {Config{CertFile: "bogus", KeyFile: "bogus"}, true, true}, @@ -403,7 +403,9 @@ func TestConfigurator_ErrorPropagation(t *testing.T) { if !v.excludeCheck { cert, err := v.config.KeyPair() require.NoError(t, err, info) - err3 = c.check(v.config, cert) + cas, _ := loadCAs(v.config.CAFile, v.config.CAPath) + require.NoError(t, err, info) + err3 = c.check(v.config, cas, cert) } if v.shouldErr { require.Error(t, err1, info) @@ -453,6 +455,7 @@ func TestConfigurator_loadCAs(t *testing.T) { {"", "", false, true, 0}, {"bogus", "", true, true, 0}, {"", "bogus", true, true, 0}, + {"", "../test/bin", true, true, 0}, {"../test/ca/root.cer", "", false, false, 1}, {"", "../test/ca_path", false, false, 2}, {"../test/ca/root.cer", "../test/ca_path", false, false, 1}, @@ -589,21 +592,25 @@ func TestConfigurator_OutgoingRPCTLSDisabled(t *testing.T) { path string expected bool } + cafile := "../test/ca/root.cer" + capath := "../test/ca_path" variants := []variant{ {false, "", "", true}, - {false, "a", "", false}, - {false, "", "a", false}, - {false, "a", "a", false}, + {false, cafile, "", false}, + {false, "", capath, false}, + {false, cafile, capath, false}, {true, "", "", false}, - {true, "a", "", false}, - {true, "", "a", false}, - {true, "a", "a", false}, + {true, cafile, "", false}, + {true, "", capath, false}, + {true, cafile, capath, false}, } - for _, v := range variants { + for i, v := range variants { + info := fmt.Sprintf("case %d", i) + cas, err := loadCAs(v.file, v.path) + require.NoError(t, err, info) + c.cas = cas c.base.VerifyOutgoing = v.verify - c.base.CAFile = v.file - c.base.CAPath = v.path - require.Equal(t, v.expected, c.outgoingRPCTLSDisabled()) + require.Equal(t, v.expected, c.outgoingRPCTLSDisabled(), info) } } From 282bd93ec4a5673c3e969eaf5da95dfb97249c0e Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Tue, 12 Mar 2019 11:25:03 +0100 Subject: [PATCH 30/31] set next protos, but do not accept everything from the client --- tlsutil/config.go | 7 +++---- tlsutil/config_test.go | 18 ++---------------- 2 files changed, 5 insertions(+), 20 deletions(-) diff --git a/tlsutil/config.go b/tlsutil/config.go index 38d4ded945cc..c9f2222bdf51 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -332,10 +332,9 @@ func (c *Configurator) IncomingRPCConfig() *tls.Config { func (c *Configurator) IncomingHTTPSConfig() *tls.Config { c.log("IncomingHTTPSConfig") config := c.commonTLSConfig(c.verifyIncomingHTTPS()) - config.GetConfigForClient = func(hello *tls.ClientHelloInfo) (*tls.Config, error) { - config := c.IncomingHTTPSConfig() - config.NextProtos = hello.SupportedProtos - return config, nil + config.NextProtos = []string{"h2", "http/1.1"} + config.GetConfigForClient = func(*tls.ClientHelloInfo) (*tls.Config, error) { + return c.IncomingHTTPSConfig(), nil } return config } diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index 04fe648a48aa..1fdf6be07c8e 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -667,22 +667,8 @@ func TestConfigurator_IncomingRPCConfig(t *testing.T) { } func TestConfigurator_IncomingHTTPSConfig(t *testing.T) { - c, err := NewConfigurator(Config{ - VerifyIncomingHTTPS: true, - CAFile: "../test/ca/root.cer", - CertFile: "../test/key/ourdomain.cer", - KeyFile: "../test/key/ourdomain.key", - }, nil) - require.NoError(t, err) - tlsConf := c.IncomingHTTPSConfig() - require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) - require.NotNil(t, tlsConf.GetConfigForClient) - tlsConf, err = tlsConf.GetConfigForClient( - &tls.ClientHelloInfo{SupportedProtos: []string{"h2"}}, - ) - require.NoError(t, err) - require.Equal(t, tls.RequireAndVerifyClientCert, tlsConf.ClientAuth) - require.Equal(t, []string{"h2"}, tlsConf.NextProtos) + c := Configurator{base: &Config{}} + require.Equal(t, nextProtos, c.IncomingHTTPSConfig().NextProtos) } func TestConfigurator_OutgoingTLSConfigForChecks(t *testing.T) { From 421cacffd6d8ec2c94f814eb704473c6febdad9b Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Tue, 12 Mar 2019 16:35:06 +0100 Subject: [PATCH 31/31] fix test :) --- tlsutil/config_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index 1fdf6be07c8e..5515871c771d 100644 --- a/tlsutil/config_test.go +++ b/tlsutil/config_test.go @@ -668,7 +668,7 @@ func TestConfigurator_IncomingRPCConfig(t *testing.T) { func TestConfigurator_IncomingHTTPSConfig(t *testing.T) { c := Configurator{base: &Config{}} - require.Equal(t, nextProtos, c.IncomingHTTPSConfig().NextProtos) + require.Equal(t, []string{"h2", "http/1.1"}, c.IncomingHTTPSConfig().NextProtos) } func TestConfigurator_OutgoingTLSConfigForChecks(t *testing.T) {