diff --git a/deploy/manifests/00-crds.yaml b/deploy/manifests/00-crds.yaml index c66e40826..db9079aa3 100644 --- a/deploy/manifests/00-crds.yaml +++ b/deploy/manifests/00-crds.yaml @@ -750,6 +750,11 @@ spec: properties: acme: properties: + 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 uri: description: URI is the unique account identifier, which can also be used to retrieve account details from the CA @@ -1029,6 +1034,11 @@ spec: properties: acme: properties: + 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 uri: description: URI is the unique account identifier, which can also be used to retrieve account details from the CA diff --git a/pkg/acme/client/fake.go b/pkg/acme/client/fake.go index 0aa0d3ab5..7dd91b9a2 100644 --- a/pkg/acme/client/fake.go +++ b/pkg/acme/client/fake.go @@ -42,6 +42,7 @@ type FakeACME struct { FakeHTTP01ChallengeResponse func(token string) (string, error) FakeDNS01ChallengeRecord func(token string) (string, error) FakeDiscover func(ctx context.Context) (acme.Directory, error) + FakeUpdateAccount func(ctx context.Context, a *acme.Account) (*acme.Account, error) } func (f *FakeACME) CreateOrder(ctx context.Context, order *acme.Order) (*acme.Order, error) { @@ -143,3 +144,10 @@ func (f *FakeACME) Discover(ctx context.Context) (acme.Directory, error) { // empty directory here will be fine return acme.Directory{}, nil } + +func (f *FakeACME) UpdateAccount(ctx context.Context, a *acme.Account) (*acme.Account, error) { + if f.FakeUpdateAccount != nil { + return f.FakeUpdateAccount(ctx, a) + } + return nil, fmt.Errorf("UpdateAccount not implemented") +} diff --git a/pkg/acme/client/interfaces.go b/pkg/acme/client/interfaces.go index 1ad6bcede..ee8179744 100644 --- a/pkg/acme/client/interfaces.go +++ b/pkg/acme/client/interfaces.go @@ -37,6 +37,7 @@ type Interface interface { HTTP01ChallengeResponse(token string) (string, error) DNS01ChallengeRecord(token string) (string, error) Discover(ctx context.Context) (acme.Directory, error) + UpdateAccount(ctx context.Context, a *acme.Account) (*acme.Account, error) } var _ Interface = &acme.Client{} diff --git a/pkg/acme/client/middleware/logger.go b/pkg/acme/client/middleware/logger.go index 1e121c09f..12e747060 100644 --- a/pkg/acme/client/middleware/logger.go +++ b/pkg/acme/client/middleware/logger.go @@ -103,3 +103,8 @@ func (l *Logger) Discover(ctx context.Context) (acme.Directory, error) { klog.Infof("Calling Discover") return l.baseCl.Discover(ctx) } + +func (l *Logger) UpdateAccount(ctx context.Context, a *acme.Account) (*acme.Account, error) { + klog.Infof("Calling UpdateAccount") + return l.baseCl.UpdateAccount(ctx, a) +} diff --git a/pkg/apis/certmanager/v1alpha1/types_issuer.go b/pkg/apis/certmanager/v1alpha1/types_issuer.go index 8348a3750..b4773a459 100644 --- a/pkg/apis/certmanager/v1alpha1/types_issuer.go +++ b/pkg/apis/certmanager/v1alpha1/types_issuer.go @@ -538,6 +538,12 @@ type ACMEIssuerStatus struct { // account details from the CA // +optional URI string `json:"uri,omitempty"` + + // LastRegisteredEmail is the email associated with the latest registered + // ACME account, in order to track changes made to registered account + // associated with the Issuer + // +optional + LastRegisteredEmail string `json:"lastRegisteredEmail,omitempty"` } // IssuerCondition contains condition information for an Issuer. diff --git a/pkg/issuer/acme/setup.go b/pkg/issuer/acme/setup.go index 0d2a388b3..9643d01c6 100644 --- a/pkg/issuer/acme/setup.go +++ b/pkg/issuer/acme/setup.go @@ -23,7 +23,7 @@ import ( "net/url" "strings" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -40,12 +40,14 @@ import ( const ( errorAccountRegistrationFailed = "ErrRegisterACMEAccount" errorAccountVerificationFailed = "ErrVerifyACMEAccount" + errorAccountUpdateFailed = "ErrUpdateACMEAccount" successAccountRegistered = "ACMEAccountRegistered" successAccountVerified = "ACMEAccountVerified" messageAccountRegistrationFailed = "Failed to register ACME account: " messageAccountVerificationFailed = "Failed to verify ACME account: " + messageAccountUpdateFailed = "Failed to update ACME account:" messageAccountRegistered = "The ACME account was registered with the ACME server" messageAccountVerified = "The ACME account was verified with the ACME server" ) @@ -147,12 +149,14 @@ func (a *Acme) Setup(ctx context.Context) error { Status: v1alpha1.ConditionTrue, }) - // If the Host components of the server URL and the account URL match, then + // If the Host components of the server URL and the account URL match, + // and the cached email matches the registered email, then // we skip re-checking the account status to save excess calls to the // ACME api. if hasReadyCondition && a.issuer.GetStatus().ACMEStatus().URI != "" && - parsedAccountURL.Host == parsedServerURL.Host { + parsedAccountURL.Host == parsedServerURL.Host && + a.issuer.GetStatus().ACMEStatus().LastRegisteredEmail == a.issuer.GetSpec().ACME.Email { log.Info("skipping re-verifying ACME account as cached registration " + "details look sufficient") return nil @@ -192,9 +196,57 @@ func (a *Acme) Setup(ctx context.Context) error { return err } + // if we got an account successfully, we must check if the registered + // email is the same as in the issuer spec + + // if no email was specified, then registeredEmail will remain empty + registeredEmail := "" + if len(account.Contact) > 0 { + registeredEmail = strings.Replace(account.Contact[0], "mailto:", "", 1) + } + + // if they are different, we update the account + specEmail := a.issuer.GetSpec().ACME.Email + if registeredEmail != specEmail { + log.Info("Updating ACME account with email %s", specEmail) + emailurl := []string(nil) + if a.issuer.GetSpec().ACME.Email != "" { + emailurl = []string{fmt.Sprintf("mailto:%s", strings.ToLower(specEmail))} + } + account.Contact = emailurl + + account, err = cl.UpdateAccount(ctx, account) + if err != nil { + s := messageAccountUpdateFailed + err.Error() + log.Error(err, "failed to update ACME account") + a.Recorder.Event(a.issuer, v1.EventTypeWarning, errorAccountUpdateFailed, s) + apiutil.SetIssuerCondition(a.issuer, v1alpha1.IssuerConditionReady, v1alpha1.ConditionFalse, errorAccountUpdateFailed, s) + + acmeErr, ok := err.(*acmeapi.Error) + // If this is not an ACME error, we will simply return it and retry later + if !ok { + return err + } + + // If the status code is 400 (BadRequest), we will *not* retry this registration + // as it implies that something about the request (i.e. email address or private key) + // is invalid. + if acmeErr.StatusCode >= 400 && acmeErr.StatusCode < 500 { + log.Error(acmeErr, "skipping updating account email as a "+ + "BadRequest response was returned from the ACME server") + return nil + } + + // Otherwise if we receive anything other than a 400, we will retry. + return err + } + + } + log.Info("verified existing registration with ACME server") apiutil.SetIssuerCondition(a.issuer, v1alpha1.IssuerConditionReady, v1alpha1.ConditionTrue, successAccountRegistered, messageAccountRegistered) a.issuer.GetStatus().ACMEStatus().URI = account.URL + a.issuer.GetStatus().ACMEStatus().LastRegisteredEmail = registeredEmail return nil } diff --git a/test/e2e/suite/issuers/acme/issuer.go b/test/e2e/suite/issuers/acme/issuer.go index 3374f0b8c..55e04bfd3 100644 --- a/test/e2e/suite/issuers/acme/issuer.go +++ b/test/e2e/suite/issuers/acme/issuer.go @@ -33,6 +33,7 @@ import ( const invalidACMEURL = "http://not-a-real-acme-url.com" const testingACMEEmail = "test@example.com" +const testingACMEEmailAlternative = "another-test@example.com" const testingACMEPrivateKey = "test-acme-private-key" var _ = framework.CertManagerDescribe("ACME Issuer", func() { @@ -190,4 +191,79 @@ var _ = framework.CertManagerDescribe("ACME Issuer", func() { }) Expect(err).NotTo(HaveOccurred()) }) + + It("should handle updates to the email field", func() { + acmeURL := pebble.Details().Host + acmeIssuer := util.NewCertManagerACMEIssuer(issuerName, acmeURL, testingACMEEmail, testingACMEPrivateKey) + + By("Creating an Issuer") + acmeIssuer, err := f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name).Create(acmeIssuer) + Expect(err).NotTo(HaveOccurred()) + + By("Waiting for Issuer to become Ready") + err = util.WaitForIssuerCondition(f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name), + acmeIssuer.Name, + v1alpha1.IssuerCondition{ + Type: v1alpha1.IssuerConditionReady, + Status: v1alpha1.ConditionTrue, + }) + Expect(err).NotTo(HaveOccurred()) + + By("Verifying the ACME account URI is set") + err = util.WaitForIssuerStatusFunc(f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name), + acmeIssuer.Name, + func(i *v1alpha1.Issuer) (bool, error) { + if i.GetStatus().ACMEStatus().URI == "" { + return false, nil + } + return true, nil + }) + Expect(err).NotTo(HaveOccurred()) + + By("Verifying ACME account private key exists") + secret, err := f.KubeClientSet.CoreV1().Secrets(f.Namespace.Name).Get(testingACMEPrivateKey, metav1.GetOptions{}) + Expect(err).NotTo(HaveOccurred()) + if len(secret.Data) != 1 { + Fail("Expected 1 key in ACME account private key secret, but there was %d", len(secret.Data)) + } + + By("Verifying the ACME account email has been registered") + err = util.WaitForIssuerStatusFunc(f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name), + acmeIssuer.Name, + func(i *v1alpha1.Issuer) (bool, error) { + registeredEmail := i.GetStatus().ACMEStatus().LastRegisteredEmail + if registeredEmail == testingACMEEmail { + return true, nil + } + return false, nil + }) + Expect(err).NotTo(HaveOccurred()) + + By("Changing the email field") + acmeIssuer, err = f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name).Get(acmeIssuer.Name, metav1.GetOptions{}) + acmeIssuer.Spec.ACME.Email = testingACMEEmailAlternative + acmeIssuer, err = f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name).Update(acmeIssuer) + Expect(err).NotTo(HaveOccurred()) + + By("Waiting for Issuer to become Ready") + err = util.WaitForIssuerCondition(f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name), + acmeIssuer.Name, + v1alpha1.IssuerCondition{ + Type: v1alpha1.IssuerConditionReady, + Status: v1alpha1.ConditionTrue, + }) + Expect(err).NotTo(HaveOccurred()) + + By("Verifying the changed ACME account email has been registered") + err = util.WaitForIssuerStatusFunc(f.CertManagerClientSet.CertmanagerV1alpha1().Issuers(f.Namespace.Name), + acmeIssuer.Name, + func(i *v1alpha1.Issuer) (bool, error) { + registeredEmail := i.GetStatus().ACMEStatus().LastRegisteredEmail + if registeredEmail == testingACMEEmailAlternative { + return true, nil + } + return false, nil + }) + Expect(err).NotTo(HaveOccurred()) + }) })