Skip to content

feat: implement TLS groups/curve preferences support #22

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
damdo wants to merge 3 commits into
base: main
Choose a base branch
from
+93 −9
Open

feat: implement TLS groups/curve preferences support #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
219b2a0
chore: bump openshift/api, add library-go replace for TLS groups support
damdo Aug 5, 2026
bded54e
feat: implement CurvePreferences support in NewTLSConfigFromProfile
damdo Aug 5, 2026
c12349a
test: add CurvePreferences tests for NewTLSConfigFromProfile
damdo Aug 5, 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
5 changes: 4 additions & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ require (
github.com/go-logr/logr v1.4.3
github.com/onsi/ginkgo/v2 v2.28.1
github.com/onsi/gomega v1.39.1
github.com/openshift/api v0.0.0-20260715165912-72066cc9718b
github.com/openshift/api v0.0.0-20260728120005-8ba0b25b0f29
github.com/openshift/library-go v0.0.0-20260720185249-0595e37fe20f
k8s.io/apimachinery v0.36.2
k8s.io/client-go v0.36.2
Expand Down Expand Up @@ -81,3 +81,6 @@ require (
sigs.k8s.io/structured-merge-diff/v6 v6.3.2 // indirect
sigs.k8s.io/yaml v1.6.0 // indirect
)

// TODO: Remove this replace directive and revendor once https://github.com/openshift/library-go/pull/2347 merges.
replace github.com/openshift/library-go => github.com/damdo/library-go v0.0.0-20260804155721-fceccb2c136d
Comment thread
coderabbitai[bot] marked this conversation as resolved.
8 changes: 4 additions & 4 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6r
github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs=
github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs=
github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
github.com/damdo/library-go v0.0.0-20260804155721-fceccb2c136d h1:k3uIWroA3znestq3AdxKpkQ7cl7ObLfmhcW77gc80Po=
github.com/damdo/library-go v0.0.0-20260804155721-fceccb2c136d/go.mod h1:iahMN6YoNSRG1xyRj03YYU5iboJzv+UZFu9S0grJmQc=
github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc h1:U9qPSI2PIWSS1VwoXQT9A3Wy9MM3WgvqSxFWenqJduM=
Expand Down Expand Up @@ -113,10 +115,8 @@ github.com/onsi/ginkgo/v2 v2.28.1 h1:S4hj+HbZp40fNKuLUQOYLDgZLwNUVn19N3Atb98NCyI
github.com/onsi/ginkgo/v2 v2.28.1/go.mod h1:CLtbVInNckU3/+gC8LzkGUb9oF+e8W8TdUsxPwvdOgE=
github.com/onsi/gomega v1.39.1 h1:1IJLAad4zjPn2PsnhH70V4DKRFlrCzGBNrNaru+Vf28=
github.com/onsi/gomega v1.39.1/go.mod h1:hL6yVALoTOxeWudERyfppUcZXjMwIMLnuSfruD2lcfg=
github.com/openshift/api v0.0.0-20260715165912-72066cc9718b h1:gN3SihCYEwoIksD+f24wHhwiEgvaV0RxNjgmkDvBBeg=
github.com/openshift/api v0.0.0-20260715165912-72066cc9718b/go.mod h1:k6qH5QOVa5GDln2VVm8Jz4NV3Z7R2SATHFLwGS6Wh3M=
github.com/openshift/library-go v0.0.0-20260720185249-0595e37fe20f h1:NdSEtKB+vvHlGELA+jX/c+TALNwe8iSsJAQYvMabgHQ=
github.com/openshift/library-go v0.0.0-20260720185249-0595e37fe20f/go.mod h1:iWcB6wgeOhsByZAZGhmzBtEnrLQzABL0s3aeou8AmSI=
github.com/openshift/api v0.0.0-20260728120005-8ba0b25b0f29 h1:WrS2vkQtwJyo5xHJnJfjmHPDasdyKLKwojhMDV3tPto=
github.com/openshift/api v0.0.0-20260728120005-8ba0b25b0f29/go.mod h1:k6qH5QOVa5GDln2VVm8Jz4NV3Z7R2SATHFLwGS6Wh3M=
github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4=
github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
Expand Down
12 changes: 8 additions & 4 deletions pkg/tls/tls.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,22 +123,26 @@ func GetTLSProfileSpec(profile *configv1.TLSSecurityProfile) (configv1.TLSProfil
// Note: CipherSuites are only set when MinVersion is below TLS 1.3, as Go's TLS 1.3 implementation
// does not allow configuring cipher suites - all TLS 1.3 ciphers are always enabled.
// See: https://github.com/golang/go/issues/29349
func NewTLSConfigFromProfile(profile configv1.TLSProfileSpec) (tlsConfig func(*tls.Config), unsupportedCiphers []string) {
func NewTLSConfigFromProfile(profile configv1.TLSProfileSpec) (tlsConfig func(*tls.Config), unsupported []string) {
minVersion := libgocrypto.TLSVersionOrDie(string(profile.MinTLSVersion))
cipherSuites, unsupportedCiphers := cipherCodes(profile.Ciphers)
curvePrefs, unsupportedGroups := libgocrypto.CurveIDsForTLSGroups(profile.Groups)

unsupported = append(unsupportedCiphers, unsupportedGroups...)

@hroyrh hroyrh Jul 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is there any value in separating the two types here?

@damdo damdo Aug 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think merging is fine here. The caller only uses unsupported for logging ("unsupported ciphers/groups that will be ignored: %v"). Separating them would change the function signature (e.g., return two slices, or a struct) for no practical benefit, the caller doesn't need to handle them differently. If a consumer ever needs to distinguish, libgocrypto.CurveIDsForTLSGroups and cipherCodes are individually callable.


return func(tlsConf *tls.Config) {
tlsConf.MinVersion = minVersion
// TODO: add curve preferences from profile once https://github.com/openshift/api/pull/2583 merges.
// tlsConf.CurvePreferences <<<<<< profile.Curves
if len(curvePrefs) > 0 {
tlsConf.CurvePreferences = curvePrefs
}

// TLS 1.3 cipher suites are not configurable in Go (https://github.com/golang/go/issues/29349), so only set CipherSuites accordingly.
// TODO: revisit this once we get an answer on the best way to handle this here:
// https://docs.google.com/document/d/1cMc9E8psHfnoK06ntR8kHSWB8d3rMtmldhnmM4nImjs/edit?disco=AAABu_nPcYg
if minVersion != tls.VersionTLS13 {
tlsConf.CipherSuites = cipherSuites
}
}, unsupportedCiphers
}, unsupported
Comment on lines +126 to +145

@coderabbitai coderabbitai Bot Jul 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Locate the vendored library-go crypto package and inspect CurveIDsForTLSGroups signature
fd -a 'crypto.go' --path '*library-go/pkg/crypto*'
rg -n -A 15 'func CurveIDsForTLSGroups' -g '*.go'

Repository: openshift/controller-runtime-common

Length of output: 405

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== go.mod references =='
rg -n 'library-go|CurveIDsForTLSGroups|TLSGroup|TLSProfileSpec' go.mod go.sum vendor . || true

echo
echo '== locate tls.go =='
fd -a 'tls.go' pkg || true

echo
echo '== outline pkg/tls/tls.go =='
if command -v ast-grep >/dev/null 2>&1; then
  ast-grep outline pkg/tls/tls.go --view expanded || true
fi

echo
echo '== relevant lines in pkg/tls/tls.go =='
nl -ba pkg/tls/tls.go | sed -n '1,220p'

Repository: openshift/controller-runtime-common

Length of output: 26899

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== dependency metadata =='
rg -n 'openshift/library-go|library-go' go.mod go.sum vendor . || true

echo
echo '== search for CurveIDsForTLSGroups in repo =='
rg -n 'CurveIDsForTLSGroups|TLSGroup' . || true

echo
echo '== inspect pkg/tls tests =='
fd -a 'tls_test.go' pkg || true
if [ -f pkg/tls/tls_test.go ]; then
  nl -ba pkg/tls/tls_test.go | sed -n '1,260p'
fi

Repository: openshift/controller-runtime-common

Length of output: 9811

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== vendor CurveIDsForTLSGroups implementation =='
sed -n '392,420p' vendor/github.com/openshift/library-go/pkg/crypto/crypto.go

echo
echo '== tls tests around unsupported groups =='
sed -n '286,360p' pkg/tls/tls_test.go

Repository: openshift/controller-runtime-common

Length of output: 3714

Normalize unsupported group names
CurveIDsForTLSGroups appends string(group) for unsupported entries, so configv1.TLSGroupSecP256r1MLKEM768 is reported as SecP256r1MLKEM768, not secp256r1_mlkem768. If the lowercase underscore form is required here, normalize before returning.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/tls/tls.go` around lines 116 - 137, Normalize the unsupported group names
returned from NewTLSConfigFromProfile so they match the expected lowercase
underscore format instead of the raw enum string. Update the unsupported-group
handling in CurveIDsForTLSGroups (or immediately after its result is used in
NewTLSConfigFromProfile) to map values like SecP256r1MLKEM768 to
secp256r1_mlkem768 before appending to unsupported, while keeping
unsupportedCiphers behavior unchanged.

}

// SetNextProtos returns a TLS configuration function that sets the ALPN
Expand Down
77 changes: 77 additions & 0 deletions pkg/tls/tls_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
configv1 "github.com/openshift/api/config/v1"
libgocrypto "github.com/openshift/library-go/pkg/crypto"
)

var _ = Describe("GetTLSProfileSpec", func() {
Expand Down Expand Up @@ -379,4 +380,80 @@ var _ = Describe("NewTLSConfigFromProfile", func() {
Expect(tlsConf.CipherSuites).To(BeNil())
})
})

Context("when profile contains Groups", func() {
It("should set CurvePreferences for supported groups", func() {
profile := configv1.TLSProfileSpec{
MinTLSVersion: configv1.VersionTLS12,
Groups: []configv1.TLSGroup{
configv1.TLSGroupX25519,
configv1.TLSGroupSecP256r1,
},
}

tlsConfigFn, unsupported := NewTLSConfigFromProfile(profile)
Expect(unsupported).To(BeEmpty())

tlsConf := &tls.Config{}
tlsConfigFn(tlsConf)

Expect(tlsConf.CurvePreferences).To(Equal([]tls.CurveID{tls.X25519, tls.CurveP256}))
})
})

Context("when profile contains unsupported Groups", func() {
It("should return unsupported groups and set only supported curves", func() {
profile := configv1.TLSProfileSpec{
MinTLSVersion: configv1.VersionTLS12,
Groups: []configv1.TLSGroup{
configv1.TLSGroupX25519,
configv1.TLSGroup("unsupported_group"),
},
}

tlsConfigFn, unsupported := NewTLSConfigFromProfile(profile)
Expect(unsupported).To(ConsistOf("unsupported_group"))

tlsConf := &tls.Config{}
tlsConfigFn(tlsConf)

Expect(tlsConf.CurvePreferences).To(HaveLen(1))
Expect(tlsConf.CurvePreferences).To(ContainElement(tls.X25519))
})
})

Context("when profile has empty Groups", func() {
It("should not set CurvePreferences", func() {
profile := configv1.TLSProfileSpec{
MinTLSVersion: configv1.VersionTLS12,
Groups: []configv1.TLSGroup{},
}

tlsConfigFn, unsupported := NewTLSConfigFromProfile(profile)
Expect(unsupported).To(BeEmpty())

tlsConf := &tls.Config{}
tlsConfigFn(tlsConf)

Expect(tlsConf.CurvePreferences).To(BeNil())
})
})

Context("when using Intermediate profile with Groups", func() {
It("should set CurvePreferences from the profile", func() {
profile := *configv1.TLSProfiles[configv1.TLSProfileIntermediateType]

tlsConfigFn, _ := NewTLSConfigFromProfile(profile)

tlsConf := &tls.Config{}
tlsConfigFn(tlsConf)

expectedCurves, _ := libgocrypto.CurveIDsForTLSGroups(profile.Groups)
if len(expectedCurves) > 0 {
Expect(tlsConf.CurvePreferences).To(Equal(expectedCurves))
} else {
Expect(tlsConf.CurvePreferences).To(BeNil())
}
})
})
Comment thread
damdo marked this conversation as resolved.
})