Retrieve TLS config from apiserver for OpenShift 5.0

[dkwon17]

What does this PR do?

Configures operator webhook and metric server to apply the cluster TLS profile when tlsAdherence is set to StrictAllComponents. Watcher restarts the operator on profile or adherence policy changes.

Screenshot/screencast of this PR

What issues does this PR fix or reference?

How to test this PR?

I have personally tested this functionality on a OCP 5.0 cluster with techpreview enabled, with this operator image: quay.io/dkwon17/che-operator:tls-adherence.

1. Deploy the operator:

OpenShift

oc create namespace devworkspace-controller
oc apply --server-side -f https://raw.githubusercontent.com/devfile/devworkspace-operator/refs/heads/main/deploy/deployment/openshift/combined.yaml

OPERATOR_IMAGE=quay.io/dkwon17/che-operator:tls-adherence
sed 's|quay.io/eclipse/che-operator:next|'${OPERATOR_IMAGE}'|g' deploy/deployment/openshift/combined.yaml | oc apply --server-side -f  -
oc apply --server-side -f deploy/deployment/openshift/org_v2_checluster.yaml
oc wait checluster eclipse-che -n eclipse-che --for=jsonpath='.status.chePhase'=Active   --timeout=120s

For testing, I have followed this markdown: https://gist.github.com/dkwon17/71e6ede0f1f94f9fba8f57a21aa1e41d

on Minikube

./build/scripts/minikube-tests/test-operator-from-sources.sh

Common Test Scenarios

• Deploy Eclipse Che
• Start an empty workspace
• Open terminal and build/run an image
• Stop a workspace
• Check operator logs for reconciliation errors or infinite reconciliation loops

PR Checklist

As the author of this Pull Request I made sure that:

• The Eclipse Contributor Agreement is valid
• Code produced is complete
• Code builds without errors
• Tests are covering the bugfix
• The repository devfile is up to date and works
• Relevant user documentation updated
• Relevant contributing documentation updated
• CI/CD changes implemented, documented and communicated

Reviewers

Reviewers, please comment how you tested the PR when approving it.

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dkwon17

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[tolusha]

@dkwon17
Could you pls rebase on main to resolve conflicts

[tolusha]

Pls update clear-defined-test.sh with following changes:
ignored_paths:

  ["k8s.io/kube-aggregator"]="Harvesting is in progress"
  ["github.com/openshift/library-go"]="Harvesting is in progress"
  ["github.com/openshift/controller-runtime-common"]="Harvesting is in progress"
  ["github.com/openshift/client-go"]="Harvesting is in progress"
  ["github.com/coreos/go-oidc"]="Harvesting is in progress"

ignored_paths_licenses:

  # https://github.com/kubernetes/kube-aggregator?tab=Apache-2.0-1-ov-file
  ["k8s.io/kube-aggregator"]="Apache-2.0"
  # https://github.com/openshift/library-go?tab=Apache-2.0-1-ov-file
  ["github.com/openshift/library-go"]="Apache-2.0"
  # https://github.com/openshift/controller-runtime-common?tab=Apache-2.0-1-ov-file
  ["github.com/openshift/controller-runtime-common"]="Apache-2.0"
  # https://github.com/openshift/client-go?tab=Apache-2.0-1-ov-file
  ["github.com/openshift/client-go"]="Apache-2.0"
  # https://github.com/coreos/go-oidc?tab=Apache-2.0-1-ov-file
  ["github.com/coreos/go-oidc"]="Apache-2.0"

[tolusha]

Probably not needed, because of
https://github.com/dkwon17/che-operator/blob/a4e19ead9d324dce09f5ca400a5cb7eff3abd253/pkg/common/signal/signal_handler.go#L37

[dkwon17]

I think this is needed to get the cancel function for https://github.com/dkwon17/che-operator/blob/3b1addd70b5d150a15c21f4cf7b5011fb690c044/cmd/main.go#L328 which is used to restart the operator when there are TLS profile changes in the apiserver resource

[tolusha]

should simply return if there is unsupported ?

[dkwon17]

I think we can continue, since unsupported ciphers are filtered out in cipherCodes function in:

che-operator/vendor/github.com/openshift/controller-runtime-common/pkg/tls/tls.go

Lines 128 to 129 in 3b1addd

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

WDYT?

[tolusha]

@dkwon17 your reading of cipherCodes is right - it filters unsupported ciphers rather than erroring, so continuing is fine for a partially-supported profile.

The case that is not covered is when every cipher gets filtered out. cipherCodes then returns a nil slice, NewTLSConfigFromProfile assigns tlsConf.CipherSuites = nil, and Go silently falls back to its default cipher list - which may be broader than the profile the cluster demanded. So the one scenario where we most need to know something went wrong currently produces a single Info line and a config that quietly does not match policy.

Rather than returning early on any unsupported, would it work to keep going but escalate when nothing survives?

tlsConfigFn, unsupported := tlspkg.NewTLSConfigFromProfile(profile)
if len(unsupported) > 0 {
	log.Info("TLS profile contains ciphers unsupported by Go", "unsupported", unsupported)
}
if len(profile.Ciphers) > 0 && len(unsupported) == len(profile.Ciphers) {
	log.Error(nil, "no ciphers from the cluster TLS profile are supported by Go; server will use Go defaults, which may not satisfy tlsAdherence",
		"profileCiphers", profile.Ciphers)
}

That keeps the permissive behaviour you described for the common case and only raises the level when the result is genuinely non-compliant. It is only reachable via a Custom profile, so it is an edge case - but it is the edge case where silence is most expensive.

[tolusha]

/che-ai-assistant ok-pr-review

Task completed.

[tolusha]

Reviewed the full non-vendor diff, and read the vendored controller-runtime-common/pkg/tls and library-go/pkg/crypto sources to verify the integration contracts rather than infer them.

The webhook path is well built and the reuse of controller-runtime-common is the right call. The main blockers are the metrics claim (see the reply on @rohanKanojia's thread) and, for a security feature, the silent fail-open with no recovery (see pkg/deploy/tls/server_tls.go:41).

Things this gets right, worth recording because several are traps that are easy to fall into:

• configv1.APIServer is deliberately absent from the ByObject selectors in getCacheFunc, so the part-of=che.eclipse.org label filter does not blind the informer.
• BuildServerTLSOptions and the upstream Reconcile both derive the spec through GetTLSProfileSpec, so reflect.DeepEqual at controller.go:137 cannot fire a spurious restart on the first reconcile.
• The RBAC split (get + resourceNames for the direct client, unscoped list/watch for the informer) is the tightest grant Kubernetes allows, read-only, on a dedicated ServiceAccount. All six generated manifests are in sync, and adding OpenShift rules to the Kubernetes ClusterRole matches pre-existing generator output.
• NeedLeaderElection: false upstream means every replica reloads - correct for a process-wide concern rather than a reconcile concern.
• IsOpenShift()'s lazy initializeIfNeeded() makes the early call site at main.go:226 safe despite running before the discovery client is built.
• Reusing controller-runtime-common avoids the TLS 1.3 "cipher suites are not configurable in Go" subtlety, which is easy to get wrong by hand.
• Commits are cleanly separated by intent.

Smaller notes that did not seem worth their own threads:

• cmd/main.go:73 - the tls alias is redundant; the package is already named tls and crypto/tls is not imported in main.go.
• pkg/deploy/tls/server_tls.go:81 - logging the full cipher list at Info on every startup is verbose; V(1) might suit it better. MinTLSVersion at Info is useful.
• RegisterSecurityProfileWatcher returns nil when it skips registration, so the caller cannot distinguish "watcher installed" from "watcher skipped because the fetch failed". One log.Info on that branch would close the gap.
• There is no metric, event, or status condition reporting whether the cluster profile is being honored. Combined with the fail-open path, a degraded operator is indistinguishable from a healthy one.
• Worth a release note: honoring an Old cluster profile sets MinVersion: VersionTLS10 on the webhook, which is lower than what we serve today. That is the correct semantics of "honor the cluster profile", but the PR title reads as hardening only.

Verified as already handled, so no action needed: the clear-defined-test.sh entries are correct and not stale (kube-aggregator, client-go and go-oidc are absent from vendor/ but do appear in go list -m -mod=mod all, which line 20 of the script uses); the context.WithCancel wrapper at main.go:325 is necessary as @dkwon17 explained, since SetupSignalHandler keeps its cancel private; and the empty-TLSOpts no-op behaviour checks out against both vendored loops.

The PR also currently carries needs-rebase and a failing ci/prow/v19-devworkspace-happy-path.

[tolusha]

libgocrypto.ShouldHonorClusterTLSProfile is identical to this - same switch, same cases, same forward-compatible default: return true. It is already in the tree as of this PR, at vendor/github.com/openshift/library-go/pkg/crypto/tls_adherence.go:16.

The upstream API type asks implementors to use it rather than reimplement:

// vendor/github.com/openshift/api/config/v1/types_apiserver.go:273-275
// TLSAdherencePolicy defines which components adhere to the TLS security profile.
// Implementors should use the ShouldHonorClusterTLSProfile helper function from library-go
// rather than checking these values directly.

Dropping the local copy keeps us aligned if OpenShift ever changes the semantics of a policy value. It needs a go mod tidy to promote library-go from // indirect to a direct require, and the call sites become:

import libgocrypto "github.com/openshift/library-go/pkg/crypto"

if libgocrypto.ShouldHonorClusterTLSProfile(adherence) {

[dkwon17]

cmd/main.go:73 - the tls alias is redundant; the package is already named tls and crypto/tls is not imported in main.go.

pkg/deploy/tls/server_tls.go:81 - logging the full cipher list at Info on every startup is verbose; V(1) might suit it better. MinTLSVersion at Info is useful.

RegisterSecurityProfileWatcher returns nil when it skips registration, so the caller cannot distinguish "watcher installed" from "watcher skipped because the fetch failed". One log.Info on that branch would close the gap.

All three have been fixed in a5888cb

[openshift-ci]

@dkwon17: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/v19-devworkspace-happy-path	b1dbe2c	link	true	/test v19-devworkspace-happy-path
ci/prow/v19-upgrade-stable-to-next	b1dbe2c	link	true	/test v19-upgrade-stable-to-next

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[dkwon17]

Closed in favour of #2203