From c69105db56649b3bc3f86007d5b8bb0e2dcdcd9c Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Sun, 23 Jan 2022 22:47:54 +0900 Subject: [PATCH 1/3] Added a TOS checker Signed-off-by: Cristian Le --- acmemanager.go | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/acmemanager.go b/acmemanager.go index 82b6cc12..78d222a4 100644 --- a/acmemanager.go +++ b/acmemanager.go @@ -229,6 +229,37 @@ func (am *ACMEManager) PreCheck(_ context.Context, names []string, interactive b return am.getEmail(interactive) } +func (am *ACMEManager) CheckAccountTOS(ctx context.Context, useTestCA, interactive bool) error { + initialAMAgreed := am.Agreed + client, err := am.newACMEClientWithAccount(ctx, useTestCA, interactive) + if err != nil { + return err + } + // override the default logic when creating new account in newACMEClient + if !interactive && !initialAMAgreed && am.Agreed { + am.Agreed = false + account := client.account + account.TermsOfServiceAgreed = false + err = am.saveAccount(client.acmeClient.Directory, account) + if err != nil { + return err + } + return fmt.Errorf("initialized account non-interactively, but TOS was not accepted") + } + if !client.account.TermsOfServiceAgreed { + var termsURL string + dir, err := client.acmeClient.GetDirectory(ctx) + if err != nil { + return err + } + if dir.Meta != nil { + termsURL = dir.Meta.TermsOfService + } + return fmt.Errorf("cached account does not accept CA's TOS [%s]", termsURL) + } + return nil +} + // Issue implements the Issuer interface. It obtains a certificate for the given csr using // the ACME configuration am. func (am *ACMEManager) Issue(ctx context.Context, csr *x509.CertificateRequest) (*IssuedCertificate, error) { From 6cbe4239032afa1737b51655a0b7515b640d9f0e Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Wed, 2 Feb 2022 12:55:51 +0900 Subject: [PATCH 2/3] Changed the default TOS check to outside of the account creation and improved the CheckAccountTOS --- acmeclient.go | 6 +----- acmemanager.go | 55 ++++++++++++++++++++++++++++---------------------- 2 files changed, 32 insertions(+), 29 deletions(-) diff --git a/acmeclient.go b/acmeclient.go index a22dc19a..8e80b57d 100644 --- a/acmeclient.go +++ b/acmeclient.go @@ -76,7 +76,7 @@ func (am *ACMEManager) newACMEClientWithAccount(ctx context.Context, useTestCA, } } - // agree to terms + // Prompt to agree to TOS, otherwise use AccountManager's settings if interactive { if !am.Agreed { var termsURL string @@ -94,10 +94,6 @@ func (am *ACMEManager) newACMEClientWithAccount(ctx context.Context, useTestCA, } } } - } else { - // can't prompt a user who isn't there; they should - // have reviewed the terms beforehand - am.Agreed = true } account.TermsOfServiceAgreed = am.Agreed diff --git a/acmemanager.go b/acmemanager.go index 78d222a4..2d225193 100644 --- a/acmemanager.go +++ b/acmemanager.go @@ -229,35 +229,35 @@ func (am *ACMEManager) PreCheck(_ context.Context, names []string, interactive b return am.getEmail(interactive) } -func (am *ACMEManager) CheckAccountTOS(ctx context.Context, useTestCA, interactive bool) error { - initialAMAgreed := am.Agreed +func (am *ACMEManager) CheckAccountTOS(ctx context.Context, useTestCA, interactive bool) (bool, string, error) { + agreed := am.Agreed + TOS := "" + // Make sure the email is retrieved first + err := am.getEmail(interactive) + if err != nil { + return agreed, TOS, err + } + + // Create the new client if necessary and get a client client, err := am.newACMEClientWithAccount(ctx, useTestCA, interactive) if err != nil { - return err + return agreed, TOS, err } - // override the default logic when creating new account in newACMEClient - if !interactive && !initialAMAgreed && am.Agreed { - am.Agreed = false - account := client.account - account.TermsOfServiceAgreed = false - err = am.saveAccount(client.acmeClient.Directory, account) - if err != nil { - return err - } - return fmt.Errorf("initialized account non-interactively, but TOS was not accepted") + agreed = client.account.TermsOfServiceAgreed + // Get the most recent TOS + var dir acme.Directory + dir, err = client.acmeClient.GetDirectory(ctx) + if err != nil { + return agreed, TOS, err } - if !client.account.TermsOfServiceAgreed { - var termsURL string - dir, err := client.acmeClient.GetDirectory(ctx) - if err != nil { - return err - } - if dir.Meta != nil { - termsURL = dir.Meta.TermsOfService - } - return fmt.Errorf("cached account does not accept CA's TOS [%s]", termsURL) + if dir.Meta != nil { + TOS = dir.Meta.TermsOfService + } + // If no TOS is found, then it should be implied to be accepted + if len(TOS) == 0 { + agreed = true } - return nil + return agreed, TOS, nil } // Issue implements the Issuer interface. It obtains a certificate for the given csr using @@ -333,6 +333,9 @@ func (am *ACMEManager) doIssue(ctx context.Context, csr *x509.CertificateRequest if err != nil { return nil, false, err } + if !client.account.TermsOfServiceAgreed { + return nil, false, fmt.Errorf("user must agree to CA terms") + } usingTestCA := client.usingTestCA() nameSet := namesFromCSR(csr) @@ -447,6 +450,10 @@ func (am *ACMEManager) Revoke(ctx context.Context, cert CertificateResource, rea if err != nil { return err } + // TODO: Maybe the user does not need to accept TOS just to revoke a certificate + if !client.account.TermsOfServiceAgreed { + return fmt.Errorf("user must agree to CA terms") + } certs, err := parseCertsFromPEMBundle(cert.CertificatePEM) if err != nil { From 28e22e39baf81ee4b635fc7c44209c14fd38f662 Mon Sep 17 00:00:00 2001 From: Cristian Le Date: Wed, 2 Mar 2022 13:17:49 +0900 Subject: [PATCH 3/3] Refactored variable name and removed TOS check when doing Revoke --- acmemanager.go | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/acmemanager.go b/acmemanager.go index 2d225193..5e02832a 100644 --- a/acmemanager.go +++ b/acmemanager.go @@ -231,33 +231,33 @@ func (am *ACMEManager) PreCheck(_ context.Context, names []string, interactive b func (am *ACMEManager) CheckAccountTOS(ctx context.Context, useTestCA, interactive bool) (bool, string, error) { agreed := am.Agreed - TOS := "" + termsURL := "" // Make sure the email is retrieved first err := am.getEmail(interactive) if err != nil { - return agreed, TOS, err + return agreed, termsURL, err } // Create the new client if necessary and get a client client, err := am.newACMEClientWithAccount(ctx, useTestCA, interactive) if err != nil { - return agreed, TOS, err + return agreed, termsURL, err } agreed = client.account.TermsOfServiceAgreed // Get the most recent TOS var dir acme.Directory dir, err = client.acmeClient.GetDirectory(ctx) if err != nil { - return agreed, TOS, err + return agreed, termsURL, err } if dir.Meta != nil { - TOS = dir.Meta.TermsOfService + termsURL = dir.Meta.TermsOfService } // If no TOS is found, then it should be implied to be accepted - if len(TOS) == 0 { + if len(termsURL) == 0 { agreed = true } - return agreed, TOS, nil + return agreed, termsURL, nil } // Issue implements the Issuer interface. It obtains a certificate for the given csr using @@ -450,10 +450,6 @@ func (am *ACMEManager) Revoke(ctx context.Context, cert CertificateResource, rea if err != nil { return err } - // TODO: Maybe the user does not need to accept TOS just to revoke a certificate - if !client.account.TermsOfServiceAgreed { - return fmt.Errorf("user must agree to CA terms") - } certs, err := parseCertsFromPEMBundle(cert.CertificatePEM) if err != nil {