WINC-1988: TLS profile adherence #4340
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,19 @@ | ||
| package main | ||
|
|
||
| import ( | ||
| "context" | ||
| "crypto/tls" | ||
| "flag" | ||
| "fmt" | ||
| "os" | ||
| "strings" | ||
|
|
||
| configv1 "github.com/openshift/api/config/v1" | ||
| openshiftconfig "github.com/openshift/api/config/v1" | ||
| mapi "github.com/openshift/api/machine/v1beta1" | ||
| mcfg "github.com/openshift/api/machineconfiguration/v1" | ||
| libgocrypto "github.com/openshift/library-go/pkg/crypto" | ||
| tlspkg "github.com/openshift/controller-runtime-common/pkg/tls" | ||
| operators "github.com/operator-framework/api/pkg/operators/v2" | ||
| "github.com/operator-framework/operator-lib/leader" | ||
| monv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" | ||
|
|
@@ -46,6 +51,9 @@ import ( | |
| // ServiceAccount permissions used to watch operator on secrets. | ||
| //+kubebuilder:rbac:groups="",resources=secrets,verbs=watch | ||
|
|
||
| // RBAC for reading APIServer configuration to fetch TLS security profile | ||
| //+kubebuilder:rbac:groups=config.openshift.io,resources=apiservers,verbs=get;list;watch | ||
|
|
||
| var ( | ||
| scheme = runtime.NewScheme() | ||
| setupLog = ctrl.Log.WithName("setup") | ||
|
|
@@ -113,8 +121,9 @@ func main() { | |
| os.Exit(1) | ||
| } | ||
|
|
||
| ctx, cancel := context.WithCancel(ctrl.SetupSignalHandler()) | ||
| defer cancel() | ||
|
|
||
| clusterConfig, err := cluster.NewConfig(ctx, cfg) | ||
| if err != nil { | ||
| setupLog.Error(err, "failed to get cluster configuration") | ||
|
|
@@ -127,6 +136,30 @@ func main() { | |
| os.Exit(1) | ||
| } | ||
|
|
||
| // Get TLS configuration from cluster config (fetched from APIServer during NewConfig) | ||
| tlsProfile := clusterConfig.TLSProfileSpec() | ||
| tlsAdherence := clusterConfig.TLSAdherencePolicy() | ||
|
|
||
| // Gate TLS profile enforcement on the adherence policy. WMCO was not previously honoring the | ||
| // cluster TLS profile, so per the centralized TLS config enhancement (StrictAllComponents mode) | ||
| // it should only enforce when ShouldHonorClusterTLSProfile returns true. | ||
| var metricsServerTLSOpts []func(*tls.Config) | ||
| if libgocrypto.ShouldHonorClusterTLSProfile(tlsAdherence) { | ||
| tlsConfigFn, unsupportedCiphers := tlspkg.NewTLSConfigFromProfile(tlsProfile) | ||
| if len(unsupportedCiphers) > 0 { | ||
| setupLog.Info("some cipher suites are not supported by Go and will be ignored", | ||
| "unsupportedCiphers", unsupportedCiphers) | ||
| } | ||
| metricsServerTLSOpts = []func(*tls.Config){ | ||
| tlsConfigFn, | ||
| tlspkg.SetNextProtos(tlspkg.HTTP1NextProtos...), | ||
| } | ||
| setupLog.Info("TLS configuration loaded", | ||
| "minVersion", tlsProfile.MinTLSVersion, | ||
| "cipherSuites", len(tlsProfile.Ciphers)-len(unsupportedCiphers), | ||
| "adherencePolicy", tlsAdherence) | ||
| } | ||
|
|
||
| setupLog.Info("platform", "type", clusterConfig.Platform()) | ||
|
|
||
| if err := logconfig.ValidateLogConfig(); err != nil { | ||
|
|
@@ -184,6 +217,7 @@ func main() { | |
| BindAddress: metricsAddr, | ||
| SecureServing: true, | ||
| FilterProvider: filters.WithAuthenticationAndAuthorization, | ||
| TLSOpts: metricsServerTLSOpts, | ||
| }, | ||
|
Contributor
There was a problem hiding this comment. curve preferences PR merged, consider setting CurvePreferences to include X25519MLKEM768 for post-quantum key exchange
Member
Author
There was a problem hiding this comment. only the enhancement merged, implementation is pending https://github.com/openshift/controller-runtime-common/pull/22/changes
Contributor
There was a problem hiding this comment.
consider re-wording the PR description, the one thing that actually matters for post-quantum readiness is the hybrid KEM group the I'd rather see the description say plainly "lays the groundwork, curve preferences to follow once controller-runtime-common#22 lands" and get a Jira follow-up filed and linked now while the context's fresh |
||
| }) | ||
| if err != nil { | ||
|
|
@@ -307,6 +341,30 @@ func main() { | |
| os.Exit(1) | ||
| } | ||
|
|
||
| // Setup TLS security profile watcher | ||
| // Watches for TLS profile or adherence policy changes and restarts pod to reload config | ||
| tlsWatcher := &tlspkg.SecurityProfileWatcher{ | ||
| Client: mgr.GetClient(), | ||
|
mansikulkarni96 marked this conversation as resolved.
|
||
| InitialTLSProfileSpec: tlsProfile, | ||
| InitialTLSAdherencePolicy: tlsAdherence, | ||
| OnProfileChange: func(ctx context.Context, oldProfile, newProfile configv1.TLSProfileSpec) { | ||
| setupLog.Info("TLS security profile changed, initiating shutdown to reload configuration", | ||
| "oldMinVersion", oldProfile.MinTLSVersion, | ||
| "newMinVersion", newProfile.MinTLSVersion) | ||
| cancel() | ||
| }, | ||
| OnAdherencePolicyChange: func(ctx context.Context, oldPolicy, newPolicy configv1.TLSAdherencePolicy) { | ||
| setupLog.Info("TLS adherence policy changed, initiating shutdown to reload configuration", | ||
| "oldPolicy", oldPolicy, | ||
| "newPolicy", newPolicy) | ||
| cancel() | ||
| }, | ||
| } | ||
| if err := tlsWatcher.SetupWithManager(mgr); err != nil { | ||
| setupLog.Error(err, "unable to setup TLS security profile watcher") | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| //+kubebuilder:scaffold:builder | ||
| // The above marker tells kubebuilder that this is where the SetupWithManager function should be inserted when new | ||
| // controllers are generated by Operator SDK. | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ import ( | |
| oconfig "github.com/openshift/api/config/v1" | ||
| configclient "github.com/openshift/client-go/config/clientset/versioned" | ||
| operatorv1 "github.com/openshift/client-go/operator/clientset/versioned/typed/operator/v1" | ||
| tlspkg "github.com/openshift/controller-runtime-common/pkg/tls" | ||
| "golang.org/x/mod/semver" | ||
| meta "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/client-go/rest" | ||
|
|
@@ -54,6 +55,10 @@ type Config interface { | |
| Platform() oconfig.PlatformType | ||
| // Network returns network configuration for the OpenShift cluster | ||
| Network() Network | ||
| // TLSProfileSpec returns the TLS profile spec fetched from the APIServer | ||
| TLSProfileSpec() oconfig.TLSProfileSpec | ||
| // TLSAdherencePolicy returns the TLS adherence policy fetched from the APIServer | ||
| TLSAdherencePolicy() oconfig.TLSAdherencePolicy | ||
| } | ||
|
|
||
| // networkType holds information for a required network type | ||
|
|
@@ -75,6 +80,10 @@ type config struct { | |
| // platform indicates the cloud on which OpenShift cluster is running | ||
| // TODO: Remove this once we figure out how to be provider agnostic | ||
| platform oconfig.PlatformType | ||
| // tlsProfileSpec is the TLS profile spec fetched from the APIServer during startup | ||
| tlsProfileSpec oconfig.TLSProfileSpec | ||
| // tlsAdherencePolicy is the TLS adherence policy fetched from the APIServer during startup | ||
| tlsAdherencePolicy oconfig.TLSAdherencePolicy | ||
| } | ||
|
|
||
| func (c *config) Platform() oconfig.PlatformType { | ||
|
|
@@ -85,6 +94,14 @@ func (c *config) Network() Network { | |
| return c.network | ||
| } | ||
|
|
||
| func (c *config) TLSProfileSpec() oconfig.TLSProfileSpec { | ||
| return c.tlsProfileSpec | ||
| } | ||
|
|
||
| func (c *config) TLSAdherencePolicy() oconfig.TLSAdherencePolicy { | ||
| return c.tlsAdherencePolicy | ||
| } | ||
|
|
||
| // NewConfig returns a Config struct pertaining to the cluster configuration | ||
| func NewConfig(ctx context.Context, restConfig *rest.Config) (Config, error) { | ||
| // get OpenShift API config client. | ||
|
|
@@ -117,11 +134,24 @@ func NewConfig(ctx context.Context, restConfig *rest.Config) (Config, error) { | |
| if len(platformStatus.Type) == 0 { | ||
| return nil, fmt.Errorf("error getting platform type") | ||
| } | ||
|
|
||
| // Fetch TLS configuration from APIServer | ||
| apiServer, err := oclient.ConfigV1().APIServers().Get(ctx, "cluster", meta.GetOptions{}) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error getting APIServer configuration: %w", err) | ||
| } | ||
| tlsProfileSpec, err := tlspkg.GetTLSProfileSpec(apiServer.Spec.TLSSecurityProfile) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error getting TLS profile from APIServer: %w", err) | ||
| } | ||
|
|
||
| return &config{ | ||
| oclient: oclient, | ||
| operatorClient: operatorClient, | ||
| network: network, | ||
| platform: platformStatus.Type, | ||
| tlsProfileSpec: tlsProfileSpec, | ||
| tlsAdherencePolicy: apiServer.Spec.TLSAdherence, | ||
|
Comment on lines
+138
to
+154
There was a problem hiding this comment. 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift Add table-driven tests for the APIServer TLS contract. Test successful profile and policy storage. Test APIServer retrieval failure. Test invalid custom profile handling. This startup path now controls manager TLS behavior. As per path instructions, “Add or update adjacent table-driven tests using testify/assert and require.” 🤖 Prompt for AI AgentsSource: Path instructions There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Relevant Go files ==='
fd -t f -e go . pkg/cluster | sort
printf '%s\n' '=== Production file outline ==='
ast-grep outline pkg/cluster/config.go --items all --type function,interface,struct --match 'NewConfig|Config|config' || true
printf '%s\n' '=== Relevant test symbols and assertions ==='
rg -n -C 4 -g '*_test.go' \
'NewConfig\s*\(|APIServers\(\)\.Get\s*\(|TLSProfileSpec\s*\(|TLSAdherencePolicy\s*\(|GetTLSProfileSpec\s*\(' \
pkg/cluster || true
printf '%s\n' '=== Current PR diff for cluster config and tests ==='
git diff --no-ext-diff --unified=60 'origin/main...HEAD' -- pkg/cluster/config.go pkg/cluster/*_test.go 2>/dev/null || \
git diff --no-ext-diff --unified=60 -- pkg/cluster/config.go pkg/cluster/*_test.goLength of output: 1814 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Test file outline ==='
ast-grep outline pkg/cluster/config_test.go --items all --type function --view expanded
printf '%s\n' '=== Test file content ==='
cat -n pkg/cluster/config_test.go
printf '%s\n' '=== Repository state ==='
git status --short
printf '%s\n' '=== HEAD metadata ==='
git show -s --format='%H%n%s%n%D' HEAD
printf '%s\n' '=== Changed paths relative to HEAD parent ==='
git diff-tree --no-commit-id --name-status -r HEADLength of output: 12324
The finding remains valid. Please add table-driven tests that cover:
This path configures the metrics server TLS behavior at operator startup. You are interacting with an AI system.
Member
Author
There was a problem hiding this comment. updated added tests. There was a problem hiding this comment.
Member
Author
There was a problem hiding this comment. updated, added tests |
||
| }, nil | ||
| } | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider bringing the final log entry down here to capture the final subset of cipher suites that will be used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated