Validate keystore cr and default password

Signed-off-by: Romain QUINIO <romain.quinio@amadeus.com>

deploy/crds/crd-certificates.yaml

@@ -134,7 +134,7 @@ spec:
                           description: Create enables JKS keystore creation for the Certificate. If true, a file named `keystore.jks` will be created in the target Secret resource, encrypted using the password stored in `passwordSecretRef`. The keystore file will be updated immediately. If the issuer provided a CA certificate, a file named `truststore.jks` will also be created in the target Secret resource, encrypted using the password stored in `passwordSecretRef` containing the issuing Certificate Authority
                           type: boolean
                         password:
-                          description: a literal password used to encrypt the JKS keystore.
+                          description: a literal password used to encrypt the JKS keystore. Defaults to `changeit` if not specified.
                           type: string
                         passwordSecretRef:
                           description: PasswordSecretRef is a reference to a key in a Secret resource containing the password used to encrypt the JKS keystore.
@@ -158,7 +158,7 @@ spec:
                           description: Create enables PKCS12 keystore creation for the Certificate. If true, a file named `keystore.p12` will be created in the target Secret resource, encrypted using the password stored in `passwordSecretRef`. The keystore file will be updated immediately. If the issuer provided a CA certificate, a file named `truststore.p12` will also be created in the target Secret resource, encrypted using the password stored in `passwordSecretRef` containing the issuing Certificate Authority
                           type: boolean
                         password:
-                          description: a literal password used to encrypt the PKCS12 keystore.
+                          description: a literal password used to encrypt the PKCS12 keystore. Defaults to `changeit` if not specified.
                           type: string
                         passwordSecretRef:
                           description: PasswordSecretRef is a reference to a key in a Secret resource containing the password used to encrypt the PKCS12 keystore.

internal/apis/certmanager/types_certificate.go

@@ -409,10 +409,10 @@ type JKSKeystore struct {
 
 	// PasswordSecretRef is a reference to a key in a Secret resource
 	// containing the password used to encrypt the JKS keystore.
-	PasswordSecretRef cmmeta.SecretKeySelector
+	PasswordSecretRef *cmmeta.SecretKeySelector
 
 	// Password literal used to encrypt the JKS keystore.
-	Password string
+	Password *string
 }
 
 // PKCS12 configures options for storing a PKCS12 keystore in the
@@ -431,10 +431,10 @@ type PKCS12Keystore struct {
 
 	// PasswordSecretRef is a reference to a key in a Secret resource
 	// containing the password used to encrypt the PKCS12 keystore.
-	PasswordSecretRef cmmeta.SecretKeySelector
+	PasswordSecretRef *cmmeta.SecretKeySelector
 
 	// Password literal used to encrypt the PKCS12 keystore.
-	Password string
+	Password *string
 
 	// Profile specifies the key and certificate encryption algorithms and the HMAC algorithm
 	// used to create the PKCS12 keystore. Default value is `LegacyRC2` for backward compatibility.

internal/apis/certmanager/validation/certificate.go

@@ -196,6 +196,10 @@ func ValidateCertificateSpec(crt *internalcmapi.CertificateSpec, fldPath *field.
 
 	el = append(el, validateAdditionalOutputFormats(crt, fldPath)...)
 
+	if crt.Keystores != nil {
+		el = append(el, validateKeystores(crt, fldPath)...)
+	}
+
 	return el
 }
 
@@ -331,3 +335,18 @@ func validateAdditionalOutputFormats(crt *internalcmapi.CertificateSpec, fldPath
 
 	return el
 }
+
+func validateKeystores(crt *internalcmapi.CertificateSpec, fldPath *field.Path) field.ErrorList {
+	var el field.ErrorList
+	if crt.Keystores.JKS != nil {
+		if crt.Keystores.JKS.Password != nil && crt.Keystores.JKS.PasswordSecretRef != nil {
+			el = append(el, field.Invalid(fldPath.Child("keystores", "jks", "password"), crt.Keystores.JKS.Password, "When providing a `PasswordSecretRef` no `Password` properties may be provided."))
+		}
+	}
+	if crt.Keystores.PKCS12 != nil {
+		if crt.Keystores.PKCS12.Password != nil && crt.Keystores.PKCS12.PasswordSecretRef != nil {
+			el = append(el, field.Invalid(fldPath.Child("keystores", "pkcs12", "password"), crt.Keystores.PKCS12.Password, "When providing a `PasswordSecretRef` no `Password` properties may be provided."))
+		}
+	}
+	return el
+}

internal/apis/certmanager/validation/certificate_test.go

@@ -1125,3 +1125,81 @@ func Test_validateLiteralSubject(t *testing.T) {
 		})
 	}
 }
+
+func Test_validateKeystores(t *testing.T) {
+	keystorePassword := "changeit"
+	fldPath := field.NewPath("spec")
+	tests := map[string]struct {
+		cfg  *internalcmapi.Certificate
+		a    *admissionv1.AdmissionRequest
+		errs []*field.Error
+	}{
+		"JKS using default password": {
+			cfg: &internalcmapi.Certificate{
+				Spec: internalcmapi.CertificateSpec{
+					CommonName: "testcn",
+					SecretName: "abc",
+					IssuerRef:  validIssuerRef,
+					Keystores: &internalcmapi.CertificateKeystores{
+						JKS: &internalcmapi.JKSKeystore{},
+					},
+				},
+			},
+			a: someAdmissionRequest,
+		},
+		"JKS PasswordSecretRef and Password are exclusive": {
+			cfg: &internalcmapi.Certificate{
+				Spec: internalcmapi.CertificateSpec{
+					CommonName: "testcn",
+					SecretName: "abc",
+					IssuerRef:  validIssuerRef,
+					Keystores: &internalcmapi.CertificateKeystores{
+						JKS: &internalcmapi.JKSKeystore{
+							PasswordSecretRef: &cmmeta.SecretKeySelector{
+								LocalObjectReference: cmmeta.LocalObjectReference{
+									Name: "secret",
+								},
+							},
+							Password: &keystorePassword,
+						},
+					},
+				},
+			},
+			errs: []*field.Error{
+				field.Invalid(fldPath.Child("keystores", "jks", "password"), &keystorePassword, "When providing a `PasswordSecretRef` no `Password` properties may be provided."),
+			},
+			a: someAdmissionRequest,
+		},
+		"PKCS12 PasswordSecretRef and Password are exclusive": {
+			cfg: &internalcmapi.Certificate{
+				Spec: internalcmapi.CertificateSpec{
+					CommonName: "testcn",
+					SecretName: "abc",
+					IssuerRef:  validIssuerRef,
+					Keystores: &internalcmapi.CertificateKeystores{
+						PKCS12: &internalcmapi.PKCS12Keystore{
+							PasswordSecretRef: &cmmeta.SecretKeySelector{
+								LocalObjectReference: cmmeta.LocalObjectReference{
+									Name: "secret",
+								},
+							},
+							Password: &keystorePassword,
+						},
+					},
+				},
+			},
+			errs: []*field.Error{
+				field.Invalid(fldPath.Child("keystores", "pkcs12", "password"), &keystorePassword, "When providing a `PasswordSecretRef` no `Password` properties may be provided."),
+			},
+			a: someAdmissionRequest,
+		},
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			errs, warnings := ValidateCertificate(test.a, test.cfg)
+			assert.ElementsMatch(t, errs, test.errs)
+			assert.ElementsMatch(t, warnings, []string{})
+		})
+	}
+}

pkg/apis/certmanager/v1/types_certificate.go

@@ -462,7 +462,7 @@ type JKSKeystore struct {
 	PasswordSecretRef *cmmeta.SecretKeySelector `json:"passwordSecretRef"`
 
 	// Password literal used to encrypt the PKCS12 keystore.
-	Password *string `json:"password"`
+	Password *string `json:"password,omitempty"`
 }
 
 // PKCS12 configures options for storing a PKCS12 keystore in the
@@ -484,7 +484,7 @@ type PKCS12Keystore struct {
 	PasswordSecretRef *cmmeta.SecretKeySelector `json:"passwordSecretRef"`
 
 	// Password litteral used to encrypt the PKCS12 keystore.
-	Password *string `json:"password"`
+	Password *string `json:"password,omitempty"`
 
 	// Profile specifies the key and certificate encryption algorithms and the HMAC algorithm
 	// used to create the PKCS12 keystore. Default value is `LegacyRC2` for backward compatibility.

pkg/controller/certificates/issuing/internal/secret.go

@@ -38,6 +38,9 @@ import (
 	utilpki "github.com/cert-manager/cert-manager/pkg/util/pki"
 )
 
+// DefaultPassword is the string "changeit", a commonly-used password for keystore files.
+const DefaultKeystorePassword = "changeit"
+
 var (
 	certificateGvk = cmapi.SchemeGroupVersion.WithKind("Certificate")
 )
@@ -260,13 +263,15 @@ func (s *SecretsManager) setKeystores(crt *cmapi.Certificate, secret *corev1.Sec
 				return fmt.Errorf("PKCS12 keystore password Secret contains no data for key %q", ref.Key)
 			}
 			pw = pwSecret.Data[ref.Key]
-		} else if crt.Spec.Keystores.PKCS12.Password != nil {
-			pw = []byte(*crt.Spec.Keystores.PKCS12.Password)
 		} else {
-			return fmt.Errorf("either PasswordSecretRef or Password is mandatory")
+			if crt.Spec.Keystores.PKCS12.Password != nil {
+				pw = []byte(*crt.Spec.Keystores.PKCS12.Password)
+			} else {
+				pw = []byte(DefaultKeystorePassword)
+			}
+			// Store the keystore password in the secret to avoid hardcoding in more than one place
+			secret.Data[cmapi.KeystorePassword] = pw
 		}
-		// Store the keystore password in the secret to help transition from PasswordSecretRef to Password
-		secret.Data[cmapi.KeystorePassword] = pw
 
 		profile := crt.Spec.Keystores.PKCS12.Profile
 		keystoreData, err := encodePKCS12Keystore(profile, string(pw), data.PrivateKey, data.Certificate, data.CA)
@@ -299,13 +304,15 @@ func (s *SecretsManager) setKeystores(crt *cmapi.Certificate, secret *corev1.Sec
 				return fmt.Errorf("JKS keystore password Secret contains no data for key %q", ref.Key)
 			}
 			pw = pwSecret.Data[ref.Key]
-		} else if crt.Spec.Keystores.JKS.Password != nil {
-			pw = []byte(*crt.Spec.Keystores.JKS.Password)
 		} else {
-			return fmt.Errorf("either PasswordSecretRef or Password is mandatory")
+			if crt.Spec.Keystores.JKS.Password != nil {
+				pw = []byte(*crt.Spec.Keystores.JKS.Password)
+			} else {
+				pw = []byte(DefaultKeystorePassword)
+			}
+			// Store the keystore password in the secret to avoid hardcoding in more than one place
+			secret.Data[cmapi.KeystorePassword] = pw
 		}
-		// Store the keystore password in the secret to help transition from PasswordSecretRef to Password
-		secret.Data[cmapi.KeystorePassword] = pw
 
 		keystoreData, err := encodeJKSKeystore(pw, data.PrivateKey, data.Certificate, data.CA)
 		if err != nil {

pkg/controller/certificates/issuing/internal/secret_test.go

@@ -93,13 +93,19 @@ func Test_SecretsManager(t *testing.T) {
 			cmapi.CertificateAdditionalOutputFormat{Type: "CombinedPEM"},
 		),
 	)
-	keystorePassword := "changeit"
+	keystorePassword := "non-default"
 	baseCertWithJKSKeystore := gen.CertificateFrom(baseCertBundle.Certificate,
 		gen.SetCertificateKeystore(&cmapi.CertificateKeystores{JKS: &cmapi.JKSKeystore{Create: true, Password: &keystorePassword}}),
 	)
+	baseCertWithJKSKeystoreNoPassword := gen.CertificateFrom(baseCertBundle.Certificate,
+		gen.SetCertificateKeystore(&cmapi.CertificateKeystores{JKS: &cmapi.JKSKeystore{Create: true}}),
+	)
 	baseCertWithPKCS12Keystore := gen.CertificateFrom(baseCertBundle.Certificate,
 		gen.SetCertificateKeystore(&cmapi.CertificateKeystores{PKCS12: &cmapi.PKCS12Keystore{Create: true, Password: &keystorePassword}}),
 	)
+	baseCertWithPKCS12KeystoreNoPassword := gen.CertificateFrom(baseCertBundle.Certificate,
+		gen.SetCertificateKeystore(&cmapi.CertificateKeystores{PKCS12: &cmapi.PKCS12Keystore{Create: true}}),
+	)
 	block, _ := pem.Decode(baseCertBundle.PrivateKeyBytes)
 	tlsDerContent := block.Bytes
 
@@ -805,6 +811,42 @@ func Test_SecretsManager(t *testing.T) {
 			},
 			expectedErr: false,
 		},
+
+		"if secret does not exist, create new Secret with JKS keystore using default password": {
+			certificateOptions: controllerpkg.CertificateOptions{EnableOwnerRef: false},
+			certificate:        baseCertWithJKSKeystoreNoPassword,
+			existingSecret:     nil,
+			secretData: SecretData{
+				Certificate: baseCertBundle.CertBytes, PrivateKey: baseCertBundle.PrivateKeyBytes,
+				CertificateName: "test", IssuerName: "ca-issuer", IssuerKind: "Issuer", IssuerGroup: "foo.io",
+			},
+			applyFn: func(t *testing.T) testcoreclients.ApplyFn {
+				return func(_ context.Context, gotCnf *applycorev1.SecretApplyConfiguration, gotOpts metav1.ApplyOptions) (*corev1.Secret, error) {
+					assert.Equal(t, []byte(DefaultKeystorePassword), gotCnf.Data[cmapi.KeystorePassword])
+					assert.NotNil(t, gotCnf.Data[cmapi.JKSSecretKey])
+					return nil, nil
+				}
+			},
+			expectedErr: false,
+		},
+
+		"if secret does not exist, create new Secret with PKCS12 keystore using default password": {
+			certificateOptions: controllerpkg.CertificateOptions{EnableOwnerRef: false},
+			certificate:        baseCertWithPKCS12KeystoreNoPassword,
+			existingSecret:     nil,
+			secretData: SecretData{
+				Certificate: baseCertBundle.CertBytes, PrivateKey: baseCertBundle.PrivateKeyBytes,
+				CertificateName: "test", IssuerName: "ca-issuer", IssuerKind: "Issuer", IssuerGroup: "foo.io",
+			},
+			applyFn: func(t *testing.T) testcoreclients.ApplyFn {
+				return func(_ context.Context, gotCnf *applycorev1.SecretApplyConfiguration, gotOpts metav1.ApplyOptions) (*corev1.Secret, error) {
+					assert.Equal(t, []byte(DefaultKeystorePassword), gotCnf.Data[cmapi.KeystorePassword])
+					assert.NotNil(t, gotCnf.Data[cmapi.PKCS12SecretKey])
+					return nil, nil
+				}
+			},
+			expectedErr: false,
+		},
 	}
 
 	for name, test := range tests {