From bc35cba8dda263052ded56a0e2674e56dfae7c94 Mon Sep 17 00:00:00 2001 From: wangke19 Date: Wed, 26 Nov 2025 14:03:50 +0800 Subject: [PATCH 1/4] Reapply "OCPBUGS-64799: TLS 1.3 / Modern profile tests (#29611)" This reverts commit 10dab7a2b1ef03f4e8d0e363bf611007e53e6463. --- test/extended/apiserver/tls.go | 226 +++++++++++++++++++++++++++++---- 1 file changed, 200 insertions(+), 26 deletions(-) diff --git a/test/extended/apiserver/tls.go b/test/extended/apiserver/tls.go index bc2a7e6b7d03..77df58692b29 100644 --- a/test/extended/apiserver/tls.go +++ b/test/extended/apiserver/tls.go @@ -1,64 +1,238 @@ package apiserver import ( + "context" "crypto/tls" + "fmt" + "io" + "math/rand" + "os/exec" + "strings" + "time" g "github.com/onsi/ginkgo/v2" + o "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + e2e "k8s.io/kubernetes/test/e2e/framework" + configv1 "github.com/openshift/api/config/v1" "github.com/openshift/library-go/pkg/crypto" + "github.com/openshift/origin/test/extended/networking" exutil "github.com/openshift/origin/test/extended/util" ) +const ( + namespace = "apiserver-tls-test" +) + +// This test only checks whether components are serving the proper TLS version based +// on the expected version set in the TLS profile config. It is a part of the +// openshift/conformance/parallel test suite, and it is expected that there are jobs +// which run that entire conformance suite against clusters running any TLS profiles +// that there is a desire to test. var _ = g.Describe("[sig-api-machinery][Feature:APIServer]", func() { defer g.GinkgoRecover() - oc := exutil.NewCLI("apiserver") + var oc = exutil.NewCLI(namespace) + var ctx = context.Background() - g.It("TestTLSDefaults", func() { - g.Skip("skipping because it was broken in master") + g.BeforeEach(func() { + isMicroShift, err := exutil.IsMicroShiftCluster(oc.AdminKubeClient()) + o.Expect(err).NotTo(o.HaveOccurred()) + + isHyperShift, err := exutil.IsHypershift(ctx, oc.AdminConfigClient()) + o.Expect(err).NotTo(o.HaveOccurred()) + + if isMicroShift || isHyperShift { + g.Skip("TLS configuration for the apiserver resource is not applicable to MicroShift or HyperShift clusters - skipping") + } + + hasIPv4, _, err := networking.GetIPAddressFamily(oc) + o.Expect(err).NotTo(o.HaveOccurred()) + if !hasIPv4 { + g.Skip("TLS configuration is only tested on IPv4 clusters, skipping") + } + }) + + g.It("TestTLSMinimumVersions", func() { + + g.By("Getting the APIServer configuration") + config, err := oc.AdminConfigClient().ConfigV1().APIServers().Get(ctx, "cluster", metav1.GetOptions{}) + o.Expect(err).NotTo(o.HaveOccurred()) + + g.By("Determining expected TLS behavior based on the cluster's TLS profile") + var tlsShouldWork, tlsShouldNotWork *tls.Config + switch { + case config.Spec.TLSSecurityProfile == nil, + config.Spec.TLSSecurityProfile.Type == configv1.TLSProfileIntermediateType: + tlsShouldWork = &tls.Config{MinVersion: tls.VersionTLS12, MaxVersion: tls.VersionTLS13, InsecureSkipVerify: true} + tlsShouldNotWork = &tls.Config{MinVersion: tls.VersionTLS11, MaxVersion: tls.VersionTLS11, InsecureSkipVerify: true} + g.By("Using intermediate TLS profile: connections with TLS ≥1.2 should work, <1.2 should fail") + case config.Spec.TLSSecurityProfile.Type == configv1.TLSProfileModernType: + tlsShouldWork = &tls.Config{MinVersion: tls.VersionTLS13, MaxVersion: tls.VersionTLS13, InsecureSkipVerify: true} + tlsShouldNotWork = &tls.Config{MinVersion: tls.VersionTLS12, MaxVersion: tls.VersionTLS12, InsecureSkipVerify: true} + g.By("Using modern TLS profile: only TLS 1.3 connections should succeed") + default: + g.Skip("Only intermediate or modern profiles are tested") + } + + targets := []struct { + name, namespace, port string + }{ + {"apiserver", "openshift-kube-apiserver", "443"}, + {"oauth-openshift", "openshift-authentication", "443"}, + {"kube-controller-manager", "openshift-kube-controller-manager", "443"}, + {"scheduler", "openshift-kube-scheduler", "443"}, + {"api", "openshift-apiserver", "443"}, + {"api", "openshift-oauth-apiserver", "443"}, + {"machine-config-controller", "openshift-machine-config-operator", "9001"}, + } + + g.By("Verifying TLS behavior for core control plane components") + for _, target := range targets { + g.By(fmt.Sprintf("Checking %s/%s on port %s", target.namespace, target.name, target.port)) + err = forwardPortAndExecute(target.name, target.namespace, target.port, + func(port int) error { return checkTLSConnection(port, tlsShouldWork, tlsShouldNotWork) }) + o.Expect(err).NotTo(o.HaveOccurred()) + } + + g.By("Checking etcd's TLS behavior") + err = forwardPortAndExecute("etcd", "openshift-etcd", "2379", func(port int) error { + conn, err := tls.Dial("tcp", fmt.Sprintf("localhost:%d", port), tlsShouldWork) + if err != nil { + if !strings.Contains(err.Error(), "remote error: tls: bad certificate") { + return fmt.Errorf("should work: %w", err) + } + } else { + err = conn.Close() + if err != nil { + return fmt.Errorf("failed to close connection: %w", err) + } + } + conn, err = tls.Dial("tcp", fmt.Sprintf("localhost:%d", port), tlsShouldNotWork) + if err == nil { + return fmt.Errorf("should not work: connection unexpectedly succeeded, closing conn status: %v", conn.Close()) + } + return nil + }) + o.Expect(err).NotTo(o.HaveOccurred()) + }) + g.It("TestTLSDefaults", func() { t := g.GinkgoT() - // Verify we fail with TLS versions less than the default, and work with TLS versions >= the default + + _, err := e2e.LoadClientset(true) + o.Expect(err).NotTo(o.HaveOccurred()) + + g.By("Getting the APIServer config") + config, err := oc.AdminConfigClient().ConfigV1().APIServers().Get(ctx, "cluster", metav1.GetOptions{}) + o.Expect(err).NotTo(o.HaveOccurred()) + + if config.Spec.TLSSecurityProfile != nil && + config.Spec.TLSSecurityProfile.Type != configv1.TLSProfileIntermediateType { + g.Skip("Cluster TLS profile is not default (intermediate), skipping cipher defaults check") + } + + g.By("Verifying TLS version behavior") for _, tlsVersionName := range crypto.ValidTLSVersions() { tlsVersion := crypto.TLSVersionOrDie(tlsVersionName) expectSuccess := tlsVersion >= crypto.DefaultTLSVersion() - config := &tls.Config{MinVersion: tlsVersion, MaxVersion: tlsVersion, InsecureSkipVerify: true} + cfg := &tls.Config{MinVersion: tlsVersion, MaxVersion: tlsVersion, InsecureSkipVerify: true} + host := strings.TrimPrefix(oc.AdminConfig().Host, "https://") - { - conn, err := tls.Dial("tcp4", oc.AdminConfig().Host, config) - if err == nil { - conn.Close() - } - if success := err == nil; success != expectSuccess { - t.Errorf("Expected success %v, got %v with TLS version %s dialing master", expectSuccess, success, tlsVersionName) + conn, err := tls.Dial("tcp", host, cfg) + if err == nil { + err := conn.Close() + if err != nil { + t.Errorf("Failed to close connection: %v", err) } } + if success := err == nil; success != expectSuccess { + t.Errorf("Expected success %v, got %v with TLS version %s dialing master", expectSuccess, success, tlsVersionName) + } } - // Verify the only ciphers we work with are in the default set. - // Not all default ciphers will succeed because they depend on the serving cert type. + g.By("Verifying cipher suites") defaultCiphers := map[uint16]bool{} - for _, defaultCipher := range crypto.DefaultCiphers() { - defaultCiphers[defaultCipher] = true + for _, c := range crypto.DefaultCiphers() { + defaultCiphers[c] = true } + for _, cipherName := range crypto.ValidCipherSuites() { cipher, err := crypto.CipherSuite(cipherName) if err != nil { t.Fatal(err) } expectFailure := !defaultCiphers[cipher] - config := &tls.Config{CipherSuites: []uint16{cipher}, InsecureSkipVerify: true} - - { - conn, err := tls.Dial("tcp4", oc.AdminConfig().Host, config) - if err == nil { - conn.Close() - if expectFailure { - t.Errorf("Expected failure on cipher %s, got success dialing master", cipherName) - } + cfg := &tls.Config{CipherSuites: []uint16{cipher}, InsecureSkipVerify: true} + + conn, err := tls.Dial("tcp", oc.AdminConfig().Host, cfg) + if err == nil { + if expectFailure { + t.Errorf("Expected failure on cipher %s, got success dialing master. Closing conn: %v", cipherName, conn.Close()) } } } - }) }) + +func forwardPortAndExecute(serviceName, namespace, remotePort string, toExecute func(localPort int) error) error { + var err error + for i := 0; i < 3; i++ { + if err = func() error { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + localPort := rand.Intn(65534-1025) + 1025 + args := []string{"port-forward", fmt.Sprintf("svc/%s", serviceName), fmt.Sprintf("%d:%s", localPort, remotePort), "-n", namespace} + + cmd := exec.CommandContext(ctx, "oc", args...) + stdout, stderr, err := e2e.StartCmdAndStreamOutput(cmd) + if err != nil { + return err + } + defer stdout.Close() + defer stderr.Close() + defer e2e.TryKill(cmd) + + e2e.Logf("oc port-forward output: %s", readPartialFrom(stdout, 1024)) + return toExecute(localPort) + }(); err == nil { + return nil + } else { + e2e.Logf("failed to start oc port-forward command or test: %v", err) + time.Sleep(2 * time.Second) + } + } + return err +} + +func readPartialFrom(r io.Reader, maxBytes int) string { + buf := make([]byte, maxBytes) + n, err := r.Read(buf) + if err != nil && err != io.EOF { + return fmt.Sprintf("error reading: %v", err) + } + return string(buf[:n]) +} + +func checkTLSConnection(port int, tlsShouldWork, tlsShouldNotWork *tls.Config) error { + conn, err := tls.Dial("tcp", fmt.Sprintf("localhost:%d", port), tlsShouldWork) + if err != nil { + return fmt.Errorf("should work: %w", err) + } + err = conn.Close() + if err != nil { + return fmt.Errorf("failed to close connection: %w", err) + } + + conn, err = tls.Dial("tcp", fmt.Sprintf("localhost:%d", port), tlsShouldNotWork) + if err == nil { + return fmt.Errorf("should not work: connection unexpectedly succeeded, closing conn status: %v", conn.Close()) + } + if !strings.Contains(err.Error(), "protocol version") && + !strings.Contains(err.Error(), "no supported versions satisfy") && + !strings.Contains(err.Error(), "handshake failure") { + return fmt.Errorf("should not work: got error, but not a TLS version mismatch: %w", err) + } + return nil +} From ff085032dd92904a8b9304f098aeaec2ee9e06bb Mon Sep 17 00:00:00 2001 From: wangke19 Date: Wed, 26 Nov 2025 14:07:14 +0800 Subject: [PATCH 2/4] Fix TestTLSDefaults to use port-forwarding like TestTLSMinimumVersions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit refactors TestTLSDefaults to use the same port-forwarding approach as TestTLSMinimumVersions, which fixes DNS resolution failures when running in CI as a pod. Problem: When the test runs as a pod in the cluster, it attempted to connect directly to the external API server hostname from the kubeconfig (e.g., api.cluster5.ocpci.eng.rdu2.redhat.com). However, the pod's internal DNS cannot resolve this external hostname, resulting in: dial tcp: lookup api.cluster5.ocpci.eng.rdu2.redhat.com on 172.30.0.10:53: no such host Solution: Use forwardPortAndExecute() to create a port-forward tunnel to the apiserver service in openshift-kube-apiserver namespace, then test against localhost:. This approach: - Works both in-cluster (CI) and externally (with kubeconfig) - Eliminates DNS resolution issues entirely - Is consistent with TestTLSMinimumVersions pattern - Includes built-in retry logic (3 attempts) - Simplifies the code by removing URL parsing and env var detection Changes: - Removed net/url and os imports (no longer needed) - Wrapped TLS version and cipher tests in forwardPortAndExecute callback - Changed error reporting from t.Errorf() to returning errors for retry support - Tests now connect to localhost via port-forward tunnel 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- test/extended/apiserver/tls.go | 86 +++++++++++++++++++++------------- 1 file changed, 53 insertions(+), 33 deletions(-) diff --git a/test/extended/apiserver/tls.go b/test/extended/apiserver/tls.go index 77df58692b29..c283b147f176 100644 --- a/test/extended/apiserver/tls.go +++ b/test/extended/apiserver/tls.go @@ -133,46 +133,66 @@ var _ = g.Describe("[sig-api-machinery][Feature:APIServer]", func() { g.Skip("Cluster TLS profile is not default (intermediate), skipping cipher defaults check") } - g.By("Verifying TLS version behavior") - for _, tlsVersionName := range crypto.ValidTLSVersions() { - tlsVersion := crypto.TLSVersionOrDie(tlsVersionName) - expectSuccess := tlsVersion >= crypto.DefaultTLSVersion() - cfg := &tls.Config{MinVersion: tlsVersion, MaxVersion: tlsVersion, InsecureSkipVerify: true} - host := strings.TrimPrefix(oc.AdminConfig().Host, "https://") - - conn, err := tls.Dial("tcp", host, cfg) - if err == nil { - err := conn.Close() - if err != nil { - t.Errorf("Failed to close connection: %v", err) + g.By("Verifying TLS version and cipher behavior via port-forward to apiserver") + err = forwardPortAndExecute("apiserver", "openshift-kube-apiserver", "443", func(port int) error { + host := fmt.Sprintf("localhost:%d", port) + t.Logf("Testing TLS versions and ciphers against %s", host) + + // Test TLS versions + for _, tlsVersionName := range crypto.ValidTLSVersions() { + tlsVersion := crypto.TLSVersionOrDie(tlsVersionName) + expectSuccess := tlsVersion >= crypto.DefaultTLSVersion() + cfg := &tls.Config{MinVersion: tlsVersion, MaxVersion: tlsVersion, InsecureSkipVerify: true} + + t.Logf("Testing TLS version %s (0x%04x), expectSuccess=%v", tlsVersionName, tlsVersion, expectSuccess) + conn, dialErr := tls.Dial("tcp", host, cfg) + if dialErr == nil { + t.Logf("TLS %s succeeded, negotiated version: 0x%04x", tlsVersionName, conn.ConnectionState().Version) + closeErr := conn.Close() + if closeErr != nil { + return fmt.Errorf("failed to close connection: %v", closeErr) + } + } else { + t.Logf("TLS %s failed with error: %v", tlsVersionName, dialErr) + } + if success := dialErr == nil; success != expectSuccess { + return fmt.Errorf("expected success %v, got %v with TLS version %s", expectSuccess, success, tlsVersionName) } } - if success := err == nil; success != expectSuccess { - t.Errorf("Expected success %v, got %v with TLS version %s dialing master", expectSuccess, success, tlsVersionName) - } - } - g.By("Verifying cipher suites") - defaultCiphers := map[uint16]bool{} - for _, c := range crypto.DefaultCiphers() { - defaultCiphers[c] = true - } - - for _, cipherName := range crypto.ValidCipherSuites() { - cipher, err := crypto.CipherSuite(cipherName) - if err != nil { - t.Fatal(err) + // Test cipher suites + defaultCiphers := map[uint16]bool{} + for _, c := range crypto.DefaultCiphers() { + defaultCiphers[c] = true } - expectFailure := !defaultCiphers[cipher] - cfg := &tls.Config{CipherSuites: []uint16{cipher}, InsecureSkipVerify: true} - conn, err := tls.Dial("tcp", oc.AdminConfig().Host, cfg) - if err == nil { - if expectFailure { - t.Errorf("Expected failure on cipher %s, got success dialing master. Closing conn: %v", cipherName, conn.Close()) + for _, cipherName := range crypto.ValidCipherSuites() { + cipher, err := crypto.CipherSuite(cipherName) + if err != nil { + return err + } + expectFailure := !defaultCiphers[cipher] + // Constrain to TLS 1.2 to prevent Go 1.23+ from silently ignoring + // deprecated ciphers and falling back to TLS 1.3 with secure defaults + cfg := &tls.Config{ + CipherSuites: []uint16{cipher}, + MinVersion: tls.VersionTLS12, + MaxVersion: tls.VersionTLS12, + InsecureSkipVerify: true, + } + + conn, dialErr := tls.Dial("tcp", host, cfg) + if dialErr == nil { + closeErr := conn.Close() + if expectFailure { + return fmt.Errorf("expected failure on cipher %s, got success. Closing conn: %v", cipherName, closeErr) + } } } - } + + return nil + }) + o.Expect(err).NotTo(o.HaveOccurred()) }) }) From f9fec479390db3cfaf345bbfd061ce7e3b03631a Mon Sep 17 00:00:00 2001 From: wangke19 Date: Mon, 1 Dec 2025 11:48:26 +0800 Subject: [PATCH 3/4] Enable TLS tests on IPv6 clusters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the IPv4-only restriction from TLS tests to support IPv6 clusters. The tests use port-forwarding to localhost, which works for both IPv4 and IPv6 environments since localhost resolves appropriately in both cases. Changes: - Removed IPv4-only check in BeforeEach that skipped tests on IPv6 clusters - Removed unused networking import This allows the TLS configuration tests to run on: - IPv4-only clusters - IPv6-only clusters - Dual-stack (IPv4+IPv6) clusters 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- test/extended/apiserver/tls.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/test/extended/apiserver/tls.go b/test/extended/apiserver/tls.go index c283b147f176..8e036295517d 100644 --- a/test/extended/apiserver/tls.go +++ b/test/extended/apiserver/tls.go @@ -17,7 +17,6 @@ import ( configv1 "github.com/openshift/api/config/v1" "github.com/openshift/library-go/pkg/crypto" - "github.com/openshift/origin/test/extended/networking" exutil "github.com/openshift/origin/test/extended/util" ) @@ -46,12 +45,6 @@ var _ = g.Describe("[sig-api-machinery][Feature:APIServer]", func() { if isMicroShift || isHyperShift { g.Skip("TLS configuration for the apiserver resource is not applicable to MicroShift or HyperShift clusters - skipping") } - - hasIPv4, _, err := networking.GetIPAddressFamily(oc) - o.Expect(err).NotTo(o.HaveOccurred()) - if !hasIPv4 { - g.Skip("TLS configuration is only tested on IPv4 clusters, skipping") - } }) g.It("TestTLSMinimumVersions", func() { From 44026036eddb473108f45ec72756249dcfde528f Mon Sep 17 00:00:00 2001 From: wangke19 Date: Wed, 3 Dec 2025 14:04:48 +0800 Subject: [PATCH 4/4] Improve TLS 1.2 constraint comment for cipher suite testing Update the comment explaining why cipher suite tests are constrained to TLS 1.2 to be more technically accurate. The previous comment suggested this was about "Go 1.23+ behavior", but the real issue is fundamental to how TLS 1.3 works: - The intermediate profile allows both TLS 1.2 and TLS 1.3 - Clients negotiate TLS 1.3 when MaxVersion is unspecified and server supports it - TLS 1.3 spec predefines cipher suites and doesn't support configuration - Therefore, specifying any cipher suite has no effect with TLS 1.3 - Forcing TLS 1.2 allows actual testing of cipher suite restrictions This makes the reasoning clearer for future maintainers. --- test/extended/apiserver/tls.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/extended/apiserver/tls.go b/test/extended/apiserver/tls.go index 8e036295517d..58ac08e1524d 100644 --- a/test/extended/apiserver/tls.go +++ b/test/extended/apiserver/tls.go @@ -165,8 +165,11 @@ var _ = g.Describe("[sig-api-machinery][Feature:APIServer]", func() { return err } expectFailure := !defaultCiphers[cipher] - // Constrain to TLS 1.2 to prevent Go 1.23+ from silently ignoring - // deprecated ciphers and falling back to TLS 1.3 with secure defaults + // Constrain to TLS 1.2 because the intermediate profile allows both TLS 1.2 and TLS 1.3. + // If MaxVersion is unspecified, the client negotiates TLS 1.3 when the server supports it. + // TLS 1.3 does not support configuring cipher suites (predetermined by the spec), so + // specifying any cipher suite (RC4 or otherwise) has no effect with TLS 1.3. + // By forcing TLS 1.2, we can actually test the cipher suite restrictions. cfg := &tls.Config{ CipherSuites: []uint16{cipher}, MinVersion: tls.VersionTLS12,