Add TLSMaxVersion, TLSCipherSuites, and TLSCurvePreferences to webhook.Options for minimal tls customization #3300

jkhelil · 2025-12-09T18:10:59Z

This change adds minimal TLS configuration API for webhooks : TLSMaxVersion, TLSCipherSuites, and TLSCurvePreferences fields to webhook.Options for granular TLS control

TLSMaxVersion: enforce Modern profile
TLSCipherSuites: custom cipher suites
TLSCurvePreferences: elliptic curve configuration

Changes:

Add TLSMaxVersion, TLSCipherSuites, and TLSCurvePreferences fields to webhook.Options
Added unit tests for all new fields
Added documentation for webhook tls (README)
Maintains backward compatibility with existing TLSMinVersion usage

🎁 Add new feature
🐛 Fix bug

/kind enhancement

Fixes #3299

Release Note

Enhanced webhook TLS configuration with support for max version, custom cipher suites, and curve preferences.

Docs

linux-foundation-easycla · 2025-12-09T18:11:05Z

The committers listed above are authorized under a signed CLA.

✅ login: jkhelil / name: khelil (c6bbfb2)

knative-prow · 2025-12-09T18:11:08Z

Welcome @jkhelil! It looks like this is your first PR to knative/pkg 🎉

knative-prow · 2025-12-09T18:11:09Z

Hi @jkhelil. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

vdemeester · 2025-12-10T08:03:22Z

cc @dprotaso

twoGiants · 2025-12-15T12:55:10Z

/cc @twoGiants

twoGiants

Hi @jkhelil ! Thank you for the contribution 😸

I propose to go a more defensive approach or if we use the tls.Config to deprecate the current API. See my comments below.

twoGiants · 2025-12-15T13:59:36Z

webhook/webhook.go

+	// Note: When providing TLSConfig, ensure MinVersion is set appropriately for security.
+	// If MinVersion is not set in the provided TLSConfig, it will default to Go's default
+	// (currently TLS 1.2), which may be less secure than the webhook's default of TLS 1.3.
+	TLSConfig *tls.Config


First point I want to mention.

Not sure if providing access to the full tls.Config API is necessary. I prefer minimal API where possible, but I am not a TLS expert and maybe that is the call here BUT hear me out 😺 :

How about expanding the current API to support:

type Options struct { TLSMinVersion uint16 // Existing TLSMaxVersion uint16 // NEW - for Modern profile (TLS 1.3 only) TLSCipherSuites []uint16 // NEW TLSCurvePreferences []tls.CurveID // NEW TLSPreferServerCiphers bool // NEW - for Old profile }

Then all you mentioned in the issue would be covered. But for future needs/changes another code change would be needed => which I personally prefer over a bigger API.

Second point I want to mention.

If it is decided to go the route of keeping the current TLSMinVersion AND adding the full tls.Config like in the PR => I would prefer to deprecating the current TLSMinVersion and then switch entirely to the tls.Config. Then the min version validation for >= 1.2 must be done on the provided tls.Config so that the current functionality remains preserved. Deprecation comments then be added here and in the documentation and in the code and we probably need an issue so we don't forget to remove it in the next or later versions. Also I would then enforce the user to be able to use only one of the options => either set TLSMinVersion or tls.Config but not both.

What do the others say? @vdemeester @dprotaso @creydr @Cali0707

I tend to agree with @twoGiants here 👼🏼

@twoGiants Thank you for the thoughtful review. I appreciate your preference for a minimal API, and I share that hesitation.
I've observed two patterns in the ecosystem:

Full api: net/http exposes complete TLSConfig https://pkg.go.dev/net/http#Server

Selected fields: Kubernetes apiserver exposes specific fields https://github.com/kubernetes/apiserver/blob/master/pkg/server/options/serving.go#L66

After reflection, I'm leaning toward the minimal API approach for the following reasons:

We can keep the existing TLSMinVersion field without deprecation

Reduces breaking changes and migration burden

WEBHOOK_TLS_MIN_VERSION env var continues working as-is(prevents breaking TLS 1.2 clients)

Users won't accidentally override certificate loading logic

Smaller API surface to review and maintain

and it makes the pr much more simple

twoGiants · 2025-12-15T14:00:54Z

webhook/webhook.go

-				}
-				return &cert, nil
-			},
+			webhook.tlsConfig.GetCertificate = getCertificate


Maybe bug: Now if the user provides a GetCertificate function with his tls.Config it will be overwritten => this is not documented. Is this desired?

With the recent update, this is nolonger relevant

twoGiants · 2025-12-15T14:03:57Z

webhook/webhook.go

+		// Use custom TLSConfig if provided, otherwise create default config
+		if opts.TLSConfig != nil {
+			// Check if both custom TLSConfig and environment variable are set
+			if envTLSVersion := TLSMinVersionFromEnv(0); envTLSVersion != 0 {


With the new configuration the TLSMinVersion can remain unset or even be set to some old insecure versions. Currently this is not possible => only TLS 1.2 and 1.3 are allowed and the default is TLS 1.3. There is a warning but the setup continues => I would prefer to return an error if it's lower than 1.2.

And as mentioned above => I would prefer to use one way OR the other and enforce the user to do so too.

With the latest changes, this concern is no longer relevant.

twoGiants · 2025-12-15T14:04:24Z

webhook/webhook.go


+			// Warn if MinVersion is not set in the provided TLSConfig
+			if webhook.tlsConfig.MinVersion == 0 {
+				logger.Warnw("TLSConfig.MinVersion is not set, will use Go's default (TLS 1.2). Consider setting MinVersion explicitly for better security.",


The default is 1.3.

With the latest changes, this concern is no longer relevant.

twoGiants · 2025-12-15T14:20:55Z

webhook/webhook_test.go

 	})
 }
+
+func TestCustomTLSConfigWebhookOption(t *testing.T) {


Missing testcase for tls.Config without TLSMinVerision or a lower than 1.2 version => which should not be possible.

With the latest changes, this concern is no longer relevant.

dprotaso · 2025-12-16T18:21:35Z

/ok-to-test

dprotaso · 2025-12-16T18:27:32Z

Change looks good - just see the linter warnings about extra whitespace

codecov · 2025-12-16T18:27:52Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.59%. Comparing base (9cc8410) to head (c6bbfb2).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3300   +/-   ##
=======================================
  Coverage   74.58%   74.59%           
=======================================
  Files         188      188           
  Lines        8187     8190    +3     
=======================================
+ Hits         6106     6109    +3     
  Misses       1841     1841           
  Partials      240      240

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…k.Options for enhanced TLS control

dprotaso · 2025-12-17T21:39:10Z

/lgtm
/approve

knative-prow · 2025-12-17T21:39:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, jkhelil

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~webhook/OWNERS~~ [dprotaso]

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

knative-prow bot added the kind/enhancement label Dec 9, 2025

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 9, 2025

knative-prow bot requested review from creydr and skonto December 9, 2025 18:11

jkhelil force-pushed the webhook_tlsconfig branch from 5f6a041 to 4104846 Compare December 9, 2025 18:33

knative-prow bot requested a review from twoGiants December 15, 2025 12:55

twoGiants suggested changes Dec 15, 2025

View reviewed changes

knative-prow bot assigned twoGiants Dec 15, 2025

jkhelil force-pushed the webhook_tlsconfig branch 3 times, most recently from 29d8caa to f75549e Compare December 16, 2025 13:59

jkhelil changed the title ~~Add TLSConfig field to webhook.Options for full TLS customization~~ Add TLSMaxVersion, TLSCipherSuites, and TLSCurvePreferences fields to webhook.Options for minimal tls customization Dec 16, 2025

jkhelil changed the title ~~Add TLSMaxVersion, TLSCipherSuites, and TLSCurvePreferences fields to webhook.Options for minimal tls customization~~ Add TLSMaxVersion, TLSCipherSuites, and TLSCurvePreferences to webhook.Options for minimal tls customization Dec 16, 2025

jkhelil force-pushed the webhook_tlsconfig branch 2 times, most recently from 1fe3ad7 to 9819d5e Compare December 16, 2025 14:10

knative-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 16, 2025

Add TLSMaxVersion, TLSCipherSuites, and TLSCurvePreferences to webhoo…

c6bbfb2

…k.Options for enhanced TLS control

jkhelil force-pushed the webhook_tlsconfig branch from 9819d5e to c6bbfb2 Compare December 17, 2025 05:39

knative-prow bot assigned dprotaso Dec 17, 2025

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Dec 17, 2025

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 17, 2025

knative-prow bot merged commit 80c8bc4 into knative:main Dec 17, 2025
36 checks passed

Add TLSMaxVersion, TLSCipherSuites, and TLSCurvePreferences to webhook.Options for minimal tls customization #3300

Add TLSMaxVersion, TLSCipherSuites, and TLSCurvePreferences to webhook.Options for minimal tls customization #3300

Uh oh!

Conversation

jkhelil commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knative-prow bot commented Dec 9, 2025

Uh oh!

knative-prow bot commented Dec 9, 2025

Uh oh!

vdemeester commented Dec 10, 2025

Uh oh!

twoGiants commented Dec 15, 2025

Uh oh!

twoGiants left a comment

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 Dec 16, 2025 • 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!

jkhelil Dec 16, 2025 • 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!

jkhelil Dec 16, 2025 • 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!

dprotaso commented Dec 16, 2025

Uh oh!

dprotaso commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dprotaso commented Dec 17, 2025

Uh oh!

knative-prow bot commented Dec 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jkhelil commented Dec 9, 2025 •

edited

Loading

linux-foundation-easycla bot commented Dec 9, 2025 •

edited

Loading

jkhelil Dec 16, 2025 •

edited

Loading

jkhelil Dec 16, 2025 •

edited

Loading

jkhelil Dec 16, 2025 •

edited

Loading

dprotaso commented Dec 16, 2025 •

edited

Loading

codecov bot commented Dec 16, 2025 •

edited

Loading