From 2d0ce9de3d875f9cd2227cc6c67aad9ec407c45f Mon Sep 17 00:00:00 2001
From: Piotr Tabor <ptab@google.com>
Date: Mon, 7 Sep 2020 11:52:45 +0200
Subject: [PATCH] etcdmain: grpc-proxy should only require CN-less certificates
 for --cert flags.

We have following communication schema:
client --- 1 ---> grpc-proxy --- 2 --- > etcd-server

There are 2 sets of flags/certs in grpc proxy [ https://github.com/etcd-io/etcd/blob/master/etcdmain/grpc_proxy.go#L140 ]:
 A. (cert-file, key-file, trusted-ca-file, auto-tls) this are controlling [1] so client to proxy connection and in particular they are describing proxy public identity.
 B. (cert,key, cacert ) - these are controlling [2] so what's the identity that proxy uses to make connections to the etcd-server.

If 2 (B.) contains certificate with CN and etcd-server is running with --client-cert-auth=true, the CN can be used as identity of 'client' from service perspective. This is permission escalation, that we should forbid.

If 1 (A.) contains certificate with CN - it should be considered perfectly valid. The server can (should) have full identity.

So only --cert flag (and not --cert-file flag) should be validated for empty CN.
---
 etcdmain/grpc_proxy.go    | 13 +++++++++----
 pkg/transport/listener.go |  2 +-
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/etcdmain/grpc_proxy.go b/etcdmain/grpc_proxy.go
index 50e3d9a64..e79dce776 100644
--- a/etcdmain/grpc_proxy.go
+++ b/etcdmain/grpc_proxy.go
@@ -181,7 +181,11 @@ func startGRPCProxy(cmd *cobra.Command, args []string) {
 	}
 	grpclog.SetLoggerV2(gl)
 
-	tlsinfo := newTLS(grpcProxyListenCA, grpcProxyListenCert, grpcProxyListenKey)
+	// The proxy itself (ListenCert) can have not-empty CN.
+	// The empty CN is required for grpcProxyCert.
+	// Please see https://github.com/etcd-io/etcd/issues/11970#issuecomment-687875315  for more context.
+	tlsinfo := newTLS(grpcProxyListenCA, grpcProxyListenCert, grpcProxyListenKey, false)
+
 	if tlsinfo == nil && grpcProxyListenAutoTLS {
 		host := []string{"https://" + grpcProxyListenAddr}
 		dir := filepath.Join(grpcProxyDataDir, "fixtures", "proxy")
@@ -320,7 +324,8 @@ func newClientCfg(lg *zap.Logger, eps []string) (*clientv3.Config, error) {
 		cfg.MaxCallRecvMsgSize = grpcMaxCallRecvMsgSize
 	}
 
-	tls := newTLS(grpcProxyCA, grpcProxyCert, grpcProxyKey)
+	lg.Info("grpcProxyCA for connections to etcd-server")
+	tls := newTLS(grpcProxyCA, grpcProxyCert, grpcProxyKey, true)
 	if tls == nil && grpcProxyInsecureSkipTLSVerify {
 		tls = &transport.TLSInfo{}
 	}
@@ -339,11 +344,11 @@ func newClientCfg(lg *zap.Logger, eps []string) (*clientv3.Config, error) {
 	return &cfg, nil
 }
 
-func newTLS(ca, cert, key string) *transport.TLSInfo {
+func newTLS(ca, cert, key string, requireEmptyCN bool) *transport.TLSInfo {
 	if ca == "" && cert == "" && key == "" {
 		return nil
 	}
-	return &transport.TLSInfo{TrustedCAFile: ca, CertFile: cert, KeyFile: key, EmptyCN: true}
+	return &transport.TLSInfo{TrustedCAFile: ca, CertFile: cert, KeyFile: key, EmptyCN: requireEmptyCN}
 }
 
 func mustListenCMux(lg *zap.Logger, tlsinfo *transport.TLSInfo) cmux.CMux {
diff --git a/pkg/transport/listener.go b/pkg/transport/listener.go
index af0bc1e5d..f1522b1ce 100644
--- a/pkg/transport/listener.go
+++ b/pkg/transport/listener.go
@@ -437,7 +437,7 @@ func (info TLSInfo) ClientConfig() (*tls.Config, error) {
 			return tls.X509KeyPair(certPEMBlock, keyPEMBlock)
 		})
 		if hasNonEmptyCN {
-			return nil, fmt.Errorf("cert has non empty Common Name (%s)", cn)
+			return nil, fmt.Errorf("cert has non empty Common Name (%s): %s", cn, info.CertFile)
 		}
 	}