feat: certificate rotation #829

fabriziosestito · 2024-07-26T08:43:17Z

Description

This PR introduces a new runnable (CertController) that rotates CA root and leaf certificates:

CA root rotation: see Rotate CA root #818
Server certificate rotation: see Rotate TLS certificates #819

It removes the cert-manager dependency.

NOTE: this PR requires helm chart changes, see: kubewarden/helm-charts#488

TODO:

remove cert-manager from the kustomize configurations.
use some certificate generation settings as helm (see: https://github.com/Masterminds/sprig/blob/master/crypto.go, sprig is the library used by helm to add genCA and genSignedCert to the templates)

Testing

The cert controller has been tested by using gingko/envtest.

Additional information

Part-of label

Before these changes, we used the kubewarden: true label in the webhook configuration, to filter kubewarden resources.
However, using the recommended label app.kubernetes is better.io/part-of is more idiomatic.
Also, the label is already set by the helm chart on the controller's webhook configurations.
Unfortunately, we need to support both labels to be compatible with the older versions.

Injecting the CA bundle

There was the need to use the retry.RetryOnConflict utility when injecting the caBundle in the validating/mutating webhook configuration since there could be a chance to have a conflict with the neighbor controllers (namely the policy controllers).
(Cluster)AdmissionPolicyController will eventually converge to the new bundle, since it reads the secret (from the cache) every time it restores the webhook configuration.
Instead, the CertController reconciliation loop is ticker-based, so we cannot wait for the next tick to reconcile again. Therefore retrying with a certain backoff is a good strategy in this situation. See: kubernetes-sigs/controller-runtime#1748

Fixes #819, fixes #818

codecov · 2024-07-26T08:46:02Z

Codecov Report

Attention: Patch coverage is 61.39706% with 105 lines in your changes missing coverage. Please review.

Project coverage is 70.14%. Comparing base (aa0a426) to head (4146066).
Report is 14 commits behind head on main.

Files	Patch %	Lines
internal/controller/cert_controller.go	53.52%	42 Missing and 24 partials ⚠️
internal/certs/certs.go	72.85%	10 Missing and 9 partials ⚠️
internal/certs/secrets.go	33.33%	8 Missing and 8 partials ⚠️
.../controller/policyserver_controller_cert_secret.go	86.36%	1 Missing and 2 partials ⚠️
internal/controller/policy_subreconciler.go	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #829      +/-   ##
==========================================
- Coverage   70.96%   70.14%   -0.83%     
==========================================
  Files          27       29       +2     
  Lines        2435     2599     +164     
==========================================
+ Hits         1728     1823      +95     
- Misses        562      601      +39     
- Partials      145      175      +30

Flag	Coverage Δ
integration-tests	`61.04% <61.39%> (-0.22%)`	⬇️
unit-tests	`35.92% <0.00%> (-3.03%)`	⬇️

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

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

flavio

Looks really good, I like it! 👏

I left some minor comments

cmd/main.go

internal/certs/certs.go

internal/certs/secrets.go

internal/controller/cert_controller.go

flavio · 2024-08-05T13:28:11Z

internal/controller/cert_controller.go

+		client.MatchingLabels{
+			"app.kubernetes.io/part-of":   "kubewarden",
+			"app.kubernetes.io/component": "policy-server",
+		},


Also here, I think we're not using these labels right now (KW 1.16). I haven't finished to review the whole PR, I guess that if this PR changes the PolicyServer reconciler to add these labels everything will be fine.

good catch, we are using those for the default policy server: https://github.com/kubewarden/helm-charts/blob/735ccfd74a44eca62f4f35093877f6197221913b/charts/kubewarden-defaults/templates/policyserver-default.yaml#L6 but they are not reconciled

internal/controller/cert_controller_test.go

internal/controller/admissionpolicy_controller.go

internal/controller/cert_controller.go

viccuad

LGTM! great work!

internal/controller/clusteradmissionpolicy_controller.go

jvanz

LGTM. Thanks for the great work!

flavio

LGTM, thanks for the hard work!

jvanz · 2024-08-08T14:17:11Z

Congrats! 🚀

Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>

…ollers Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>

Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>

In the recent root CA controller change we changed the labels used in the controller. Therefore, the policy groups policies tests need to be updated to make them work in the latest controller. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>

jvanz · 2024-08-23T11:55:04Z

@kubewarden/kubewarden-developers I've rebased this PR on top of the latest main branch. I think that after tests passes, we can merge it.

viccuad · 2024-08-23T12:33:29Z

Thanks! everything green, merging 🚀

viccuad · 2024-08-23T12:40:34Z

Moved the TODO tasks on this PR to the certs epic #7.

fabriziosestito self-assigned this Jul 26, 2024

fabriziosestito force-pushed the feat/server-cert-rotation branch 3 times, most recently from 6372337 to 787677e Compare July 26, 2024 11:25

fabriziosestito added this to the 1.16 milestone Jul 26, 2024

fabriziosestito force-pushed the feat/server-cert-rotation branch 3 times, most recently from a24541a to dd5fc7a Compare July 29, 2024 12:53

fabriziosestito mentioned this pull request Jul 29, 2024

Handle certificate rotation #7

Closed

13 tasks

fabriziosestito force-pushed the feat/server-cert-rotation branch 2 times, most recently from 225d8c8 to fb6a2cf Compare August 1, 2024 06:50

fabriziosestito changed the title ~~wip: server cert rotation~~ feat: certificate rotation Aug 1, 2024

github-actions bot added the kind/feature label Aug 1, 2024

fabriziosestito force-pushed the feat/server-cert-rotation branch 2 times, most recently from 2146a33 to 43be290 Compare August 1, 2024 07:31

fabriziosestito marked this pull request as ready for review August 1, 2024 07:32

fabriziosestito requested a review from a team as a code owner August 1, 2024 07:32

fabriziosestito force-pushed the feat/server-cert-rotation branch from 43be290 to 4731544 Compare August 1, 2024 09:40

flavio reviewed Aug 5, 2024

View reviewed changes

viccuad reviewed Aug 6, 2024

View reviewed changes

internal/controller/admissionpolicy_controller.go Outdated Show resolved Hide resolved

viccuad reviewed Aug 6, 2024

View reviewed changes

internal/controller/cert_controller.go Show resolved Hide resolved

viccuad approved these changes Aug 6, 2024

View reviewed changes

jvanz reviewed Aug 7, 2024

View reviewed changes

internal/controller/clusteradmissionpolicy_controller.go Outdated Show resolved Hide resolved

jvanz approved these changes Aug 7, 2024

View reviewed changes

fabriziosestito force-pushed the feat/server-cert-rotation branch from 8460ed6 to 382e7ca Compare August 8, 2024 11:37

fabriziosestito requested a review from flavio August 8, 2024 11:37

fabriziosestito force-pushed the feat/server-cert-rotation branch from 382e7ca to 28a7495 Compare August 8, 2024 13:21

flavio approved these changes Aug 8, 2024

View reviewed changes

jvanz approved these changes Aug 8, 2024

View reviewed changes

flavio modified the milestones: 1.16, 1.17 Aug 9, 2024

jvanz self-assigned this Aug 23, 2024

fabriziosestito and others added 13 commits August 23, 2024 08:31

feat: update and clean-up cert utilities

ea5d6e5

Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>

feat: add cert secrets utils

1f582e1

Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>

feat: add and updates cert-related constants

897f9ae

Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>

feat: add CertController

10251dc

Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>

feat: use part-of label in policy controllers

f4c6158

Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>

refactor: use new secrets, constants and cert utils in existing contr…

fb87ad7

…ollers Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>

test: update controller tests

721e915

Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>

feat: wire-up CertController

c6c6a50

Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>

chore: remove cert-manager installation from Tiltfile

bf6efd9

Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>

fix: reconcile policy server secret labels

18f1e8e

Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>

chore: use constants for part-of and component labels

c27644a

Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>

chore: rename ok -> found

ccc1229

Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>

jvanz force-pushed the feat/server-cert-rotation branch from 28a7495 to 4146066 Compare August 23, 2024 11:54

viccuad merged commit 97e1e69 into kubewarden:main Aug 23, 2024
8 of 9 checks passed

fabriziosestito mentioned this pull request Sep 3, 2024

Update certificate handling RFC kubewarden/rfc#39

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: certificate rotation #829

feat: certificate rotation #829

fabriziosestito commented Jul 26, 2024 •

edited

Loading

codecov bot commented Jul 26, 2024 •

edited

Loading

flavio left a comment

flavio Aug 5, 2024

fabriziosestito Aug 5, 2024

viccuad left a comment

jvanz left a comment

flavio left a comment

jvanz commented Aug 8, 2024

jvanz commented Aug 23, 2024

viccuad commented Aug 23, 2024

viccuad commented Aug 23, 2024

feat: certificate rotation #829

feat: certificate rotation #829

Conversation

fabriziosestito commented Jul 26, 2024 • edited Loading

Description

Testing

Additional information

Part-of label

Injecting the CA bundle

codecov bot commented Jul 26, 2024 • edited Loading

Codecov Report

flavio left a comment

Choose a reason for hiding this comment

flavio Aug 5, 2024

Choose a reason for hiding this comment

fabriziosestito Aug 5, 2024

Choose a reason for hiding this comment

viccuad left a comment

Choose a reason for hiding this comment

jvanz left a comment

Choose a reason for hiding this comment

flavio left a comment

Choose a reason for hiding this comment

jvanz commented Aug 8, 2024

jvanz commented Aug 23, 2024

viccuad commented Aug 23, 2024

viccuad commented Aug 23, 2024

fabriziosestito commented Jul 26, 2024 •

edited

Loading

codecov bot commented Jul 26, 2024 •

edited

Loading