Repository: knative/pkg
Pull request #2721 (2023-04-10 18:19:14)

Make minimum TLS version configurable for webhooks
<!-- Thanks for sending a pull request! -->

# Changes

- :gift: make TLS minimum version configurable via environment variables for webhooks
- changes the TLS minimum version to 1.3

/kind enhancement

TLS 1.3 comes with numerous enhancements, such as a quicker TLS handshake and more secure cipher suites, which make it simpler to use. Additionally, the introduction of Zero Round-Trip Time (0-RTT) key exchanges further simplifies the TLS handshake. 

**Release Note**

<!--
:page_facing_up: If this change has user-visible impact, write a release note in the block
below. Include the string "action required" if additional action is required of
users switching to the new release, for example in case of a breaking change.

Write as if you are speaking to users, not other Knative contributors. If this
change has no user-visible impact, no release-note is needed.
-->
```release-note
- Added an additional option when creating a webhook, which allows to configure the minimum TLS version acceptable to communicate with the API server.
- The default TLS version was also changed from TLS 1.2 to TLS 1.3.
```

**Docs**
N/A
<!--
:book: If this change has user-visible impact, link to an issue or PR in
https://github.com/knative/docs.

See here for guidance on getting permanent links to files: https://help.github.com/en/articles/getting-permanent-links-to-files

Please use the following format for linking documentation:
- [knative/docs]: <issue or pr link>
- [Feature Track]: <link>
- [Usage]: <link>
- [Other doc]: <link>
-->



knative-prow[bot]: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2721#" title="Author self-approved">izabelacg</a>*
**Once this PR has been reviewed and has the lgtm label**, please assign nak3 for approval. For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

<details open>
Needs approval from an approver in each of these files:

- **[webhook/OWNERS](https://github.com/knative/pkg/blob/main/webhook/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["nak3"]} -->
knative-prow[bot]: @izabelacg: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
unit-tests_pkg_main | 000410dcd69076843358c096508b86467effdeac | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/2721/unit-tests_pkg_main/1645492052458213376) | true | `/test unit-tests`

[Your PR dashboard](https://prow.knative.dev/pr).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
knative-prow[bot]: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2721#" title="Author self-approved">izabelacg</a>*
**Once this PR has been reviewed and has the lgtm label**, please assign lberk for approval. For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/knative/pkg/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["lberk"]} -->
izabelacg: /re-test

codecov[bot]: ## [Codecov](https://codecov.io/gh/knative/pkg/pull/2721?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) Report
Patch coverage: **`100.00`**% and project coverage change: **`+0.01`** :tada:
> Comparison is base [(`ee73c93`)](https://codecov.io/gh/knative/pkg/commit/ee73c9355c9d6d17bef0bb5403fa303758e07d58?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) 81.46% compared to head [(`fbc7554`)](https://codecov.io/gh/knative/pkg/pull/2721?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) 81.48%.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main    #2721      +/-   ##
==========================================
+ Coverage   81.46%   81.48%   +0.01%     
==========================================
  Files         163      163              
  Lines        9938     9948      +10     
==========================================
+ Hits         8096     8106      +10     
  Misses       1598     1598              
  Partials      244      244              
```


| [Impacted Files](https://codecov.io/gh/knative/pkg/pull/2721?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) | Coverage Δ | |
|---|---|---|
| [metrics/opencensus\_exporter.go](https://codecov.io/gh/knative/pkg/pull/2721?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-bWV0cmljcy9vcGVuY2Vuc3VzX2V4cG9ydGVyLmdv) | `67.21% <100.00%> (ø)` | |
| [webhook/env.go](https://codecov.io/gh/knative/pkg/pull/2721?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-d2ViaG9vay9lbnYuZ28=) | `100.00% <100.00%> (ø)` | |
| [webhook/webhook.go](https://codecov.io/gh/knative/pkg/pull/2721?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-d2ViaG9vay93ZWJob29rLmdv) | `75.00% <100.00%> (ø)` | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)

</details>

[:umbrella: View full report in Codecov by Sentry](https://codecov.io/gh/knative/pkg/pull/2721?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).

izabelacg: /retest
dprotaso: @ReToCode @nak3 do you have any qualms with bumping the min TLS version for our webhooks to 1.3?
dprotaso: Also @izabelacg any chance you can test this change in a WIP serving PR. Curious to see it run against a few k8s version.
nak3: > @ReToCode @nak3 do you have any qualms with bumping the min TLS version for our webhooks to 1.3?

We will confirm it once the PR was merged into serving but I have no qualm for now.
With said, could you share the context why bumping the default TLS minimum version to 1.3?
dprotaso: It's been stable since 2018

I found these informative (essentially there's a perf & security gain)
https://kinsta.com/blog/tls-1-3/
https://blog.cloudflare.com/rfc-8446-aka-tls-1-3/
nak3: Thank you. Okay, so it is not because TLS 1.2 has an issue/vulnerability but just gain the power of 1.3. I have no objection for it.
dprotaso: /lgtm
/approve
knative-prow[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2721#issuecomment-1508819243" title="Approved">dprotaso</a>*, *<a href="https://github.com/knative/pkg/pull/2721#" title="Author self-approved">izabelacg</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative/pkg/blob/main/OWNERS)~~ [dprotaso]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/metrics/opencensus_exporter.go b/metrics/opencensus_exporter.go
index eaeac4a56..59e33ab09 100644
--- a/metrics/opencensus_exporter.go
+++ b/metrics/opencensus_exporter.go
@@ -99,7 +99,7 @@ func getCredentials(component string, secret *corev1.Secret, logger *zap.Sugared
 		return nil
 	}
 	return credentials.NewTLS(&tls.Config{
-		MinVersion: tls.VersionTLS12,
+		MinVersion: tls.VersionTLS13,
 		GetClientCertificate: func(*tls.CertificateRequestInfo) (*tls.Certificate, error) {
 			cert, err := tls.X509KeyPair(secret.Data["client-cert.pem"], secret.Data["client-key.pem"])
 			if err != nil {
diff --git a/webhook/env.go b/webhook/env.go
index da8a1bd89..ffb757011 100644
--- a/webhook/env.go
+++ b/webhook/env.go
@@ -17,6 +17,7 @@ limitations under the License.
 package webhook
 
 import (
+	"crypto/tls"
 	"fmt"
 	"os"
 	"strconv"
@@ -29,6 +30,8 @@ const (
 	webhookNameEnvKey = "WEBHOOK_NAME"
 
 	secretNameEnvKey = "WEBHOOK_SECRET_NAME" //nolint:gosec // This is not a hardcoded credential
+
+	tlsMinVersionEnvKey = "WEBHOOK_TLS_MIN_VERSION"
 )
 
 // PortFromEnv returns the webhook port set by portEnvKey, or default port if env var is not set.
@@ -66,3 +69,16 @@ func SecretNameFromEnv(defaultSecretName string) string {
 	}
 	return secret
 }
+
+func TLSMinVersionFromEnv(defaultTLSMinVersion uint16) uint16 {
+	switch tlsMinVersion := os.Getenv(tlsMinVersionEnvKey); tlsMinVersion {
+	case "1.2":
+		return tls.VersionTLS12
+	case "1.3":
+		return tls.VersionTLS13
+	case "":
+		return defaultTLSMinVersion
+	default:
+		panic(fmt.Sprintf("the environment variable %q has to be either '1.2' or '1.3'", tlsMinVersionEnvKey))
+	}
+}
diff --git a/webhook/webhook.go b/webhook/webhook.go
index 6099e6ac5..779d388d2 100644
--- a/webhook/webhook.go
+++ b/webhook/webhook.go
@@ -40,6 +40,10 @@ import (
 
 // Options contains the configuration for the webhook
 type Options struct {
+	// TLSMinVersion contains the minimum TLS version that is acceptable to communicate with the API server.
+	// TLS 1.3 is the minimum version if not specified otherwise.
+	TLSMinVersion uint16
+
 	// ServiceName is the service name of the webhook.
 	ServiceName string
 
@@ -119,6 +123,13 @@ func New(
 		opts.StatsReporter = reporter
 	}
 
+	defaultTLSMinVersion := uint16(tls.VersionTLS13)
+	if opts.TLSMinVersion == 0 {
+		opts.TLSMinVersion = TLSMinVersionFromEnv(defaultTLSMinVersion)
+	} else if opts.TLSMinVersion != tls.VersionTLS12 && opts.TLSMinVersion != tls.VersionTLS13 {
+		return nil, fmt.Errorf("unsupported TLS version: %d", opts.TLSMinVersion)
+	}
+
 	syncCtx, cancel := context.WithCancel(context.Background())
 
 	webhook = &Webhook{
@@ -136,7 +147,7 @@ func New(
 		secretInformer := kubeinformerfactory.Get(ctx).Core().V1().Secrets()
 
 		webhook.tlsConfig = &tls.Config{
-			MinVersion: tls.VersionTLS12,
+			MinVersion: opts.TLSMinVersion,
 
 			// If we return (nil, error) the client sees - 'tls: internal error"
 			// If we return (nil, nil) the client sees - 'tls: no certificates configured'


Test patch:
diff --git a/network/transports_test.go b/network/transports_test.go
index 3cb58ebda..112219dd3 100644
--- a/network/transports_test.go
+++ b/network/transports_test.go
@@ -189,7 +189,7 @@ func exampleTLSConf() *tls.Config {
 	return &tls.Config{
 		InsecureSkipVerify: false,
 		ServerName:         "example.com",
-		MinVersion:         tls.VersionTLS12,
+		MinVersion:         tls.VersionTLS13,
 	}
 }
 
diff --git a/webhook/env_test.go b/webhook/env_test.go
index df5a0db9e..b60beb349 100644
--- a/webhook/env_test.go
+++ b/webhook/env_test.go
@@ -17,14 +17,16 @@ limitations under the License.
 package webhook
 
 import (
+	"crypto/tls"
 	"testing"
 )
 
 const (
 	testMissingInputName = "MissingInput"
 
-	testDefaultPort       = 8888
-	testDefaultSecretName = "webhook-certs"
+	testDefaultPort          = 8888
+	testDefaultSecretName    = "webhook-certs"
+	testDefaultTLSMinVersion = tls.VersionTLS13
 )
 
 type portTest struct {
@@ -48,6 +50,13 @@ type secretNameTest struct {
 	wantPanic bool
 }
 
+type tlsMinVersionTest struct {
+	name      string
+	in        string
+	want      uint16
+	wantPanic bool
+}
+
 func TestPort(t *testing.T) {
 	tests := []portTest{{
 		name: testMissingInputName,
@@ -164,3 +173,51 @@ func TestSecretName(t *testing.T) {
 		})
 	}
 }
+
+func TestTLSMinVersion(t *testing.T) {
+	tests := []tlsMinVersionTest{{
+		name: testMissingInputName,
+		want: testDefaultTLSMinVersion,
+	}, {
+		name: "EmptyInput",
+		in:   "",
+		want: testDefaultTLSMinVersion,
+	}, {
+		name:      "InvalidInputTrailingSpace",
+		in:        "1.2  ",
+		wantPanic: true,
+	}, {
+		name:      "InvalidInput",
+		in:        "1.0",
+		wantPanic: true,
+	}, {
+		name: "ValidInputTLS12",
+		in:   "1.2",
+		want: tls.VersionTLS12,
+	}, {
+		name: "ValidInputTLS13",
+		in:   "1.3",
+		want: tls.VersionTLS13,
+	}}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			// tlsMinVersionEnvKey is unset when testing missing input.
+			if tc.name != testMissingInputName {
+				t.Setenv(tlsMinVersionEnvKey, tc.in)
+			}
+
+			defer func() {
+				if r := recover(); r == nil && tc.wantPanic {
+					t.Error("Did not panic")
+				} else if r != nil && !tc.wantPanic {
+					t.Error("Got unexpected panic")
+				}
+			}()
+
+			if got := TLSMinVersionFromEnv(testDefaultTLSMinVersion); got != tc.want {
+				t.Errorf("TLSMinVersionFromEnv = %d, want: %d", got, tc.want)
+			}
+		})
+	}
+}
diff --git a/webhook/webhook_test.go b/webhook/webhook_test.go
index 8cd4bc03b..34c95d3fe 100644
--- a/webhook/webhook_test.go
+++ b/webhook/webhook_test.go
@@ -18,6 +18,7 @@ package webhook
 
 import (
 	"context"
+	"crypto/tls"
 	"fmt"
 	"net"
 	"testing"
@@ -93,3 +94,34 @@ func TestRegistrationStopChanFire(t *testing.T) {
 		t.Error("Unexpected success to dial to port", opts.Port)
 	}
 }
+
+func newAdmissionControllerWebhook(t *testing.T, options Options, acs ...interface{}) (*Webhook, error) {
+	ctx, cancel, _ := SetupFakeContextWithCancel(t)
+	defer cancel()
+	ctx = WithOptions(ctx, options)
+	return New(ctx, acs)
+}
+
+func TestTLSMinVersionWebhookOption(t *testing.T) {
+	opts := newDefaultOptions()
+	t.Run("when TLSMinVersion is not configured, and the default is used", func(t *testing.T) {
+		_, err := newAdmissionControllerWebhook(t, opts)
+		if err != nil {
+			t.Fatal("Unexpected error", err)
+		}
+	})
+	t.Run("when the TLS minimum version configured is supported", func(t *testing.T) {
+		opts.TLSMinVersion = tls.VersionTLS12
+		_, err := newAdmissionControllerWebhook(t, opts)
+		if err != nil {
+			t.Fatal("Unexpected error", err)
+		}
+	})
+	t.Run("when the TLS minimum version configured is not supported", func(t *testing.T) {
+		opts.TLSMinVersion = tls.VersionTLS11
+		_, err := newAdmissionControllerWebhook(t, opts)
+		if err == nil {
+			t.Fatal("Admission Controller Webhook creation expected to fail due to unsupported TLS version")
+		}
+	})
+}