Merge pull request #5949 from TrilokGeer/key-replace-sha256checksum

Fixes status change on privateKey update on acme issuer

pkg/acme/accounts/registry.go

@@ -18,6 +18,8 @@ package accounts
 
 import (
 	"crypto/rsa"
+	"crypto/sha256"
+	"crypto/x509"
 	"errors"
 	"net/http"
 	"sync"
@@ -41,6 +43,10 @@ type Registry interface {
 	// resource that constructed it.
 	RemoveClient(uid string)
 
+	// IsKeyCheckSumCached checks if the private key checksum is cached with registered client.
+	// If not cached, the account is re-verified for the private key.
+	IsKeyCheckSumCached(uid string, privateKey *rsa.PrivateKey) bool
+
 	Getter
 }
 
@@ -83,6 +89,7 @@ type stableOptions struct {
 	publicKey     string
 	exponent      int
 	caBundle      string
+	keyChecksum   [sha256.Size]byte
 }
 
 func (c stableOptions) equalTo(c2 stableOptions) bool {
@@ -92,6 +99,8 @@ func (c stableOptions) equalTo(c2 stableOptions) bool {
 func newStableOptions(uid string, config cmacme.ACMEIssuer, privateKey *rsa.PrivateKey) stableOptions {
 	// Encoding a big.Int cannot fail
 	publicNBytes, _ := privateKey.PublicKey.N.GobEncode()
+	checksum := sha256.Sum256(x509.MarshalPKCS1PrivateKey(privateKey))
+
 	return stableOptions{
 		serverURL:     config.Server,
 		skipVerifyTLS: config.SkipTLSVerify,
@@ -99,6 +108,7 @@ func newStableOptions(uid string, config cmacme.ACMEIssuer, privateKey *rsa.Priv
 		publicKey:     string(publicNBytes),
 		exponent:      privateKey.PublicKey.E,
 		caBundle:      string(config.CABundle),
+		keyChecksum:   checksum,
 	}
 }
 
@@ -181,3 +191,26 @@ func (r *registry) ListClients() map[string]acmecl.Interface {
 	}
 	return out
 }
+
+// IsKeyCheckSumCached returns true when there is no difference in private key checksum.
+// This can be used to identify if the private key has changed for the existing
+// registered client.
+func (r *registry) IsKeyCheckSumCached(uid string, privateKey *rsa.PrivateKey) bool {
+	r.lock.RLock()
+	defer r.lock.RUnlock()
+
+	if privateKey != nil {
+		privateKeyBytes := x509.MarshalPKCS1PrivateKey(privateKey)
+		checksum := sha256.Sum256(privateKeyBytes)
+
+		if clientMeta, ok := r.clients[uid]; ok {
+			if clientMeta.keyChecksum == checksum {
+				return true
+			}
+		}
+	}
+
+	// Either there is no entry found in client cache for uid
+	// or private key checksum does not match with cached entry
+	return false
+}

pkg/acme/accounts/registry_test.go

@@ -145,3 +145,32 @@ func TestRegistry_AddClient_UpdatesExistingWhenPrivateKeyChanges(t *testing.T) {
 		t.Errorf("expected ListClients to have 1 item but it has %d", len(l))
 	}
 }
+
+func TestRegistry_AddClient_UpdatesClientPKChecksum(t *testing.T) {
+	r := NewDefaultRegistry()
+	pk, err := pki.GenerateRSAPrivateKey(2048)
+	if err != nil {
+		t.Fatal(err)
+	}
+	pk2, err := pki.GenerateRSAPrivateKey(2048)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Register a new client
+	r.AddClient(http.DefaultClient, "abc", cmacme.ACMEIssuer{}, pk, "cert-manager-test")
+	l := r.ListClients()
+	if len(l) != 1 {
+		t.Errorf("expected ListClients to have 1 item but it has %d", len(l))
+	}
+
+	isCached := r.IsKeyCheckSumCached("abc", pk)
+	if isCached == false {
+		t.Fatal("checksum failed for same key")
+	}
+
+	isCached = r.IsKeyCheckSumCached("abc", pk2)
+	if isCached == true {
+		t.Fatal("checksum reported same for different keys")
+	}
+}

pkg/acme/accounts/test/registry.go

@@ -30,10 +30,11 @@ var _ accounts.Registry = &FakeRegistry{}
 
 // FakeRegistry implements the accounts.Registry interface using stub functions
 type FakeRegistry struct {
-	AddClientFunc    func(uid string, config cmacme.ACMEIssuer, privateKey *rsa.PrivateKey, userAgent string)
-	RemoveClientFunc func(uid string)
-	GetClientFunc    func(uid string) (acmecl.Interface, error)
-	ListClientsFunc  func() map[string]acmecl.Interface
+	AddClientFunc           func(uid string, config cmacme.ACMEIssuer, privateKey *rsa.PrivateKey, userAgent string)
+	RemoveClientFunc        func(uid string)
+	GetClientFunc           func(uid string) (acmecl.Interface, error)
+	ListClientsFunc         func() map[string]acmecl.Interface
+	IsKeyCheckSumCachedFunc func(uid string, privateKey *rsa.PrivateKey) bool
 }
 
 func (f *FakeRegistry) AddClient(client *http.Client, uid string, config cmacme.ACMEIssuer, privateKey *rsa.PrivateKey, userAgent string) {
@@ -51,3 +52,7 @@ func (f *FakeRegistry) GetClient(uid string) (acmecl.Interface, error) {
 func (f *FakeRegistry) ListClients() map[string]acmecl.Interface {
 	return f.ListClientsFunc()
 }
+
+func (f *FakeRegistry) IsKeyCheckSumCached(uid string, privateKey *rsa.PrivateKey) bool {
+	return f.IsKeyCheckSumCachedFunc(uid, privateKey)
+}

pkg/issuer/acme/setup.go

@@ -147,6 +147,8 @@ func (a *Acme) Setup(ctx context.Context) error {
 		return nil
 	}
 
+	isPKChecksumSame := a.accountRegistry.IsKeyCheckSumCached(string(a.issuer.GetUID()), rsaPk)
+
 	// TODO: don't always clear the client cache.
 	//  In future we should intelligently manage items in the account cache
 	//  and remove them when the corresponding issuer is updated/deleted.
@@ -187,7 +189,6 @@ func (a *Acme) Setup(ctx context.Context) error {
 		// absorb errors as retrying will not help resolve this error
 		return nil
 	}
-
 	hasReadyCondition := apiutil.IssuerHasCondition(a.issuer, v1.IssuerCondition{
 		Type:   v1.IssuerConditionReady,
 		Status: cmmeta.ConditionTrue,
@@ -200,7 +201,8 @@ func (a *Acme) Setup(ctx context.Context) error {
 	if hasReadyCondition &&
 		a.issuer.GetStatus().ACMEStatus().URI != "" &&
 		parsedAccountURL.Host == parsedServerURL.Host &&
-		a.issuer.GetStatus().ACMEStatus().LastRegisteredEmail == a.issuer.GetSpec().ACME.Email {
+		a.issuer.GetStatus().ACMEStatus().LastRegisteredEmail == a.issuer.GetSpec().ACME.Email &&
+		isPKChecksumSame {
 		log.V(logf.InfoLevel).Info("skipping re-verifying ACME account as cached registration " +
 			"details look sufficient")
 

pkg/issuer/acme/setup_test.go

@@ -538,6 +538,9 @@ func TestAcme_Setup(t *testing.T) {
 				AddClientFunc: func(string, cmacme.ACMEIssuer, *rsa.PrivateKey, string) {
 					addClientWasCalled = true
 				},
+				IsKeyCheckSumCachedFunc: func(uid string, privateKey *rsa.PrivateKey) bool {
+					return true
+				},
 			}
 
 			// Mock ACME client.