Skip to content

Commit 3c1da4b

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #1401 from bentito/OCPBUGS-3917
OCPBUGS-3917: filter non-FIPS TLS 1.3 ciphers from ROUTER_CIPHERSUITES on FIPS clusters
2 parents c79099d + 0da1246 commit 3c1da4b

4 files changed

Lines changed: 248 additions & 37 deletions

File tree

‎pkg/operator/controller/ingress/controller.go‎

Lines changed: 71 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,14 @@ var (
8484
"TLS_AES_128_CCM_SHA256",
8585
"TLS_AES_128_CCM_8_SHA256",
8686
)
87+
88+
// fipsApprovedTLS13Ciphers is the set of TLS 1.3 cipher suites that are
89+
// FIPS-approved. These are kept in ROUTER_CIPHERSUITES when the operator
90+
// is running in FIPS mode, while others are filtered out.
91+
fipsApprovedTLS13Ciphers = sets.NewString(
92+
"TLS_AES_128_GCM_SHA256",
93+
"TLS_AES_256_GCM_SHA384",
94+
)
8795
)
8896

8997
// New creates the ingress controller from configuration. This is the controller
@@ -356,7 +364,7 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
356364
// successful, immediately re-queue to refresh state.
357365
alreadyAdmitted := ingresscontroller.IsAdmitted(ingress)
358366
if !alreadyAdmitted || needsReadmission(ingress) {
359-
if err := r.admit(ingress, ingressConfig, platformStatus, dnsConfig, alreadyAdmitted); err != nil {
367+
if err := r.admit(ingress, ingressConfig, platformStatus, dnsConfig, apiConfig, alreadyAdmitted); err != nil {
360368
switch err := err.(type) {
361369
case *admissionRejection:
362370
log.Info("IngressController rejected", "namespace", ingress.Namespace, "name", ingress.Name, "reason", err.Reason)
@@ -389,7 +397,7 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
389397
// fields. Returns an error value, which will have a non-nil value of type
390398
// admissionRejection if the ingresscontroller was rejected, or a non-nil
391399
// value of a different type if the ingresscontroller could not be processed.
392-
func (r *reconciler) admit(current *operatorv1.IngressController, ingressConfig *configv1.Ingress, platformStatus *configv1.PlatformStatus, dnsConfig *configv1.DNS, alreadyAdmitted bool) error {
400+
func (r *reconciler) admit(current *operatorv1.IngressController, ingressConfig *configv1.Ingress, platformStatus *configv1.PlatformStatus, dnsConfig *configv1.DNS, apiConfig *configv1.APIServer, alreadyAdmitted bool) error {
393401
updated := current.DeepCopy()
394402

395403
setDefaultDomain(updated, ingressConfig)
@@ -404,7 +412,7 @@ func (r *reconciler) admit(current *operatorv1.IngressController, ingressConfig
404412
// get the default from the APIServer config (which is assumed to be
405413
// valid).
406414

407-
if err := r.validate(updated); err != nil {
415+
if err := r.validate(updated, apiConfig); err != nil {
408416
switch err := err.(type) {
409417
case *admissionRejection:
410418
updated.Status.Conditions = MergeConditions(updated.Status.Conditions, operatorv1.OperatorCondition{
@@ -905,7 +913,7 @@ func hasTLSSecurityProfile(ic *operatorv1.IngressController) bool {
905913
// returns an error value, which will have a non-nil value of type
906914
// admissionRejection if the ingresscontroller is invalid, or a non-nil value of
907915
// a different type if validation could not be completed.
908-
func (r *reconciler) validate(ic *operatorv1.IngressController) error {
916+
func (r *reconciler) validate(ic *operatorv1.IngressController, apiConfig *configv1.APIServer) error {
909917
var errors []error
910918

911919
ingresses := &operatorv1.IngressControllerList{}
@@ -919,7 +927,7 @@ func (r *reconciler) validate(ic *operatorv1.IngressController) error {
919927
if err := validateDomainUniqueness(ic, ingresses.Items); err != nil {
920928
errors = append(errors, err)
921929
}
922-
if err := validateTLSSecurityProfile(ic); err != nil {
930+
if err := validateTLSSecurityProfile(ic, apiConfig); err != nil {
923931
errors = append(errors, err)
924932
}
925933
if err := validateHTTPHeaderBufferValues(ic); err != nil {
@@ -972,49 +980,76 @@ var (
972980
)
973981

974982
// validateTLSSecurityProfile validates the given ingresscontroller's TLS
975-
// security profile, if it specifies one.
976-
func validateTLSSecurityProfile(ic *operatorv1.IngressController) error {
977-
if !hasTLSSecurityProfile(ic) {
978-
return nil
979-
}
983+
// security profile. It resolves the effective profile (which may be inherited
984+
// from the APIServer config) and ensures it is properly configured and FIPS-compliant.
985+
func validateTLSSecurityProfile(ic *operatorv1.IngressController, apiConfig *configv1.APIServer) error {
986+
var errs []error
980987

981-
if ic.Spec.TLSSecurityProfile.Type != configv1.TLSProfileCustomType {
982-
return nil
988+
if apiConfig == nil {
989+
apiConfig = &configv1.APIServer{}
983990
}
984991

985-
spec := ic.Spec.TLSSecurityProfile.Custom
986-
if spec == nil {
987-
return fmt.Errorf("security profile is not defined")
992+
// 1. Figure out which profile is in effect.
993+
var effectiveProfile *configv1.TLSSecurityProfile
994+
if hasTLSSecurityProfile(ic) {
995+
effectiveProfile = ic.Spec.TLSSecurityProfile
996+
} else {
997+
effectiveProfile = apiConfig.Spec.TLSSecurityProfile
988998
}
989999

990-
var errs []error
1000+
// 2. Validate the effective profile.
1001+
if effectiveProfile != nil && effectiveProfile.Type == configv1.TLSProfileCustomType {
1002+
spec := effectiveProfile.Custom
1003+
if spec == nil {
1004+
return fmt.Errorf("security profile is not defined")
1005+
}
9911006

992-
if len(spec.Ciphers) == 0 {
993-
errs = append(errs, fmt.Errorf("security profile has an empty ciphers list"))
994-
} else {
995-
invalidCiphers := []string{}
996-
for _, cipher := range spec.Ciphers {
997-
if !isValidCipher(strings.TrimPrefix(cipher, "!")) {
998-
invalidCiphers = append(invalidCiphers, cipher)
1007+
if len(spec.Ciphers) == 0 {
1008+
errs = append(errs, fmt.Errorf("security profile has an empty ciphers list"))
1009+
} else {
1010+
invalidCiphers := []string{}
1011+
for _, cipher := range spec.Ciphers {
1012+
if !isValidCipher(strings.TrimPrefix(cipher, "!")) {
1013+
invalidCiphers = append(invalidCiphers, cipher)
1014+
}
9991015
}
1000-
}
1001-
if len(invalidCiphers) != 0 {
1002-
errs = append(errs, fmt.Errorf("security profile has invalid ciphers: %s", strings.Join(invalidCiphers, ", ")))
1003-
}
1004-
switch spec.MinTLSVersion {
1005-
case configv1.VersionTLS10, configv1.VersionTLS11, configv1.VersionTLS12:
1006-
if tlsVersion13Ciphers.HasAll(spec.Ciphers...) {
1007-
errs = append(errs, fmt.Errorf("security profile specifies minTLSVersion: %s and contains only TLSv1.3 cipher suites", spec.MinTLSVersion))
1016+
if len(invalidCiphers) != 0 {
1017+
errs = append(errs, fmt.Errorf("security profile has invalid ciphers: %s", strings.Join(invalidCiphers, ", ")))
10081018
}
1009-
case configv1.VersionTLS13:
1010-
if !tlsVersion13Ciphers.HasAny(spec.Ciphers...) {
1011-
errs = append(errs, fmt.Errorf("security profile specifies minTLSVersion: %s and contains no TLSv1.3 cipher suites", spec.MinTLSVersion))
1019+
switch spec.MinTLSVersion {
1020+
case configv1.VersionTLS10, configv1.VersionTLS11, configv1.VersionTLS12:
1021+
if tlsVersion13Ciphers.HasAll(spec.Ciphers...) {
1022+
errs = append(errs, fmt.Errorf("security profile specifies minTLSVersion: %s and contains only TLSv1.3 cipher suites", spec.MinTLSVersion))
1023+
}
1024+
case configv1.VersionTLS13:
1025+
if !tlsVersion13Ciphers.HasAny(spec.Ciphers...) {
1026+
errs = append(errs, fmt.Errorf("security profile specifies minTLSVersion: %s and contains no TLSv1.3 cipher suites", spec.MinTLSVersion))
1027+
}
10121028
}
10131029
}
1030+
1031+
if _, ok := validTLSVersions[spec.MinTLSVersion]; !ok {
1032+
errs = append(errs, fmt.Errorf("security profile has invalid minimum security protocol version: %q", spec.MinTLSVersion))
1033+
}
10141034
}
10151035

1016-
if _, ok := validTLSVersions[spec.MinTLSVersion]; !ok {
1017-
errs = append(errs, fmt.Errorf("security profile has invalid minimum security protocol version: %q", spec.MinTLSVersion))
1036+
// On FIPS-enabled clusters, non-FIPS TLS 1.3 ciphers are filtered from
1037+
// ROUTER_CIPHERSUITES. If the effective profile's only TLS 1.3 cipher suites
1038+
// are non-FIPS, they would all be removed, leaving no TLS 1.3 ciphers
1039+
// configured. Reject such profiles with a clear error message.
1040+
if isFIPSEnabled {
1041+
resolvedSpec := tlsProfileSpecForIngressController(ic, apiConfig)
1042+
tls13InProfile := tlsVersion13Ciphers.Intersection(sets.NewString(resolvedSpec.Ciphers...))
1043+
if tls13InProfile.Len() > 0 && !tls13InProfile.HasAny(fipsApprovedTLS13Ciphers.UnsortedList()...) {
1044+
errs = append(errs, fmt.Errorf(
1045+
"security profile's TLS 1.3 cipher suites (%s) are not FIPS-compliant"+
1046+
" and will be removed on this FIPS-enabled cluster,"+
1047+
" leaving no TLS 1.3 ciphers configured;"+
1048+
" specify at least one FIPS-compliant TLS 1.3 cipher suite"+
1049+
" (e.g. TLS_AES_128_GCM_SHA256 or TLS_AES_256_GCM_SHA384)"+
1050+
" or remove the non-FIPS cipher suites from the profile",
1051+
strings.Join(tls13InProfile.List(), ", ")))
1052+
}
10181053
}
10191054

10201055
return utilerrors.NewAggregate(errs)

‎pkg/operator/controller/ingress/controller_test.go‎

Lines changed: 93 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1153,7 +1153,7 @@ func Test_TLSProfileSpecForSecurityProfile(t *testing.T) {
11531153
},
11541154
}
11551155
tlsProfileSpec := operatorcontroller.TLSProfileSpecForSecurityProfile(ic.Spec.TLSSecurityProfile)
1156-
err := validateTLSSecurityProfile(ic)
1156+
err := validateTLSSecurityProfile(ic, &configv1.APIServer{})
11571157
if tc.valid && err != nil {
11581158
t.Fatalf("unexpected error: %v\nprofile:\n%s", err, util.ToYaml(tlsProfileSpec))
11591159
}
@@ -1168,6 +1168,98 @@ func Test_TLSProfileSpecForSecurityProfile(t *testing.T) {
11681168
}
11691169
}
11701170

1171+
// Test_validateTLSSecurityProfileFIPS verifies that validateTLSSecurityProfile
1172+
// rejects custom profiles whose only TLS 1.3 cipher suites are non-FIPS when
1173+
// the cluster is running in FIPS mode (OCPBUGS-3917).
1174+
func Test_validateTLSSecurityProfileFIPS(t *testing.T) {
1175+
// Simulate FIPS mode by overriding the package-level isFIPSEnabled var.
1176+
origFIPS := isFIPSEnabled
1177+
isFIPSEnabled = true
1178+
defer func() { isFIPSEnabled = origFIPS }()
1179+
1180+
testCases := []struct {
1181+
description string
1182+
ciphers []string
1183+
expectError bool
1184+
inheritFromAPI bool
1185+
}{
1186+
{
1187+
description: "FIPS mode: profile with only TLS_CHACHA20_POLY1305_SHA256 as TLS 1.3 cipher should be rejected",
1188+
ciphers: []string{
1189+
"ECDHE-RSA-AES128-GCM-SHA256",
1190+
"TLS_CHACHA20_POLY1305_SHA256",
1191+
},
1192+
expectError: true,
1193+
},
1194+
{
1195+
description: "FIPS mode: profile with FIPS-compliant TLS 1.3 cipher alongside CHACHA20 should be accepted",
1196+
ciphers: []string{
1197+
"ECDHE-RSA-AES128-GCM-SHA256",
1198+
"TLS_AES_128_GCM_SHA256",
1199+
"TLS_CHACHA20_POLY1305_SHA256",
1200+
},
1201+
expectError: false,
1202+
},
1203+
{
1204+
description: "FIPS mode: profile with only FIPS-compliant TLS 1.3 ciphers should be accepted",
1205+
ciphers: []string{
1206+
"ECDHE-RSA-AES128-GCM-SHA256",
1207+
"TLS_AES_128_GCM_SHA256",
1208+
"TLS_AES_256_GCM_SHA384",
1209+
},
1210+
expectError: false,
1211+
},
1212+
{
1213+
description: "FIPS mode: inherited custom profile from APIServer with only TLS_CHACHA20_POLY1305_SHA256 as TLS 1.3 cipher should be rejected",
1214+
ciphers: []string{
1215+
"ECDHE-RSA-AES128-GCM-SHA256",
1216+
"TLS_CHACHA20_POLY1305_SHA256",
1217+
},
1218+
expectError: true,
1219+
inheritFromAPI: true,
1220+
},
1221+
{
1222+
description: "FIPS mode: profile with only TLS 1.2 ciphers (no TLS 1.3 at all) should be accepted",
1223+
ciphers: []string{
1224+
"ECDHE-RSA-AES128-GCM-SHA256",
1225+
"ECDHE-RSA-AES256-GCM-SHA384",
1226+
},
1227+
expectError: false,
1228+
},
1229+
}
1230+
1231+
for _, tc := range testCases {
1232+
t.Run(tc.description, func(t *testing.T) {
1233+
ic := &operatorv1.IngressController{}
1234+
apiConfig := &configv1.APIServer{}
1235+
1236+
profile := &configv1.TLSSecurityProfile{
1237+
Type: configv1.TLSProfileCustomType,
1238+
Custom: &configv1.CustomTLSProfile{
1239+
TLSProfileSpec: configv1.TLSProfileSpec{
1240+
Ciphers: tc.ciphers,
1241+
MinTLSVersion: configv1.VersionTLS12,
1242+
},
1243+
},
1244+
}
1245+
1246+
if tc.inheritFromAPI {
1247+
apiConfig.Spec.TLSSecurityProfile = profile
1248+
} else {
1249+
ic.Spec.TLSSecurityProfile = profile
1250+
}
1251+
1252+
err := validateTLSSecurityProfile(ic, apiConfig)
1253+
if tc.expectError && err == nil {
1254+
t.Error("expected error for FIPS-incompatible profile, got nil")
1255+
}
1256+
if !tc.expectError && err != nil {
1257+
t.Errorf("unexpected error: %v", err)
1258+
}
1259+
})
1260+
}
1261+
}
1262+
11711263
func Test_tlsProfileSpecForIngressController(t *testing.T) {
11721264
testCases := []struct {
11731265
description string

‎pkg/operator/controller/ingress/deployment.go‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"math"
1010
"net"
1111
"net/url"
12+
"os"
1213
"path/filepath"
1314
"sort"
1415
"strconv"
@@ -39,6 +40,17 @@ import (
3940
securityv1 "github.com/openshift/api/security/v1"
4041
)
4142

43+
// isFIPSEnabled reports whether the current node is operating in FIPS
44+
// mode by checking /proc/sys/crypto/fips_enabled. This is a
45+
// package-level variable so tests can override it to simulate FIPS mode.
46+
var isFIPSEnabled = func() bool {
47+
data, err := os.ReadFile("/proc/sys/crypto/fips_enabled")
48+
if err != nil {
49+
return false
50+
}
51+
return len(data) > 0 && data[0] == '1'
52+
}()
53+
4254
const (
4355
WildcardRouteAdmissionPolicy = "ROUTER_ALLOW_WILDCARD_ROUTES"
4456

@@ -960,6 +972,20 @@ func desiredRouterDeployment(ci *operatorv1.IngressController, config *Config, i
960972
otherCiphers = append(otherCiphers, cipher)
961973
}
962974
}
975+
// On FIPS-enabled clusters, remove non-FIPS-compliant TLS 1.3 cipher
976+
// suites (specifically TLS_CHACHA20_POLY1305_SHA256) from
977+
// ROUTER_CIPHERSUITES. HAProxy would fail TLS handshakes when a client
978+
// offers a non-FIPS cipher first if that cipher is listed in
979+
// ssl-default-bind-ciphersuites but excluded by the OS FIPS policy.
980+
if isFIPSEnabled {
981+
fipsCiphers := tls13Ciphers[:0]
982+
for _, c := range tls13Ciphers {
983+
if fipsApprovedTLS13Ciphers.Has(c) {
984+
fipsCiphers = append(fipsCiphers, c)
985+
}
986+
}
987+
tls13Ciphers = fipsCiphers
988+
}
963989
env = append(env, corev1.EnvVar{
964990
Name: "ROUTER_CIPHERS",
965991
Value: strings.Join(otherCiphers, ":"),

‎pkg/operator/controller/ingress/deployment_test.go‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1002,6 +1002,64 @@ func TestDesiredRouterDeploymentVariety(t *testing.T) {
10021002
checkContainerPort(t, deployment, "metrics", 9146)
10031003
}
10041004

1005+
// TestDesiredRouterDeploymentFIPS verifies that on a FIPS-enabled cluster,
1006+
// TLS_CHACHA20_POLY1305_SHA256 is removed from ROUTER_CIPHERSUITES while
1007+
// FIPS-compliant TLS 1.3 ciphers are kept (OCPBUGS-3917).
1008+
func TestDesiredRouterDeploymentFIPS(t *testing.T) {
1009+
// Simulate FIPS mode by swapping the package-level isFIPSEnabled var.
1010+
origFIPS := isFIPSEnabled
1011+
isFIPSEnabled = true
1012+
defer func() { isFIPSEnabled = origFIPS }()
1013+
1014+
ic, ingressConfig, infraConfig, apiConfig, networkConfig, proxyNeeded, clusterProxyConfig := getRouterDeploymentComponents(t)
1015+
1016+
// Profile: two FIPS-compliant TLS 1.3 ciphers + one non-FIPS TLS 1.3 cipher.
1017+
ic.Spec.TLSSecurityProfile = &configv1.TLSSecurityProfile{
1018+
Type: configv1.TLSProfileCustomType,
1019+
Custom: &configv1.CustomTLSProfile{
1020+
TLSProfileSpec: configv1.TLSProfileSpec{
1021+
Ciphers: []string{
1022+
"ECDHE-RSA-AES128-GCM-SHA256", // TLS 1.2
1023+
"TLS_AES_128_GCM_SHA256", // TLS 1.3, FIPS-compliant
1024+
"TLS_AES_256_GCM_SHA384", // TLS 1.3, FIPS-compliant
1025+
"TLS_CHACHA20_POLY1305_SHA256", // TLS 1.3, non-FIPS — must be removed
1026+
},
1027+
MinTLSVersion: configv1.VersionTLS12,
1028+
},
1029+
},
1030+
}
1031+
1032+
deployment, err := desiredRouterDeployment(ic, &Config{IngressControllerImage: ingressControllerImage}, ingressConfig, infraConfig, apiConfig, networkConfig, proxyNeeded, false, nil, clusterProxyConfig)
1033+
if err != nil {
1034+
t.Fatalf("unexpected error: %v", err)
1035+
}
1036+
1037+
// CHACHA20 must be absent; the two FIPS-compliant ciphers must be present.
1038+
expected := []envData{
1039+
{"ROUTER_CIPHERS", true, "ECDHE-RSA-AES128-GCM-SHA256"},
1040+
{"ROUTER_CIPHERSUITES", true, "TLS_AES_128_GCM_SHA256:TLS_AES_256_GCM_SHA384"},
1041+
}
1042+
if err := checkDeploymentEnvironment(t, deployment, expected); err != nil {
1043+
t.Errorf("FIPS cipher filtering: %v", err)
1044+
}
1045+
1046+
checkDeploymentHasEnvSorted(t, deployment)
1047+
1048+
// Also verify non-FIPS mode leaves all ciphers intact.
1049+
isFIPSEnabled = false
1050+
deploymentNonFIPS, err := desiredRouterDeployment(ic, &Config{IngressControllerImage: ingressControllerImage}, ingressConfig, infraConfig, apiConfig, networkConfig, proxyNeeded, false, nil, clusterProxyConfig)
1051+
if err != nil {
1052+
t.Fatalf("unexpected error (non-FIPS): %v", err)
1053+
}
1054+
expectedNonFIPS := []envData{
1055+
{"ROUTER_CIPHERS", true, "ECDHE-RSA-AES128-GCM-SHA256"},
1056+
{"ROUTER_CIPHERSUITES", true, "TLS_AES_128_GCM_SHA256:TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_SHA256"},
1057+
}
1058+
if err := checkDeploymentEnvironment(t, deploymentNonFIPS, expectedNonFIPS); err != nil {
1059+
t.Errorf("non-FIPS cipher passthrough: %v", err)
1060+
}
1061+
}
1062+
10051063
// TestDesiredRouterDeploymentHostNetworkNil verifies that
10061064
// desiredRouterDeployment behaves correctly when
10071065
// status.endpointPublishingStrategy.type is "HostNetwork" but

0 commit comments

Comments
 (0)