From ec14b44bca826897d7b58c695b346d7283570ad2 Mon Sep 17 00:00:00 2001 From: Divyam pateriya Date: Fri, 10 Apr 2026 12:38:21 +0530 Subject: [PATCH 1/4] pkg/cvo/egress: trust user-ca-bundle in HyperShift mode In a HyperShift hosted cluster, HostedCluster.spec.additionalTrustBundle is synced by HCCO into openshift-config/user-ca-bundle, but that ConfigMap is never merged into openshift-config-managed/trusted-ca-bundle (which is the only source getTLSConfig() currently reads). As a result, when spec.updateService points to an update service whose TLS certificate is signed by an internal CA provided exclusively via additionalTrustBundle, CVO's Cincinnati client fails with: x509: certificate signed by unknown authority Fix: when running with --hypershift, also read openshift-config/user-ca-bundle via the already-wired cmConfigLister and merge it into the cert pool alongside trusted-ca-bundle. The new code path is strictly gated on optr.hypershift, so non-HyperShift clusters are completely unaffected. Add TestGetTLSConfig covering all combinations of bundle presence and the hypershift flag. Made-with: Cursor --- pkg/cvo/egress.go | 44 ++++++--- pkg/cvo/egress_test.go | 220 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 250 insertions(+), 14 deletions(-) create mode 100644 pkg/cvo/egress_test.go diff --git a/pkg/cvo/egress.go b/pkg/cvo/egress.go index cead0b4965..ff0ce3b521 100644 --- a/pkg/cvo/egress.go +++ b/pkg/cvo/egress.go @@ -72,27 +72,43 @@ func (optr *Operator) getProxyConfig() (*httpproxy.Config, error) { } func (optr *Operator) getTLSConfig() (*tls.Config, error) { + certPool := x509.NewCertPool() + found := false + + // 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 apierrors.IsNotFound(err) { - return nil, nil - } - if err != nil { + if err != nil && !apierrors.IsNotFound(err) { return nil, err } - - certPool := x509.NewCertPool() - - if cm.Data["ca-bundle.crt"] != "" { + 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 + return &tls.Config{RootCAs: certPool}, nil } diff --git a/pkg/cvo/egress_test.go b/pkg/cvo/egress_test.go new file mode 100644 index 0000000000..5615adac5f --- /dev/null +++ b/pkg/cvo/egress_test.go @@ -0,0 +1,220 @@ +package cvo + +import ( + "crypto/x509" + "encoding/pem" + "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] + + tests := []struct { + name string + trustedCABundle *corev1.ConfigMap // openshift-config-managed/trusted-ca-bundle + userCABundle *corev1.ConfigMap // openshift-config/user-ca-bundle + hypershift bool + wantNilConfig bool + wantCA1Present bool + wantCA2Present 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, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + managedLister := &cmConfigLister{} + if tt.trustedCABundle != nil { + managedLister.Items = append(managedLister.Items, tt.trustedCABundle) + } + + configLister := &cmConfigLister{} + 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 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") + } + } + }) + } +} From d335cc86afe4809a83cdd2b271e430dfe5b423e4 Mon Sep 17 00:00:00 2001 From: Divyam pateriya Date: Fri, 10 Apr 2026 13:09:53 +0530 Subject: [PATCH 2/4] pkg/cvo/egress: address CodeRabbit review suggestions - Add comment explaining why tls.Config.MinVersion is intentionally omitted (Go defaults to TLS 1.2 for clients, acceptable for Cincinnati/OSUS and avoids breaking services not yet on TLS 1.3) - Add error-path test cases for non-NotFound lister errors from both cmConfigManagedLister and cmConfigLister to verify error propagation Made-with: Cursor --- pkg/cvo/egress.go | 3 +++ pkg/cvo/egress_test.go | 43 +++++++++++++++++++++++++++++++++--------- 2 files changed, 37 insertions(+), 9 deletions(-) diff --git a/pkg/cvo/egress.go b/pkg/cvo/egress.go index ff0ce3b521..512edfb819 100644 --- a/pkg/cvo/egress.go +++ b/pkg/cvo/egress.go @@ -110,5 +110,8 @@ func (optr *Operator) getTLSConfig() (*tls.Config, error) { return nil, 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 } diff --git a/pkg/cvo/egress_test.go b/pkg/cvo/egress_test.go index 5615adac5f..6f21a0aaf3 100644 --- a/pkg/cvo/egress_test.go +++ b/pkg/cvo/egress_test.go @@ -3,6 +3,7 @@ package cvo import ( "crypto/x509" "encoding/pem" + "fmt" "testing" corev1 "k8s.io/api/core/v1" @@ -99,14 +100,19 @@ func TestGetTLSConfig(t *testing.T) { 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 - hypershift bool - wantNilConfig bool - wantCA1Present bool - wantCA2Present bool + 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", @@ -161,16 +167,29 @@ func TestGetTLSConfig(t *testing.T) { 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{} + managedLister := &cmConfigLister{Err: tt.managedListerErr} if tt.trustedCABundle != nil { managedLister.Items = append(managedLister.Items, tt.trustedCABundle) } - configLister := &cmConfigLister{} + configLister := &cmConfigLister{Err: tt.configListerErr} if tt.userCABundle != nil { configLister.Items = append(configLister.Items, tt.userCABundle) } @@ -182,6 +201,12 @@ func TestGetTLSConfig(t *testing.T) { } 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) } From 7d0b4a56e9d3404d5fa1bed6ded7967c0809c44b Mon Sep 17 00:00:00 2001 From: Divyam pateriya Date: Fri, 10 Apr 2026 13:35:49 +0530 Subject: [PATCH 3/4] pkg/cvo/egress_test: fix gofmt formatting Made-with: Cursor --- pkg/cvo/egress_test.go | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/pkg/cvo/egress_test.go b/pkg/cvo/egress_test.go index 6f21a0aaf3..59293efdad 100644 --- a/pkg/cvo/egress_test.go +++ b/pkg/cvo/egress_test.go @@ -103,16 +103,16 @@ func TestGetTLSConfig(t *testing.T) { 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 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", @@ -175,10 +175,10 @@ func TestGetTLSConfig(t *testing.T) { wantErr: true, }, { - name: "hypershift: user-ca-bundle lister non-NotFound error is propagated", - hypershift: true, - configListerErr: listerErr, - wantErr: true, + name: "hypershift: user-ca-bundle lister non-NotFound error is propagated", + hypershift: true, + configListerErr: listerErr, + wantErr: true, }, } From 56c19731776ffb2cf8c6c0df154d3d85b560da77 Mon Sep 17 00:00:00 2001 From: Divyam pateriya Date: Fri, 10 Apr 2026 13:39:10 +0530 Subject: [PATCH 4/4] pkg/cvo/egress: seed cert pool from system roots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Starting from x509.NewCertPool() causes RootCAs to replace system trust entirely when custom bundles are present. Seed from x509.SystemCertPool() instead so that custom CAs are additive — system CA trust is preserved and public-CA endpoints (e.g. a Cincinnati service with a Let's Encrypt cert) continue to work alongside internal-CA endpoints. If SystemCertPool is unavailable on the current platform, fall back to an empty pool to preserve the previous behaviour rather than hard-failing. Made-with: Cursor --- pkg/cvo/egress.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/cvo/egress.go b/pkg/cvo/egress.go index 512edfb819..d384b4fbf7 100644 --- a/pkg/cvo/egress.go +++ b/pkg/cvo/egress.go @@ -72,7 +72,14 @@ func (optr *Operator) getProxyConfig() (*httpproxy.Config, error) { } func (optr *Operator) getTLSConfig() (*tls.Config, error) { - certPool := x509.NewCertPool() + // 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 { + certPool = x509.NewCertPool() + } found := false // Always try the managed trusted CA bundle (proxy-merged bundle written by