From e16076a8e0f62cb9360be04ed46f9014cbcc6731 Mon Sep 17 00:00:00 2001 From: Andy Liu Date: Thu, 3 Oct 2019 09:37:00 +0800 Subject: [PATCH 1/3] etcdserver: cherry-pick skip client san verification option for 3.2 version. Co-authored-by: Martin Weindel Co-authored-by: Jingyi Hu Co-authored-by: Liming Liu --- etcdmain/config.go | 2 ++ pkg/transport/listener.go | 7 ++++++- pkg/transport/listener_tls.go | 36 ++++++++++++++++++++++++----------- 3 files changed, 33 insertions(+), 12 deletions(-) diff --git a/etcdmain/config.go b/etcdmain/config.go index c4a7d409dbd..532369b53ef 100644 --- a/etcdmain/config.go +++ b/etcdmain/config.go @@ -193,6 +193,8 @@ func newConfig() *config { fs.Var(flags.NewStringsValueV2(""), "cipher-suites", "Comma-separated list of supported TLS cipher suites between client/server and peers (empty will be auto-populated by Go).") + fs.BoolVar(&cfg.PeerTLSInfo.SkipClientSANVerify, "experimental-peer-skip-client-san-verification", false, "Skip verification of SAN field in client certificate for peer connections.") + // logging fs.BoolVar(&cfg.Debug, "debug", false, "Enable debug-level logging for etcd.") fs.StringVar(&cfg.LogPkgLevels, "log-package-levels", "", "Specify a particular log level for each etcd package (eg: 'etcdmain=CRITICAL,etcdserver=DEBUG').") diff --git a/pkg/transport/listener.go b/pkg/transport/listener.go index 9dac85ade86..939e2965ffb 100644 --- a/pkg/transport/listener.go +++ b/pkg/transport/listener.go @@ -52,7 +52,10 @@ func wrapTLS(addr, scheme string, tlsinfo *TLSInfo, l net.Listener) (net.Listene if scheme != "https" && scheme != "unixs" { return l, nil } - return newTLSListener(l, tlsinfo) + if tlsinfo != nil && tlsinfo.SkipClientSANVerify { + return NewTLSListener(l, tlsinfo) + } + return newTLSListener(l, tlsinfo, checkSAN) } type TLSInfo struct { @@ -62,6 +65,8 @@ type TLSInfo struct { TrustedCAFile string ClientCertAuth bool + SkipClientSANVerify bool + // ServerName ensures the cert matches the given host in case of discovery / virtual hosting ServerName string diff --git a/pkg/transport/listener_tls.go b/pkg/transport/listener_tls.go index 86511860335..2d51e54d650 100644 --- a/pkg/transport/listener_tls.go +++ b/pkg/transport/listener_tls.go @@ -32,9 +32,18 @@ type tlsListener struct { donec chan struct{} err error handshakeFailure func(*tls.Conn, error) + check tlsCheckFunc } -func newTLSListener(l net.Listener, tlsinfo *TLSInfo) (net.Listener, error) { +type tlsCheckFunc func(context.Context, *tls.Conn) error + +// NewTLSListener handshakes TLS connections and performs optional CRL checking. +func NewTLSListener(l net.Listener, tlsinfo *TLSInfo) (net.Listener, error) { + check := func(context.Context, *tls.Conn) error { return nil } + return newTLSListener(l, tlsinfo, check) +} + +func newTLSListener(l net.Listener, tlsinfo *TLSInfo, check tlsCheckFunc) (net.Listener, error) { if tlsinfo == nil || tlsinfo.Empty() { l.Close() return nil, fmt.Errorf("cannot listen on TLS for %s: KeyFile and CertFile are not presented", l.Addr().String()) @@ -53,6 +62,7 @@ func newTLSListener(l net.Listener, tlsinfo *TLSInfo) (net.Listener, error) { connc: make(chan net.Conn), donec: make(chan struct{}), handshakeFailure: hf, + check: check, } go tlsl.acceptLoop() return tlsl, nil @@ -116,14 +126,9 @@ func (l *tlsListener) acceptLoop() { return } - st := tlsConn.ConnectionState() - if len(st.PeerCertificates) > 0 { - cert := st.PeerCertificates[0] - addr := tlsConn.RemoteAddr().String() - if cerr := checkCert(ctx, cert, addr); cerr != nil { - l.handshakeFailure(tlsConn, cerr) - return - } + if err := l.check(ctx, tlsConn); err != nil { + l.handshakeFailure(tlsConn, err) + return } select { case l.connc <- tlsConn: @@ -134,11 +139,20 @@ func (l *tlsListener) acceptLoop() { } } -func checkCert(ctx context.Context, cert *x509.Certificate, remoteAddr string) error { - h, _, herr := net.SplitHostPort(remoteAddr) +func checkSAN(ctx context.Context, tlsConn *tls.Conn) error { + st := tlsConn.ConnectionState() + if certs := st.PeerCertificates; len(certs) > 0 { + addr := tlsConn.RemoteAddr().String() + return checkCertSAN(ctx, certs[0], addr) + } + return nil +} + +func checkCertSAN(ctx context.Context, cert *x509.Certificate, remoteAddr string) error { if len(cert.IPAddresses) == 0 && len(cert.DNSNames) == 0 { return nil } + h, _, herr := net.SplitHostPort(remoteAddr) if herr != nil { return herr } From ab1f4fec073cf363eb31564b4804bd6861acfa47 Mon Sep 17 00:00:00 2001 From: Andy Liu Date: Thu, 3 Oct 2019 10:39:42 +0800 Subject: [PATCH 2/3] etcdserver: add unit test. --- pkg/transport/listener.go | 4 +- pkg/transport/listener_test.go | 108 ++++++++++++++++++++++++++++++++- 2 files changed, 107 insertions(+), 5 deletions(-) diff --git a/pkg/transport/listener.go b/pkg/transport/listener.go index 939e2965ffb..c5067f94c5a 100644 --- a/pkg/transport/listener.go +++ b/pkg/transport/listener.go @@ -94,7 +94,7 @@ func (info TLSInfo) Empty() bool { return info.CertFile == "" && info.KeyFile == "" } -func SelfCert(dirpath string, hosts []string) (info TLSInfo, err error) { +func SelfCert(dirpath string, hosts []string, additionalUsages ...x509.ExtKeyUsage) (info TLSInfo, err error) { if err = os.MkdirAll(dirpath, 0700); err != nil { return } @@ -123,7 +123,7 @@ func SelfCert(dirpath string, hosts []string) (info TLSInfo, err error) { NotAfter: time.Now().Add(365 * (24 * time.Hour)), KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature, - ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + ExtKeyUsage: append([]x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, additionalUsages...), BasicConstraintsValid: true, } diff --git a/pkg/transport/listener_test.go b/pkg/transport/listener_test.go index 6cc44a118f9..4a70e3130cd 100644 --- a/pkg/transport/listener_test.go +++ b/pkg/transport/listener_test.go @@ -16,20 +16,26 @@ package transport import ( "crypto/tls" + "crypto/x509" "errors" "io/ioutil" + "net" "net/http" "os" "testing" "time" ) -func createSelfCert() (*TLSInfo, func(), error) { +func createSelfCert(hosts ...string) (*TLSInfo, func(), error) { + return createSelfCertEx("127.0.0.1") +} + +func createSelfCertEx(host string, additionalUsages ...x509.ExtKeyUsage) (*TLSInfo, func(), error) { d, terr := ioutil.TempDir("", "etcd-test-tls-") if terr != nil { return nil, nil, terr } - info, err := SelfCert(d, []string{"127.0.0.1"}) + info, err := SelfCert(d, []string{host + ":0"}, additionalUsages...) if err != nil { return nil, nil, err } @@ -70,7 +76,103 @@ func testNewListenerTLSInfoAccept(t *testing.T, tlsInfo TLSInfo) { } defer conn.Close() if _, ok := conn.(*tls.Conn); !ok { - t.Errorf("failed to accept *tls.Conn") + t.Error("failed to accept *tls.Conn") + } +} + +func TestNewListenerTLSInfoSkipClientSANVerify(t *testing.T) { + tests := []struct { + skipClientSANVerify bool + goodClientHost bool + acceptExpected bool + }{ + {false, true, true}, + {false, false, false}, + {true, true, true}, + {true, false, true}, + } + for _, test := range tests { + testNewListenerTLSInfoClientCheck(t, test.skipClientSANVerify, test.goodClientHost, test.acceptExpected) + } +} + +func testNewListenerTLSInfoClientCheck(t *testing.T, skipClientSANVerify, goodClientHost, acceptExpected bool) { + tlsInfo, del, err := createSelfCert() + if err != nil { + t.Fatalf("unable to create cert: %v", err) + } + defer del() + + host := "127.0.0.222" + if goodClientHost { + host = "127.0.0.1" + } + clientTLSInfo, del2, err := createSelfCertEx(host, x509.ExtKeyUsageClientAuth) + if err != nil { + t.Fatalf("unable to create cert: %v", err) + } + defer del2() + + tlsInfo.SkipClientSANVerify = skipClientSANVerify + tlsInfo.CAFile = clientTLSInfo.CertFile + + rootCAs := x509.NewCertPool() + loaded, err := ioutil.ReadFile(tlsInfo.CertFile) + if err != nil { + t.Fatalf("unexpected missing certfile: %v", err) + } + rootCAs.AppendCertsFromPEM(loaded) + + clientCert, err := tls.LoadX509KeyPair(clientTLSInfo.CertFile, clientTLSInfo.KeyFile) + if err != nil { + t.Fatalf("unable to create peer cert: %v", err) + } + + tlsConfig := &tls.Config{} + tlsConfig.InsecureSkipVerify = false + tlsConfig.Certificates = []tls.Certificate{clientCert} + tlsConfig.RootCAs = rootCAs + + ln, err := NewListener("127.0.0.1:0", "https", tlsInfo) + if err != nil { + t.Fatalf("unexpected NewListener error: %v", err) + } + defer ln.Close() + + tr := &http.Transport{TLSClientConfig: tlsConfig} + cli := &http.Client{Transport: tr} + chClientErr := make(chan error) + go func() { + _, err := cli.Get("https://" + ln.Addr().String()) + chClientErr <- err + }() + + chAcceptErr := make(chan error) + chAcceptConn := make(chan net.Conn) + go func() { + conn, err := ln.Accept() + if err != nil { + chAcceptErr <- err + } else { + chAcceptConn <- conn + } + }() + + select { + case <-chClientErr: + if acceptExpected { + t.Errorf("accepted for good client address: skipClientSANVerify=%v, goodClientHost=%v", skipClientSANVerify, goodClientHost) + } + case acceptErr := <-chAcceptErr: + t.Fatalf("unexpected Accept error: %v", acceptErr) + case conn := <-chAcceptConn: + defer conn.Close() + if _, ok := conn.(*tls.Conn); !ok { + t.Errorf("failed to accept *tls.Conn") + } + if !acceptExpected { + t.Errorf("accepted for bad client address: skipClientSANVerify=%v, goodClientHost=%v", skipClientSANVerify, goodClientHost) + } } } From c552f11bfe5782cbb5e1c45bac47516bb5643cc9 Mon Sep 17 00:00:00 2001 From: Andy Liu Date: Wed, 9 Oct 2019 13:11:56 +0800 Subject: [PATCH 3/3] helper document update. --- Documentation/op-guide/configuration.md | 5 +++++ etcdmain/help.go | 2 ++ 2 files changed, 7 insertions(+) diff --git a/Documentation/op-guide/configuration.md b/Documentation/op-guide/configuration.md index 6d4761c958b..082015951a6 100644 --- a/Documentation/op-guide/configuration.md +++ b/Documentation/op-guide/configuration.md @@ -253,6 +253,11 @@ The security flags help to [build a secure etcd cluster][security]. + default: false + env variable: ETCD_PEER_AUTO_TLS +### --experimental-peer-skip-client-san-verification ++ Skip verification of SAN field in client certificate for peer connections. ++ default: false ++ env variable: ETCD_EXPERIMENTAL_PEER_SKIP_CLIENT_SAN_VERIFICATION + ## Logging flags ### --debug diff --git a/etcdmain/help.go b/etcdmain/help.go index b40231112b3..f8c88b36271 100644 --- a/etcdmain/help.go +++ b/etcdmain/help.go @@ -152,6 +152,8 @@ security flags: peer TLS using self-generated certificates if --peer-key-file and --peer-cert-file are not provided. --cipher-suites '' comma-separated list of supported TLS cipher suites between client/server and peers (empty will be auto-populated by Go). + --experimental-peer-skip-client-san-verification 'false' + Skip verification of SAN field in client certificate for peer connections. logging flags