From bdc0cb7c40b0fd6b76e90b3e0ba9445164974392 Mon Sep 17 00:00:00 2001 From: TrilokGeer Date: Thu, 13 Apr 2023 20:33:53 +0530 Subject: [PATCH] Fixes status change on privateKey update on acme issuer Signed-off-by: TrilokGeer --- pkg/acme/accounts/registry.go | 33 ++++++++++++++++++++++++++++++ pkg/acme/accounts/registry_test.go | 29 ++++++++++++++++++++++++++ pkg/acme/accounts/test/registry.go | 13 ++++++++---- pkg/issuer/acme/setup.go | 6 ++++-- pkg/issuer/acme/setup_test.go | 3 +++ 5 files changed, 78 insertions(+), 6 deletions(-) diff --git a/pkg/acme/accounts/registry.go b/pkg/acme/accounts/registry.go index 076703538..d79569a86 100644 --- a/pkg/acme/accounts/registry.go +++ b/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 +} diff --git a/pkg/acme/accounts/registry_test.go b/pkg/acme/accounts/registry_test.go index 5590d10b1..045dcd6c6 100644 --- a/pkg/acme/accounts/registry_test.go +++ b/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") + } +} diff --git a/pkg/acme/accounts/test/registry.go b/pkg/acme/accounts/test/registry.go index db40f492d..681e495c5 100644 --- a/pkg/acme/accounts/test/registry.go +++ b/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) +} diff --git a/pkg/issuer/acme/setup.go b/pkg/issuer/acme/setup.go index a689e6238..a3b5e65d3 100644 --- a/pkg/issuer/acme/setup.go +++ b/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") diff --git a/pkg/issuer/acme/setup_test.go b/pkg/issuer/acme/setup_test.go index 050596e54..ba4a61d64 100644 --- a/pkg/issuer/acme/setup_test.go +++ b/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.