diff --git a/deploy/crds/crd-clusterissuers.yaml b/deploy/crds/crd-clusterissuers.yaml index 028054f28..1e3763518 100644 --- a/deploy/crds/crd-clusterissuers.yaml +++ b/deploy/crds/crd-clusterissuers.yaml @@ -1267,6 +1267,9 @@ spec: description: ACME specific status options. This field should only be set if the Issuer is configured to use an ACME server to issue certificates. type: object properties: + lastPrivateKeyHash: + description: LastPrivateKeyHash is a hash of the private key associated with the latest registered ACME account, in order to track changes made to registered account associated with the Issuer + type: string lastRegisteredEmail: description: LastRegisteredEmail is the email associated with the latest registered ACME account, in order to track changes made to registered account associated with the Issuer type: string diff --git a/deploy/crds/crd-issuers.yaml b/deploy/crds/crd-issuers.yaml index f6e8b6316..bdaf0dcb2 100644 --- a/deploy/crds/crd-issuers.yaml +++ b/deploy/crds/crd-issuers.yaml @@ -1267,6 +1267,9 @@ spec: description: ACME specific status options. This field should only be set if the Issuer is configured to use an ACME server to issue certificates. type: object properties: + lastPrivateKeyHash: + description: LastPrivateKeyHash is a hash of the private key associated with the latest registered ACME account, in order to track changes made to registered account associated with the Issuer + type: string lastRegisteredEmail: description: LastRegisteredEmail is the email associated with the latest registered ACME account, in order to track changes made to registered account associated with the Issuer type: string diff --git a/internal/apis/acme/types_issuer.go b/internal/apis/acme/types_issuer.go index 240108e94..b928ceb3e 100644 --- a/internal/apis/acme/types_issuer.go +++ b/internal/apis/acme/types_issuer.go @@ -557,4 +557,9 @@ type ACMEIssuerStatus struct { // ACME account, in order to track changes made to registered account // associated with the Issuer LastRegisteredEmail string + + // LastPrivateKeyHash is a hash of the private key associated with the latest + // registered ACME account, in order to track changes made to registered account + // associated with the Issuer + LastPrivateKeyHash string } diff --git a/internal/apis/acme/v1/zz_generated.conversion.go b/internal/apis/acme/v1/zz_generated.conversion.go index df119898f..68235d2fa 100644 --- a/internal/apis/acme/v1/zz_generated.conversion.go +++ b/internal/apis/acme/v1/zz_generated.conversion.go @@ -1286,6 +1286,7 @@ func Convert_acme_ACMEIssuerDNS01ProviderWebhook_To_v1_ACMEIssuerDNS01ProviderWe func autoConvert_v1_ACMEIssuerStatus_To_acme_ACMEIssuerStatus(in *v1.ACMEIssuerStatus, out *acme.ACMEIssuerStatus, s conversion.Scope) error { out.URI = in.URI out.LastRegisteredEmail = in.LastRegisteredEmail + out.LastPrivateKeyHash = in.LastPrivateKeyHash return nil } @@ -1297,6 +1298,7 @@ func Convert_v1_ACMEIssuerStatus_To_acme_ACMEIssuerStatus(in *v1.ACMEIssuerStatu func autoConvert_acme_ACMEIssuerStatus_To_v1_ACMEIssuerStatus(in *acme.ACMEIssuerStatus, out *v1.ACMEIssuerStatus, s conversion.Scope) error { out.URI = in.URI out.LastRegisteredEmail = in.LastRegisteredEmail + out.LastPrivateKeyHash = in.LastPrivateKeyHash return nil } diff --git a/internal/apis/acme/v1alpha2/types_issuer.go b/internal/apis/acme/v1alpha2/types_issuer.go index 8622bcf0f..1d380d087 100644 --- a/internal/apis/acme/v1alpha2/types_issuer.go +++ b/internal/apis/acme/v1alpha2/types_issuer.go @@ -629,4 +629,9 @@ type ACMEIssuerStatus struct { // associated with the Issuer // +optional LastRegisteredEmail string `json:"lastRegisteredEmail,omitempty"` + + // LastPrivateKeyHash is a hash of the private key associated with the latest + // registered ACME account, in order to track changes made to registered account + // associated with the Issuer + LastPrivateKeyHash string `json:"lastPrivateKeyHash,omitempty"` } diff --git a/internal/apis/acme/v1alpha2/zz_generated.conversion.go b/internal/apis/acme/v1alpha2/zz_generated.conversion.go index 7de51defd..b50cd61e1 100644 --- a/internal/apis/acme/v1alpha2/zz_generated.conversion.go +++ b/internal/apis/acme/v1alpha2/zz_generated.conversion.go @@ -1285,6 +1285,7 @@ func Convert_acme_ACMEIssuerDNS01ProviderWebhook_To_v1alpha2_ACMEIssuerDNS01Prov func autoConvert_v1alpha2_ACMEIssuerStatus_To_acme_ACMEIssuerStatus(in *ACMEIssuerStatus, out *acme.ACMEIssuerStatus, s conversion.Scope) error { out.URI = in.URI out.LastRegisteredEmail = in.LastRegisteredEmail + out.LastPrivateKeyHash = in.LastPrivateKeyHash return nil } @@ -1296,6 +1297,7 @@ func Convert_v1alpha2_ACMEIssuerStatus_To_acme_ACMEIssuerStatus(in *ACMEIssuerSt func autoConvert_acme_ACMEIssuerStatus_To_v1alpha2_ACMEIssuerStatus(in *acme.ACMEIssuerStatus, out *ACMEIssuerStatus, s conversion.Scope) error { out.URI = in.URI out.LastRegisteredEmail = in.LastRegisteredEmail + out.LastPrivateKeyHash = in.LastPrivateKeyHash return nil } diff --git a/internal/apis/acme/v1alpha3/types_issuer.go b/internal/apis/acme/v1alpha3/types_issuer.go index afd4c0c66..1ef61b97b 100644 --- a/internal/apis/acme/v1alpha3/types_issuer.go +++ b/internal/apis/acme/v1alpha3/types_issuer.go @@ -629,4 +629,9 @@ type ACMEIssuerStatus struct { // associated with the Issuer // +optional LastRegisteredEmail string `json:"lastRegisteredEmail,omitempty"` + + // LastPrivateKeyHash is a hash of the private key associated with the latest + // registered ACME account, in order to track changes made to registered account + // associated with the Issuer + LastPrivateKeyHash string `json:"lastPrivateKeyHash,omitempty"` } diff --git a/internal/apis/acme/v1alpha3/zz_generated.conversion.go b/internal/apis/acme/v1alpha3/zz_generated.conversion.go index ef41efb6f..264b95c89 100644 --- a/internal/apis/acme/v1alpha3/zz_generated.conversion.go +++ b/internal/apis/acme/v1alpha3/zz_generated.conversion.go @@ -1285,6 +1285,7 @@ func Convert_acme_ACMEIssuerDNS01ProviderWebhook_To_v1alpha3_ACMEIssuerDNS01Prov func autoConvert_v1alpha3_ACMEIssuerStatus_To_acme_ACMEIssuerStatus(in *ACMEIssuerStatus, out *acme.ACMEIssuerStatus, s conversion.Scope) error { out.URI = in.URI out.LastRegisteredEmail = in.LastRegisteredEmail + out.LastPrivateKeyHash = in.LastPrivateKeyHash return nil } @@ -1296,6 +1297,7 @@ func Convert_v1alpha3_ACMEIssuerStatus_To_acme_ACMEIssuerStatus(in *ACMEIssuerSt func autoConvert_acme_ACMEIssuerStatus_To_v1alpha3_ACMEIssuerStatus(in *acme.ACMEIssuerStatus, out *ACMEIssuerStatus, s conversion.Scope) error { out.URI = in.URI out.LastRegisteredEmail = in.LastRegisteredEmail + out.LastPrivateKeyHash = in.LastPrivateKeyHash return nil } diff --git a/internal/apis/acme/v1beta1/types_issuer.go b/internal/apis/acme/v1beta1/types_issuer.go index 468422ed7..01d3be702 100644 --- a/internal/apis/acme/v1beta1/types_issuer.go +++ b/internal/apis/acme/v1beta1/types_issuer.go @@ -628,4 +628,9 @@ type ACMEIssuerStatus struct { // associated with the Issuer // +optional LastRegisteredEmail string `json:"lastRegisteredEmail,omitempty"` + + // LastPrivateKeyHash is a hash of the private key associated with the latest + // registered ACME account, in order to track changes made to registered account + // associated with the Issuer + LastPrivateKeyHash string `json:"lastPrivateKeyHash,omitempty"` } diff --git a/internal/apis/acme/v1beta1/zz_generated.conversion.go b/internal/apis/acme/v1beta1/zz_generated.conversion.go index c9b4237ac..a6855bc22 100644 --- a/internal/apis/acme/v1beta1/zz_generated.conversion.go +++ b/internal/apis/acme/v1beta1/zz_generated.conversion.go @@ -1285,6 +1285,7 @@ func Convert_acme_ACMEIssuerDNS01ProviderWebhook_To_v1beta1_ACMEIssuerDNS01Provi func autoConvert_v1beta1_ACMEIssuerStatus_To_acme_ACMEIssuerStatus(in *ACMEIssuerStatus, out *acme.ACMEIssuerStatus, s conversion.Scope) error { out.URI = in.URI out.LastRegisteredEmail = in.LastRegisteredEmail + out.LastPrivateKeyHash = in.LastPrivateKeyHash return nil } @@ -1296,6 +1297,7 @@ func Convert_v1beta1_ACMEIssuerStatus_To_acme_ACMEIssuerStatus(in *ACMEIssuerSta func autoConvert_acme_ACMEIssuerStatus_To_v1beta1_ACMEIssuerStatus(in *acme.ACMEIssuerStatus, out *ACMEIssuerStatus, s conversion.Scope) error { out.URI = in.URI out.LastRegisteredEmail = in.LastRegisteredEmail + out.LastPrivateKeyHash = in.LastPrivateKeyHash return nil } diff --git a/pkg/acme/accounts/registry.go b/pkg/acme/accounts/registry.go index d79569a86..df7ed0658 100644 --- a/pkg/acme/accounts/registry.go +++ b/pkg/acme/accounts/registry.go @@ -20,6 +20,7 @@ import ( "crypto/rsa" "crypto/sha256" "crypto/x509" + "encoding/base64" "errors" "net/http" "sync" @@ -45,7 +46,7 @@ type Registry interface { // 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 + IsKeyCheckSumCached(lastPrivateKeyHash string, privateKey *rsa.PrivateKey) bool Getter } @@ -195,19 +196,19 @@ func (r *registry) ListClients() map[string]acmecl.Interface { // 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 { +func (r *registry) IsKeyCheckSumCached(lastPrivateKeyHash string, privateKey *rsa.PrivateKey) bool { r.lock.RLock() defer r.lock.RUnlock() - if privateKey != nil { + if privateKey != nil && lastPrivateKeyHash != "" { privateKeyBytes := x509.MarshalPKCS1PrivateKey(privateKey) checksum := sha256.Sum256(privateKeyBytes) + checksumString := base64.StdEncoding.EncodeToString(checksum[:]) - if clientMeta, ok := r.clients[uid]; ok { - if clientMeta.keyChecksum == checksum { - return true - } + if lastPrivateKeyHash == checksumString { + return true } + } // Either there is no entry found in client cache for uid diff --git a/pkg/acme/accounts/registry_test.go b/pkg/acme/accounts/registry_test.go index 045dcd6c6..d4aa7dc9c 100644 --- a/pkg/acme/accounts/registry_test.go +++ b/pkg/acme/accounts/registry_test.go @@ -17,6 +17,9 @@ limitations under the License. package accounts import ( + "crypto/sha256" + "crypto/x509" + "encoding/base64" "net/http" "testing" @@ -157,6 +160,10 @@ func TestRegistry_AddClient_UpdatesClientPKChecksum(t *testing.T) { t.Fatal(err) } + pkBytes := x509.MarshalPKCS1PrivateKey(pk) + pkChecksum := sha256.Sum256(pkBytes) + pkChecksumString := base64.StdEncoding.EncodeToString(pkChecksum[:]) + // Register a new client r.AddClient(http.DefaultClient, "abc", cmacme.ACMEIssuer{}, pk, "cert-manager-test") l := r.ListClients() @@ -164,12 +171,12 @@ func TestRegistry_AddClient_UpdatesClientPKChecksum(t *testing.T) { t.Errorf("expected ListClients to have 1 item but it has %d", len(l)) } - isCached := r.IsKeyCheckSumCached("abc", pk) + isCached := r.IsKeyCheckSumCached(pkChecksumString, pk) if isCached == false { t.Fatal("checksum failed for same key") } - isCached = r.IsKeyCheckSumCached("abc", pk2) + isCached = r.IsKeyCheckSumCached(pkChecksumString, 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 681e495c5..2f7fb734e 100644 --- a/pkg/acme/accounts/test/registry.go +++ b/pkg/acme/accounts/test/registry.go @@ -34,7 +34,7 @@ type FakeRegistry struct { 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 + IsKeyCheckSumCachedFunc func(lastPrivateKeyHash string, privateKey *rsa.PrivateKey) bool } func (f *FakeRegistry) AddClient(client *http.Client, uid string, config cmacme.ACMEIssuer, privateKey *rsa.PrivateKey, userAgent string) { @@ -53,6 +53,6 @@ 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) +func (f *FakeRegistry) IsKeyCheckSumCached(lastPrivateKeyHash string, privateKey *rsa.PrivateKey) bool { + return f.IsKeyCheckSumCachedFunc(lastPrivateKeyHash, privateKey) } diff --git a/pkg/apis/acme/v1/types_issuer.go b/pkg/apis/acme/v1/types_issuer.go index f3f5e437f..fa94893a4 100644 --- a/pkg/apis/acme/v1/types_issuer.go +++ b/pkg/apis/acme/v1/types_issuer.go @@ -641,4 +641,10 @@ type ACMEIssuerStatus struct { // associated with the Issuer // +optional LastRegisteredEmail string `json:"lastRegisteredEmail,omitempty"` + + // LastPrivateKeyHash is a hash of the private key associated with the latest + // registered ACME account, in order to track changes made to registered account + // associated with the Issuer + // +optional + LastPrivateKeyHash string `json:"lastPrivateKeyHash,omitempty"` } diff --git a/pkg/issuer/acme/setup.go b/pkg/issuer/acme/setup.go index a3b5e65d3..89780a530 100644 --- a/pkg/issuer/acme/setup.go +++ b/pkg/issuer/acme/setup.go @@ -19,6 +19,8 @@ package acme import ( "context" "crypto/rsa" + "crypto/sha256" + "crypto/x509" "encoding/base64" "fmt" "net/url" @@ -147,7 +149,7 @@ func (a *Acme) Setup(ctx context.Context) error { return nil } - isPKChecksumSame := a.accountRegistry.IsKeyCheckSumCached(string(a.issuer.GetUID()), rsaPk) + isPKChecksumSame := a.accountRegistry.IsKeyCheckSumCached(a.issuer.GetStatus().ACMEStatus().LastPrivateKeyHash, rsaPk) // TODO: don't always clear the client cache. // In future we should intelligently manage items in the account cache @@ -314,8 +316,12 @@ func (a *Acme) Setup(ctx context.Context) error { status = cmmeta.ConditionTrue reason = successAccountRegistered msg = messageAccountRegistered + privateKeyBytes := x509.MarshalPKCS1PrivateKey(rsaPk) + checksum := sha256.Sum256(privateKeyBytes) + checksumString := base64.StdEncoding.EncodeToString(checksum[:]) a.issuer.GetStatus().ACMEStatus().URI = account.URI a.issuer.GetStatus().ACMEStatus().LastRegisteredEmail = registeredEmail + a.issuer.GetStatus().ACMEStatus().LastPrivateKeyHash = checksumString // ensure the cached client in the account registry is up to date a.accountRegistry.AddClient(httpClient, string(a.issuer.GetUID()), *a.issuer.GetSpec().ACME, rsaPk, a.userAgent) diff --git a/pkg/issuer/acme/setup_test.go b/pkg/issuer/acme/setup_test.go index ba4a61d64..caf44044d 100644 --- a/pkg/issuer/acme/setup_test.go +++ b/pkg/issuer/acme/setup_test.go @@ -253,6 +253,7 @@ func TestAcme_Setup(t *testing.T) { gen.SetIssuerACMEAccountURL(acmev2Prod), gen.SetIssuerACMEEmail(someEmail), gen.SetIssuerACMELastRegisteredEmail(someEmail), + gen.SetIssuerACMELastPrivateKeyHash(someString), gen.AddIssuerCondition( *gen.IssuerConditionFrom(readyTrueCondition, gen.SetIssuerConditionStatus(cmmeta.ConditionTrue)))), @@ -538,7 +539,7 @@ func TestAcme_Setup(t *testing.T) { AddClientFunc: func(string, cmacme.ACMEIssuer, *rsa.PrivateKey, string) { addClientWasCalled = true }, - IsKeyCheckSumCachedFunc: func(uid string, privateKey *rsa.PrivateKey) bool { + IsKeyCheckSumCachedFunc: func(lastPrivateKeyHash string, privateKey *rsa.PrivateKey) bool { return true }, } diff --git a/test/unit/gen/issuer.go b/test/unit/gen/issuer.go index e54fe2599..1ab46f34f 100644 --- a/test/unit/gen/issuer.go +++ b/test/unit/gen/issuer.go @@ -226,6 +226,16 @@ func SetIssuerACMELastRegisteredEmail(email string) IssuerModifier { } } +func SetIssuerACMELastPrivateKeyHash(privateKeyHash string) IssuerModifier { + return func(iss v1.GenericIssuer) { + status := iss.GetStatus() + if status.ACME == nil { + status.ACME = &cmacme.ACMEIssuerStatus{} + } + status.ACME.LastPrivateKeyHash = privateKeyHash + } +} + func SetIssuerCA(a v1.CAIssuer) IssuerModifier { return func(iss v1.GenericIssuer) { iss.GetSpec().CA = &a