SECURESIGN-4993: Add TLS profile resolver for cluster-wide TLS policy - #2040
Conversation
9dd1985 to
cf82b05
Compare
cf82b05 to
c14ef18
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2040 +/- ##
==========================================
- Coverage 56.40% 56.37% -0.03%
==========================================
Files 269 269
Lines 15267 15339 +72
==========================================
+ Hits 8611 8648 +37
- Misses 5772 5803 +31
- Partials 884 888 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
96f64f4 to
17edac8
Compare
17edac8 to
7e7b4ed
Compare
osmman
left a comment
There was a problem hiding this comment.
Thanks for putting this together — before we merge, I'd like this reworked to build on existing OpenShift tooling instead of re-implementing TLS profile resolution and crypto/cipher mapping from scratch.
There are already prepared libraries and examples from OpenShift that cover this ground:
- https://github.com/openshift/library-go/tree/master/pkg/crypto
- https://github.com/openshift/controller-runtime-common/tree/main/pkg/tls
- AI helper skill: https://github.com/openshift-eng/ai-helpers/blob/main/.claude-plugin/marketplace.json#L229
- Design doc: https://docs.google.com/document/d/1cMc9E8psHfnoK06ntR8kHSWB8d3rMtmldhnmM4nImjs/edit?tab=t.4cxmujrb3zyn
We're a controller-runtime based operator, so controller-runtime-common's pkg/tls package is directly applicable — for example, it already watches the cluster TLS profile config and terminates the operator to reload the new config on change, which overlaps with the bootstrap wiring in "Feature 2" here. library-go/pkg/crypto also already has the profile-to-cipher-suite mappings this PR re-derives in internal/utils/tls/profile.
Please rework this PR to build on top of these libraries rather than the custom resolver, and let's sync if any gaps remain that genuinely need net-new code.
|
@osmman Thank you for the direction. Marking this as draft. will iterate and open for review again. |
14365af to
7286829
Compare
7286829 to
4d36e34
Compare
3fb6385 to
b53bbe7
Compare
@osmman Thank you. The PR is reworked and ready for review now. Please consider the following points: Why distribution/distribution/v3 is pinned to v3.1.1? the version selected transitively resolves to a release carrying a known CVE. Pinning to v3.1.1 pulls in the fixed release. It's kept as an explicit pin (rather than letting MVS pick) because go mod tidy would otherwise silently downgrade it back to the vulnerable version. Snyk failure go-ntlmssp (Azure auth path via library-go): the flagged package (github.com/Azure/go-ntlmssp) is pulled in transitively through github.com/openshift/library-go's Azure authentication path. It is not reachable from our code. confirmed via go mod why ("main module does not need package") and it does not appear in the built binary (go version -m). There is no upstream fix version available, so it can't be resolved by bumping. Recommend suppressing it via a Snyk ignore rather than a dependency change, since a pin would have no effect on a non-reachable, no-fix-available transitive dependency. |
osmman
left a comment
There was a problem hiding this comment.
Can you please squash commits into single feat commit.
SECURESIGN-4993 On OpenShift, resolve the effective cluster TLS profile at startup from the APIServer config using openshift/controller-runtime-common/pkg/tls (FetchAPIServerTLSProfile / FetchAPIServerTLSAdherencePolicy) and apply it to the operator's own serving endpoints via NewTLSConfigFromProfile. - Run resolution under a cancellable context bounded by APIServerTimeout so a slow or unreachable API server aborts startup instead of hanging. - Add opt-out flag --disable-cluster-tls-profile / env DISABLE_CLUSTER_TLS_PROFILE (default false); non-OpenShift and IsNotFound/NoMatch cases fall back to the built-in Intermediate profile. - Wire ostls.SecurityProfileWatcher so the operator restarts and picks up cluster TLS profile changes. - Scope the informer cache: pin APIServer/Ingress informers to metadata.name=cluster and filter operator-owned objects by app.kubernetes.io/part-of. - Add config.openshift.io/apiservers get;list;watch RBAC (resourceNames=cluster) backed by a +kubebuilder:rbac marker. - Pin distribution/distribution/v3 to v3.1.1 via a replace directive so go mod tidy cannot downgrade back to the vulnerable version; bump docker/cli. - Add unit tests for resolveClusterTLSProfile.
b53bbe7 to
a5e73c2
Compare
Summary
Resolves the cluster-wide TLS profile at operator startup and applies it to the
operator's own serving endpoints (metrics / webhooks), so the operator honors the
platform TLS policy configured on OpenShift.
Jira: SECURESIGN-4993
What this does
the cluster
APIServerconfig usingopenshift/controller-runtime-common/pkg/tls(
FetchAPIServerTLSProfile/FetchAPIServerTLSAdherencePolicy), and builds theserving
tls.ConfigviaNewTLSConfigFromProfile.APIServerTimeout(flag
--apiserver-timeout/ envAPISERVER_TIMEOUT, default 30s), so a slow orunreachable API server aborts startup instead of hanging.
--disable-cluster-tls-profile/ envDISABLE_CLUSTER_TLS_PROFILE(defaultfalse) lets operators fall back to thebuilt-in Intermediate profile. Non-OpenShift clusters and
IsNotFound/NoMatchcases also fall back gracefully.ostls.SecurityProfileWatcheris wired in so the operator restarts when thecluster TLS profile changes, picking up the new policy on the next boot.
APIServerandIngressinformers to
metadata.name=cluster, and operator-owned objects are filtered bythe
app.kubernetes.io/part-oflabel.RBAC
Adds a
config.openshift.io/apiserversget;list;watchrule (scoped toresourceNames=cluster) backed by a+kubebuilder:rbacmarker ininternal/controller/types.gosomake manifestsregenerates it deterministically.Dependencies / security
github.com/openshift/controller-runtime-common.github.com/distribution/distribution/v3tov3.1.1via areplacedirective (durable against
go mod tidydowngrades) and bumpsdocker/clitoaddress transitive CVEs pulled in by the new dependency.
Tests
resolveClusterTLSProfilecovering OpenShift, non-OpenShift,disabled-flag, profile-fetch fallback, and adherence-fetch-error paths
(100% function coverage of the resolver).
Note on patch coverage
The overall patch-coverage number is dominated by
cmd/main.go, which is operatorbootstrap/wiring (manager construction, cache options, signal handling) that is
exercised by e2e rather than unit tests. The genuinely testable unit —
resolveClusterTLSProfile— is fully covered.Follow-up / out of scope
controller-runtime-common#22
and is intentionally not part of this PR.