-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat: cert rotator #488
Conversation
9d29d86
to
5972d90
Compare
There was a problem hiding this 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
{{ $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 }} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
6b762bf
to
02e5b36
Compare
There was a problem hiding this 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 }} |
There was a problem hiding this comment.
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 }}
There was a problem hiding this comment.
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-----
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
top! 💯
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description
Related to: kubewarden/kubewarden-controller#820, kubewarden/kubewarden-controller#7
Additional Information
Tradeoff
Potential improvement