From edb7087b348c6479a634b191ee11253f9dc10090 Mon Sep 17 00:00:00 2001 From: Martin Weindel Date: Thu, 21 Mar 2024 16:52:07 +0100 Subject: [PATCH] addressed feedback from review --- README.md | 2 +- .../cert.gardener.cloud_certificates.yaml | 7 +++ .../cert.gardener.cloud_certificates.yaml | 7 +++ pkg/apis/cert/crds/zz_generated_crds.go | 7 +++ pkg/apis/cert/v1alpha1/types.go | 8 +++- .../cert/v1alpha1/zz_generated.deepcopy.go | 12 ++++- pkg/cert/legobridge/certificate.go | 45 ++++++++++++------- pkg/cert/legobridge/certificate_test.go | 30 ++++++++----- pkg/cert/source/reconciler.go | 40 ++++++++--------- 9 files changed, 105 insertions(+), 53 deletions(-) diff --git a/README.md b/README.md index d9ad10fe7..aa9f9573f 100644 --- a/README.md +++ b/README.md @@ -356,7 +356,7 @@ In this case the secret `my-secret` will contains the labels. ### Specifying private key algorithm and size -By default, the certificate uses `RSA` with a key size of 2048 bits for the private key +By default, the certificate uses `RSA` with a key size of 2048 bits for the private key. Add the `privateKey` section to specify private key algorithm and/or size. Example: diff --git a/charts/cert-management/templates/cert.gardener.cloud_certificates.yaml b/charts/cert-management/templates/cert.gardener.cloud_certificates.yaml index fadc52a40..aa729a42c 100644 --- a/charts/cert-management/templates/cert.gardener.cloud_certificates.yaml +++ b/charts/cert-management/templates/cert.gardener.cloud_certificates.yaml @@ -204,6 +204,13 @@ spec: to `2048` if not specified. If `algorithm` is set to `ECDSA`, valid values are `256` or `384`, and will default to `256` if not specified. No other values are allowed." + enum: + - 256 + - 384 + - 2048 + - 3072 + - 4096 + format: int32 type: integer type: object renew: diff --git a/pkg/apis/cert/crds/cert.gardener.cloud_certificates.yaml b/pkg/apis/cert/crds/cert.gardener.cloud_certificates.yaml index 606b72c3b..93df5ccba 100644 --- a/pkg/apis/cert/crds/cert.gardener.cloud_certificates.yaml +++ b/pkg/apis/cert/crds/cert.gardener.cloud_certificates.yaml @@ -199,6 +199,13 @@ spec: to `2048` if not specified. If `algorithm` is set to `ECDSA`, valid values are `256` or `384`, and will default to `256` if not specified. No other values are allowed." + enum: + - 256 + - 384 + - 2048 + - 3072 + - 4096 + format: int32 type: integer type: object renew: diff --git a/pkg/apis/cert/crds/zz_generated_crds.go b/pkg/apis/cert/crds/zz_generated_crds.go index 3e0065c99..3fe02856d 100644 --- a/pkg/apis/cert/crds/zz_generated_crds.go +++ b/pkg/apis/cert/crds/zz_generated_crds.go @@ -499,6 +499,13 @@ spec: to ` + "`" + `2048` + "`" + ` if not specified. If ` + "`" + `algorithm` + "`" + ` is set to ` + "`" + `ECDSA` + "`" + `, valid values are ` + "`" + `256` + "`" + ` or ` + "`" + `384` + "`" + `, and will default to ` + "`" + `256` + "`" + ` if not specified. No other values are allowed." + enum: + - 256 + - 384 + - 2048 + - 3072 + - 4096 + format: int32 type: integer type: object renew: diff --git a/pkg/apis/cert/v1alpha1/types.go b/pkg/apis/cert/v1alpha1/types.go index 99eff592f..34056755b 100644 --- a/pkg/apis/cert/v1alpha1/types.go +++ b/pkg/apis/cert/v1alpha1/types.go @@ -108,6 +108,10 @@ const ( ECDSAKeyAlgorithm PrivateKeyAlgorithm = "ECDSA" ) +// PrivateKeySize is the size for the algorithm +// +kubebuilder:validation:Enum=256;384;2048;3072;4096 +type PrivateKeySize int32 + // CertificatePrivateKey contains configuration options for private keys // used by the Certificate controller. // These include the key algorithm and size. @@ -120,7 +124,7 @@ type CertificatePrivateKey struct { // key size of 2048 will be used for `RSA` key algorithm and // key size of 256 will be used for `ECDSA` key algorithm. // +optional - Algorithm PrivateKeyAlgorithm `json:"algorithm,omitempty"` + Algorithm *PrivateKeyAlgorithm `json:"algorithm,omitempty"` // Size is the key bit size of the corresponding private key for this certificate. // @@ -130,7 +134,7 @@ type CertificatePrivateKey struct { // and will default to `256` if not specified. // No other values are allowed. // +optional - Size int `json:"size,omitempty"` + Size *PrivateKeySize `json:"size,omitempty"` } // BackOffState stores the status for exponential back off on repeated cert request failure diff --git a/pkg/apis/cert/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/cert/v1alpha1/zz_generated.deepcopy.go index b0ef551e9..347a04f7f 100644 --- a/pkg/apis/cert/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/cert/v1alpha1/zz_generated.deepcopy.go @@ -206,6 +206,16 @@ func (in *CertificateList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *CertificatePrivateKey) DeepCopyInto(out *CertificatePrivateKey) { *out = *in + if in.Algorithm != nil { + in, out := &in.Algorithm, &out.Algorithm + *out = new(PrivateKeyAlgorithm) + **out = **in + } + if in.Size != nil { + in, out := &in.Size, &out.Size + *out = new(PrivateKeySize) + **out = **in + } return } @@ -441,7 +451,7 @@ func (in *CertificateSpec) DeepCopyInto(out *CertificateSpec) { if in.PrivateKey != nil { in, out := &in.PrivateKey, &out.PrivateKey *out = new(CertificatePrivateKey) - **out = **in + (*in).DeepCopyInto(*out) } return } diff --git a/pkg/cert/legobridge/certificate.go b/pkg/cert/legobridge/certificate.go index 49e600a44..4802daf99 100644 --- a/pkg/cert/legobridge/certificate.go +++ b/pkg/cert/legobridge/certificate.go @@ -14,10 +14,11 @@ import ( "sync" "time" - "github.com/gardener/cert-management/pkg/apis/cert/v1alpha1" + api "github.com/gardener/cert-management/pkg/apis/cert/v1alpha1" "github.com/gardener/cert-management/pkg/cert/metrics" "github.com/gardener/cert-management/pkg/cert/utils" "github.com/go-acme/lego/v4/certcrypto" + "k8s.io/utils/ptr" "github.com/go-acme/lego/v4/certificate" "github.com/go-acme/lego/v4/challenge/dns01" @@ -148,12 +149,20 @@ func obtainForDomains(client *lego.Client, domains []string, input ObtainInput) } // ToKeyType extracts the key type from the -func ToKeyType(privateKeySpec *v1alpha1.CertificatePrivateKey) (certcrypto.KeyType, error) { +func ToKeyType(privateKeySpec *api.CertificatePrivateKey) (certcrypto.KeyType, error) { keyType := certcrypto.RSA2048 if privateKeySpec != nil { - switch privateKeySpec.Algorithm { - case "", v1alpha1.RSAKeyAlgorithm: - switch privateKeySpec.Size { + algorithm := api.RSAKeyAlgorithm + if privateKeySpec.Algorithm != nil && *privateKeySpec.Algorithm != "" { + algorithm = *privateKeySpec.Algorithm + } + size := 0 + if privateKeySpec.Size != nil && *privateKeySpec.Size != 0 { + size = int(*privateKeySpec.Size) + } + switch algorithm { + case api.RSAKeyAlgorithm: + switch size { case 0, 2048: keyType = certcrypto.RSA2048 case 3072: @@ -161,43 +170,47 @@ func ToKeyType(privateKeySpec *v1alpha1.CertificatePrivateKey) (certcrypto.KeyTy case 4096: keyType = certcrypto.RSA4096 default: - return "", fmt.Errorf("invalid key size for RSA: %d (allowed values are 2048, 3072, and 4096)", privateKeySpec.Size) + return "", fmt.Errorf("invalid key size for RSA: %d (allowed values are 2048, 3072, and 4096)", size) } - case v1alpha1.ECDSAKeyAlgorithm: - switch privateKeySpec.Size { + case api.ECDSAKeyAlgorithm: + switch size { case 0, 256: keyType = certcrypto.EC256 case 384: keyType = certcrypto.EC384 default: - return "", fmt.Errorf("invalid key size for ECDSA: %d (allowed values are 256 and 384)", privateKeySpec.Size) + return "", fmt.Errorf("invalid key size for ECDSA: %d (allowed values are 256 and 384)", size) } default: return "", fmt.Errorf("invalid private key algorithm %s (allowed values are '%s' and '%s')", - privateKeySpec.Algorithm, v1alpha1.RSAKeyAlgorithm, v1alpha1.ECDSAKeyAlgorithm) + algorithm, api.RSAKeyAlgorithm, api.ECDSAKeyAlgorithm) } } return keyType, nil } // FromKeyType converts key type back to -func FromKeyType(keyType certcrypto.KeyType) *v1alpha1.CertificatePrivateKey { +func FromKeyType(keyType certcrypto.KeyType) *api.CertificatePrivateKey { switch keyType { case certcrypto.RSA2048: - return &v1alpha1.CertificatePrivateKey{Algorithm: v1alpha1.RSAKeyAlgorithm, Size: 2048} + return newCertificatePrivateKey(api.RSAKeyAlgorithm, 2048) case certcrypto.RSA3072: - return &v1alpha1.CertificatePrivateKey{Algorithm: v1alpha1.RSAKeyAlgorithm, Size: 3072} + return newCertificatePrivateKey(api.RSAKeyAlgorithm, 3072) case certcrypto.RSA4096: - return &v1alpha1.CertificatePrivateKey{Algorithm: v1alpha1.RSAKeyAlgorithm, Size: 4096} + return newCertificatePrivateKey(api.RSAKeyAlgorithm, 4096) case certcrypto.EC256: - return &v1alpha1.CertificatePrivateKey{Algorithm: v1alpha1.ECDSAKeyAlgorithm, Size: 256} + return newCertificatePrivateKey(api.ECDSAKeyAlgorithm, 256) case certcrypto.EC384: - return &v1alpha1.CertificatePrivateKey{Algorithm: v1alpha1.ECDSAKeyAlgorithm, Size: 384} + return newCertificatePrivateKey(api.ECDSAKeyAlgorithm, 384) default: return nil } } +func newCertificatePrivateKey(algorithm api.PrivateKeyAlgorithm, size api.PrivateKeySize) *api.CertificatePrivateKey { + return &api.CertificatePrivateKey{Algorithm: ptr.To(algorithm), Size: ptr.To(size)} +} + func obtainForCSR(client *lego.Client, csr []byte, input ObtainInput) (*certificate.Resource, error) { cert, err := extractCertificateRequest(csr) if err != nil { diff --git a/pkg/cert/legobridge/certificate_test.go b/pkg/cert/legobridge/certificate_test.go index fa7796889..fc7b252f6 100644 --- a/pkg/cert/legobridge/certificate_test.go +++ b/pkg/cert/legobridge/certificate_test.go @@ -7,14 +7,19 @@ package legobridge import ( - "github.com/gardener/cert-management/pkg/apis/cert/v1alpha1" + api "github.com/gardener/cert-management/pkg/apis/cert/v1alpha1" + "github.com/go-acme/lego/v4/certcrypto" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) var _ = DescribeTable("KeyType conversion", - func(keyType certcrypto.KeyType, key *v1alpha1.CertificatePrivateKey) { + func(keyType certcrypto.KeyType, algorithm api.PrivateKeyAlgorithm, size int) { + var key *api.CertificatePrivateKey + if size >= 0 { + key = newCertificatePrivateKey(algorithm, api.PrivateKeySize(size)) + } actualKeyType, err := ToKeyType(key) if keyType == "" { Expect(err).To(HaveOccurred()) @@ -26,14 +31,15 @@ var _ = DescribeTable("KeyType conversion", Expect(actualKeyType).To(Equal(keyType)) } }, - Entry("default", certcrypto.RSA2048, nil), - Entry("RSA with default size", certcrypto.RSA2048, &v1alpha1.CertificatePrivateKey{Algorithm: v1alpha1.RSAKeyAlgorithm}), - Entry("RSA2048", certcrypto.RSA2048, &v1alpha1.CertificatePrivateKey{Algorithm: v1alpha1.RSAKeyAlgorithm, Size: 2048}), - Entry("RSA3072", certcrypto.RSA3072, &v1alpha1.CertificatePrivateKey{Algorithm: v1alpha1.RSAKeyAlgorithm, Size: 3072}), - Entry("RSA4096", certcrypto.RSA4096, &v1alpha1.CertificatePrivateKey{Algorithm: v1alpha1.RSAKeyAlgorithm, Size: 4096}), - Entry("ECDSA with default size", certcrypto.EC256, &v1alpha1.CertificatePrivateKey{Algorithm: v1alpha1.ECDSAKeyAlgorithm}), - Entry("EC256", certcrypto.EC256, &v1alpha1.CertificatePrivateKey{Algorithm: v1alpha1.ECDSAKeyAlgorithm, Size: 256}), - Entry("EC384", certcrypto.EC384, &v1alpha1.CertificatePrivateKey{Algorithm: v1alpha1.ECDSAKeyAlgorithm, Size: 384}), - Entry("RSA with wrong size", certcrypto.KeyType(""), &v1alpha1.CertificatePrivateKey{Algorithm: v1alpha1.RSAKeyAlgorithm, Size: 8192}), - Entry("ECDSA with wrong size", certcrypto.KeyType(""), &v1alpha1.CertificatePrivateKey{Algorithm: v1alpha1.ECDSAKeyAlgorithm, Size: 511}), + Entry("default", certcrypto.RSA2048, api.PrivateKeyAlgorithm(""), -1), + Entry("empty", certcrypto.RSA2048, api.PrivateKeyAlgorithm(""), 0), + Entry("RSA from empty config", certcrypto.RSA2048, api.RSAKeyAlgorithm, 0), + Entry("RSA2048", certcrypto.RSA2048, api.RSAKeyAlgorithm, 2048), + Entry("RSA3072", certcrypto.RSA3072, api.RSAKeyAlgorithm, 3072), + Entry("RSA4096", certcrypto.RSA4096, api.RSAKeyAlgorithm, 4096), + Entry("ECDSA with default size", certcrypto.EC256, api.ECDSAKeyAlgorithm, 0), + Entry("EC256", certcrypto.EC256, api.ECDSAKeyAlgorithm, 256), + Entry("EC384", certcrypto.EC384, api.ECDSAKeyAlgorithm, 384), + Entry("RSA with wrong size", certcrypto.KeyType(""), api.RSAKeyAlgorithm, 8192), + Entry("ECDSA with wrong size", certcrypto.KeyType(""), api.ECDSAKeyAlgorithm, 511), ) diff --git a/pkg/cert/source/reconciler.go b/pkg/cert/source/reconciler.go index d8b47f68b..7af8f7424 100644 --- a/pkg/cert/source/reconciler.go +++ b/pkg/cert/source/reconciler.go @@ -14,6 +14,7 @@ import ( core "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/utils/ptr" "github.com/gardener/controller-manager-library/pkg/controllermanager/controller" "github.com/gardener/controller-manager-library/pkg/controllermanager/controller/reconcile" @@ -350,12 +351,7 @@ func (r *sourceReconciler) createEntryFor(logger logger.LogContext, obj resource cert.Spec.PreferredChain = &info.PreferredChain } - if info.PrivateKeyAlgorithm != "" || info.PrivateKeySize != 0 { - cert.Spec.PrivateKey = &api.CertificatePrivateKey{ - Algorithm: api.PrivateKeyAlgorithm(info.PrivateKeyAlgorithm), - Size: info.PrivateKeySize, - } - } + cert.Spec.PrivateKey = createPrivateKey(info.PrivateKeyAlgorithm, info.PrivateKeySize) e, _ := r.SlaveResoures()[0].Wrap(cert) @@ -459,21 +455,9 @@ func (r *sourceReconciler) updateEntry(logger logger.LogContext, info CertInfo, mod.Modify(true) } - oldAlgorithm := "" - oldKeySize := 0 - if spec.PrivateKey != nil { - oldAlgorithm = string(spec.PrivateKey.Algorithm) - oldKeySize = spec.PrivateKey.Size - } - if info.PrivateKeyAlgorithm != oldAlgorithm || info.PrivateKeySize != oldKeySize { - if info.PrivateKeyAlgorithm != "" || info.PrivateKeySize != 0 { - spec.PrivateKey = &api.CertificatePrivateKey{ - Algorithm: api.PrivateKeyAlgorithm(info.PrivateKeyAlgorithm), - Size: info.PrivateKeySize, - } - } else { - spec.PrivateKey = nil - } + newPrivateKey := createPrivateKey(info.PrivateKeyAlgorithm, info.PrivateKeySize) + if !reflect.DeepEqual(spec.PrivateKey, newPrivateKey) { + spec.PrivateKey = newPrivateKey mod.Modify(true) } @@ -484,3 +468,17 @@ func (r *sourceReconciler) updateEntry(logger logger.LogContext, info CertInfo, } return obj.Modify(f) } + +func createPrivateKey(algorithm string, size int) *api.CertificatePrivateKey { + if algorithm == "" && size == 0 { + return nil + } + obj := &api.CertificatePrivateKey{} + if algorithm != "" { + obj.Algorithm = ptr.To(api.PrivateKeyAlgorithm(algorithm)) + } + if size != 0 { + obj.Size = ptr.To(api.PrivateKeySize(size)) + } + return obj +}