Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add TOS checker #165

Closed
wants to merge 3 commits into from
Closed

Add TOS checker #165

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c69105d
Added a TOS checker
LecrisUT Jan 23, 2022
6cbe423
Changed the default TOS check to outside of the account creation and …
LecrisUT Feb 2, 2022
28e22e3
Refactored variable name and removed TOS check when doing Revoke
LecrisUT Mar 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions acmeclient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Comment on lines -97 to -100
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worry that removing this could break some important behavior. Are you sure the new CheckAccountTOS() preserves this?

Copy link
Author

@LecrisUT LecrisUT Mar 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to my GoLand, newAcmeClientWithAccount was only used in doIssue and revoke. I have added the necessary checks for those in lines in acmemanager.go lines 336-338 and 454-456. And since it's a private function, it should cover all usecases as far as I understand golang.

As for the logic, the only change is that client.account.TermsOfServiceAgreed is no longer taken to be agreed by default, but instead use whatever the accountmanager option is set to (default is false if I understand correctly). I believe this is a safer choice so that the application manages if there are implicit TOS acceptance or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the past we've been bitten in major ways by being less careful with this logic; I think we should leave this unchanged unless there's a specific case that is breaking with this here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About not doing the proposed change, here is the workaround I had to do without it. It is messy to say the least.

However I think it is better if this actually breaks, because it only happens if the user did not accept the TOS, i.e. the current method assumes that the TOS is accepted regardless (hence the need for this PR). Yes, we do need to check the other testers and add CheckAccountTOS call beforehand to make sure they are automated properly. If you can point me to some of the potential places where it needs I can add appropriate test. Also can we add something like step_ca to the go tests to automate all the necessary tests?

}
account.TermsOfServiceAgreed = am.Agreed

Expand Down
38 changes: 38 additions & 0 deletions acmemanager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) (bool, string, error) {
agreed := am.Agreed
TOS := ""
LecrisUT marked this conversation as resolved.
Show resolved Hide resolved
// 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 agreed, TOS, 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
}
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 agreed, TOS, 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) {
Expand Down Expand Up @@ -302,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")
}
Comment on lines +336 to +338
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the CA should enforce this, not the client.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a standard for the CA to check/enforce the TOS? How do we communicate with the CA to confirm that the client is accepting TOS?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CA can choose to enforce this if that's important to them -- the CA sends the TOS link in the directory, and we send a boolean value during ACME to indicate whether client has agreed or not.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been a while I kinda lost track of this. The main idea here was that the URL/hash of the TOS should be shaved locally as a cache, and if the TOS is not agreed send it to the CA as not agreed? Isn't it equivalent to error-out there and ask the user to accept manually? You are considering that the CA would have special behaviour that allow to be used when new TOS is not accepted? But the user should be informed, so I think it is more reasonable to require the user to explicitly accept/decline the new TOS.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the TOS have changed, then the CA will return an error to the ACME client.

usingTestCA := client.usingTestCA()

nameSet := namesFromCSR(csr)
Expand Down Expand Up @@ -416,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
mholt marked this conversation as resolved.
Show resolved Hide resolved
if !client.account.TermsOfServiceAgreed {
return fmt.Errorf("user must agree to CA terms")
}

certs, err := parseCertsFromPEMBundle(cert.CertificatePEM)
if err != nil {
Expand Down