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
+190 −212
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
1edb16d
resourcebuilder/core: Improve user logs and comments
vincentdephily May 7, 2026
f8e2063
resourcebuilder/core: Keep ciphersuites order when injecting TLS
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
29 changes: 16 additions & 13 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 @@ -40,13 +39,15 @@ type tlsConfig struct {
cipherSuites optional[[]string]
}

// modifyConfigMap sets/clears minTLSVersion and cipherSuites in the CM's data entries if
// * ConfigMapInjectTLSAnnotation is "true"
// * Data entry kind is GenericOperatorConfig or GenericControllerConfig
func (b *builder) modifyConfigMap(ctx context.Context, cm *corev1.ConfigMap) error {
// Check for TLS injection annotation
if value, ok := cm.Annotations[ConfigMapInjectTLSAnnotation]; !ok || value != "true" {
return nil
}

klog.V(2).Infof("ConfigMap %s/%s has %s annotation set to true", cm.Namespace, cm.Name, ConfigMapInjectTLSAnnotation)
klog.V(2).Infof("ConfigMap %s/%s has annotation %s: true", cm.Namespace, cm.Name, ConfigMapInjectTLSAnnotation)

// Empty data, nothing to inject into
if cm.Data == nil {
Expand All @@ -71,7 +72,7 @@ func (b *builder) modifyConfigMap(ctx context.Context, cm *corev1.ConfigMap) err
klog.V(4).Infof("ConfigMap %s/%s: observed minTLSVersion=%v, cipherSuites=%v",
cm.Namespace, cm.Name, minTLSLog, cipherSuitesLog)

// Process each data entry that contains GenericOperatorConfig
// Process each data key
for key, value := range cm.Data {
klog.V(4).Infof("Processing %q key", key)
// Parse YAML into RNode to preserve formatting and field order
Expand All @@ -82,18 +83,19 @@ func (b *builder) modifyConfigMap(ctx context.Context, cm *corev1.ConfigMap) err
continue
}

// Check if this is a supported config kind
// Check if this is a supported data node 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
// Inject TLS settings into the data node while preserving structure
if err := updateRNodeWithTLSSettings(rnode, tlsConf); err != nil {
return fmt.Errorf("failed to inject the TLS configuration: %v", err)
}
Expand All @@ -106,7 +108,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 @@ -142,18 +144,19 @@ func (b *builder) observeTLSConfiguration(ctx context.Context, cm *corev1.Config
}

// Extract cipherSuites from the observed config
// We pass the list as-is, even though TLS implementations may ignore the order and/or content (Go currently does).
// This future-proofs for other implementations, and avoids inconsistencies between the CVO-injected list and the original.
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)

@DavidHurta DavidHurta May 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

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?

@ingvagabund ingvagabund May 12, 2026

Copy link
Copy Markdown
Member

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.

@vincentdephily vincentdephily May 19, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Having investigated this properly, it turns out that Go actually does ignore the configured cipher order (and cipher list, with TLS1.3). This is Go-specific behaviour (and new since 1.17), so I don't feel too bad for having assumed that order mattered 😅

I'll update this PR with clarifying comments.

There's a future-proofing/least-surprise argument to be made for keeping the user's order, but I think that the config-reconcilation-minimizing argument wins out, so I'll put the sort back in place unless you have other opinions.

@ingvagabund ingvagabund May 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice exploration. This is quite surprising.

Yeah, dropping down a comment makes sense here.

@DavidHurta DavidHurta Jun 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, a nice exploration indeed 🙇

I think that the config-reconcilation-minimizing argument wins out

Me as well 👍

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

return config, nil
}

// updateRNodeWithTLSSettings injects TLS settings into a GenericOperatorConfig RNode while preserving structure.
// updateRNodeWithTLSSettings injects TLS settings into an RNode while preserving structure.
// Assumes a GenericOperatorConfig or GenericControllerConfig schema.
// If a field in tlsConf is not found, the corresponding field will be deleted from the RNode.
func updateRNodeWithTLSSettings(rnode *yaml.RNode, tlsConf *tlsConfig) error {
servingInfo, err := rnode.Pipe(yaml.LookupCreate(yaml.MappingNode, "servingInfo"))
Expand Down
Loading