From 65887ae1b495c55e5c51371ff9807dbe5cb3fbc4 Mon Sep 17 00:00:00 2001 From: Hitoshi Mitake Date: Sun, 23 Dec 2018 00:21:53 +0900 Subject: [PATCH 1/3] pkg, clientv3, etcdmain: let grpcproxy rise an error when its cert has non empty CN Fix https://github.com/etcd-io/etcd/issues/9521 --- etcdmain/grpc_proxy.go | 2 +- pkg/tlsutil/tlsutil.go | 1 + pkg/transport/listener.go | 26 ++++++++++++++++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/etcdmain/grpc_proxy.go b/etcdmain/grpc_proxy.go index 425ba8fc8..581863d99 100644 --- a/etcdmain/grpc_proxy.go +++ b/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 { diff --git a/pkg/tlsutil/tlsutil.go b/pkg/tlsutil/tlsutil.go index 79b1f632e..3a5aef089 100644 --- a/pkg/tlsutil/tlsutil.go +++ b/pkg/tlsutil/tlsutil.go @@ -41,6 +41,7 @@ func NewCertPool(CAFiles []string) (*x509.CertPool, error) { if err != nil { return nil, err } + certPool.AddCert(cert) } } diff --git a/pkg/transport/listener.go b/pkg/transport/listener.go index 620b9e790..0c593e8e2 100644 --- a/pkg/transport/listener.go +++ b/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 } From b1afe210e4f745a823e8488e599f58aed8b40319 Mon Sep 17 00:00:00 2001 From: Hitoshi Mitake Date: Mon, 24 Dec 2018 23:45:56 +0900 Subject: [PATCH 2/3] Documentation: describe the problem of CN based auth + grpcproxy --- Documentation/op-guide/authentication.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/op-guide/authentication.md b/Documentation/op-guide/authentication.md index 471eff1db..aa0880587 100644 --- a/Documentation/op-guide/authentication.md +++ b/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. From a1f964afd3f8859f9a2705b58ebfefbb49288df8 Mon Sep 17 00:00:00 2001 From: Hitoshi Mitake Date: Fri, 25 Jan 2019 00:24:30 +0900 Subject: [PATCH 3/3] tests: add a new e2e test case for the combination of non empty CN and grpc proxy --- tests/e2e/etcd_config_test.go | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/tests/e2e/etcd_config_test.go b/tests/e2e/etcd_config_test.go index 68370104f..7c660b03e 100644 --- a/tests/e2e/etcd_config_test.go +++ b/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() +}