Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: cert rotator #488

Merged
merged 8 commits into from
Aug 26, 2024
Merged

feat: cert rotator #488

merged 8 commits into from
Aug 26, 2024

Conversation

fabriziosestito
Copy link
Contributor

@fabriziosestito fabriziosestito commented Jul 24, 2024

Description

Related to: kubewarden/kubewarden-controller#820, kubewarden/kubewarden-controller#7

Additional Information

Tradeoff

Potential improvement

Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall ok, I think there are small things to fix

charts/kubewarden-controller/templates/webhooks.yaml Outdated Show resolved Hide resolved
charts/kubewarden-controller/templates/webhooks.yaml Outdated Show resolved Hide resolved
charts/kubewarden-controller/values.yaml Outdated Show resolved Hide resolved
{{ $serverPrivateKey := ($cert.Key | b64enc) }}
# check if the secrets already exist and if so, use the existing values
{{ $caSecret := (lookup "v1" "Secret" .Release.Namespace "kubewarden-ca") }}
{{ if $caSecret }}
Copy link
Member

@viccuad viccuad Aug 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we add the genCA, genSignedCert and related variables as part of an else block of this if?
Variables aren't global and are indeed scoped, but we could create them empty, and only call the cert gen if needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I don't consider this a blocker for merging)

@fabriziosestito fabriziosestito changed the title wip: cert rotator and cert-manager removal feat: cert rotator and cert-manager removal Aug 8, 2024
charts/kubewarden-controller/values.yaml Outdated Show resolved Hide resolved
Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for having done the changes requested. There's one last question I have before approving the PR, see my comment

{{ $caCert = (index $caSecret.data "ca.crt") }}
{{ $caPrivateKey = (index $caSecret.data "ca.key") }}
{{ $oldCACert = (index $caSecret.data "old-ca.crt") }}
{{ $caBundle = printf "%s%s" ($caCert | b64dec) ($oldCACert | b64dec) | b64enc }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we have to put a \n here:

{{ $caBundle = printf "%s\n%s" ($caCert | b64dec) ($oldCACert | b64dec) | b64enc }}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not necessary. I've simulated an old certificate and install/upgrade the chart twice:

diff --git a/charts/kubewarden-controller/templates/webhooks.yaml b/charts/kubewarden-controller/templates/webhooks.yaml
index 0a37cde..5ae365f 100644
--- a/charts/kubewarden-controller/templates/webhooks.yaml
+++ b/charts/kubewarden-controller/templates/webhooks.yaml
@@ -1,9 +1,10 @@
 # generate certificates
 {{ $dnsName :=  printf "%s-webhook-service.%s.svc" (include "kubewarden-controller.fullname" .) .Release.Namespace }}
 {{ $ca := genCA "kubewarden-controller-ca" 365 }}
+{{ $oldCa := genCA "kubewarden-controller-ca" 365 }}
 {{ $cert := genSignedCert $dnsName nil ( list $dnsName ) 3650 $ca }}
 {{ $caCert := ($ca.Cert | b64enc) }}
-{{ $oldCaCert := "" }}
+{{ $oldCaCert := ($oldCa.Cert | b64enc) }}
 {{ $caBundle := $caCert }}
 {{ $caPrivateKey := ($ca.Key | b64enc) }}
 {{ $serverCert := ($cert.Cert | b64enc) }}

This is the bundle in the webhooks:

k8s get validatingwebhookconfiguration kubewarden-controller-validating-webhook-configuration -o json | jq -r ".webhooks[0].clientConfig.caBundle" | base64 -d -
-----BEGIN CERTIFICATE-----
MIIDMjCCAhqgAwIBAgIRAOE1t6L8fXP1Ck/1nn5IudQwDQYJKoZIhvcNAQELBQAw
IzEhMB8GA1UEAxMYa3ViZXdhcmRlbi1jb250cm9sbGVyLWNhMB4XDTI0MDgyMzE5
MTAwNFoXDTI1MDgyMzE5MTAwNFowIzEhMB8GA1UEAxMYa3ViZXdhcmRlbi1jb250
cm9sbGVyLWNhMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAlsef9LNJ
cZpgMyrlDnV1a6FE41j3gi3jA1Ud/JNBE9DDP7dSBXCBWwsAF4wOK6DFN5gbofcB
fyG08mDjFCWVyRE6QWtOkfIWAiusWQNQxvu3hGRxULbXCLzXfYRhCZln/VDPmLu4
z9EwYyYRWQ8r02Fg1jXDX5hkEUpcaS/G0+o3XyS2OvdZ2OcaEvSnXB9my5HZk39E
PW1GB4s9IMqK1kK/B5ymylm1ZBNFMouXUBn7y00P8ECM3YcOMdi8Kuook9nJ7b1D
H22bkTSRLZD2pd8UDv44+sBfPyHqyYerCzcQAj74fWNbUEW/vKV/6u0aiFRa9aJV
06hTmYDSwYyPwwIDAQABo2EwXzAOBgNVHQ8BAf8EBAMCAqQwHQYDVR0lBBYwFAYI
KwYBBQUHAwEGCCsGAQUFBwMCMA8GA1UdEwEB/wQFMAMBAf8wHQYDVR0OBBYEFFdX
SHwWpx5SNlL2hA4N9/WQPKLuMA0GCSqGSIb3DQEBCwUAA4IBAQAqWtNd+nLntgMZ
NjhSUn7VGQMzoyTdJ6Od12CQT9YwgN9Oil4cw/LZxulYa8M9E9jUmz15izQyWRiF
9YPc43D0DDJRPudaY49CTB702VzJc4iQA6nC/kwaTK/xkHMNUNyv/7/KXOOkZrm9
hdCitBSMfklXAVQwF42JlY6ap4B+DYHZSg3sAIkdsM09A7cFxPi3rEHZXgEwGFnd
OiK2nu2jPVUFqZr1xZZqdgup7PgWVc1p3HA3SV5ZYOEmylp/QIwI8++oxi0T8A7b
dHGedC5rFXUoCJ48dlQ2zRHrBhQafw3noSN8x9pIkMzYU7gNdulPUJWYUXha8gZa
JC0qGcsp
-----END CERTIFICATE-----
-----BEGIN CERTIFICATE-----
MIIDMjCCAhqgAwIBAgIRAOIMg0SuASwH/au/tk5IH+MwDQYJKoZIhvcNAQELBQAw
IzEhMB8GA1UEAxMYa3ViZXdhcmRlbi1jb250cm9sbGVyLWNhMB4XDTI0MDgyMzE5
MTAwNFoXDTI1MDgyMzE5MTAwNFowIzEhMB8GA1UEAxMYa3ViZXdhcmRlbi1jb250
cm9sbGVyLWNhMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAsNqejz7e
EerdolKXZ2tEOVxzCWdCUmt7y2qqm02SePFJlC28LI26PXOnbC2eqJZ04DP2OtFL
Pl8pweyYIRbrt1qYdmxV8LMuAo3pbygfV1huX0V9IUiQv2Qv9yQ00PXtchXUDANZ
451npHkGTQ3ruklv25p9QyLw22bWXAPlaQxPCKWmGiKhhKAG3heCtfioa4/oGKRa
EF3KRJuJi7qkvE0PIm2Xn1Vnua5vfgu7x9Qdt77gLwf7Dy+qfG0sTWJNr0mnExc7
cl1PwuAUBpOLBvBVO8M19qPsV4oOQTeyqpBpRmmVtn2rVcB2g4vDOtvt3fGNmmbI
mHepcQG5NztKewIDAQABo2EwXzAOBgNVHQ8BAf8EBAMCAqQwHQYDVR0lBBYwFAYI
KwYBBQUHAwEGCCsGAQUFBwMCMA8GA1UdEwEB/wQFMAMBAf8wHQYDVR0OBBYEFDzh
RPcE3X+v4UynawWuEQ4IVuDCMA0GCSqGSIb3DQEBCwUAA4IBAQAxHBODv3PlJz5j
CvF/VEKAXVjVIpCQiIHIi4kr5ZV3iLd3kjcZBS8BS9+WZ3KoacqntABDMAuNAy5/
wTbQG460saXB5uXDM/BNW88intqzRWv7k/MeXAVlAYJc5z+S62cTETYaK8tUxVUO
5u4wWADvh9ABXJMhZdW94X27qfuFLsySHJJxOvWSXTmjHstgdxhw3zcMsu9EQa0H
Hbz9J5zkkf7v5wFpiaDCG2EHqQRBay2pwRAluJwZu0wiryAldhhpNQi41NmBIr6s
eCeenHbIjimIE/vUHc47iKoqEwGTX/jr3ZSZ0iJswVkL/tM6WvtbiZQMK5INkIHh
dY4LZzCr
-----END CERTIFICATE-----

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

top! 💯

@flavio flavio modified the milestones: 1.16, 1.17 Aug 9, 2024
@jvanz jvanz self-assigned this Aug 23, 2024
fabriziosestito and others added 8 commits August 23, 2024 16:01
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
…ficate, inject them in the validating/mutating webhook

Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
The variable to store the old certificate variable is wrong. There was a
typo. This commit fixes that.

Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
@jvanz jvanz marked this pull request as ready for review August 23, 2024 19:25
@jvanz jvanz requested a review from a team as a code owner August 23, 2024 19:25
Copy link
Member

@viccuad viccuad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@viccuad viccuad merged commit de05eff into kubewarden:main Aug 26, 2024
3 checks passed
@viccuad viccuad changed the title feat: cert rotator and cert-manager removal feat: cert rotator Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants