From 154dba1767654eaf23d5d555063bbe9890515760 Mon Sep 17 00:00:00 2001 From: James Blair Date: Sat, 6 Jul 2024 23:39:58 +1200 Subject: [PATCH] Backport: fix(server/embed): enforce non-empty client TLS if scheme is https/unixs Backports a657f069a1d3be9f04d037789b6b9394d4292306, 22f20a827bc3af96e1deda21792f736bd1c38feb, 497f1a45a3f3730fa94a8af4efa3d4a0e7fa7403 and 3e86af6843568931c281600b962f5ae05ebfc42f from https://github.com/etcd-io/etcd/pull/18186. Also backports required test util elements of 4c77726914401640f03b1371cf011bdf554c20f5 from https://github.com/etcd-io/etcd/pull/17661. Signed-off-by: James Blair --- server/embed/etcd.go | 28 ++++++--- server/embed/etcd_test.go | 38 ++++++++++++ server/etcdmain/help.go | 2 +- tests/e2e/etcd_config_test.go | 57 +++++++++++++++++ tests/e2e/utils.go | 114 ++++++++++++++++++++++++++++++++++ 5 files changed, 230 insertions(+), 9 deletions(-) create mode 100644 server/embed/etcd_test.go diff --git a/server/embed/etcd.go b/server/embed/etcd.go index 78d9bef3400a..147114e7922b 100644 --- a/server/embed/etcd.go +++ b/server/embed/etcd.go @@ -17,6 +17,7 @@ package embed import ( "context" "crypto/tls" + "errors" "fmt" "io/ioutil" defaultLog "log" @@ -831,6 +832,24 @@ func (e *Etcd) pickGrpcGatewayServeContext(splitHttp bool) *serveCtx { panic("Expect at least one context able to serve grpc") } +var ErrMissingClientTLSInfoForMetricsURL = errors.New("client TLS key/cert (--cert-file, --key-file) must be provided for metrics secure url") + +func (e *Etcd) createMetricsListener(murl url.URL) (net.Listener, error) { + tlsInfo := &e.cfg.ClientTLSInfo + switch murl.Scheme { + case "http": + tlsInfo = nil + case "https", "unixs": + if e.cfg.ClientTLSInfo.Empty() { + return nil, ErrMissingClientTLSInfoForMetricsURL + } + } + return transport.NewListenerWithOpts(murl.Host, murl.Scheme, + transport.WithTLSInfo(tlsInfo), + transport.WithSocketOpts(&e.cfg.SocketOpts), + ) +} + func (e *Etcd) serveMetrics() (err error) { if e.cfg.Metrics == "extensive" { grpc_prometheus.EnableHandlingTimeHistogram() @@ -842,14 +861,7 @@ func (e *Etcd) serveMetrics() (err error) { etcdhttp.HandleHealth(e.cfg.logger, metricsMux, e.Server) for _, murl := range e.cfg.ListenMetricsUrls { - tlsInfo := &e.cfg.ClientTLSInfo - if murl.Scheme == "http" { - tlsInfo = nil - } - ml, err := transport.NewListenerWithOpts(murl.Host, murl.Scheme, - transport.WithTLSInfo(tlsInfo), - transport.WithSocketOpts(&e.cfg.SocketOpts), - ) + ml, err := e.createMetricsListener(murl) if err != nil { return err } diff --git a/server/embed/etcd_test.go b/server/embed/etcd_test.go new file mode 100644 index 000000000000..e22b6d3544a2 --- /dev/null +++ b/server/embed/etcd_test.go @@ -0,0 +1,38 @@ +// Copyright 2024 The etcd Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package embed + +import ( + "net/url" + "testing" + + "go.etcd.io/etcd/client/pkg/v3/transport" +) + +func TestEmptyClientTLSInfo_createMetricsListener(t *testing.T) { + e := &Etcd{ + cfg: Config{ + ClientTLSInfo: transport.TLSInfo{}, + }, + } + + murl := url.URL{ + Scheme: "https", + Host: "localhost:8080", + } + if _, err := e.createMetricsListener(murl); err != ErrMissingClientTLSInfoForMetricsURL { + t.Fatalf("expected error %v, got %v", ErrMissingClientTLSInfoForMetricsURL, err) + } +} diff --git a/server/etcdmain/help.go b/server/etcdmain/help.go index be7b0263e0fe..1fd278ba9a10 100644 --- a/server/etcdmain/help.go +++ b/server/etcdmain/help.go @@ -211,7 +211,7 @@ Profiling and Monitoring: --metrics 'basic' Set level of detail for exported metrics, specify 'extensive' to include server side grpc histogram metrics. --listen-metrics-urls '' - List of URLs to listen on for the metrics and health endpoints. + List of URLs to listen on for the /metrics and /health endpoints. For https, the client URL TLS info is used. Logging: --logger 'zap' diff --git a/tests/e2e/etcd_config_test.go b/tests/e2e/etcd_config_test.go index 5b8c9a003de3..b659bfd2d979 100644 --- a/tests/e2e/etcd_config_test.go +++ b/tests/e2e/etcd_config_test.go @@ -17,12 +17,16 @@ package e2e import ( "fmt" "io/ioutil" + "net" "os" "strings" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.etcd.io/etcd/pkg/v3/expect" + "go.etcd.io/etcd/server/v3/embed" "go.etcd.io/etcd/tests/v3/framework/e2e" ) @@ -122,6 +126,59 @@ func TestEtcdUnixPeers(t *testing.T) { } } +// TestEtcdListenMetricsURLsWithMissingClientTLSInfo checks that the HTTPs listen metrics URL +// but without the client TLS info will fail its verification. +func TestEtcdListenMetricsURLsWithMissingClientTLSInfo(t *testing.T) { + e2e.SkipInShortMode(t) + + tempDir := t.TempDir() + defer os.RemoveAll(tempDir) + + caFile, certFiles, keyFiles, err := generateCertsForIPs(tempDir, []net.IP{net.ParseIP("127.0.0.1")}) + require.NoError(t, err) + + // non HTTP but metrics URL is HTTPS, invalid when the client TLS info is not provided + clientURL := fmt.Sprintf("http://localhost:%d", e2e.EtcdProcessBasePort) + peerURL := fmt.Sprintf("https://localhost:%d", e2e.EtcdProcessBasePort+1) + listenMetricsURL := fmt.Sprintf("https://localhost:%d", e2e.EtcdProcessBasePort+2) + + commonArgs := []string{ + e2e.BinPath, + "--name", "e0", + "--data-dir", tempDir, + + "--listen-client-urls", clientURL, + "--advertise-client-urls", clientURL, + + "--initial-advertise-peer-urls", peerURL, + "--listen-peer-urls", peerURL, + + "--initial-cluster", "e0=" + peerURL, + + "--listen-metrics-urls", listenMetricsURL, + + "--peer-cert-file", certFiles[0], + "--peer-key-file", keyFiles[0], + "--peer-trusted-ca-file", caFile, + "--peer-client-cert-auth", + } + + proc, err := e2e.SpawnCmd(commonArgs, nil) + if err != nil { + t.Fatal(err) + } + defer func() { + if err := proc.Stop(); err != nil { + t.Error(err) + } + _ = proc.Close() + }() + + if err := e2e.WaitReadyExpectProc(proc, []string{embed.ErrMissingClientTLSInfoForMetricsURL.Error()}); err != nil { + t.Fatal(err) + } +} + // TestEtcdPeerCNAuth checks that the inter peer auth based on CN of cert is working correctly. func TestEtcdPeerCNAuth(t *testing.T) { e2e.SkipInShortMode(t) diff --git a/tests/e2e/utils.go b/tests/e2e/utils.go index d5cf69b54659..c528cc462acd 100644 --- a/tests/e2e/utils.go +++ b/tests/e2e/utils.go @@ -15,8 +15,17 @@ package e2e import ( + "bytes" "context" + "crypto/rand" + "crypto/rsa" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" "fmt" + "math/big" + "net" + "os" "strings" "testing" "time" @@ -141,3 +150,108 @@ func patchArgs(args []string, flag, newValue string) []string { args = append(args, fmt.Sprintf("--%s=%s", flag, newValue)) return args } + +func generateCertsForIPs(tempDir string, ips []net.IP) (caFile string, certFiles []string, keyFiles []string, err error) { + ca := &x509.Certificate{ + SerialNumber: big.NewInt(1001), + Subject: pkix.Name{ + Organization: []string{"etcd"}, + OrganizationalUnit: []string{"etcd Security"}, + Locality: []string{"San Francisco"}, + Province: []string{"California"}, + Country: []string{"USA"}, + }, + NotBefore: time.Now(), + NotAfter: time.Now().AddDate(0, 0, 1), + IsCA: true, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign, + BasicConstraintsValid: true, + } + + caKey, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + return "", nil, nil, err + } + caBytes, err := x509.CreateCertificate(rand.Reader, ca, ca, &caKey.PublicKey, caKey) + if err != nil { + return "", nil, nil, err + } + + caFile, _, err = saveCertToFile(tempDir, caBytes, nil) + if err != nil { + return "", nil, nil, err + } + + for i, ip := range ips { + cert := &x509.Certificate{ + SerialNumber: big.NewInt(1001 + int64(i)), + Subject: pkix.Name{ + Organization: []string{"etcd"}, + OrganizationalUnit: []string{"etcd Security"}, + Locality: []string{"San Francisco"}, + Province: []string{"California"}, + Country: []string{"USA"}, + }, + IPAddresses: []net.IP{ip}, + NotBefore: time.Now(), + NotAfter: time.Now().AddDate(0, 0, 1), + SubjectKeyId: []byte{1, 2, 3, 4, 5}, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, + KeyUsage: x509.KeyUsageDigitalSignature, + } + certKey, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + return "", nil, nil, err + } + certBytes, err := x509.CreateCertificate(rand.Reader, cert, ca, &certKey.PublicKey, caKey) + if err != nil { + return "", nil, nil, err + } + certFile, keyFile, err := saveCertToFile(tempDir, certBytes, certKey) + if err != nil { + return "", nil, nil, err + } + certFiles = append(certFiles, certFile) + keyFiles = append(keyFiles, keyFile) + } + + return caFile, certFiles, keyFiles, nil +} + +func saveCertToFile(tempDir string, certBytes []byte, key *rsa.PrivateKey) (certFile string, keyFile string, err error) { + certPEM := new(bytes.Buffer) + pem.Encode(certPEM, &pem.Block{ + Type: "CERTIFICATE", + Bytes: certBytes, + }) + cf, err := os.CreateTemp(tempDir, "*.crt") + if err != nil { + return "", "", err + } + defer cf.Close() + if _, err := cf.Write(certPEM.Bytes()); err != nil { + return "", "", err + } + + if key != nil { + certKeyPEM := new(bytes.Buffer) + pem.Encode(certKeyPEM, &pem.Block{ + Type: "RSA PRIVATE KEY", + Bytes: x509.MarshalPKCS1PrivateKey(key), + }) + + kf, err := os.CreateTemp(tempDir, "*.key.insecure") + if err != nil { + return "", "", err + } + defer kf.Close() + if _, err := kf.Write(certKeyPEM.Bytes()); err != nil { + return "", "", err + } + + return cf.Name(), kf.Name(), nil + } + + return cf.Name(), "", nil +}