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 <rchincha.dev@gmail.com> * feat(metrics): add HTTPS configuration for metrics exporter Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * fix(security): enhance CA certificate handling in metrics client and add tests Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * fix(security): improve CA certificate error handling in metrics client and update tests Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * fix(tests): correct package name in minimal_client_test.go and simplify error declaration Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> * fix(tests): update package name in minimal_client_test.go for consistency Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com> --------- Signed-off-by: Ramkumar Chinchani <rchincha.dev@gmail.com>
2026-06-17 21:17:58 +08:00 · 2026-04-18 22:57:24 -07:00
parent bfc59ad120
commit b47b643e05
5 changed files with 276 additions and 7 deletions
@@ -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}
@@ -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
+}