From b47b643e05bab5c2d7fc0466d11ec5842a9d269c Mon Sep 17 00:00:00 2001 From: Ramkumar Chinchani <45800463+rchincha@users.noreply.github.com> Date: Sat, 18 Apr 2026 22:57:24 -0700 Subject: [PATCH] fix(security): remove InsecureSkipVerify from metrics client (TLS-1) (#3982) * fix(security): remove InsecureSkipVerify from metrics client (TLS-1) Replace the unconditional InsecureSkipVerify: true TLS config in newHTTPMetricsClient with the system cert pool (+ TLS 1.2 minimum). Add an optional CACert field to MetricsConfig and to the exporter ServerConfig so operators running zot with a self-signed or private CA can point the exporter at the correct CA file instead of disabling certificate verification entirely. Signed-off-by: Ramkumar Chinchani * feat(metrics): add HTTPS configuration for metrics exporter Signed-off-by: Ramkumar Chinchani * fix(security): enhance CA certificate handling in metrics client and add tests Signed-off-by: Ramkumar Chinchani * fix(security): improve CA certificate error handling in metrics client and update tests Signed-off-by: Ramkumar Chinchani * fix(tests): correct package name in minimal_client_test.go and simplify error declaration Signed-off-by: Ramkumar Chinchani * fix(tests): update package name in minimal_client_test.go for consistency Signed-off-by: Ramkumar Chinchani --------- Signed-off-by: Ramkumar Chinchani --- .../exporter/config-https-private-ca.json | 18 ++ pkg/exporter/api/config.go | 4 + pkg/exporter/api/exporter.go | 2 +- pkg/extensions/monitoring/minimal_client.go | 63 +++++- .../monitoring/minimal_client_test.go | 196 ++++++++++++++++++ 5 files changed, 276 insertions(+), 7 deletions(-) create mode 100644 examples/metrics/exporter/config-https-private-ca.json create mode 100644 pkg/extensions/monitoring/minimal_client_test.go diff --git a/examples/metrics/exporter/config-https-private-ca.json b/examples/metrics/exporter/config-https-private-ca.json new file mode 100644 index 00000000..3339fbda --- /dev/null +++ b/examples/metrics/exporter/config-https-private-ca.json @@ -0,0 +1,18 @@ +{ + "Server": { + "protocol": "https", + "host": "127.0.0.1", + "port": "8443", + "cacert": "test/data/ca.crt" + }, + "Exporter": { + "port": "8081", + "log": { + "level": "info", + "output": "/tmp/zot_exporter.log" + }, + "metrics": { + "path": "/mymetrics" + } + } +} diff --git a/pkg/exporter/api/config.go b/pkg/exporter/api/config.go index 36dc69b5..8744ffaa 100644 --- a/pkg/exporter/api/config.go +++ b/pkg/exporter/api/config.go @@ -16,6 +16,10 @@ type ServerConfig struct { Protocol string Host string Port string + // CACert is an optional path to a PEM-encoded CA certificate used to verify + // the zot server's TLS certificate. Required when the server uses a + // self-signed or private CA. Leave empty to use the system cert pool. + CACert string } type ExporterConfig struct { diff --git a/pkg/exporter/api/exporter.go b/pkg/exporter/api/exporter.go index 8d144b51..a3a8d367 100644 --- a/pkg/exporter/api/exporter.go +++ b/pkg/exporter/api/exporter.go @@ -160,7 +160,7 @@ func GetCollector(c *Controller) *Collector { // parameters to connect to the zot server serverAddr := fmt.Sprintf("%s://%s:%s", c.Config.Server.Protocol, c.Config.Server.Host, c.Config.Server.Port) - cfg := &monitoring.MetricsConfig{Address: serverAddr} + cfg := &monitoring.MetricsConfig{Address: serverAddr, CACert: c.Config.Server.CACert} return &Collector{ Client: monitoring.NewMetricsClient(cfg, c.Log), diff --git a/pkg/extensions/monitoring/minimal_client.go b/pkg/extensions/monitoring/minimal_client.go index 6edf39c1..44961a5f 100644 --- a/pkg/extensions/monitoring/minimal_client.go +++ b/pkg/extensions/monitoring/minimal_client.go @@ -5,9 +5,12 @@ package monitoring import ( "context" "crypto/tls" + "crypto/x509" "encoding/json" + "errors" "fmt" "net/http" + "os" "time" "zotregistry.dev/zot/v2/pkg/log" @@ -17,12 +20,19 @@ const ( httpTimeout = 1 * time.Minute ) +var errInvalidCACert = errors.New("invalid CA certificate") + // MetricsConfig is used to configure the creation of a Node Exporter http client // that will connect to a particular zot instance. type MetricsConfig struct { // Address of the zot http server Address string + // CACert is an optional path to a PEM-encoded CA certificate file used to + // verify the zot server's TLS certificate. When empty the system cert pool + // is used. Set this when the zot server uses a self-signed or private CA. + CACert string + // Transport to use for the http client. Transport *http.Transport @@ -36,14 +46,42 @@ type MetricsClient struct { log log.Logger } -func newHTTPMetricsClient() *http.Client { - defaultTransport := http.DefaultTransport.(*http.Transport).Clone() //nolint: forcetypeassert - defaultTransport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} //nolint: gosec +func newHTTPMetricsClient(caCertFile string) (*http.Client, error) { + var rootCAs *x509.CertPool + + if caCertFile != "" { + caCertPool, err := x509.SystemCertPool() + if err != nil || caCertPool == nil { + caCertPool = x509.NewCertPool() + } + + caCert, err := os.ReadFile(caCertFile) + if err != nil { + return nil, fmt.Errorf("metrics client: failed to read CA cert %s: %w", caCertFile, err) + } + + // Ensure caCertPool is not nil before appending (defensive against SystemCertPool returning (nil, nil)) + if caCertPool == nil { + caCertPool = x509.NewCertPool() + } + + if !caCertPool.AppendCertsFromPEM(caCert) { + return nil, fmt.Errorf("metrics client: no valid PEM certificate found in %s: %w", caCertFile, errInvalidCACert) + } + + rootCAs = caCertPool + } + + transport := http.DefaultTransport.(*http.Transport).Clone() //nolint: forcetypeassert + transport.TLSClientConfig = &tls.Config{ + RootCAs: rootCAs, + MinVersion: tls.VersionTLS12, + } return &http.Client{ Timeout: httpTimeout, - Transport: defaultTransport, - } + Transport: transport, + }, nil } // NewMetricsClient creates a MetricsClient that can be used to retrieve in memory metrics. @@ -51,7 +89,20 @@ func newHTTPMetricsClient() *http.Client { // in order to prevent concurrent memory leaks. func NewMetricsClient(config *MetricsConfig, logger log.Logger) *MetricsClient { if config.HTTPClient == nil { - config.HTTPClient = newHTTPMetricsClient() + client, err := newHTTPMetricsClient(config.CACert) + if err != nil { + logger.Error().Err(err).Msg("failed to create metrics HTTP client; falling back to TLS12/system-root transport") + + fallbackClient, fallbackErr := newHTTPMetricsClient("") + if fallbackErr != nil { + logger.Error().Err(fallbackErr).Msg("failed to create fallback metrics HTTP client; using default transport") + config.HTTPClient = &http.Client{Timeout: httpTimeout} + } else { + config.HTTPClient = fallbackClient + } + } else { + config.HTTPClient = client + } } return &MetricsClient{config: *config, headers: make(http.Header), log: logger} diff --git a/pkg/extensions/monitoring/minimal_client_test.go b/pkg/extensions/monitoring/minimal_client_test.go new file mode 100644 index 00000000..10fb014e --- /dev/null +++ b/pkg/extensions/monitoring/minimal_client_test.go @@ -0,0 +1,196 @@ +//go:build !metrics + +//nolint:testpackage // Tests intentionally cover unexported client construction helpers. +package monitoring + +import ( + "context" + "crypto/rand" + "crypto/rsa" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "math/big" + "net" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "testing" + "time" + + "zotregistry.dev/zot/v2/pkg/log" +) + +func TestNewHTTPMetricsClientDefaultRootsAndTLSMinVersion(t *testing.T) { + t.Parallel() + + client, err := newHTTPMetricsClient("") + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + + transport, ok := client.Transport.(*http.Transport) + if !ok { + t.Fatalf("expected *http.Transport, got %T", client.Transport) + } + + if transport.TLSClientConfig == nil { + t.Fatal("expected TLSClientConfig to be set") + } + + if transport.TLSClientConfig.MinVersion != tls.VersionTLS12 { + t.Fatalf("expected MinVersion TLS1.2, got: %d", transport.TLSClientConfig.MinVersion) + } + + if transport.TLSClientConfig.RootCAs != nil { + t.Fatal("expected RootCAs to be nil when no custom CA is provided") + } +} + +func TestNewHTTPMetricsClientInvalidCACertPath(t *testing.T) { + t.Parallel() + + _, err := newHTTPMetricsClient(filepath.Join(t.TempDir(), "missing-ca.pem")) + if err == nil { + t.Fatal("expected error for missing CA cert file") + } +} + +func TestNewHTTPMetricsClientInvalidCACertPEM(t *testing.T) { + t.Parallel() + + tmpDir := t.TempDir() + caPath := filepath.Join(tmpDir, "ca.pem") + + if err := os.WriteFile(caPath, []byte("not-a-pem-cert"), 0o600); err != nil { + t.Fatalf("failed writing temp CA file: %v", err) + } + + _, err := newHTTPMetricsClient(caPath) + if err == nil { + t.Fatal("expected error for invalid PEM CA cert file") + } +} + +func TestNewHTTPMetricsClientCustomCAValidatesServer(t *testing.T) { + t.Parallel() + + caPEM, serverCert, serverKey, err := generateServerCertificateChain() + if err != nil { + t.Fatalf("failed generating cert chain: %v", err) + } + + tmpDir := t.TempDir() + caPath := filepath.Join(tmpDir, "ca.pem") + + if err := os.WriteFile(caPath, caPEM, 0o600); err != nil { + t.Fatalf("failed writing CA PEM: %v", err) + } + + tlsCert, err := tls.X509KeyPair(serverCert, serverKey) + if err != nil { + t.Fatalf("failed loading server key pair: %v", err) + } + + srv := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("ok")) + })) + srv.TLS = &tls.Config{Certificates: []tls.Certificate{tlsCert}, MinVersion: tls.VersionTLS12} + srv.StartTLS() + defer srv.Close() + + client, err := newHTTPMetricsClient(caPath) + if err != nil { + t.Fatalf("expected no error creating client with CA cert, got: %v", err) + } + + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, srv.URL, nil) + if err != nil { + t.Fatalf("failed to create request: %v", err) + } + + resp, err := client.Do(req) + if err != nil { + t.Fatalf("expected TLS handshake to succeed with custom CA, got: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + t.Fatalf("expected status %d, got %d", http.StatusOK, resp.StatusCode) + } +} + +func TestNewMetricsClientFallbackKeepsTLSHardening(t *testing.T) { + t.Parallel() + + cfg := &MetricsConfig{Address: "https://127.0.0.1:8443", CACert: filepath.Join(t.TempDir(), "missing-ca.pem")} + mc := NewMetricsClient(cfg, log.NewLogger("debug", "")) + + transport, ok := mc.config.HTTPClient.Transport.(*http.Transport) + if !ok { + t.Fatalf("expected fallback transport to be *http.Transport, got %T", mc.config.HTTPClient.Transport) + } + + if transport.TLSClientConfig == nil { + t.Fatal("expected TLSClientConfig to be present on fallback client") + } + + if transport.TLSClientConfig.MinVersion != tls.VersionTLS12 { + t.Fatalf("expected fallback MinVersion TLS1.2, got: %d", transport.TLSClientConfig.MinVersion) + } +} + +func generateServerCertificateChain() ([]byte, []byte, []byte, error) { + now := time.Now() + + caTemplate := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "zot-test-ca"}, + NotBefore: now.Add(-1 * time.Hour), + NotAfter: now.Add(24 * time.Hour), + KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageDigitalSignature, + IsCA: true, + BasicConstraintsValid: true, + } + + caKey, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + return nil, nil, nil, err + } + + caDER, err := x509.CreateCertificate(rand.Reader, caTemplate, caTemplate, &caKey.PublicKey, caKey) + if err != nil { + return nil, nil, nil, err + } + + caPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: caDER}) + + serverTemplate := &x509.Certificate{ + SerialNumber: big.NewInt(2), + Subject: pkix.Name{CommonName: "localhost"}, + NotBefore: now.Add(-1 * time.Hour), + NotAfter: now.Add(24 * time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + DNSNames: []string{"localhost"}, + IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, + } + + serverKey, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + return nil, nil, nil, err + } + + serverDER, err := x509.CreateCertificate(rand.Reader, serverTemplate, caTemplate, &serverKey.PublicKey, caKey) + if err != nil { + return nil, nil, nil, err + } + + serverCertPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: serverDER}) + serverKeyPEM := pem.EncodeToMemory(&pem.Block{Type: "RSA PRIVATE KEY", Bytes: x509.MarshalPKCS1PrivateKey(serverKey)}) + + return caPEM, serverCertPEM, serverKeyPEM, nil +}