chore: Authenticating api server against webhook #2359

JaydipGabani · 2022-10-25T22:59:03Z

Signed-off-by: Jaydip Gabani gabanijaydip@gmail.com

What this PR does / why we need it:
To authenticate incoming requests from api-server to webhook

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #1294

Special notes for your reviewer:

codecov-commenter · 2022-10-25T23:06:18Z

Codecov Report

Base: 53.58% // Head: 53.44% // Decreases project coverage by -0.14% ⚠️

Coverage data is based on head (109db1f) compared to base (4ec4f34).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2359      +/-   ##
==========================================
- Coverage   53.58%   53.44%   -0.15%     
==========================================
  Files         117      117              
  Lines       10229    10230       +1     
==========================================
- Hits         5481     5467      -14     
- Misses       4331     4343      +12     
- Partials      417      420       +3

Flag	Coverage Δ
unittests	`53.44% <0.00%> (-0.15%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/webhook/common.go	`59.32% <0.00%> (-7.99%)`	⬇️
pkg/webhook/mutation.go	`25.51% <0.00%> (+0.34%)`	⬆️
pkg/webhook/namespacelabel.go	`70.00% <0.00%> (+3.33%)`	⬆️
pkg/webhook/policy.go	`40.50% <0.00%> (+0.18%)`	⬆️
...onstrainttemplate/constrainttemplate_controller.go	`55.50% <0.00%> (-3.35%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

maxsmythe

Thank you for the PR! I'm glad to have the ground work for verifying the K8s API server.

One thought around decoupling this from disabling cert rotation.

maxsmythe · 2022-10-26T04:17:04Z

website/docs/externaldata.md

+
+To have requests coming from apiserver authenticated by webhook, following configuration can be made:
+
+1. Deploy gatekeeper with cert rotator disabled and with a client ca name. Provide name of the client ca with the flag `--client-cert-name`. The same name will be used to read certificate from the webhook secret. The webhook will only authenticate apiserver requests if both - cert rotator disabled and client ca name is provided with flag - of the criteria are met.


Is there a reason cert rotation needs to be disabled for this to work? Is there a way to make this orthogonal? Maybe store the cert in a separate secret/configmap (IIUI WRT secret vs. configmap the cert is not sensitive)

Interesting. Why can't we use the same secret? does cert rotator delete and creates new secret while rotating cert or does it just update the whole data field making it impossible to persist manually added entries to data map?

TBH I can't remember how cert-rotator behaves, but I wouldn't be surprised if it clobbered the whole data field.

In any case, if we could use the same secret I'd still like to enable/disable either option separately for operational flexibility.

Looks like cert rotator is not rewriting whole data map blindly, so we should be able to use the same secret

maxsmythe · 2022-10-26T04:18:55Z

pkg/webhook/mutation.go

@@ -90,6 +90,9 @@ func AddMutatingWebhook(mgr manager.Manager, deps Dependencies) error {
 	}
 	server := mgr.GetWebhookServer()
 	server.TLSMinVersion = *tlsMinVersion
+	if *DisableCertRotation && *clientCAName != "" {
+		server.ClientCAName = *clientCAName


We should also add this to the policy.go webhook file, in case the user has disabled the mutation webhook (or refactor the initialization pipeline so we set global server configurations in a single place).

Should we also add this to namespacelabel.go as well?

+1 good call

maxsmythe · 2022-10-26T04:25:17Z

charts/gatekeeper/values.yaml

@@ -145,6 +145,7 @@ controllerManager:
  priorityClassName: system-cluster-critical
  disableCertRotation: false
  tlsMinVersion: 1.3
+  certName: ""


No need to modify this value, it will get updated automatically on the next release.

maxsmythe · 2022-10-26T04:26:44Z

manifest_staging/charts/gatekeeper/values.yaml

@@ -145,6 +145,7 @@ controllerManager:
  priorityClassName: system-cluster-critical
  disableCertRotation: false
  tlsMinVersion: 1.3
+  certName: ""


This will need to be set here in order to prevent this value from being clobbered:

https://github.com/open-policy-agent/gatekeeper/blob/master/cmd/build/helmify/static/values.yaml

see https://open-policy-agent.github.io/gatekeeper/website/docs/help/#contributing-to-helm-chart for more info

sozercan · 2022-10-26T01:58:51Z

manifest_staging/charts/gatekeeper/values.yaml

@@ -145,6 +145,7 @@ controllerManager:
  priorityClassName: system-cluster-critical
  disableCertRotation: false
  tlsMinVersion: 1.3
+  certName: ""


let's call this

Suggested change

certName: ""

clientCAName: ""

sozercan · 2022-10-26T02:00:22Z

pkg/webhook/common.go

@@ -59,6 +59,8 @@ var (
 	emitAdmissionEvents                = flag.Bool("emit-admission-events", false, "(alpha) emit Kubernetes events in gatekeeper namespace for each admission violation")
 	tlsMinVersion                      = flag.String("tls-min-version", "1.3", "minimum version of TLS supported")
 	serviceaccount                     = fmt.Sprintf("system:serviceaccount:%s:%s", util.GetNamespace(), serviceAccountName)
+	clientCAName                       = flag.String("client-cert-name", "", "name of the client certificate to authenticate apiserver requrest against")


Suggested change

clientCAName = flag.String("client-cert-name", "", "name of the client certificate to authenticate apiserver requrest against")

clientCAName = flag.String("client-cert-name", "", "name of the client certificate to authenticate apiserver request against")

Good catch!

maxsmythe

Minor nits, but other than that LGTM!

maxsmythe · 2022-10-28T01:21:38Z

main.go

@@ -194,7 +193,7 @@ func main() {

 	// Make sure certs are generated and valid if cert rotation is enabled.
 	setupFinished := make(chan struct{})
-	if !*disableCertRotation {
+	if !*webhook.DisableCertRotation {


nit: I would leave this flag in main.go since that is where the value is consumed (and it looks like webhooks no longer use this value)

maxsmythe · 2022-10-28T01:23:14Z

website/docs/externaldata.md

+
+To have requests coming from apiserver authenticated by webhook, following configuration can be made:
+
+1. Deploy gatekeeper a client ca name. Provide name of the client ca with the flag `--client-cert-name`. The same name will be used to read certificate from the webhook secret. The webhook will only authenticate apiserver requests if both client ca name is provided with flag.


nit "ca" -> "CA"

maxsmythe · 2022-10-28T01:24:18Z

website/docs/externaldata.md

+      namespace: gatekeeper-system
+    type: Opaque
+    ```
+3. You will need to make sure that apiserver includes appropriate certificate while sending requests to apiserver, otherwise webhook will not accept these requests and will log error of `tls client didn't provide a certificate`. To make sure apiserver attaches correct certificate to requests being sent to webhook, you must specify the location of the admission control configuration file via the `--admission-control-config-file` flag while starting apiserver. Here is an example admission control configuration file


nit: add a colon (:) at the end of the line

sozercan · 2022-10-28T19:02:44Z

website/docs/externaldata.md

+    clusters:
+    - cluster:
+        certificate-authority-data: <clientca.crt> # same value as provided in gatekeeper webhook secret's clientca.crt 
+        server: https://gatekeeper-webhook-service.gatekeeper-system.svc:443


can we add note about service or namespace name and it should be updated if it's different

sozercan · 2022-10-28T20:22:24Z

website/docs/externaldata.md

+    metadata:
+      ...
+      name: gatekeeper-webhook-server-cert
+      namespace: gatekeeper-system


let's update here too re: name and namespace

sozercan · 2022-10-28T20:23:30Z

manifest_staging/charts/gatekeeper/values.yaml

@@ -145,6 +145,7 @@ controllerManager:
  priorityClassName: system-cluster-critical
  disableCertRotation: false
  tlsMinVersion: 1.3
+  clientCAName: ""


do we need to update the helm chart to set this value?

Yip! Missed that. Updating chart as well

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

sozercan

LGTM

maxsmythe

LGTM, thank you for the PR!

ritazh · 2022-11-01T00:27:59Z

website/docs/externaldata.md

+
+To have requests coming from apiserver authenticated by webhook, following configuration can be made:
+
+1. Deploy gatekeeper a client CA name. Provide name of the client ca with the flag `--client-cert-name`. The same name will be used to read certificate from the webhook secret. The webhook will only authenticate apiserver requests if both client ca name is provided with flag.


Suggested change

1. Deploy gatekeeper a client CA name. Provide name of the client ca with the flag `--client-cert-name`. The same name will be used to read certificate from the webhook secret. The webhook will only authenticate apiserver requests if both client ca name is provided with flag.

1. Deploy gatekeeper with a client CA cert name. Provide name of the client CA with the flag `--client-cert-name`. The same name will be used to read certificate from the webhook secret. The webhook will only authenticate apiserver requests if both client CA name is provided with flag.

if both client CA name is provided with flag

what do you mean by "both" here?

Ohh! "both" isn't supposed to be here.

ritazh · 2022-11-01T00:33:20Z

website/docs/externaldata.md

+      namespace: <gatekeeper-namespace>
+    type: Opaque
+    ```
+3. You will need to make sure that apiserver includes appropriate certificate while sending requests to apiserver, otherwise webhook will not accept these requests and will log error of `tls client didn't provide a certificate`. To make sure apiserver attaches correct certificate to requests being sent to webhook, you must specify the location of the admission control configuration file via the `--admission-control-config-file` flag while starting apiserver. Here is an example admission control configuration file:


Suggested change

3. You will need to make sure that apiserver includes appropriate certificate while sending requests to apiserver, otherwise webhook will not accept these requests and will log error of `tls client didn't provide a certificate`. To make sure apiserver attaches correct certificate to requests being sent to webhook, you must specify the location of the admission control configuration file via the `--admission-control-config-file` flag while starting apiserver. Here is an example admission control configuration file:

3. You will need to make sure that apiserver includes appropriate certificate while sending requests to the webhook, otherwise webhook will not accept these requests and will log error of `tls client didn't provide a certificate`. To make sure apiserver attaches correct certificate to requests being sent to webhook, you must specify the location of the admission control configuration file via the `--admission-control-config-file` flag while starting apiserver. Here is an example admission control configuration file:

ritazh · 2022-11-01T00:41:21Z

website/docs/externaldata.md

+
+### Authenticate API server against Webhook (Self managed kube cluster only)
+
+To have requests coming from apiserver authenticated by webhook, following configuration can be made:


Suggested change

To have requests coming from apiserver authenticated by webhook, following configuration can be made:

To ensure a request to the Gatekeeper webhook is coming from the api server, Gatekeeper needs to validate the client cert in the request. To enable authenticate api server, the following configuration can be made:

ritazh · 2022-11-01T00:45:43Z

To clarify, this issue addresses the first bullet of #1294 to do this manually. Might be good to add a comment that currently there is no way to do this automatically and for managed k8s.

To enable authenticate api server requires modifying cluster resources that may not be possible in managed K8s cluster

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

ritazh

LGTM

JaydipGabani force-pushed the master branch from a75eab3 to 8ac591c Compare October 25, 2022 22:59

maxsmythe requested changes Oct 26, 2022

View reviewed changes

maxsmythe reviewed Oct 26, 2022

View reviewed changes

JaydipGabani force-pushed the master branch from 8ac591c to bd72be5 Compare October 26, 2022 18:27

sozercan reviewed Oct 26, 2022

View reviewed changes

JaydipGabani force-pushed the master branch 5 times, most recently from f0def98 to edb9df1 Compare October 27, 2022 19:14

JaydipGabani requested review from sozercan and maxsmythe and removed request for sozercan October 27, 2022 19:15

maxsmythe reviewed Oct 28, 2022

View reviewed changes

JaydipGabani force-pushed the master branch from edb9df1 to 9214f12 Compare October 28, 2022 17:42

JaydipGabani requested a review from maxsmythe October 28, 2022 18:04

sozercan reviewed Oct 28, 2022

View reviewed changes

JaydipGabani force-pushed the master branch from 9214f12 to 223f0fc Compare October 28, 2022 19:52

sozercan reviewed Oct 28, 2022

View reviewed changes

JaydipGabani force-pushed the master branch from 223f0fc to 4c7a8c7 Compare October 28, 2022 23:24

chore: Authenticating api server against webhook

fc83bc1

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

JaydipGabani force-pushed the master branch from 4c7a8c7 to fc83bc1 Compare October 31, 2022 17:05

sozercan approved these changes Oct 31, 2022

View reviewed changes

maxsmythe approved these changes Oct 31, 2022

View reviewed changes

Merge branch 'master' into master

109db1f

ritazh reviewed Nov 1, 2022

View reviewed changes

JaydipGabani added 3 commits November 1, 2022 16:16

chore: Authenticating api server against webhook

a6913f2

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

Merge branch 'master' of https://github.com/JaydipGabani/gatekeeper

0adb7ff

fixing doc for api server authentication

4d38f5e

Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>

JaydipGabani force-pushed the master branch from 8925d4c to 4d38f5e Compare November 1, 2022 16:54

JaydipGabani requested a review from ritazh November 1, 2022 16:54

ritazh approved these changes Nov 1, 2022

View reviewed changes

ritazh merged commit 65f8245 into open-policy-agent:master Nov 1, 2022

ritazh mentioned this pull request Nov 1, 2022

Authenticate API Server for Gatekeeper webhook #1294

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Authenticating api server against webhook #2359

chore: Authenticating api server against webhook #2359

JaydipGabani commented Oct 25, 2022 •

edited by ritazh

Loading

codecov-commenter commented Oct 25, 2022 •

edited

Loading

maxsmythe left a comment

maxsmythe Oct 26, 2022

JaydipGabani Oct 26, 2022

maxsmythe Oct 27, 2022

JaydipGabani Oct 27, 2022

maxsmythe Oct 26, 2022

JaydipGabani Oct 26, 2022

maxsmythe Oct 27, 2022

maxsmythe Oct 26, 2022

maxsmythe Oct 26, 2022

sozercan Oct 26, 2022 •

edited

Loading

sozercan Oct 26, 2022

JaydipGabani Oct 26, 2022

maxsmythe left a comment

maxsmythe Oct 28, 2022

maxsmythe Oct 28, 2022

maxsmythe Oct 28, 2022

sozercan Oct 28, 2022

sozercan Oct 28, 2022

sozercan Oct 28, 2022

JaydipGabani Oct 28, 2022

sozercan left a comment

maxsmythe left a comment

ritazh Nov 1, 2022 •

edited

Loading

ritazh Nov 1, 2022

JaydipGabani Nov 1, 2022

ritazh Nov 1, 2022

ritazh Nov 1, 2022

ritazh commented Nov 1, 2022

ritazh left a comment


		To have requests coming from apiserver authenticated by webhook, following configuration can be made:

		1. Deploy gatekeeper with cert rotator disabled and with a client ca name. Provide name of the client ca with the flag `--client-cert-name`. The same name will be used to read certificate from the webhook secret. The webhook will only authenticate apiserver requests if both - cert rotator disabled and client ca name is provided with flag - of the criteria are met.

	clientCAName = flag.String("client-cert-name", "", "name of the client certificate to authenticate apiserver requrest against")
	clientCAName = flag.String("client-cert-name", "", "name of the client certificate to authenticate apiserver request against")


		To have requests coming from apiserver authenticated by webhook, following configuration can be made:

		1. Deploy gatekeeper a client ca name. Provide name of the client ca with the flag `--client-cert-name`. The same name will be used to read certificate from the webhook secret. The webhook will only authenticate apiserver requests if both client ca name is provided with flag.

	1. Deploy gatekeeper a client CA name. Provide name of the client ca with the flag `--client-cert-name`. The same name will be used to read certificate from the webhook secret. The webhook will only authenticate apiserver requests if both client ca name is provided with flag.
	1. Deploy gatekeeper with a client CA cert name. Provide name of the client CA with the flag `--client-cert-name`. The same name will be used to read certificate from the webhook secret. The webhook will only authenticate apiserver requests if both client CA name is provided with flag.


		### Authenticate API server against Webhook (Self managed kube cluster only)

		To have requests coming from apiserver authenticated by webhook, following configuration can be made:

	To have requests coming from apiserver authenticated by webhook, following configuration can be made:
	To ensure a request to the Gatekeeper webhook is coming from the api server, Gatekeeper needs to validate the client cert in the request. To enable authenticate api server, the following configuration can be made:

chore: Authenticating api server against webhook #2359

chore: Authenticating api server against webhook #2359

Conversation

JaydipGabani commented Oct 25, 2022 • edited by ritazh Loading

codecov-commenter commented Oct 25, 2022 • edited Loading

Codecov Report

maxsmythe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sozercan Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxsmythe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sozercan left a comment

Choose a reason for hiding this comment

maxsmythe left a comment

Choose a reason for hiding this comment

ritazh Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritazh commented Nov 1, 2022

ritazh left a comment

Choose a reason for hiding this comment

JaydipGabani commented Oct 25, 2022 •

edited by ritazh

Loading

codecov-commenter commented Oct 25, 2022 •

edited

Loading

sozercan Oct 26, 2022 •

edited

Loading

ritazh Nov 1, 2022 •

edited

Loading