OCPBUGS-81675: pkg/cvo/egress: trust user-ca-bundle in HyperShift mode

pkg/cvo/egress.go

@@ -72,27 +72,53 @@ func (optr *Operator) getProxyConfig() (*httpproxy.Config, error) {
 }
 
 func (optr *Operator) getTLSConfig() (*tls.Config, error) {
-	cm, err := optr.cmConfigManagedLister.Get("trusted-ca-bundle")
-	if apierrors.IsNotFound(err) {
-		return nil, nil
-	}
+	// Seed from the OS system cert pool so that any custom CAs we append
+	// augment rather than replace system trust.  SystemCertPool may fail on
+	// some platforms (e.g. no system root store); fall back to an empty pool
+	// so the function degrades gracefully instead of hard-failing.
+	certPool, err := x509.SystemCertPool()
 	if err != nil {
-		return nil, err
+		certPool = x509.NewCertPool()
 	}
+	found := false
 
-	certPool := x509.NewCertPool()
-
-	if cm.Data["ca-bundle.crt"] != "" {
+	// Always try the managed trusted CA bundle (proxy-merged bundle written by
+	// the platform into openshift-config-managed/trusted-ca-bundle).
+	cm, err := optr.cmConfigManagedLister.Get("trusted-ca-bundle")
+	if err != nil && !apierrors.IsNotFound(err) {
+		return nil, err
+	}
+	if err == nil && cm.Data["ca-bundle.crt"] != "" {
 		if ok := certPool.AppendCertsFromPEM([]byte(cm.Data["ca-bundle.crt"])); !ok {
-			return nil, fmt.Errorf("unable to add ca-bundle.crt certificates")
+			return nil, fmt.Errorf("unable to add trusted-ca-bundle certificates")
+		}
+		found = true
+	}
+
+	// In HyperShift the platform does not automatically merge additionalTrustBundle
+	// into trusted-ca-bundle, so also load openshift-config/user-ca-bundle which
+	// HCCO syncs from HostedCluster.spec.additionalTrustBundle.  This allows CVO
+	// to verify TLS to a custom spec.updateService whose certificate is signed by
+	// an internal CA that is present only in additionalTrustBundle.
+	if optr.hypershift {
+		ucm, err := optr.cmConfigLister.Get("user-ca-bundle")
+		if err != nil && !apierrors.IsNotFound(err) {
+			return nil, err
+		}
+		if err == nil && ucm.Data["ca-bundle.crt"] != "" {
+			if ok := certPool.AppendCertsFromPEM([]byte(ucm.Data["ca-bundle.crt"])); !ok {
+				return nil, fmt.Errorf("unable to add user-ca-bundle certificates")
+			}
+			found = true
 		}
-	} else {
-		return nil, nil
 	}
 
-	config := &tls.Config{
-		RootCAs: certPool,
+	if !found {
+		return nil, nil
 	}
 
-	return config, nil
+	// MinVersion is intentionally omitted; Go defaults to TLS 1.2 for clients,
+	// which is acceptable for Cincinnati/OSUS connectivity and avoids breaking
+	// compatibility with update services that do not yet support TLS 1.3.
+	return &tls.Config{RootCAs: certPool}, nil
 }

pkg/cvo/egress_test.go

@@ -0,0 +1,245 @@
+package cvo
+
+import (
+	"crypto/x509"
+	"encoding/pem"
+	"fmt"
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+// testCABundle1 is a self-signed CA certificate for testing only (CN=test-ca).
+const testCABundle1 = `-----BEGIN CERTIFICATE-----
+MIIDBTCCAe2gAwIBAgIUALqftleDqWJSShxMMRUv/WcAppQwDQYJKoZIhvcNAQEL
+BQAwEjEQMA4GA1UEAwwHdGVzdC1jYTAeFw0yNjA0MTAwNjU5MTdaFw0yNjA0MTEw
+NjU5MTdaMBIxEDAOBgNVBAMMB3Rlc3QtY2EwggEiMA0GCSqGSIb3DQEBAQUAA4IB
+DwAwggEKAoIBAQC1xT5ZKQc+ZiRXIDOcXhNvZ88KhHDF9iqU91s8Gvj0yiXqB9UO
+HDFysRjqiXEMXqazw7Wq6OE0hCoM3znXw0NGjgA6Tnuvhw8uGd0Q34R0iC20YMUA
+ln1Q93b5TRZjh97ViPhQ0hY2jZkaDM5F/gR9up1Z24okdXtzKd4fA0qjxDx6Yz/k
+Zze+uf9Xy287cIwAc2ILhq9qRl/Z9nXdGLUVbZwLahidA+KqorGOSRhUTK2srTaU
+Jzj0Z6XKcY6de/8quc9h8+UQ4ku5twPzwVfYUPkRAb0zm9A8jB1N4tyDXNiSIofi
+JKbCnaD+6uw/Rl/cu498vKDZsx3Ug5PKNDmXAgMBAAGjUzBRMB0GA1UdDgQWBBRh
+uVHEUPevz3HWe8xJplggeePnUDAfBgNVHSMEGDAWgBRhuVHEUPevz3HWe8xJplgg
+eePnUDAPBgNVHRMBAf8EBTADAQH/MA0GCSqGSIb3DQEBCwUAA4IBAQCZDWO/T97v
+IECb2vw+glagUup5nepZBpD/akiLLhpF8mZaS5NIfx3SsOVRJXodPHjahYvv0XzT
+hniX9tlPp55vaAuCsEQnqP01Bm9YHkszB2C0saqwbchKJRqKUwcF6lQvVgAwm6O6
+R9pKVa5Yqew8EhYla6YYP2Mvc3e8XhYfO/N5gqgb0BT9RjXIn5Ejb8YV/L1nKOtH
+kWWacgzG2cTvoHiQUIik7SzPMFAU0ZpuOAOUuGp1YQq0pnD7ZPvZ4Xe9XhYB5Whi
+CKo2N6BMFrMzrfyYf4oMm0TkTzA0wQdy5vcesbMa2cBKOv1m447AuNQryxc8zEZx
+umQEpCT2KLUo
+-----END CERTIFICATE-----
+`
+
+// testCABundle2 is a second self-signed CA certificate for testing only (CN=test-ca-2).
+const testCABundle2 = `-----BEGIN CERTIFICATE-----
+MIIDCTCCAfGgAwIBAgIUdsXWk3Ayiv/1BO/NrI0rpOr8q94wDQYJKoZIhvcNAQEL
+BQAwFDESMBAGA1UEAwwJdGVzdC1jYS0yMB4XDTI2MDQxMDA2NTkyNFoXDTI2MDQx
+MTA2NTkyNFowFDESMBAGA1UEAwwJdGVzdC1jYS0yMIIBIjANBgkqhkiG9w0BAQEF
+AAOCAQ8AMIIBCgKCAQEAukjI/q0yQHN+uYbjW1YDxUbQdwN9oGo+p3e5eFvxShHL
+EahJvZ673JrZhULRKMsoESJn9eMtkABXNMtugEGTexQag++Zly7xvEYDciyVxncE
+uh8yrKyzyfkgYaBUoj+XimGnWk5DyGHET/7Ex2xY/3iXRknPkOO4TTlMyp0KZkAL
+2xWQFgAnrrZ89HHAqgtpQ1gHvbmC80215vTBynBSbacgRjDK1NPnSmaA6Pkxa7mX
+znUyBGeRct1+RIq9n0Fyadz/glZV8S7cGJUGAWuL2v7i6aWY1WJ+vt+SOzSkx/bK
+wYOk9l6hvZKkqHgzAy1fFxGzdJ+eHNqllRKjsyWsdQIDAQABo1MwUTAdBgNVHQ4E
+FgQUobxtuZWbsm6Xa1r6+v3VRmR81HUwHwYDVR0jBBgwFoAUobxtuZWbsm6Xa1r6
++v3VRmR81HUwDwYDVR0TAQH/BAUwAwEB/zANBgkqhkiG9w0BAQsFAAOCAQEAQdal
+mib3V/3dMkHiajIIafT6IJp0peJh3LvP7BSpTfAE5v4cAApmmKZFko1TLq0qcItd
+A9jdA8VcbGsCcWWMDTYQZhSZurGLrIbZ26tqlVX/jDcun75jx4P3Bcgj9tpEA7WF
+DM9AIBF0SHmZKN2mOo7Dn+BcyUrlM1Sygyb1hAfpcu7D0bI+yJQj5aqNW8OG2zK0
+zzWzHyuLepB67+vrl6Zoi3QPzR7nfXiGU52ggVZpCbluRhBIn5okNMBf3DdCtV2U
+KjER4+2xPvFl3thrdb3V8ph7ujmtyPHx51F5+dDNmRRr/VOYSPxHfHgAPZ0bEvwN
+bfKkg9UU+lJbK6aEOw==
+-----END CERTIFICATE-----
+`
+
+// parseCertsFromPEM decodes all PEM certificates in data and returns them.
+func parseCertsFromPEM(t *testing.T, data string) []*x509.Certificate {
+	t.Helper()
+	var certs []*x509.Certificate
+	rest := []byte(data)
+	for {
+		var block *pem.Block
+		block, rest = pem.Decode(rest)
+		if block == nil {
+			break
+		}
+		cert, err := x509.ParseCertificate(block.Bytes)
+		if err != nil {
+			t.Fatalf("failed to parse certificate: %v", err)
+		}
+		certs = append(certs, cert)
+	}
+	return certs
+}
+
+// poolContainsCert returns true when cert's raw subject DER is present in pool.
+func poolContainsCert(pool *x509.CertPool, cert *x509.Certificate) bool {
+	for _, s := range pool.Subjects() { //nolint:staticcheck // SA1019: acceptable here, we build the pool ourselves
+		if string(s) == string(cert.RawSubject) {
+			return true
+		}
+	}
+	return false
+}
+
+func newCMForTest(name, caBundle string) *corev1.ConfigMap {
+	return &corev1.ConfigMap{
+		ObjectMeta: metav1.ObjectMeta{Name: name},
+		Data:       map[string]string{"ca-bundle.crt": caBundle},
+	}
+}
+
+func TestGetTLSConfig(t *testing.T) {
+	ca1Certs := parseCertsFromPEM(t, testCABundle1)
+	ca2Certs := parseCertsFromPEM(t, testCABundle2)
+	if len(ca1Certs) == 0 || len(ca2Certs) == 0 {
+		t.Fatal("failed to parse test CA certificates")
+	}
+	ca1Cert := ca1Certs[0]
+	ca2Cert := ca2Certs[0]
+
+	listerErr := fmt.Errorf("transient lister error")
+
+	tests := []struct {
+		name             string
+		trustedCABundle  *corev1.ConfigMap // openshift-config-managed/trusted-ca-bundle
+		userCABundle     *corev1.ConfigMap // openshift-config/user-ca-bundle
+		managedListerErr error             // non-NotFound error from the managed lister
+		configListerErr  error             // non-NotFound error from the config lister
+		hypershift       bool
+		wantNilConfig    bool
+		wantCA1Present   bool
+		wantCA2Present   bool
+		wantErr          bool
+	}{
+		{
+			name:          "non-hypershift: no bundles → nil config",
+			hypershift:    false,
+			wantNilConfig: true,
+		},
+		{
+			name:            "non-hypershift: trusted-ca-bundle only → CA1 trusted",
+			hypershift:      false,
+			trustedCABundle: newCMForTest("trusted-ca-bundle", testCABundle1),
+			wantCA1Present:  true,
+			wantCA2Present:  false,
+		},
+		{
+			name:          "non-hypershift: user-ca-bundle present but hypershift=false → ignored",
+			hypershift:    false,
+			userCABundle:  newCMForTest("user-ca-bundle", testCABundle2),
+			wantNilConfig: true,
+		},
+		{
+			name:            "non-hypershift: both bundles but hypershift=false → only trusted-ca-bundle used",
+			hypershift:      false,
+			trustedCABundle: newCMForTest("trusted-ca-bundle", testCABundle1),
+			userCABundle:    newCMForTest("user-ca-bundle", testCABundle2),
+			wantCA1Present:  true,
+			wantCA2Present:  false,
+		},
+		{
+			name:          "hypershift: no bundles → nil config",
+			hypershift:    true,
+			wantNilConfig: true,
+		},
+		{
+			name:            "hypershift: trusted-ca-bundle only → CA1 trusted",
+			hypershift:      true,
+			trustedCABundle: newCMForTest("trusted-ca-bundle", testCABundle1),
+			wantCA1Present:  true,
+			wantCA2Present:  false,
+		},
+		{
+			name:           "hypershift: user-ca-bundle only → CA2 trusted",
+			hypershift:     true,
+			userCABundle:   newCMForTest("user-ca-bundle", testCABundle2),
+			wantCA1Present: false,
+			wantCA2Present: true,
+		},
+		{
+			name:            "hypershift: both bundles → both CAs trusted",
+			hypershift:      true,
+			trustedCABundle: newCMForTest("trusted-ca-bundle", testCABundle1),
+			userCABundle:    newCMForTest("user-ca-bundle", testCABundle2),
+			wantCA1Present:  true,
+			wantCA2Present:  true,
+		},
+		// Error path: non-NotFound errors from listers must be propagated.
+		{
+			name:             "managed lister non-NotFound error is propagated",
+			hypershift:       false,
+			managedListerErr: listerErr,
+			wantErr:          true,
+		},
+		{
+			name:            "hypershift: user-ca-bundle lister non-NotFound error is propagated",
+			hypershift:      true,
+			configListerErr: listerErr,
+			wantErr:         true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			managedLister := &cmConfigLister{Err: tt.managedListerErr}
+			if tt.trustedCABundle != nil {
+				managedLister.Items = append(managedLister.Items, tt.trustedCABundle)
+			}
+
+			configLister := &cmConfigLister{Err: tt.configListerErr}
+			if tt.userCABundle != nil {
+				configLister.Items = append(configLister.Items, tt.userCABundle)
+			}
+
+			optr := &Operator{
+				hypershift:            tt.hypershift,
+				cmConfigManagedLister: managedLister,
+				cmConfigLister:        configLister,
+			}
+
+			tlsCfg, err := optr.getTLSConfig()
+			if tt.wantErr {
+				if err == nil {
+					t.Fatal("expected an error but got nil")
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+
+			if tt.wantNilConfig {
+				if tlsCfg != nil {
+					t.Fatal("expected nil tls.Config, got non-nil")
+				}
+				return
+			}
+
+			if tlsCfg == nil {
+				t.Fatal("expected non-nil tls.Config, got nil")
+			}
+			if tlsCfg.RootCAs == nil {
+				t.Fatal("expected non-nil RootCAs, got nil")
+			}
+
+			if got := poolContainsCert(tlsCfg.RootCAs, ca1Cert); got != tt.wantCA1Present {
+				if tt.wantCA1Present {
+					t.Error("expected CA1 (from trusted-ca-bundle) to be in RootCAs but it is absent")
+				} else {
+					t.Error("expected CA1 (from trusted-ca-bundle) NOT to be in RootCAs but it is present")
+				}
+			}
+
+			if got := poolContainsCert(tlsCfg.RootCAs, ca2Cert); got != tt.wantCA2Present {
+				if tt.wantCA2Present {
+					t.Error("expected CA2 (from user-ca-bundle) to be in RootCAs but it is absent")
+				} else {
+					t.Error("expected CA2 (from user-ca-bundle) NOT to be in RootCAs but it is present")
+				}
+			}
+		})
+	}
+}