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.

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

Add TOS checker #165

Closed
wants to merge 3 commits into from
Closed

Add TOS checker #165

wants to merge 3 commits into from

Conversation

LecrisUT
Copy link

@LecrisUT LecrisUT commented Jan 31, 2022

Here's a sketch of a TOS checker that would close #164.

There might be missing locks and other standards, so let me know what needs to be changed, or directly edit them.

For my usecase in gitea it seems to be doing what I intended to do.

A few changes I want to discuss:

  • Can we refactor these lines to a separate function, and make newACMEClientWithAccount optionally depend on it, e.g. as part of NewAccountFunc? The reasoning is that
    • It can be useful to have the client without accepting the TOS, e.g. in here where it is used to expose the TOS url.
    • The current implementation of the checker overrides the default assumption that interactive == false is the same as TermsOfServiceAgreed == true. It's rather inelegant to have it like such.
  • I'm thinking of returning the types bool, string, error (TOS_Agreed, TOS_URL, error) to more easily distinguish if it's a TOS not being accepted or read errors. It will also be useful if the user wants to check if the TOS have changed

Signed-off-by: Cristian Le <[email protected]>
@mholt
Copy link
Member

mholt commented Feb 2, 2022

Thanks for the PR! I'm swamped with a few things this week including getting married but I'll try to circle back to this soon!

@LecrisUT
Copy link
Author

LecrisUT commented Feb 2, 2022

Oh, congratulations! Take your time :).

The latest commit has all of the refactoring I had in mind, so as long as I am not overlooking some interactions, it should be fine to add if you agree to add this interface.

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution; just remembered I started reviewing this and didn't finish my first pass of it due to being sick. I'm catching up on things but here's a couple notes to get you started.

Comment on lines -97 to -100
} else {
// can't prompt a user who isn't there; they should
// have reviewed the terms beforehand
am.Agreed = true
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

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?

acmemanager.go Outdated Show resolved Hide resolved
acmemanager.go Outdated Show resolved Hide resolved
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Should the CheckAccountTOS() method check to make sure the current version of the ToS have been agreed to? Since that can change, and might require re-agreement if updated.

Comment on lines +336 to +338
if !client.account.TermsOfServiceAgreed {
return nil, false, fmt.Errorf("user must agree to CA terms")
}
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.

Comment on lines -97 to -100
} else {
// can't prompt a user who isn't there; they should
// have reviewed the terms beforehand
am.Agreed = true
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.

@mholt
Copy link
Member

mholt commented Mar 25, 2022

I think I'd be more comfortable accepting this change if the only change was adding CheckAccountTOS() -- and leave the other changes out for now. What do you think about that?

@LecrisUT
Copy link
Author

LecrisUT commented May 5, 2023

I have lost track of this one, do you want to take over this one, or should I come back to it in a few weeks after I get caught up on it.

@mholt
Copy link
Member

mholt commented May 5, 2023

Yeah, sorry about dropping the ball on this one. Based on my understanding, I think we can close this, since the CA server will notify client if there is a change and new ToS needs to be accepted.

@LecrisUT
Copy link
Author

LecrisUT commented May 5, 2023

I think there was something relevant to gitea in order to make the process more explicit. Iirc in the version that was there, the TOS was automatically accepted without the user's explicit consent if it was run in non-interactive mode, which I wanted to fix in this PR

@mholt
Copy link
Member

mholt commented May 5, 2023

You mean in gitea's non-interactive mode?

@LecrisUT
Copy link
Author

LecrisUT commented May 5, 2023

I believe I had to make the portion of gitea related to acme be non-interactive because I did not know of a way to make that an interactive process.

@mholt
Copy link
Member

mholt commented May 5, 2023

So to make sure I understand, the change you're proposing would force noninteractive environments to become interactive?

@LecrisUT
Copy link
Author

LecrisUT commented May 5, 2023

Yes, I think this was so that gitea does not wait for any response that it would not have been able to handle. The current version was always accepting, but instead the PR was trying to check the current configuration first, and if the TOS was not accepted (or different) it should fail instead of wait for interactive response.

@mholt
Copy link
Member

mholt commented May 5, 2023

I don't think we can reliably fail in non-interactive mode, that would take sites offline.

@LecrisUT
Copy link
Author

LecrisUT commented May 5, 2023

You mean like when this is run while the site is in production 🤔. Isn't that a legally safe option despite being inconvenient, but I understand the difficulty of balancing. I don't think just throwing errors would be helpful because the admin might just ignore it. Maybe exposing this as a configurable option? This might not be an issue for LE, but for some random enterprise CA 🤔?

@mholt
Copy link
Member

mholt commented May 5, 2023

Ok, but I think we can't allow an external factor to take a site offline without consent. Maybe there could be an option for the admin to require manually approving ToS changes.

Sorry if this is disappointing after so long (my bad for leaving this PR hanging) -- I'll close it now though, maybe we can discuss an alternative approach in an issue.

@mholt mholt closed this May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check TOS
2 participants