Backport: fix(server/embed): enforce non-empty client TLS if scheme i…

…s https/unixs

Backports a657f06, 22f20a8, 497f1a4 and 3e86af6 from #18186.

Also backports required test util elements of 4c77726 from #17661.

Signed-off-by: James Blair <mail@jamesblair.net>

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
 			}

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)
+	}
+}

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'

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,58 @@ 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() {
+		// Don't check the error returned by Stop(), as we expect the process to exit with an error.
+		_ = proc.Stop()
+		_ = 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)

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
+}