add certificate-hash annotation that perfectly detects whether reissu…

…ance is necessary

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

internal/controller/certificates/policies/checks.go

@@ -157,6 +157,27 @@ func SecretKeystoreFormatMismatch(input Input) (string, string, bool) {
 	return "", "", false
 }
 
+func SecretCertificateHashAnnotationMismatch(
+	fallbackChain Chain,
+) Func {
+	return func(input Input) (reason, message string, failed bool) {
+		certificateHash := input.Secret.Annotations[cmapi.CertificateHashAnnotationKey]
+		if certificateHash != "" {
+			return fallbackChain.Evaluate(input)
+		}
+
+		upToDate, err := pki.IsCertificateUpToDateWithVersionedHash(input.Certificate, certificateHash)
+		if err != nil {
+			return CertificateHashMismatch, fmt.Sprintf("Failed to check certificate is up to date: %v", err), true
+		}
+		if !upToDate {
+			return CertificateHashMismatch, "Certificate has been changed since issuance (hash has been updated)", true
+		}
+
+		return "", "", false
+	}
+}
+
 // SecretIssuerAnnotationsMismatch - When the issuer annotations are defined,
 // it must match the issuer ref.
 func SecretIssuerAnnotationsMismatch(input Input) (string, string, bool) {

internal/controller/certificates/policies/constants.go

@@ -36,6 +36,9 @@ const (
 	// SecretMismatch is a policy violation reason for a scenario where Secret's
 	// private key does not match spec.
 	SecretMismatch string = "SecretMismatch"
+	// CertificateHashMismatch is a policy violation reason for a scenario where
+	// Certificate's hash does not match the hash of the certificate on the Secret.
+	CertificateHashMismatch string = "CertificateHashMismatch"
 	// IncorrectIssuer is a policy violation reason for a scenario where
 	// Certificate has been issued by incorrect Issuer.
 	IncorrectIssuer string = "IncorrectIssuer"

internal/controller/certificates/policies/policies.go

@@ -71,13 +71,21 @@ func NewTriggerPolicyChain(c clock.Clock) Chain {
 		SecretIsMissingData,    // Make sure the Secret has the required keys set
 		SecretPublicKeysDiffer, // Make sure the PrivateKey and PublicKey match in the Secret
 
-		SecretIssuerAnnotationsMismatch,          // Make sure the Secret's IssuerRef annotations match the Certificate spec
-		SecretCertificateNameAnnotationsMismatch, // Make sure the Secret's CertificateName annotation matches the Certificate's name
-
-		SecretPrivateKeyMismatchesSpec,                      // Make sure the PrivateKey Type and Size match the Certificate spec
-		SecretPublicKeyDiffersFromCurrentCertificateRequest, // Make sure the Secret's PublicKey matches the current CertificateRequest
-		CurrentCertificateRequestMismatchesSpec,             // Make sure the current CertificateRequest matches the Certificate spec
-		CurrentCertificateNearingExpiry(c),                  // Make sure the Certificate in the Secret is not nearing expiry
+		SecretCertificateHashAnnotationMismatch(
+			// BACKWARDS COMPATIBILITY: The following checks are only performed in case no
+			// CertificateRequest hash annotation is present on the Secret. This is to
+			// ensure that existing users of cert-manager do not have all their existing
+			// Certificates marked as not ready.
+			Chain{
+				SecretIssuerAnnotationsMismatch,                     // Make sure the Secret's IssuerRef annotations match the Certificate spec
+				SecretCertificateNameAnnotationsMismatch,            // Make sure the Secret's CertificateName annotation matches the Certificate's name
+				SecretPrivateKeyMismatchesSpec,                      // Make sure the PrivateKey Type and Size match the Certificate spec
+				SecretPublicKeyDiffersFromCurrentCertificateRequest, // Make sure the Secret's PublicKey matches the current CertificateRequest
+				CurrentCertificateRequestMismatchesSpec,             // Make sure the current CertificateRequest matches the Certificate spec
+			},
+		),
+
+		CurrentCertificateNearingExpiry(c), // Make sure the Certificate in the Secret is not nearing expiry
 	}
 }
 
@@ -89,13 +97,21 @@ func NewReadinessPolicyChain(c clock.Clock) Chain {
 		SecretIsMissingData,    // Make sure the Secret has the required keys set
 		SecretPublicKeysDiffer, // Make sure the PrivateKey and PublicKey match in the Secret
 
-		SecretIssuerAnnotationsMismatch,          // Make sure the Secret's IssuerRef annotations match the Certificate spec
-		SecretCertificateNameAnnotationsMismatch, // Make sure the Secret's CertificateName annotation matches the Certificate's name
-
-		SecretPrivateKeyMismatchesSpec,                      // Make sure the PrivateKey Type and Size match the Certificate spec
-		SecretPublicKeyDiffersFromCurrentCertificateRequest, // Make sure the Secret's PublicKey matches the current CertificateRequest
-		CurrentCertificateRequestMismatchesSpec,             // Make sure the current CertificateRequest matches the Certificate spec
-		CurrentCertificateHasExpired(c),                     // Make sure the Certificate in the Secret has not expired
+		SecretCertificateHashAnnotationMismatch(
+			// BACKWARDS COMPATIBILITY: The following checks are only performed in case no
+			// CertificateRequest hash annotation is present on the Secret. This is to
+			// ensure that existing users of cert-manager do not have all their existing
+			// Certificates marked as not ready.
+			Chain{
+				SecretIssuerAnnotationsMismatch,                     // Make sure the Secret's IssuerRef annotations match the Certificate spec
+				SecretCertificateNameAnnotationsMismatch,            // Make sure the Secret's CertificateName annotation matches the Certificate's name
+				SecretPrivateKeyMismatchesSpec,                      // Make sure the PrivateKey Type and Size match the Certificate spec
+				SecretPublicKeyDiffersFromCurrentCertificateRequest, // Make sure the Secret's PublicKey matches the current CertificateRequest
+				CurrentCertificateRequestMismatchesSpec,             // Make sure the current CertificateRequest matches the Certificate spec
+			},
+		),
+
+		CurrentCertificateHasExpired(c), // Make sure the Certificate in the Secret has not expired
 	}
 }
 

pkg/apis/certmanager/v1/types.go

@@ -89,6 +89,10 @@ const (
 	// Annotation key for the 'group' of the Issuer resource.
 	IssuerGroupAnnotationKey = "cert-manager.io/issuer-group"
 
+	// Annotation key for the hash of the Certificate that created the certificate
+	// block encoded in this Secret resource.
+	CertificateHashAnnotationKey = "cert-manager.io/certificate-hash"
+
 	// Annotation key for the name of the certificate that a resource is related to.
 	CertificateNameKey = "cert-manager.io/certificate-name"
 

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

@@ -60,6 +60,7 @@ type SecretsManager struct {
 // SecretData is a structure wrapping private key, Certificate and CA data
 type SecretData struct {
 	PrivateKey, Certificate, CA         []byte
+	CertificateHash                     string
 	CertificateName                     string
 	IssuerName, IssuerKind, IssuerGroup string
 }
@@ -200,6 +201,10 @@ func (s *SecretsManager) setValues(crt *cmapi.Certificate, secret *corev1.Secret
 		secret.Annotations[cmapi.IssuerGroupAnnotationKey] = data.IssuerGroup
 	}
 
+	if data.CertificateHash != "" {
+		secret.Annotations[cmapi.CertificateHashAnnotationKey] = data.CertificateHash
+	}
+
 	secret.Labels[cmapi.PartOfCertManagerControllerLabelKey] = "true"
 
 	return nil

pkg/controller/certificates/issuing/issuing_controller.go

@@ -385,6 +385,11 @@ func (c *controller) issueCertificate(ctx context.Context, nextRevision int, crt
 		crt.Spec.PrivateKey = &cmapi.CertificatePrivateKey{}
 	}
 
+	certificateHash, err := utilpki.CertificateVersionedHash(crt)
+	if err != nil {
+		return err
+	}
+
 	pkData, err := utilpki.EncodePrivateKey(pk, crt.Spec.PrivateKey.Encoding)
 	if err != nil {
 		return err
@@ -393,6 +398,7 @@ func (c *controller) issueCertificate(ctx context.Context, nextRevision int, crt
 		PrivateKey:      pkData,
 		Certificate:     req.Status.Certificate,
 		CA:              req.Status.CA,
+		CertificateHash: certificateHash,
 		CertificateName: crt.Name,
 		IssuerName:      req.Spec.IssuerRef.Name,
 		IssuerKind:      req.Spec.IssuerRef.Kind,

pkg/controller/certificates/issuing/secret_manager.go

@@ -69,6 +69,7 @@ func (c *controller) ensureSecretData(ctx context.Context, log logr.Logger, crt
 		PrivateKey:      secret.Data[corev1.TLSPrivateKeyKey],
 		Certificate:     secret.Data[corev1.TLSCertKey],
 		CA:              secret.Data[cmmeta.TLSCAKey],
+		CertificateHash: secret.Annotations[cmapi.CertificateHashAnnotationKey],
 		CertificateName: secret.Annotations[cmapi.CertificateNameKey],
 		IssuerName:      secret.Annotations[cmapi.IssuerNameAnnotationKey],
 		IssuerKind:      secret.Annotations[cmapi.IssuerKindAnnotationKey],

pkg/util/pki/match.go

@@ -21,7 +21,9 @@ import (
 	"crypto/ecdsa"
 	"crypto/ed25519"
 	"crypto/rsa"
+	"hash/fnv"
 	"net"
+	"sort"
 
 	"fmt"
 	"reflect"
@@ -31,11 +33,104 @@ import (
 
 	cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
 	"github.com/cert-manager/cert-manager/pkg/util"
+	"github.com/davecgh/go-spew/spew"
 )
 
+func CertificateVersionedHash(cert *cmapi.Certificate) (string, error) {
+	hash, err := certificateHashVersion001(cert)
+	if err != nil {
+		return "", err
+	}
+
+	return "v001:" + hash, nil
+}
+
+func IsCertificateUpToDateWithVersionedHash(cert *cmapi.Certificate, hash string) (bool, error) {
+	if hash == "" {
+		return false, fmt.Errorf("hash cannot be empty")
+	}
+
+	if len(hash) < 5 {
+		return false, fmt.Errorf("hash is too short")
+	}
+
+	version := hash[:5]
+	hash = hash[5:]
+
+	var err error
+	var objHash string
+	switch version {
+	case "v001:":
+		objHash, err = certificateHashVersion001(cert)
+	default:
+		return false, fmt.Errorf("unknown hash version %q", version)
+	}
+
+	if err != nil {
+		return false, err
+	}
+
+	return objHash == hash, nil
+}
+
+func certificateHashVersion001(cert *cmapi.Certificate) (string, error) {
+	usages := make([]string, 0, len(cert.Spec.Usages))
+	for _, usage := range cert.Spec.Usages {
+		usages = append(usages, string(usage))
+	}
+	sort.Strings(usages)
+
+	subject := cert.Spec.Subject
+	if subject == nil {
+		subject = &cmapi.X509Subject{}
+	}
+
+	objectToWrite := []interface{}{
+		subject.Organizations,
+		subject.Countries,
+		subject.OrganizationalUnits,
+		subject.Localities,
+		subject.Provinces,
+		subject.StreetAddresses,
+		subject.PostalCodes,
+		subject.SerialNumber,
+		cert.Spec.LiteralSubject,
+
+		cert.Spec.CommonName,
+		cert.Spec.DNSNames,
+		cert.Spec.IPAddresses,
+		cert.Spec.URIs,
+		cert.Spec.EmailAddresses,
+
+		cert.Spec.IssuerRef.Name,
+		cert.Spec.IssuerRef.Group,
+		cert.Spec.IssuerRef.Kind,
+
+		cert.Spec.IsCA,
+
+		usages,
+		cert.Spec.EncodeUsagesInRequest,
+	}
+
+	hasher := fnv.New64a()
+	printer := spew.ConfigState{
+		Indent:         " ",
+		SortKeys:       true,
+		DisableMethods: true,
+		SpewKeys:       true,
+	}
+	_, err := printer.Fprintf(hasher, "%#v", objectToWrite)
+	if err != nil {
+		return "", err
+	}
+
+	return fmt.Sprintf("%x016", hasher.Sum64()), nil
+}
+
 // PrivateKeyMatchesSpec returns an error if the private key bit size
 // doesn't match the provided spec. RSA, Ed25519 and ECDSA are supported.
 // If any error is returned, a list of violations will also be returned.
+// Deprecated: use CertificateVersionedHash and IsCertificateUpToDateWithVersionedHash instead.
 func PrivateKeyMatchesSpec(pk crypto.PrivateKey, spec cmapi.CertificateSpec) ([]string, error) {
 	spec = *spec.DeepCopy()
 	if spec.PrivateKey == nil {
@@ -118,6 +213,7 @@ func ipSlicesMatch(parsedIPs []net.IP, stringIPs []string) bool {
 // and returns a list of field names on the Certificate that do not match their
 // counterpart fields on the CertificateRequest.
 // If decoding the x509 certificate request fails, an error will be returned.
+// Deprecated: use CertificateVersionedHash and IsCertificateUpToDateWithVersionedHash instead.
 func RequestMatchesSpec(req *cmapi.CertificateRequest, spec cmapi.CertificateSpec) ([]string, error) {
 	x509req, err := DecodeX509CertificateRequestBytes(req.Spec.Request)
 	if err != nil {
@@ -221,6 +317,7 @@ func RequestMatchesSpec(req *cmapi.CertificateRequest, spec cmapi.CertificateSpe
 // do not match their counterparts.
 // This is a purposely less comprehensive check than RequestMatchesSpec as some
 // issuers override/force certain fields.
+// Deprecated: use CertificateVersionedHash and IsCertificateUpToDateWithVersionedHash instead.
 func SecretDataAltNamesMatchSpec(secret *corev1.Secret, spec cmapi.CertificateSpec) ([]string, error) {
 	x509cert, err := DecodeX509CertificateBytes(secret.Data[corev1.TLSCertKey])
 	if err != nil {

pkg/util/pki/match_test.go

@@ -50,6 +50,53 @@ func mustGenerateEd25519(t *testing.T) crypto.PrivateKey {
 	return pk
 }
 
+func TestCertificateVersionedHash(t *testing.T) {
+	tests := map[string]struct {
+		cert       *cmapi.Certificate
+		hash       string
+		err        string
+		violations []string
+	}{
+		"should match if certificate is unchanged": {
+			cert: &cmapi.Certificate{
+				Spec: cmapi.CertificateSpec{
+					CommonName: "cn",
+					DNSNames:   []string{"at", "least", "one"},
+				},
+			},
+			hash: "v001:b32301b6e4140720016",
+		},
+		"should not match if commonName is changed": {
+			cert: &cmapi.Certificate{
+				Spec: cmapi.CertificateSpec{
+					CommonName: "cn-changed",
+					DNSNames:   []string{"at", "least", "one"},
+				},
+			},
+			hash: "v001:fd70680aa2703c5b016",
+		},
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			hash, err := CertificateVersionedHash(test.cert)
+			switch {
+			case err != nil:
+				if test.err != err.Error() {
+					t.Errorf("error text did not match, got=%s, exp=%s", err.Error(), test.err)
+				}
+			default:
+				if test.err != "" {
+					t.Errorf("got no error but expected: %s", test.err)
+				}
+			}
+			if hash != test.hash {
+				t.Errorf("hash did not match, got=%s, exp=%s", hash, test.hash)
+			}
+		})
+	}
+}
+
 func TestPrivateKeyMatchesSpec(t *testing.T) {
 	tests := map[string]struct {
 		key          crypto.PrivateKey