Skip to content

feat(crypto): 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
+112 −10
Open

feat(crypto): 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
42e1c0a
chore: bump openshift/api and openshift/library-go for TLS groups sup…
damdo Aug 5, 2026
2f85f6d
feat: implement CurvePreferences support in NewTLSConfigFromProfile
damdo Aug 5, 2026
d15ac97
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
4 changes: 2 additions & 2 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ 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/library-go v0.0.0-20260720185249-0595e37fe20f
github.com/openshift/api v0.0.0-20260805215214-cfb63858e9d7
github.com/openshift/library-go v0.0.0-20260807194649-ee0a87843dda
k8s.io/apimachinery v0.36.2
k8s.io/client-go v0.36.2
k8s.io/utils v0.0.0-20260707023825-cf1189d6abe3
Expand Down
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 @@ -113,10 +113,10 @@ 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-20260805215214-cfb63858e9d7 h1:Z6p+yoWjFXbfnN2zPdQ8SRXPKDs9QtT3P8hN3SP2zuw=
github.com/openshift/api v0.0.0-20260805215214-cfb63858e9d7/go.mod h1:k6qH5QOVa5GDln2VVm8Jz4NV3Z7R2SATHFLwGS6Wh3M=
github.com/openshift/library-go v0.0.0-20260807194649-ee0a87843dda h1:yVQlJiQZGCi3ydS74ysAkLbP+bVbvAQaAuCDZi6UgDY=
github.com/openshift/library-go v0.0.0-20260807194649-ee0a87843dda/go.mod h1:IrZbEK+wVUMEd+aXzYR2DCCh0p5IaQ7DvycYGP7qIYM=
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
15 changes: 11 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,29 @@ 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) {

@joelanford joelanford Aug 11, 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.

Worth breaking the API to make it possible to distinguish unsupported ciphers from groups? No strong opinion from me. We can always revisit if implementers have issues with it being combined.

@damdo damdo Aug 11, 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 keeping them combined is fine for now. The only consumer logs them as a single list ("unsupported ciphers/groups that will be ignored: %v"), so there's no practical need to distinguish. If a future caller needs to handle them differently, they can call cipherCodes and libgocrypto.TLSGroupsToCurveIDs directly. Agreed we can revisit if it becomes a pain point.

minVersion := libgocrypto.TLSVersionOrDie(string(profile.MinTLSVersion))
cipherSuites, unsupportedCiphers := cipherCodes(profile.Ciphers)
curvePrefs, unsupportedGroups := libgocrypto.TLSGroupsToCurveIDs(profile.Groups)

unsupported = unsupportedCiphers
for _, g := range unsupportedGroups {
unsupported = append(unsupported, string(g))
}

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 +148

@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
95 changes: 95 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,98 @@ 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() {
Comment thread
damdo marked this conversation as resolved.
It("should set CurvePreferences from the profile", func() {
profile := *configv1.TLSProfiles[configv1.TLSProfileIntermediateType]

tlsConfigFn, _ := NewTLSConfigFromProfile(profile)

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

expectedCurves, _ := libgocrypto.TLSGroupsToCurveIDs(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.

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

tlsConfigFn, _ := NewTLSConfigFromProfile(profile)

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

expectedCurves, _ := libgocrypto.TLSGroupsToCurveIDs(profile.Groups)
if len(expectedCurves) > 0 {
Expect(tlsConf.CurvePreferences).To(Equal(expectedCurves))
} else {
Expect(tlsConf.CurvePreferences).To(BeNil())
}
})
})
})