From d174cbe7f461b70138cff449246ac01d2f0d290d Mon Sep 17 00:00:00 2001 From: Nelson Elhage Date: Sun, 22 Jun 2014 12:49:51 -0700 Subject: [PATCH 1/2] Restore the 0.2 TLS verification behavior. Namely, don't check the DNS names in TLS certificates when connecting to other servers. As of golang 1.3, crypto/tls no longer natively supports doing partial verification (verifying the cert issuer but not the hostname), so we have to disable verification entirely and then do the issuer verification ourselves. Fortunately, crypto/x509 makes this relatively straightforward. If the "server_name" configuration option is passed, we preserve the existing behavior of checking that server name everywhere. No option is provided to retain the current behavior of checking the remote certificate against the local node name, since that behavior seems clearly buggy and unintentional, and I have difficulty imagining it is actually being used anywhere. It would be relatively straightforward to restore if desired, however. --- consul/client.go | 6 ++-- consul/config.go | 68 +++++++++++++++++++++++++++++++++++++++---- consul/config_test.go | 43 ++++++++++++++++++++------- consul/pool.go | 6 +++- consul/raft_rpc.go | 5 +++- consul/server.go | 9 ++---- 6 files changed, 109 insertions(+), 28 deletions(-) diff --git a/consul/client.go b/consul/client.go index 5b1ea29467..92d9231959 100644 --- a/consul/client.go +++ b/consul/client.go @@ -88,10 +88,8 @@ func NewClient(config *Config) (*Client, error) { // Create the tlsConfig var tlsConfig *tls.Config var err error - if config.VerifyOutgoing { - if tlsConfig, err = config.OutgoingTLSConfig(); err != nil { - return nil, err - } + if tlsConfig, err = config.OutgoingTLSConfig(); err != nil { + return nil, err } // Create a logger diff --git a/consul/config.go b/consul/config.go index 6000177a8f..4904880498 100644 --- a/consul/config.go +++ b/consul/config.go @@ -172,16 +172,21 @@ func (c *Config) KeyPair() (*tls.Certificate, error) { return &cert, err } -// OutgoingTLSConfig generates a TLS configuration for outgoing requests +// OutgoingTLSConfig generates a TLS configuration for outgoing +// requests. It will return a nil config if this configuration should +// not use TLS for outgoing connections. func (c *Config) OutgoingTLSConfig() (*tls.Config, error) { + if !c.VerifyOutgoing { + return nil, nil + } // Create the tlsConfig tlsConfig := &tls.Config{ - ServerName: c.ServerName, RootCAs: x509.NewCertPool(), - InsecureSkipVerify: !c.VerifyOutgoing, + InsecureSkipVerify: true, } - if tlsConfig.ServerName == "" { - tlsConfig.ServerName = c.NodeName + if c.ServerName != "" { + tlsConfig.ServerName = c.ServerName + tlsConfig.InsecureSkipVerify = false } // Ensure we have a CA if VerifyOutgoing is set @@ -206,6 +211,59 @@ func (c *Config) OutgoingTLSConfig() (*tls.Config, error) { return tlsConfig, nil } +// Wrap a net.Conn into a 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 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 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 +} + // IncomingTLSConfig generates a TLS configuration for incoming requests func (c *Config) IncomingTLSConfig() (*tls.Config, error) { // Create the tlsConfig diff --git a/consul/config_test.go b/consul/config_test.go index c6081603ee..dc20cf942f 100644 --- a/consul/config_test.go +++ b/consul/config_test.go @@ -78,14 +78,8 @@ func TestConfig_OutgoingTLS_OnlyCA(t *testing.T) { if err != nil { t.Fatalf("err: %v", err) } - if tls == nil { - t.Fatalf("expected config") - } - if len(tls.RootCAs.Subjects()) != 1 { - t.Fatalf("expect root cert") - } - if !tls.InsecureSkipVerify { - t.Fatalf("expect to skip verification") + if tls != nil { + t.Fatalf("expected no config") } } @@ -104,8 +98,35 @@ func TestConfig_OutgoingTLS_VerifyOutgoing(t *testing.T) { if len(tls.RootCAs.Subjects()) != 1 { t.Fatalf("expect root cert") } + if tls.ServerName != "" { + t.Fatalf("expect no server name verification") + } + if !tls.InsecureSkipVerify { + t.Fatalf("should skip built-in verification") + } +} + +func TestConfig_OutgoingTLS_ServerName(t *testing.T) { + conf := &Config{ + VerifyOutgoing: true, + CAFile: "../test/ca/root.cer", + ServerName: "consul.example.com", + } + tls, err := conf.OutgoingTLSConfig() + if err != nil { + t.Fatalf("err: %v", err) + } + if tls == nil { + t.Fatalf("expected config") + } + if len(tls.RootCAs.Subjects()) != 1 { + t.Fatalf("expect root cert") + } + if tls.ServerName != "consul.example.com" { + t.Fatalf("expect server name") + } if tls.InsecureSkipVerify { - t.Fatalf("should not skip verification") + t.Fatalf("should not skip built-in verification") } } @@ -126,8 +147,8 @@ func TestConfig_OutgoingTLS_WithKeyPair(t *testing.T) { if len(tls.RootCAs.Subjects()) != 1 { t.Fatalf("expect root cert") } - if tls.InsecureSkipVerify { - t.Fatalf("should not skip verification") + if !tls.InsecureSkipVerify { + t.Fatalf("should skip verification") } if len(tls.Certificates) != 1 { t.Fatalf("expected client cert") diff --git a/consul/pool.go b/consul/pool.go index 804a900f5a..3b7e80c297 100644 --- a/consul/pool.go +++ b/consul/pool.go @@ -221,7 +221,11 @@ func (p *ConnPool) getNewConn(addr net.Addr, version int) (*Conn, error) { } // Wrap the connection in a TLS client - conn = tls.Client(conn, p.tlsConfig) + conn, err = wrapTLSClient(conn, p.tlsConfig) + if err != nil { + conn.Close() + return nil, err + } } // Switch the multiplexing based on version diff --git a/consul/raft_rpc.go b/consul/raft_rpc.go index 1221ce06f2..1024cd9878 100644 --- a/consul/raft_rpc.go +++ b/consul/raft_rpc.go @@ -94,7 +94,10 @@ func (l *RaftLayer) Dial(address string, timeout time.Duration) (net.Conn, error } // Wrap the connection in a TLS client - conn = tls.Client(conn, l.tlsConfig) + conn, err = wrapTLSClient(conn, l.tlsConfig) + if err != nil { + return nil, err + } } // Write the Raft byte to set the mode diff --git a/consul/server.go b/consul/server.go index e7dd195f34..8af3fd4ef8 100644 --- a/consul/server.go +++ b/consul/server.go @@ -145,12 +145,9 @@ func NewServer(config *Config) (*Server, error) { } // Create the tlsConfig for outgoing connections - var tlsConfig *tls.Config - var err error - if config.VerifyOutgoing { - if tlsConfig, err = config.OutgoingTLSConfig(); err != nil { - return nil, err - } + tlsConfig, err := config.OutgoingTLSConfig() + if err != nil { + return nil, err } // Get the incoming tls config From 12a7f765b61c7c9b62533a29a205cbe2f7af3b72 Mon Sep 17 00:00:00 2001 From: Nelson Elhage Date: Sun, 29 Jun 2014 18:11:32 -0700 Subject: [PATCH 2/2] Add some basic smoke tests for wrapTLSclient. Check the success case, and check that we reject a self-signed certificate. --- consul/config.go | 4 +- consul/config_test.go | 96 ++++++++++++++++++++++++++++++++++ test/key/ssl-cert-snakeoil.key | 28 ++++++++++ test/key/ssl-cert-snakeoil.pem | 17 ++++++ 4 files changed, 143 insertions(+), 2 deletions(-) create mode 100644 test/key/ssl-cert-snakeoil.key create mode 100644 test/key/ssl-cert-snakeoil.pem diff --git a/consul/config.go b/consul/config.go index 4904880498..9aa0c86234 100644 --- a/consul/config.go +++ b/consul/config.go @@ -211,8 +211,8 @@ func (c *Config) OutgoingTLSConfig() (*tls.Config, error) { return tlsConfig, nil } -// Wrap a net.Conn into a tls connection, performing any additional -// verification as needed. +// 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 diff --git a/consul/config_test.go b/consul/config_test.go index dc20cf942f..1007ffba77 100644 --- a/consul/config_test.go +++ b/consul/config_test.go @@ -3,6 +3,9 @@ package consul import ( "crypto/tls" "crypto/x509" + "io" + "io/ioutil" + "net" "testing" ) @@ -155,6 +158,99 @@ func TestConfig_OutgoingTLS_WithKeyPair(t *testing.T) { } } +func startTLSServer(config *Config) (net.Conn, chan error) { + errc := make(chan error, 1) + + tlsConfigServer, err := config.IncomingTLSConfig() + if err != nil { + errc <- err + return nil, errc + } + + client, server := net.Pipe() + go func() { + tlsServer := tls.Server(server, tlsConfigServer) + if err := tlsServer.Handshake(); err != nil { + errc <- err + } + close(errc) + // Because net.Pipe() is unbuffered, if both sides + // Close() simultaneously, we will deadlock as they + // both send an alert and then block. So we make the + // server read any data from the client until error or + // EOF, which will allow the client to Close(), and + // *then* we Close() the server. + io.Copy(ioutil.Discard, tlsServer) + tlsServer.Close() + }() + return client, errc +} + +func TestConfig_wrapTLS_OK(t *testing.T) { + config := &Config{ + CAFile: "../test/ca/root.cer", + CertFile: "../test/key/ourdomain.cer", + KeyFile: "../test/key/ourdomain.key", + VerifyOutgoing: true, + } + + client, errc := startTLSServer(config) + if client == nil { + t.Fatalf("startTLSServer err: %v", <-errc) + } + + clientConfig, err := config.OutgoingTLSConfig() + if err != nil { + t.Fatalf("OutgoingTLSConfig err: %v", err) + } + + tlsClient, err := wrapTLSClient(client, clientConfig) + if err != nil { + t.Fatalf("wrapTLS err: %v", err) + } else { + tlsClient.Close() + } + err = <-errc + if err != nil { + t.Fatalf("server: %v", err) + } +} + +func TestConfig_wrapTLS_BadCert(t *testing.T) { + serverConfig := &Config{ + CertFile: "../test/key/ssl-cert-snakeoil.pem", + KeyFile: "../test/key/ssl-cert-snakeoil.key", + } + + client, errc := startTLSServer(serverConfig) + if client == nil { + t.Fatalf("startTLSServer err: %v", <-errc) + } + + clientConfig := &Config{ + CAFile: "../test/ca/root.cer", + VerifyOutgoing: true, + } + + clientTLSConfig, err := clientConfig.OutgoingTLSConfig() + if err != nil { + t.Fatalf("OutgoingTLSConfig err: %v", err) + } + + tlsClient, err := wrapTLSClient(client, clientTLSConfig) + if err == nil { + t.Fatalf("wrapTLS no err") + } + if tlsClient != nil { + t.Fatalf("returned a client") + } + + err = <-errc + if err != nil { + t.Fatalf("server: %v", err) + } +} + func TestConfig_IncomingTLS(t *testing.T) { conf := &Config{ VerifyIncoming: true, diff --git a/test/key/ssl-cert-snakeoil.key b/test/key/ssl-cert-snakeoil.key new file mode 100644 index 0000000000..22cc4acb14 --- /dev/null +++ b/test/key/ssl-cert-snakeoil.key @@ -0,0 +1,28 @@ +-----BEGIN PRIVATE KEY----- +MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQDYVw5skn/3Ka72 +32ZaCrKtRVoQzan3tghq41KpQe3yZxIZbKy7sbwfdXnXVSwTAbq/3BYi9rya2t/v +W95yZh6JgfrLBvWl9Jo1EttZIxDhzCXGP+MPWm2KdNtHr84JznJbdxRpR0Jb4ykK +2d9dXbLJvCw8eEDFgOVGrj60USMir46sZFRvGWlMi+yHSOE+WQXaU40Dr0ZJqNvd +RNO9BtqpLaecZQaYTvlkyVdhjUE3+gQ0zEAQqpLcWi+zB5/IyR2+KwxDT3vAJumd +G7rIaGatPE8k0Ahb+zMKFFGYCoQ3sjbAbrQmrVtH4SU6ggl+CxpVdxshrK1W05Ms +WAiPw81/AgMBAAECggEAKjDIKlpjxGMHsTOeNV8yu2H0D6TcSefhOl885q9p5UU+ +nWC5Sx19b7EsYtdEcix7LCGS25y86YJX+8kx16OcvvpvW5ru2z+Zt1IHHxocl7yF +fWVGNd9Pz5m8jf12NClj2fyeKW3xPhROE8Srr/yu+nLNObnF//6EOEWRCv9r176C ++dzYvYVNPP48Ug7NpjQB94CBprtJyqvuoXvBPtpARXazVniYEhnzG1Gaj1TiCII5 ++emaMjKcWIEJ5stbBb3lUtqgm8bRNb/qcxoFfqTzHP+hbum9hbRz0KEIlAkm7uAv +S0TlyLuaj+gPQ+LwNX8EhGKUdlK/VM5bj2kq/tg3AQKBgQD/+A8ruHNa5nKGKNzP +dp+hXiL2sSzefMjDa2+sRJ0yftIMqYRfCJwzYumjfyycfCsu1LHainlQjSO6Kkgc +c0xVxnahWyPCQiqZuo9lLx4EVXCdRqWRg+pbyQhTSz90hfWEKD7XWsI8uRkOEnW8 +36FiyovGDFxl0esaKrFNSFdmgQKBgQDYXcSIRJk41f7vL6FVmchpUnVYoD75k9YT +FqEplNMw6gXcqbC2aNH5wj7EJlRboyVpjXV4N0d2Cz6AwREJpr/rYpq68AixXmVs +kTKwevoHm/tln7CN+CyIEy6KXdLp4KoWLFfSG6tHWRwIGFxWEGrrIZS6Eznu4GPe +V2yOnMkz/wKBgC6nXtSALP5PbGZJgl2J6HR3/PVru5rdsZX0ugjzBJfUh6JpL0hH +AHlZOO5k2pO3CgPiHnyPqqbk4rMmy7frx+kGYE7ulqjseGlGmKY/nT/69qij3L+W +BJwwGwVbfLhXRjWNRE7qKub4cbmf4bfIJtkjw7AYRqsERM6jI2fLnKqBAoGAUBzY +CkSsHxlNXa7bI+DfDfBUNs6OwsZ0e3jjj4vlbrUYGo5SOhgxtzKvHt26Wnvb/Gs+ +VZbSROkA6ZeTAWnWogdOl20NKu9yynIwvJusPGkK+qPYMZj0lCXWE7GNyL9A+xjM +I6XPE4nxESZD+jH2BL3YXdWEm+hF0iu4rE1tSm0CgYEAxssvvX7qcfTmxsp1YSHJ +H5j9ifkakci5W2VbCbdMtdOlgIlCFr2JYguaL98jx7WIJ4iH54ue/fbOdlkPCOsz +YGU4TceSRHeEJ7F6c67NOXm8j2TquAW2uYH87w07g2PIUwl/pp439qoDiThA6jEX +2ztyXgNUi7poqehPUoQuvC0= +-----END PRIVATE KEY----- diff --git a/test/key/ssl-cert-snakeoil.pem b/test/key/ssl-cert-snakeoil.pem new file mode 100644 index 0000000000..b8ad2c8a6a --- /dev/null +++ b/test/key/ssl-cert-snakeoil.pem @@ -0,0 +1,17 @@ +-----BEGIN CERTIFICATE----- +MIICsjCCAZqgAwIBAgIJAMi7aUCplU3VMA0GCSqGSIb3DQEBBQUAMBExDzANBgNV +BAMTBnVidW50dTAeFw0xMjEyMDIwNDQ3MzBaFw0yMjExMzAwNDQ3MzBaMBExDzAN +BgNVBAMTBnVidW50dTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBANhX +DmySf/cprvbfZloKsq1FWhDNqfe2CGrjUqlB7fJnEhlsrLuxvB91eddVLBMBur/c +FiL2vJra3+9b3nJmHomB+ssG9aX0mjUS21kjEOHMJcY/4w9abYp020evzgnOclt3 +FGlHQlvjKQrZ311dssm8LDx4QMWA5UauPrRRIyKvjqxkVG8ZaUyL7IdI4T5ZBdpT +jQOvRkmo291E070G2qktp5xlBphO+WTJV2GNQTf6BDTMQBCqktxaL7MHn8jJHb4r +DENPe8Am6Z0bushoZq08TyTQCFv7MwoUUZgKhDeyNsButCatW0fhJTqCCX4LGlV3 +GyGsrVbTkyxYCI/DzX8CAwEAAaMNMAswCQYDVR0TBAIwADANBgkqhkiG9w0BAQUF +AAOCAQEAQaS5yAih5NBV2edX1wkIQfAUElqmzoXvxsozDYy+P+S5tJeFXDSqzTAy +qkd/6qjkBdaARfKUJZeT/jRjqxoNtE9SR4PMOnD4zrqD26ujgZRVtPImbmVxCnMI +1B9LwvhpDHZuPGN5bPp3o+iDYea8zkS3Y31Ic889KSwKBDb1LlNogOdved+2DGd1 +yCxEErImbl4B0+QPrRk2bWbDfKhDfJ2FV+9kWIoEuCQBpr2tj1E5zvTadOVm5P2M +u7kjGl4w0GIAONiMC9l2TwMmPuG1jpM/WjQkG0sTKOCl7xQKgXBNJ78Wm2bfGtgb +shr/PNbS/EyISlUa07+zJtiRnr/EiQ== +-----END CERTIFICATE-----