From acc9d7c9feaeed7ca0143bfa2db395caee4d0573 Mon Sep 17 00:00:00 2001 From: lhy1024 Date: Thu, 6 Jun 2024 21:25:17 +0800 Subject: [PATCH] Support multiple values for allowed client and peer TLS identities Signed-off-by: lhy1024 --- client/pkg/transport/listener.go | 50 ++++++++++++++++-- server/embed/config.go | 28 +++++----- server/embed/config_test.go | 18 +++++-- server/etcdmain/config.go | 4 ++ server/etcdmain/help.go | 6 +-- tests/e2e/etcd_config_test.go | 90 ++++++++++++++++++++++++++++++++ 6 files changed, 173 insertions(+), 23 deletions(-) diff --git a/client/pkg/transport/listener.go b/client/pkg/transport/listener.go index bbb84af4b58..1e34f1181db 100644 --- a/client/pkg/transport/listener.go +++ b/client/pkg/transport/listener.go @@ -180,12 +180,23 @@ type TLSInfo struct { parseFunc func([]byte, []byte) (tls.Certificate, error) // AllowedCN is a CN which must be provided by a client. + // + // Deprecated: use AllowedCNs instead. AllowedCN string // AllowedHostname is an IP address or hostname that must match the TLS // certificate provided by a client. + // + // Deprecated: use AllowedHostnames instead. AllowedHostname string + // AllowedCNs is a list of acceptable CNs which must be provided by a client. + AllowedCNs []string + + // AllowedHostnames is a list of acceptable IP addresses or hostnames that must match the + // TLS certificate provided by a client. + AllowedHostnames []string + // Logger logs TLS errors. // If nil, all logs are discarded. Logger *zap.Logger @@ -414,19 +425,52 @@ func (info TLSInfo) baseConfig() (*tls.Config, error) { // Client certificates may be verified by either an exact match on the CN, // or a more general check of the CN and SANs. var verifyCertificate func(*x509.Certificate) bool + + if info.AllowedCN != "" && len(info.AllowedCNs) > 0 { + return nil, fmt.Errorf("AllowedCN and AllowedCNs are mutually exclusive (cn=%q, cns=%q)", info.AllowedCN, info.AllowedCNs) + } + if info.AllowedHostname != "" && len(info.AllowedHostnames) > 0 { + return nil, fmt.Errorf("AllowedHostname and AllowedHostnames are mutually exclusive (hostname=%q, hostnames=%q)", info.AllowedHostname, info.AllowedHostnames) + } + if info.AllowedCN != "" && info.AllowedHostname != "" { + return nil, fmt.Errorf("AllowedCN and AllowedHostname are mutually exclusive (cn=%q, hostname=%q)", info.AllowedCN, info.AllowedHostname) + } + if len(info.AllowedCNs) > 0 && len(info.AllowedHostnames) > 0 { + return nil, fmt.Errorf("AllowedCNs and AllowedHostnames are mutually exclusive (cns=%q, hostnames=%q)", info.AllowedCNs, info.AllowedHostnames) + } + if info.AllowedCN != "" { - if info.AllowedHostname != "" { - return nil, fmt.Errorf("AllowedCN and AllowedHostname are mutually exclusive (cn=%q, hostname=%q)", info.AllowedCN, info.AllowedHostname) - } + info.Logger.Warn("AllowedCN is deprecated, use AllowedCNs instead") verifyCertificate = func(cert *x509.Certificate) bool { return info.AllowedCN == cert.Subject.CommonName } } if info.AllowedHostname != "" { + info.Logger.Warn("AllowedHostname is deprecated, use AllowedHostnames instead") verifyCertificate = func(cert *x509.Certificate) bool { return cert.VerifyHostname(info.AllowedHostname) == nil } } + if len(info.AllowedCNs) > 0 { + verifyCertificate = func(cert *x509.Certificate) bool { + for _, allowedCN := range info.AllowedCNs { + if allowedCN == cert.Subject.CommonName { + return true + } + } + return false + } + } + if len(info.AllowedHostnames) > 0 { + verifyCertificate = func(cert *x509.Certificate) bool { + for _, allowedHostname := range info.AllowedHostnames { + if cert.VerifyHostname(allowedHostname) == nil { + return true + } + } + return false + } + } if verifyCertificate != nil { cfg.VerifyPeerCertificate = func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error { for _, chains := range verifiedChains { diff --git a/server/embed/config.go b/server/embed/config.go index 17b88e11d3f..0545c80cd57 100644 --- a/server/embed/config.go +++ b/server/embed/config.go @@ -472,15 +472,15 @@ type configJSON struct { } type securityConfig struct { - CertFile string `json:"cert-file"` - KeyFile string `json:"key-file"` - ClientCertFile string `json:"client-cert-file"` - ClientKeyFile string `json:"client-key-file"` - CertAuth bool `json:"client-cert-auth"` - TrustedCAFile string `json:"trusted-ca-file"` - AutoTLS bool `json:"auto-tls"` - AllowedCN string `json:"allowed-cn"` - AllowedHostname string `json:"allowed-hostname"` + CertFile string `json:"cert-file"` + KeyFile string `json:"key-file"` + ClientCertFile string `json:"client-cert-file"` + ClientKeyFile string `json:"client-key-file"` + CertAuth bool `json:"client-cert-auth"` + TrustedCAFile string `json:"trusted-ca-file"` + AutoTLS bool `json:"auto-tls"` + AllowedCNs []string `json:"allowed-cn"` + AllowedHostnames []string `json:"allowed-hostname"` } // NewConfig creates a new Config populated with default values. @@ -671,7 +671,7 @@ func (cfg *Config) AddFlags(fs *flag.FlagSet) { fs.StringVar(&cfg.ClientTLSInfo.ClientKeyFile, "client-key-file", "", "Path to an explicit peer client TLS key file otherwise key file will be used when client auth is required.") fs.BoolVar(&cfg.ClientTLSInfo.ClientCertAuth, "client-cert-auth", false, "Enable client cert authentication.") fs.StringVar(&cfg.ClientTLSInfo.CRLFile, "client-crl-file", "", "Path to the client certificate revocation list file.") - fs.StringVar(&cfg.ClientTLSInfo.AllowedHostname, "client-cert-allowed-hostname", "", "Allowed TLS hostname for client cert authentication.") + fs.Var(flags.NewStringsValue(""), "client-cert-allowed-hostname", "Comma-separated list of allowed SAN hostnames for client cert authentication.") fs.StringVar(&cfg.ClientTLSInfo.TrustedCAFile, "trusted-ca-file", "", "Path to the client server TLS trusted CA cert file.") fs.BoolVar(&cfg.ClientAutoTLS, "auto-tls", false, "Client TLS using generated certificates") fs.StringVar(&cfg.PeerTLSInfo.CertFile, "peer-cert-file", "", "Path to the peer server TLS cert file.") @@ -683,8 +683,8 @@ func (cfg *Config) AddFlags(fs *flag.FlagSet) { fs.BoolVar(&cfg.PeerAutoTLS, "peer-auto-tls", false, "Peer TLS using generated certificates") fs.UintVar(&cfg.SelfSignedCertValidity, "self-signed-cert-validity", 1, "The validity period of the client and peer certificates, unit is year") fs.StringVar(&cfg.PeerTLSInfo.CRLFile, "peer-crl-file", "", "Path to the peer certificate revocation list file.") - fs.StringVar(&cfg.PeerTLSInfo.AllowedCN, "peer-cert-allowed-cn", "", "Allowed CN for inter peer authentication.") - fs.StringVar(&cfg.PeerTLSInfo.AllowedHostname, "peer-cert-allowed-hostname", "", "Allowed TLS hostname for inter peer authentication.") + fs.Var(flags.NewStringsValue(""), "peer-cert-allowed-cn", "Comma-separated list of allowed CNs for inter-peer TLS authentication.") + fs.Var(flags.NewStringsValue(""), "peer-cert-allowed-hostname", "Comma-separated list of allowed SAN hostnames for inter-peer TLS authentication.") fs.Var(flags.NewStringsValue(""), "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.") fs.StringVar(&cfg.TlsMinVersion, "tls-min-version", string(tlsutil.TLSVersion12), "Minimum TLS version supported by etcd. Possible values: TLS1.2, TLS1.3.") @@ -857,8 +857,8 @@ func (cfg *configYAML) configFromFile(path string) error { tls.ClientKeyFile = ysc.ClientKeyFile tls.ClientCertAuth = ysc.CertAuth tls.TrustedCAFile = ysc.TrustedCAFile - tls.AllowedCN = ysc.AllowedCN - tls.AllowedHostname = ysc.AllowedHostname + tls.AllowedCNs = ysc.AllowedCNs + tls.AllowedHostnames = ysc.AllowedHostnames } copySecurityDetails(&cfg.ClientTLSInfo, &cfg.ClientSecurityJSON) copySecurityDetails(&cfg.PeerTLSInfo, &cfg.PeerSecurityJSON) diff --git a/server/embed/config_test.go b/server/embed/config_test.go index 3e79f7cd43e..9153feb06e6 100644 --- a/server/embed/config_test.go +++ b/server/embed/config_test.go @@ -41,7 +41,7 @@ func notFoundErr(service, domain string) error { func TestConfigFileOtherFields(t *testing.T) { ctls := securityConfig{TrustedCAFile: "cca", CertFile: "ccert", KeyFile: "ckey"} // Note AllowedCN and AllowedHostname are mutually exclusive, this test is just to verify the fields can be correctly marshalled & unmarshalled. - ptls := securityConfig{TrustedCAFile: "pca", CertFile: "pcert", KeyFile: "pkey", AllowedCN: "etcd", AllowedHostname: "whatever.example.com"} + ptls := securityConfig{TrustedCAFile: "pca", CertFile: "pcert", KeyFile: "pkey", AllowedCNs: []string{"etcd"}, AllowedHostnames: []string{"whatever.example.com"}} yc := struct { ClientSecurityCfgFile securityConfig `json:"client-transport-security"` PeerSecurityCfgFile securityConfig `json:"peer-transport-security"` @@ -292,8 +292,20 @@ func (s *securityConfig) equals(t *transport.TLSInfo) bool { s.ClientCertFile == t.ClientCertFile && s.ClientKeyFile == t.ClientKeyFile && s.KeyFile == t.KeyFile && - s.AllowedCN == t.AllowedCN && - s.AllowedHostname == t.AllowedHostname + compareSlices(s.AllowedCNs, t.AllowedCNs) && + compareSlices(s.AllowedHostnames, t.AllowedHostnames) +} + +func compareSlices(slice1, slice2 []string) bool { + if len(slice1) != len(slice2) { + return false + } + for i, v := range slice1 { + if v != slice2[i] { + return false + } + } + return true } func mustCreateCfgFile(t *testing.T, b []byte) *os.File { diff --git a/server/etcdmain/config.go b/server/etcdmain/config.go index 987edd25fcc..4a46192cf0f 100644 --- a/server/etcdmain/config.go +++ b/server/etcdmain/config.go @@ -213,6 +213,10 @@ func (cfg *config) configFromCmdLine() error { cfg.ec.CORS = flags.UniqueURLsMapFromFlag(cfg.cf.flagSet, "cors") cfg.ec.HostWhitelist = flags.UniqueStringsMapFromFlag(cfg.cf.flagSet, "host-whitelist") + cfg.ec.ClientTLSInfo.AllowedHostnames = flags.StringsFromFlag(cfg.cf.flagSet, "client-cert-allowed-hostname") + cfg.ec.PeerTLSInfo.AllowedCNs = flags.StringsFromFlag(cfg.cf.flagSet, "peer-cert-allowed-cn") + cfg.ec.PeerTLSInfo.AllowedHostnames = flags.StringsFromFlag(cfg.cf.flagSet, "peer-cert-allowed-hostname") + cfg.ec.CipherSuites = flags.StringsFromFlag(cfg.cf.flagSet, "cipher-suites") cfg.ec.MaxConcurrentStreams = flags.Uint32FromFlag(cfg.cf.flagSet, "max-concurrent-streams") diff --git a/server/etcdmain/help.go b/server/etcdmain/help.go index cc8e8ef7185..8633d7c2f6f 100644 --- a/server/etcdmain/help.go +++ b/server/etcdmain/help.go @@ -186,7 +186,7 @@ Security: --client-crl-file '' Path to the client certificate revocation list file. --client-cert-allowed-hostname '' - Allowed TLS hostname for client cert authentication. + Comma-separated list of SAN hostnames for client cert authentication. --trusted-ca-file '' Path to the client server TLS trusted CA cert file. --auto-tls 'false' @@ -204,9 +204,9 @@ Security: --peer-trusted-ca-file '' Path to the peer server TLS trusted CA file. --peer-cert-allowed-cn '' - Required CN for client certs connecting to the peer endpoint. + Comma-separated list of allowed CNs for inter-peer TLS authentication. --peer-cert-allowed-hostname '' - Allowed TLS hostname for inter peer authentication. + Comma-separated list of allowed SAN hostnames for inter-peer TLS authentication. --peer-auto-tls 'false' Peer TLS using self-generated certificates if --peer-key-file and --peer-cert-file are not provided. --self-signed-cert-validity '1' diff --git a/tests/e2e/etcd_config_test.go b/tests/e2e/etcd_config_test.go index ca500eeff02..a6da16b04ed 100644 --- a/tests/e2e/etcd_config_test.go +++ b/tests/e2e/etcd_config_test.go @@ -199,6 +199,96 @@ func TestEtcdPeerCNAuth(t *testing.T) { } } +// TestEtcdPeerMultiCNAuth checks that the inter peer auth based on CN of cert is working correctly +// when there are multiple allowed values for the CN. +func TestEtcdPeerMultiCNAuth(t *testing.T) { + e2e.SkipInShortMode(t) + + peers, tmpdirs := make([]string, 3), make([]string, 3) + for i := range peers { + peers[i] = fmt.Sprintf("e%d=https://127.0.0.1:%d", i, e2e.EtcdProcessBasePort+i) + tmpdirs[i] = t.TempDir() + } + ic := strings.Join(peers, ",") + procs := make([]*expect.ExpectProcess, len(peers)) + defer func() { + for i := range procs { + if procs[i] != nil { + procs[i].Stop() + procs[i].Close() + } + } + }() + + // all nodes have unique certs with different CNs + // node 0 and 1 have a cert with one of the correct CNs, node 2 doesn't + for i := range procs { + commonArgs := []string{ + e2e.BinPath.Etcd, + "--name", fmt.Sprintf("e%d", i), + "--listen-client-urls", "http://0.0.0.0:0", + "--data-dir", tmpdirs[i], + "--advertise-client-urls", "http://0.0.0.0:0", + "--listen-peer-urls", fmt.Sprintf("https://127.0.0.1:%d,https://127.0.0.1:%d", e2e.EtcdProcessBasePort+i, e2e.EtcdProcessBasePort+len(peers)+i), + "--initial-advertise-peer-urls", fmt.Sprintf("https://127.0.0.1:%d", e2e.EtcdProcessBasePort+i), + "--initial-cluster", ic, + } + + var args []string + switch i { + case 0: + args = []string{ + "--peer-cert-file", e2e.CertPath, // server.crt has CN "example.com". + "--peer-key-file", e2e.PrivateKeyPath, + "--peer-client-cert-file", e2e.CertPath, + "--peer-client-key-file", e2e.PrivateKeyPath, + "--peer-trusted-ca-file", e2e.CaPath, + "--peer-client-cert-auth", + "--peer-cert-allowed-cn", "example.com,example2.com", + } + case 1: + args = []string{ + "--peer-cert-file", e2e.CertPath2, // server2.crt has CN "example2.com". + "--peer-key-file", e2e.PrivateKeyPath2, + "--peer-client-cert-file", e2e.CertPath2, + "--peer-client-key-file", e2e.PrivateKeyPath2, + "--peer-trusted-ca-file", e2e.CaPath, + "--peer-client-cert-auth", + "--peer-cert-allowed-cn", "example.com,example2.com", + } + default: + args = []string{ + "--peer-cert-file", e2e.CertPath3, // server3.crt has CN "ca". + "--peer-key-file", e2e.PrivateKeyPath3, + "--peer-client-cert-file", e2e.CertPath3, + "--peer-client-key-file", e2e.PrivateKeyPath3, + "--peer-trusted-ca-file", e2e.CaPath, + "--peer-client-cert-auth", + "--peer-cert-allowed-cn", "example.com,example2.com", + } + } + + commonArgs = append(commonArgs, args...) + p, err := e2e.SpawnCmd(commonArgs, nil) + if err != nil { + t.Fatal(err) + } + procs[i] = p + } + + for i, p := range procs { + var expect []string + if i <= 1 { + expect = e2e.EtcdServerReadyLines + } else { + expect = []string{"remote error: tls: bad certificate"} + } + if err := e2e.WaitReadyExpectProc(context.TODO(), p, expect); err != nil { + t.Fatal(err) + } + } +} + // TestEtcdPeerNameAuth checks that the inter peer auth based on cert name validation is working correctly. func TestEtcdPeerNameAuth(t *testing.T) { e2e.SkipInShortMode(t)