From a657f069a1d3be9f04d037789b6b9394d4292306 Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Mon, 17 Jun 2024 18:21:38 +0800 Subject: [PATCH 1/4] fix(server/embed): enforce non-empty client TLS if scheme is https/unixs Signed-off-by: Gyuho Lee --- server/embed/etcd.go | 27 +++++++++++++++++++-------- server/embed/etcd_test.go | 24 ++++++++++++++++++++++++ server/etcdmain/help.go | 2 +- 3 files changed, 44 insertions(+), 9 deletions(-) create mode 100644 server/embed/etcd_test.go diff --git a/server/embed/etcd.go b/server/embed/etcd.go index ce3b3398643..6d9fc76fcae 100644 --- a/server/embed/etcd.go +++ b/server/embed/etcd.go @@ -822,6 +822,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 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() @@ -833,14 +851,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 00000000000..f90d3de6697 --- /dev/null +++ b/server/embed/etcd_test.go @@ -0,0 +1,24 @@ +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 8633d7c2f6f..d9bb14525e5 100644 --- a/server/etcdmain/help.go +++ b/server/etcdmain/help.go @@ -238,7 +238,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' From 22f20a827bc3af96e1deda21792f736bd1c38feb Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Mon, 17 Jun 2024 21:09:24 +0800 Subject: [PATCH 2/4] test(e2e): add a case where client tls is missing for https metrics url Signed-off-by: Gyuho Lee --- server/embed/etcd.go | 4 +-- server/embed/etcd_test.go | 4 +-- tests/e2e/etcd_config_test.go | 55 +++++++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 4 deletions(-) diff --git a/server/embed/etcd.go b/server/embed/etcd.go index 6d9fc76fcae..3f9bfcec885 100644 --- a/server/embed/etcd.go +++ b/server/embed/etcd.go @@ -822,7 +822,7 @@ 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 url") +var ErrMissingClientTLSInfoForMetricsURL = errors.New("client TLS key/cert (--cert-file, --key-file) must be provided for metrics url") func (e *Etcd) createMetricsListener(murl url.URL) (net.Listener, error) { tlsInfo := &e.cfg.ClientTLSInfo @@ -831,7 +831,7 @@ func (e *Etcd) createMetricsListener(murl url.URL) (net.Listener, error) { tlsInfo = nil case "https", "unixs": if e.cfg.ClientTLSInfo.Empty() { - return nil, errMissingClientTLSInfoForMetricsURL + return nil, ErrMissingClientTLSInfoForMetricsURL } } return transport.NewListenerWithOpts(murl.Host, murl.Scheme, diff --git a/server/embed/etcd_test.go b/server/embed/etcd_test.go index f90d3de6697..793563999ac 100644 --- a/server/embed/etcd_test.go +++ b/server/embed/etcd_test.go @@ -18,7 +18,7 @@ func TestEmptyClientTLSInfo_createMetricsListener(t *testing.T) { Scheme: "https", Host: "localhost:8080", } - if _, err := e.createMetricsListener(murl); err != errMissingClientTLSInfoForMetricsURL { - t.Fatalf("expected error %v, got %v", errMissingClientTLSInfoForMetricsURL, err) + if _, err := e.createMetricsListener(murl); err != ErrMissingClientTLSInfoForMetricsURL { + t.Fatalf("expected error %v, got %v", ErrMissingClientTLSInfoForMetricsURL, err) } } diff --git a/tests/e2e/etcd_config_test.go b/tests/e2e/etcd_config_test.go index a6da16b04ed..c455bc208e8 100644 --- a/tests/e2e/etcd_config_test.go +++ b/tests/e2e/etcd_config_test.go @@ -28,6 +28,7 @@ import ( "golang.org/x/sync/errgroup" "go.etcd.io/etcd/pkg/v3/expect" + "go.etcd.io/etcd/server/v3/embed" "go.etcd.io/etcd/tests/v3/framework/config" "go.etcd.io/etcd/tests/v3/framework/e2e" ) @@ -120,6 +121,60 @@ 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.Etcd, + "--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.Wait() // ensure the port has been released + _ = proc.Close() + }() + + if err := e2e.WaitReadyExpectProc(context.TODO(), 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) From 497f1a45a3f3730fa94a8af4efa3d4a0e7fa7403 Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Tue, 18 Jun 2024 07:28:43 +0800 Subject: [PATCH 3/4] license Signed-off-by: Gyuho Lee --- server/embed/etcd_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/server/embed/etcd_test.go b/server/embed/etcd_test.go index 793563999ac..e22b6d3544a 100644 --- a/server/embed/etcd_test.go +++ b/server/embed/etcd_test.go @@ -1,3 +1,17 @@ +// 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 ( From 3e86af6843568931c281600b962f5ae05ebfc42f Mon Sep 17 00:00:00 2001 From: Gyuho Lee Date: Tue, 18 Jun 2024 18:18:54 +0800 Subject: [PATCH 4/4] remove unnecessary wait call Signed-off-by: Gyuho Lee --- tests/e2e/etcd_config_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/e2e/etcd_config_test.go b/tests/e2e/etcd_config_test.go index c455bc208e8..87a5f7666d9 100644 --- a/tests/e2e/etcd_config_test.go +++ b/tests/e2e/etcd_config_test.go @@ -166,7 +166,6 @@ func TestEtcdListenMetricsURLsWithMissingClientTLSInfo(t *testing.T) { if err := proc.Stop(); err != nil { t.Error(err) } - proc.Wait() // ensure the port has been released _ = proc.Close() }()