Merge pull request etcd-io#10343 from mitake/proxy-cn

*: let grpcproxy rise an error when its cert has non empty CN

Documentation/op-guide/authentication.md

@@ -167,7 +167,7 @@ $ etcdctl --user user --password password get foo
 Otherwise, all `etcdctl` commands remain the same. Users and roles can still be created and modified, but require authentication by a user with the root role.
 
 ## Using TLS Common Name
-As of version v3.2 if an etcd server is launched with the option `--client-cert-auth=true`, the field of Common Name (CN) in the client's TLS cert will be used as an etcd user. In this case, the common name authenticates the user and the client does not need a password. Note that if both of 1. `--client-cert-auth=true` is passed and CN is provided by the client, and 2. username and password are provided by the client, the username and password based authentication is prioritized. gRPC-gateway does not support authentication using TLS Common Name.
+As of version v3.2 if an etcd server is launched with the option `--client-cert-auth=true`, the field of Common Name (CN) in the client's TLS cert will be used as an etcd user. In this case, the common name authenticates the user and the client does not need a password. Note that if both of 1. `--client-cert-auth=true` is passed and CN is provided by the client, and 2. username and password are provided by the client, the username and password based authentication is prioritized. Note that this feature cannot be used with gRPC-proxy and gRPC-gateway. This is because gRPC-proxy terminates TLS from its client so all the clients share a cert of the proxy. gRPC-gateway uses a TLS connection internally for transforming HTTP request to gRPC request so it shares the same limitation. Therefore the clients cannot provide their CN to the server correctly. gRPC-proxy will cause an error and stop if a given cert has non empty CN. gRPC-proxy returns an error which indicates that the client has an non empty CN in its cert.
 
 As of version v3.3 if an etcd server is launched with the option `--peer-cert-allowed-cn` filtering of CN inter-peer connections is enabled.  Nodes can only join the etcd cluster if their CN match the allowed one.
 See [etcd security page](https://github.com/etcd-io/etcd/blob/master/Documentation/op-guide/security.md) for more details.

etcdmain/grpc_proxy.go

@@ -307,7 +307,7 @@ func newTLS(ca, cert, key string) *transport.TLSInfo {
 	if ca == "" && cert == "" && key == "" {
 		return nil
 	}
-	return &transport.TLSInfo{TrustedCAFile: ca, CertFile: cert, KeyFile: key}
+	return &transport.TLSInfo{TrustedCAFile: ca, CertFile: cert, KeyFile: key, EmptyCN: true}
 }
 
 func mustListenCMux(lg *zap.Logger, tlsinfo *transport.TLSInfo) cmux.CMux {

pkg/tlsutil/tlsutil.go

@@ -41,6 +41,7 @@ func NewCertPool(CAFiles []string) (*x509.CertPool, error) {
 			if err != nil {
 				return nil, err
 			}
+
 			certPool.AddCert(cert)
 		}
 	}

pkg/transport/listener.go

@@ -91,6 +91,10 @@ type TLSInfo struct {
 	// Logger logs TLS errors.
 	// If nil, all logs are discarded.
 	Logger *zap.Logger
+
+	// EmptyCN indicates that the cert must have empty CN.
+	// If true, ClientConfig() will return an error for a cert with non empty CN.
+	EmptyCN bool
 }
 
 func (info TLSInfo) String() string {
@@ -378,6 +382,28 @@ func (info TLSInfo) ClientConfig() (*tls.Config, error) {
 	if info.selfCert {
 		cfg.InsecureSkipVerify = true
 	}
+
+	if info.EmptyCN {
+		hasNonEmptyCN := false
+		cn := ""
+		tlsutil.NewCert(info.CertFile, info.KeyFile, func(certPEMBlock []byte, keyPEMBlock []byte) (tls.Certificate, error) {
+			var block *pem.Block
+			block, _ = pem.Decode(certPEMBlock)
+			cert, err := x509.ParseCertificate(block.Bytes)
+			if err != nil {
+				return tls.Certificate{}, err
+			}
+			if len(cert.Subject.CommonName) != 0 {
+				hasNonEmptyCN = true
+				cn = cert.Subject.CommonName
+			}
+			return tls.X509KeyPair(certPEMBlock, keyPEMBlock)
+		})
+		if hasNonEmptyCN {
+			return nil, fmt.Errorf("cert has non empty Common Name (%s)", cn)
+		}
+	}
+
 	return cfg, nil
 }
 

tests/e2e/etcd_config_test.go

@@ -190,3 +190,34 @@ func TestEtcdPeerCNAuth(t *testing.T) {
 		}
 	}
 }
+
+func TestGrpcproxyAndCommonName(t *testing.T) {
+	argsWithNonEmptyCN := []string{
+		binDir + "/etcd",
+		"grpc-proxy",
+		"start",
+		"--cert", certPath2,
+		"--key", privateKeyPath2,
+		"--cacert", caPath,
+	}
+
+	argsWithEmptyCN := []string{
+		binDir + "/etcd",
+		"grpc-proxy",
+		"start",
+		"--cert", certPath3,
+		"--key", privateKeyPath3,
+		"--cacert", caPath,
+	}
+
+	err := spawnWithExpect(argsWithNonEmptyCN, "cert has non empty Common Name")
+	if err != nil {
+		t.Errorf("Unexpected error: %s", err)
+	}
+
+	p, err := spawnCmd(argsWithEmptyCN)
+	if err != nil {
+		t.Errorf("Unexpected error: %s", err)
+	}
+	p.Stop()
+}