Skip to content

CNTRLPLANE-3429: Respect configured cipher order for inject-tls #1386

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vincentdephily wants to merge 2 commits into
base: main
Choose a base branch
from
Open

CNTRLPLANE-3429: Respect configured cipher order for inject-tls #1386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
11c8dba
fix: Keep TLSSecurityProfile cipher order when injecting TLS
vincentdephily May 7, 2026
4efff84
qa: Log the actual NodeKind during tls injection
vincentdephily May 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions lib/resourcebuilder/core.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"errors"
"fmt"
"sort"

"sigs.k8s.io/kustomize/kyaml/yaml"

Expand Down Expand Up @@ -83,15 +82,16 @@ func (b *builder) modifyConfigMap(ctx context.Context, cm *corev1.ConfigMap) err
}

// Check if this is a supported config kind
rnodeKind := rnode.GetKind()
switch {
case rnode.GetKind() == "GenericOperatorConfig" && rnode.GetApiVersion() == operatorv1alpha1.GroupVersion.String():
case rnode.GetKind() == "GenericControllerConfig" && rnode.GetApiVersion() == configv1.GroupVersion.String():
case rnodeKind == "GenericOperatorConfig" && rnode.GetApiVersion() == operatorv1alpha1.GroupVersion.String():
case rnodeKind == "GenericControllerConfig" && rnode.GetApiVersion() == configv1.GroupVersion.String():
default:
klog.V(4).Infof("ConfigMap's %q entry is not a supported config type. Only GenericOperatorConfig (%v) and GenericControllerConfig (%v) are. Skipping this entry", key, operatorv1alpha1.GroupVersion.String(), configv1.GroupVersion.String())
continue
}

klog.V(2).Infof("ConfigMap %s/%s processing GenericOperatorConfig in key %s", cm.Namespace, cm.Name, key)
klog.V(2).Infof("ConfigMap %s/%s processing %s in key %s", cm.Namespace, cm.Name, rnodeKind, key)

// Inject TLS settings into the GenericOperatorConfig while preserving structure
if err := updateRNodeWithTLSSettings(rnode, tlsConf); err != nil {
Expand All @@ -106,7 +106,7 @@ func (b *builder) modifyConfigMap(ctx context.Context, cm *corev1.ConfigMap) err

// Update the ConfigMap data entry with the modified YAML
cm.Data[key] = modifiedYAML
klog.V(2).Infof("ConfigMap %s/%s updated GenericOperatorConfig with TLS profile in key %s", cm.Namespace, cm.Name, key)
klog.V(2).Infof("ConfigMap %s/%s updated TLS profile of %s in key %s", cm.Namespace, cm.Name, rnodeKind, key)
}
return nil
}
Expand Down Expand Up @@ -145,8 +145,6 @@ func (b *builder) observeTLSConfiguration(ctx context.Context, cm *corev1.Config
if cipherSuites, ciphersFound, err := unstructured.NestedStringSlice(observedConfig, "servingInfo", "cipherSuites"); err != nil {
return nil, err
} else if ciphersFound {
// Sort cipher suites for consistent ordering
sort.Strings(cipherSuites)
Copy link
Copy Markdown
Contributor

@DavidHurta DavidHurta May 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very interesting.

Have you reached out to the relevant centralized-tls-profile folks about whether the API description should be updated?

Currently, the relevant APIServer API is described as [1]:

// TLSProfileSpec is the desired behavior of a TLSSecurityProfile.
type TLSProfileSpec struct {
	// ciphers is used to specify the cipher algorithms that are negotiated
	// during the TLS handshake. Operators may remove entries that their operands
	// do not support. For example, to use only ECDHE-RSA-AES128-GCM-SHA256 (yaml):
	//
	//   ciphers:
	//     - ECDHE-RSA-AES128-GCM-SHA256
	//
	// TLS 1.3 cipher suites (e.g. TLS_AES_128_GCM_SHA256) are not configurable
	// and are always enabled when TLS 1.3 is negotiated.
	// +listType=atomic
	Ciphers []string `json:"ciphers"`

No mention of preferred ordering. Maybe there should be?

Ideally, the majority of components use library functions, which hopefully respect the ordering. Hopefully. However, at a minimum, some components will not respect this.

Also, the tls-scanner compliance check does not seem to check the ordering [2] as well. Maybe it should?

Copy link
Copy Markdown
Member

@ingvagabund ingvagabund May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note: this will be less a problem from TLS v1.3 given the list of ciphers will be ignored/rejected.

config.cipherSuites = optional[[]string]{value: cipherSuites, found: true}
}

Expand Down
47 changes: 23 additions & 24 deletions lib/resourcebuilder/core_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"encoding/json"
"fmt"
"reflect"
"sort"
"testing"
"text/template"

Expand Down Expand Up @@ -77,10 +76,10 @@ func makeGenericControllerConfigYAML(cipherSuites []string, minTLSVersion string
return makeGenericConfigYAML(configv1.GroupVersion.String(), "GenericControllerConfig", cipherSuites, minTLSVersion)
}

func getDefaultCipherSuitesSorted() []string {
cipherSuites := crypto.CipherSuitesToNamesOrDie(crypto.DefaultCiphers())
Copy link
Copy Markdown
Member

@ingvagabund ingvagabund May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just drop the sort.Strings statement?

Copy link
Copy Markdown
Author

@vincentdephily vincentdephily May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because crypto.DefaultCiphers() and configv1.TLSProfiles[crypto.DefaultTLSProfileType] return the same ciphers but in a different order. The new TLS1.3 ciphers have a higher precedence in os library-go than in golang crypto.

Ideally I would have taken the cipher suite by calling apiserver.ObserveTLSSecurityProfile(), but the context args made that akward. So instead I extracted the core logic for the 'default case', and added a comment. If the apiserver code drifts, we'll have to update this test.

Copy link
Copy Markdown
Member

@ingvagabund ingvagabund May 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new TLS1.3 ciphers have a higher precedence in os library-go than in golang crypto.

That does not play well with the cipher ordering either. Both config/v1/types_tlssecurityprofile.go and DefaultCiphers() need to respect the same ordering then. Maybe noone has though of it before?

sort.Strings(cipherSuites)
return cipherSuites
// Returns the same default cipher suite as apiserver.ObserveTLSSecurityProfile()
func getDefaultCipherSuites() []string {
profile := configv1.TLSProfiles[crypto.DefaultTLSProfileType]
return crypto.OpenSSLToIANACipherSuites(profile.Ciphers)
}

const (
Expand All @@ -99,35 +98,37 @@ const (
)

var (
// testCipherSuites is a common cipher suite list used across multiple test cases
// Common cipher suite used across multiple test cases
testCipherSuites = []string{
"TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256",
"TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256",
}

testCipherSuites2 = []string{
"TLS_RSA_WITH_AES_128_CBC_SHA256",
"TLS_RSA_WITH_AES_128_GCM_SHA256",
"TLS_RSA_WITH_AES_256_GCM_SHA384",
}

// Same as previous, but using OpenSSL ids
testOpenSSLCipherSuites = []string{
"ECDHE-ECDSA-AES128-GCM-SHA256",
"ECDHE-RSA-AES128-GCM-SHA256",
}

// Same as previous, but in a different order
testOpenSSLCipherSuitesReversed = []string{
"ECDHE-RSA-AES128-GCM-SHA256",
"ECDHE-ECDSA-AES128-GCM-SHA256",
}

// Alternate cipher suite
Comment thread
vincentdephily marked this conversation as resolved.
testCipherSuites2 = []string{
"TLS_RSA_WITH_AES_128_CBC_SHA256",
"TLS_RSA_WITH_AES_128_GCM_SHA256",
"TLS_RSA_WITH_AES_256_GCM_SHA384",
}
// Same as previous, but using OpenSSL ids
testOpenSSLCipherSuites2 = []string{
"AES128-SHA256",
"AES128-GCM-SHA256",
"AES256-GCM-SHA384",
"AES128-SHA256",
}

defaultCipherSuites = getDefaultCipherSuitesSorted()
// Default cipher suite returned bu library-go
Copy link
Copy Markdown
Member

@ingvagabund ingvagabund May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: bu -> by

defaultCipherSuites = getDefaultCipherSuites()
)

// makeConfigMap is a helper to create a ConfigMap with optional annotation and data
Expand Down Expand Up @@ -273,8 +274,6 @@ func validateGenericConfigTLSInjected(modified *corev1.ConfigMap, fieldName stri
return fmt.Errorf("servingInfo.cipherSuites not found")
}

sort.Strings(expectedCiphers)
sort.Strings(cipherSuites)
if diff := cmp.Diff(expectedCiphers, cipherSuites); diff != "" {
return fmt.Errorf("list of ciphers mismatch (-want +got):\n%s", diff)
}
Expand Down Expand Up @@ -563,16 +562,16 @@ servingInfo:
},
},
{
name: "ConfigMap with TLS already injected - same ciphers in different order with no modification",
// Cipher order matters for TLS preference, so different order should trigger update
name: "ConfigMap with TLS already injected - same ciphers in different order - TLS injected with correct order",
configMap: makeConfigMap(true, map[string]string{
genericOperatorConfigCMKey: makeGenericOperatorConfigYAML(testCipherSuites, tlsVersion13),
genericOperatorConfigCMKey: makeGenericOperatorConfigYAML(testOpenSSLCipherSuitesReversed, tlsVersion13),
}),
// Reverse the cipher order to test that same ciphers in different order don't trigger update
apiServer: makeAPIServerConfig(withCustomTLSProfile(testOpenSSLCipherSuitesReversed, configv1.VersionTLS13)),
apiServer: makeAPIServerConfig(withCustomTLSProfile(testOpenSSLCipherSuites, configv1.VersionTLS13)),
expectError: false,
validateConfigMap: func(t *testing.T, original, modified *corev1.ConfigMap) {
if diff := cmp.Diff(original.Data[genericOperatorConfigCMKey], modified.Data[genericOperatorConfigCMKey]); diff != "" {
t.Errorf("ConfigMap YAML mismatch (-want +got):\n%s", diff)
if err := validateGenericOperatorConfigTLSInjected(modified, genericOperatorConfigCMKey, testCipherSuites, tlsVersion13); err != nil {
t.Fatalf("validation failed: %v", err)
}
},
},
Expand Down