Merge pull request #233 from nelhage/tls-no-subjname

Restore the 0.2 TLS verification behavior.

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

consul/config.go

@@ -177,16 +177,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
@@ -211,6 +216,59 @@ func (c *Config) OutgoingTLSConfig() (*tls.Config, error) {
 	return tlsConfig, nil
 }
 
+// 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 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

consul/config_test.go

@@ -3,6 +3,9 @@ package consul
 import (
 	"crypto/tls"
 	"crypto/x509"
+	"io"
+	"io/ioutil"
+	"net"
 	"testing"
 )
 
@@ -78,14 +81,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 +101,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,14 +150,107 @@ 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")
 	}
 }
 
+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,

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

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

consul/server.go

@@ -146,12 +146,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

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-----

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-----