Centralized TLS config by enarha · Pull Request #3225 · tektoncd/operator

enarha · 2026-02-17T14:44:12Z

Changes

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Run make test lint before submitting a PR
Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Release Notes

Add support for centralized TLS configuration on OpenShift. When
`enableCentralTLSConfig` is set to `true` in `TektonConfig.spec.platforms.openshift`,
Tekton components automatically inherit TLS settings (minimum version, cipher suites)
from the cluster's APIServer TLS security profile. Changes to the APIServer profile
are detected and propagated to components without manual intervention. TektonResult
is the first component to support this; other components will follow.

enarha · 2026-02-17T14:46:06Z

This is still missing documentation, maybe add few more tests, but in general it's tested working and ready for review.

jkhelil · 2026-02-18T05:50:01Z

@enarha Please add PR deescrtipion with changes, architecture diagram,

theakshaypant · 2026-02-18T07:49:56Z

@@ -0,0 +1,229 @@
+/*
+Copyright 2024 The Tekton Authors


Question: Should it not be 2026?

theakshaypant · 2026-02-18T07:53:19Z

@@ -0,0 +1,167 @@
+/*
+Copyright 2024 The Tekton Authors


jkhelil · 2026-02-18T06:02:05Z

@@ -0,0 +1,229 @@
+/*
+Copyright 2024 The Tekton Authors


s/2024/2026

jkhelil · 2026-02-18T06:02:24Z

@@ -0,0 +1,167 @@
+/*
+Copyright 2024 The Tekton Authors


s/2024/2026

jkhelil · 2026-02-18T06:25:46Z

+// GetTLSEnvVarsFromAPIServer fetches the TLS security profile from the OpenShift APIServer
+// resource and converts it to environment variable values that can be used by Tekton components.
+// Returns nil if the APIServer has no TLS profile configured or if the shared lister is not initialized.
+func GetTLSEnvVarsFromAPIServer(ctx context.Context, _ *rest.Config) (*TLSEnvVars, error) {


Can we return tls.config here from crypto/tls
We Could need other configration in the future
We could use then func TLSEnvVarsFromConfig(cfg *tls.Config) *TLSEnvVars to turn it to envvars

We can. But we'll need to do the conversation twice APIServer string -> tls.Config -> env var strings instead of APIServer string -> env var strings. If you think we can immediately use the tls.Config with a specific consumer in mind (webhooks, nginx or whatever), then I'll follow your suggestion. If we do not have specific case immediately waiting for that, then I'll suggest we add this when we need it.
Please let me know what do you think.

jkhelil · 2026-02-18T06:29:18Z

+	recorder := events.NewLoggingEventRecorder("tekton-operator")
+	observedConfig, errs := apiserver.ObserveTLSSecurityProfile(listers, recorder, existingConfig)
+	if len(errs) > 0 {
+		logger.Warnf("Errors observing TLS security profile: %v", errs)


should we return error here

Good question. It is a matter of policy - what do we want to happen in the case of a failure. I encountered two types of errors. In the first case we are able to read the APIServer, but failed to populate the map existingConfig with values. It was a bug in the way library-go handles this case in the older version we use, which I fixed, thus we have that two level check. The second case is we fail to read the APIServer resource. If we return an error and bubble it up, we'll break the reconciliation of the operands (e.g. TektonResult). The current behavior is to log a warning, degrade the TLS funcitonality (centralized TLS configuration does not have an effect), but operands continue working. The question is if we prefer that harder or softer form of failure. I believe with the hard failure, the Tekton administrator can still disable the centralized TLS configuration by disabling it through the TektonConfig and if that's the case I'm inclined to agree with a hard failure. WDYT?

jkhelil · 2026-02-18T06:29:44Z

+
+	// If no TLS configuration is present, return nil
+	if minVersion == "" && len(cipherSuites) == 0 {
+		return nil, nil


should we log and return error ?

jkhelil · 2026-02-18T07:27:07Z

+	return &TLSEnvVars{
+		MinVersion:       convertTLSVersionToEnvFormat(minVersion),
+		CipherSuites:     strings.Join(cipherSuites, ","),
+		CurvePreferences: "", // Will be populated once openshift/api#2583 is merged


can we mention this in PR description openshift/api#2583
because empty curve preferences means go defaults

jkhelil · 2026-02-18T07:36:55Z

+
+// convertTLSVersionToEnvFormat converts library-go TLS version format (VersionTLSxx) to
+// the format expected by Go's crypto/tls (1.x)
+func convertTLSVersionToEnvFormat(version string) string {


We should consider parse TLSVversion to go format, webhook for example are expecting this in go native constant , I suppose the same thing for go apis using tls
https://github.com/knative/pkg/blob/main/webhook/webhook.go#L53

A conversion in this format feels more suitable to error prone constant
func parseTLSVersion(version string) (uint16, error) {
switch version {
case "VersionTLS10", "TLSv1.0":
return tls.VersionTLS10, nil
case "VersionTLS11", "TLSv1.1":
return tls.VersionTLS11, nil
case "VersionTLS12", "TLSv1.2":
return tls.VersionTLS12, nil
case "VersionTLS13", "TLSv1.3":
return tls.VersionTLS13, nil
default:
return 0, fmt.Errorf("unknown TLS version: %s", version)
}
}

Switched to function which returns an error if unknown tls version is set through the apiserver record. Also split the retrieval of the tls configuration from the transformation. If we need to add more transformations in the future (e.g. to tsl.Config) we can do that without affecting existing code.

jkhelil · 2026-02-18T07:54:58Z

+	if oe.resolvedTLSConfig == nil {
+		return ""
+	}
+	return fmt.Sprintf("%s:%s:%s", oe.resolvedTLSConfig.MinVersion, oe.resolvedTLSConfig.CipherSuites, oe.resolvedTLSConfig.CurvePreferences)


can we use a real hash function for the fingerprint like sha256, the ":" thingy cna be present in ciphers ro curves

Ok first look, thaught that this function is calculating a hash, but hte hash is calculted in isntallerset code
mm I think the name is misleading here, could we called GetPlatformData rather than GetHash Data

jkhelil · 2026-02-18T07:59:18Z

 	}
 }
+
+// injectTLSConfig injects the TLS configuration as environment variables into the Results API deployment


this should be common i think

jkhelil · 2026-02-18T08:02:51Z

+	if err := setupAPIServerTLSWatch(ctx, ctrl); err != nil {
+		// On OpenShift clusters the APIServer resource should always exist.
+		// This env var is an escape hatch for edge cases and must be explicitly enabled.
+		if os.Getenv("SKIP_APISERVER_TLS_WATCH") == "true" {


may be move this to a constant

jkhelil · 2026-02-19T06:29:58Z

+	// and check with annotation, if they are same then we skip updating the object
 	// otherwise we update the manifest
-	specHash, err := hash.Compute(tr.Spec)
+	hashInput := struct {


This seems redudant and duplciate for all component
Can we have a common function that computes Hash give spec and common extension

Would make thing clear when redaing

I kept it per component for now. Each component has specific deployments to update. I considered a separate function which takes a resource type (e.g. deployment/statefulset) and resource object (e.g. tekton-results-api), but it requires any type params and extra import of the common package, so I think for now the advantages are not much bigger than the disadvantages.

jkhelil · 2026-02-19T06:42:41Z

+	if oe.resolvedTLSConfig == nil {
+		return ""
+	}
+	return fmt.Sprintf("%s:%s:%s", oe.resolvedTLSConfig.MinVersion, oe.resolvedTLSConfig.CipherSuites, oe.resolvedTLSConfig.CurvePreferences)


Ok first look, thaught that this function is calculating a hash, but hte hash is calculted in isntallerset code
mm I think the name is misleading here, could we called GetPlatformData rather than GetHash Data

jkhelil · 2026-03-02T07:34:50Z

@enarha can we add in this PR transformation for other components
we will need to inject tls env vars to pipeline,, pac, triggers,

jkhelil · 2026-03-02T06:07:20Z

+
+### 3. Extension Interface Enhancement
+
+- Added `GetHashData() string` method to the Extension interface


can you use the new function name GetPlatformData

jkhelil · 2026-03-02T06:14:07Z

+
+	// Wait for caches to sync
+	if !cache.WaitForCacheSync(ctx.Done(), apiServerInformer.Informer().HasSynced) {
+		logger.Warn("Failed to sync APIServer informer cache")


What happens if the cache is not synced (because of network issue or ai server not reachable), is the code safe on this ? We should probably retrigger a reconcile if cache is not synced, but that will be more complex as for now, let just verify code is safe nothing will panic just after, may be we need to add unit test

From what I see, nothing will panic if the cluster apiserver resource is not found in the cache. The reconciliation of the operand (e.g. tektonresult) will fail repeatedly though. There will be some very rapid reconcile retries with an exponential backoff. The same goes for the cache sync. The admin can still disable the central TLS configuration. If they opt-in into that feature, then I think louder failure without a panic makes sense. If you prefer we do not affect the operand reconciliation, then we can ignore that error and threat it like "no TLS config available" in the APIServer resource and just log some warning.

jkhelil · 2026-03-02T06:22:22Z

+func ResolveCentralTLSToEnvVars(ctx context.Context, lister TektonConfigLister) (*TLSEnvVars, error) {
+	tc, err := lister.Get(v1alpha1.ConfigResourceName)
+	if err != nil {
+		return nil, nil


Here we should propagate the error: failed to get tektonconfig

jkhelil · 2026-03-02T08:06:51Z

+	}
+
+	// TLS 1.3 cipher suite names (IANA names)
+	tls13Ciphers := map[string]bool{


should we add this
As per this https://go.dev/blog/tls-cipher-suites#:~:text=Go%20does%20allow%20configuring%20cipher,order%20by%20default%2C%20unless%20Config.

ciphers are not configrable. with tls 1.3, because they are considered as safe
Now the issue if we add them here, that may procude issues, right ?

The reason this was included in the first place is that the newer version of library-go does the same. They are commented out in the 2023 version, but in the newer ones they are not.
They are not configurable indeed and not required for golang based operands. But we have some non golang based dependencies like PostgreSQL and Nginx maybe more. I'm not sure how will these handle the 1.3 ciphers if we do not explicitly specify them. So that's one reason to include them. For Tekton Results we already have the code which handles the ciphers list and there the 1.3 ciphers are simply ignored. To me it makes more sense to keep these for now. WDYT?

Yes we can have the "ignore mecansim for our go apps" centralised, not just for results

jkhelil · 2026-03-02T08:19:05Z

@pratap0007 @anithapriyanatarajan @mbpavan Can you review please ?

enarha · 2026-03-09T10:54:01Z

/hold
Waiting for more reviews and after getting the required approvals I'll do another cycle of e2e testing, thus the hold.

enarha · 2026-03-23T19:30:01Z

@anithapriyanatarajan @mbpavan any chance you could have a final look to this or lgtm ?

kind reminder.

anithapriyanatarajan · 2026-03-30T17:09:12Z

@enarha - could you rebase this PR.

Add support for centralized TLS configuration derived from the OpenShift APIServer TLS security profile. This enables Tekton components to inherit cluster-wide TLS settings (minimum version, cipher suites) via the TektonConfig CR. Key changes: - Watch the APIServer resource for TLS profile changes and enqueue TektonConfig for reconciliation - Add GetTLSProfileFromAPIServer and TLSEnvVarsFromProfile to extract and convert TLS profiles to environment variables - Add ResolveCentralTLSToEnvVars and InjectTLSEnvVars as reusable helpers for component extensions - Add EnableCentralTLSConfig flag in TektonConfig OpenShift platform spec (opt-in, default false) - Add GetPlatformData to the Extension interface for platform-specific hash data (e.g., TLS config fingerprint) - Add RBAC permissions for reading the APIServer resource Note: curve preferences (CurvePreferences) are currently omitted and default to Go's standard library values until openshift/api#2583 is merged. Assisted-by: Cursor

Activate the centralized TLS configuration infrastructure for the TektonResult component: - Resolve TLS config from APIServer via ResolveCentralTLSToEnvVars in PreReconcile - Inject TLS env vars into the results-api deployment using the generic InjectTLSEnvVars transformer - Include TLS config fingerprint in GetPlatformData for installer set hash computation, triggering updates on TLS profile changes - Log injected TLS config at Info level for observability Assisted-by: Cursor

Testing of the central TLS functionality requires full OpenShift cluster (not ROSA, Hypershift) because it requires access to the APIServer resource. This change adds testing plan and easy way to run the required commands.

jkhelil · 2026-04-02T06:40:07Z

@anithapriyanatarajan @mbpavan any chance to have lgtm here ?

anithapriyanatarajan · 2026-04-09T05:41:16Z

/hold

anithapriyanatarajan · 2026-04-09T08:43:43Z

@enarha marked to be on hold to check the possibility of refactoring the implementation to be inclusive of plain kubernetes users as well. Basically to check the option to have tektonconfig as central to manage what TLS version and cipher suites to use. For Openshift, this gets populated by the API server CR watch and for Kubernetes, admins can manually configure as preferred. Thank you.

enarha · 2026-04-09T09:31:27Z

@enarha marked to be on hold to check the possibility of refactoring the implementation to be inclusive of plain kubernetes users as well. Basically to check the option to have tektonconfig as central to manage what TLS version and cipher suites to use. For Openshift, this gets populated by the API server CR watch and for Kubernetes, admins can manually configure as preferred. Thank you.

Personally I'm against such approach until we get a clean evidence such requirement exists. That will complicate implementation and maintenance with no clear benefit (YAGNI). To be honest, before implementing it that way, I researched the topic (Kubernetes way of setting application configuration cluster wide) and found nothing specific, so I decided to stick to the current requirements. That also goes beyond the scope of Tekton, so I do not see how we can influence Kubernetes even if add support for it. That's IMO.

anithapriyanatarajan · 2026-04-09T11:07:13Z

@enarha marked to be on hold to check the possibility of refactoring the implementation to be inclusive of plain kubernetes users as well. Basically to check the option to have tektonconfig as central to manage what TLS version and cipher suites to use. For Openshift, this gets populated by the API server CR watch and for Kubernetes, admins can manually configure as preferred. Thank you.

Personally I'm against such approach until we get a clean evidence such requirement exists. That will complicate implementation and maintenance with no clear benefit (YAGNI). To be honest, before implementing it that way, I researched the topic (Kubernetes way of setting application configuration cluster wide) and found nothing specific, so I decided to stick to the current requirements. That also goes beyond the scope of Tekton, so I do not see how we can influence Kubernetes even if add support for it. That's IMO.

In its current form the variables (TLS_MIN_VERSION, TLS_CIPHER_SUITES, TLS_CURVE_PREFERENCES) are directly read from API server and Injected to Envars. This is tightly coupled to openshift API. My request is to have this stored to the tektonconfig CR and mark it as populated manually / automated. This way an admin gets an opportunity to override if required and also for kuberenetes plain version, admins can set this manually as required. This way any other tekton components consumption model from Operator remains intact even if the source for reading the policy changes.

anithapriyanatarajan · 2026-04-09T13:27:39Z

/hold cancel

anithapriyanatarajan · 2026-04-09T13:28:06Z

/kind enhancement

tekton-robot · 2026-04-09T13:28:13Z

@anithapriyanatarajan: The label(s) kind/enhancement cannot be applied, because the repository doesn't have them.

Details

In response to this:

/kind enhancement

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/test-infra repository.

anithapriyanatarajan · 2026-04-09T13:29:05Z

/ok-to-test

anithapriyanatarajan · 2026-04-09T13:29:40Z

/kind feature

anithapriyanatarajan · 2026-04-09T13:33:31Z

@enarha - Thank you for the PR. Approving the changes limiting the scope of Cryptoagility to Openshift.

anithapriyanatarajan · 2026-04-09T13:33:42Z

/lgtm

tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesnt merit a release note. labels Feb 17, 2026

tekton-robot requested review from PuneetPunamiya and jkhelil February 17, 2026 14:44

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 17, 2026

enarha mentioned this pull request Feb 17, 2026

WIP: Inject tls policy results #3151

Closed

4 tasks

enarha force-pushed the central-tls-config branch 2 times, most recently from c641544 to bded46d Compare February 17, 2026 16:45

theakshaypant reviewed Feb 18, 2026

View reviewed changes

jkhelil reviewed Feb 18, 2026

View reviewed changes

enarha force-pushed the central-tls-config branch from bded46d to 2de7f83 Compare February 18, 2026 12:07

jkhelil reviewed Feb 19, 2026

View reviewed changes

enarha force-pushed the central-tls-config branch 2 times, most recently from b744aba to 63a684f Compare February 26, 2026 15:59

jkhelil reviewed Mar 2, 2026

View reviewed changes

enarha force-pushed the central-tls-config branch from 63a684f to 6137592 Compare March 2, 2026 18:58

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2026

enarha force-pushed the central-tls-config branch from 6137592 to 04296ed Compare March 9, 2026 10:52

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 9, 2026

enarha changed the title ~~WIP: Centralized TLS config~~ Centralized TLS config Mar 9, 2026

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 9, 2026

tekton-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 9, 2026

enarha force-pushed the central-tls-config branch 2 times, most recently from 6f72622 to 485b2e0 Compare March 15, 2026 17:35

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2026

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2026

enarha force-pushed the central-tls-config branch from 7dcd289 to 6eb7fbb Compare March 30, 2026 17:26

tekton-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 30, 2026

enarha added 3 commits April 1, 2026 22:33

Add testing plan for central TLS functionality

611010c

Testing of the central TLS functionality requires full OpenShift cluster (not ROSA, Hypershift) because it requires access to the APIServer resource. This change adds testing plan and easy way to run the required commands.

enarha force-pushed the central-tls-config branch from 6eb7fbb to 611010c Compare April 1, 2026 19:33

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 1, 2026

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 9, 2026

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 9, 2026

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 9, 2026

tekton-robot assigned anithapriyanatarajan Apr 9, 2026

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2026

tekton-robot merged commit fff9269 into tektoncd:main Apr 9, 2026
15 checks passed


		### 3. Extension Interface Enhancement

		- Added `GetHashData() string` method to the Extension interface

Conversation

enarha commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Submitter Checklist

Release Notes

Uh oh!

enarha commented Feb 17, 2026

Uh oh!

jkhelil commented Feb 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enarha Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkhelil commented Mar 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkhelil commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

enarha commented Mar 9, 2026

Uh oh!

enarha commented Mar 23, 2026

Uh oh!

anithapriyanatarajan commented Mar 30, 2026

Uh oh!

jkhelil commented Apr 2, 2026

Uh oh!

enarha commented Feb 17, 2026 •

edited

Loading

enarha Feb 24, 2026 •

edited

Loading

jkhelil commented Mar 2, 2026 •

edited

Loading

enarha commented Apr 9, 2026 •

edited

Loading