Add TOS checker #165

LecrisUT · 2022-01-31T02:29:26Z

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 <git@lecris.me>

…improved the CheckAccountTOS

mholt · 2022-02-02T03:58:26Z

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 · 2022-02-02T04:01:56Z

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.

mholt

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.

mholt · 2022-02-18T06:53:40Z

acmeclient.go

-		} else {
-			// can't prompt a user who isn't there; they should
-			// have reviewed the terms beforehand
-			am.Agreed = true


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

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.

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.

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

mholt

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.

mholt · 2022-03-24T19:50:09Z

acmemanager.go

+	if !client.account.TermsOfServiceAgreed {
+		return nil, false, fmt.Errorf("user must agree to CA terms")
+	}


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

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?

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.

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.

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

mholt · 2022-03-24T19:50:45Z

acmeclient.go

-		} else {
-			// can't prompt a user who isn't there; they should
-			// have reviewed the terms beforehand
-			am.Agreed = true


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 · 2022-03-25T16:47:55Z

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 · 2023-05-05T14:38:09Z

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 · 2023-05-05T15:35:59Z

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 · 2023-05-05T15:39:46Z

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 · 2023-05-05T15:54:04Z

You mean in gitea's non-interactive mode?

LecrisUT · 2023-05-05T15:56:53Z

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 · 2023-05-05T16:40:18Z

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

LecrisUT · 2023-05-05T17:02:25Z

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 · 2023-05-05T17:21:28Z

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

LecrisUT · 2023-05-05T17:33:02Z

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 · 2023-05-05T20:59:28Z

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.

Added a TOS checker

c69105d

Signed-off-by: Cristian Le <git@lecris.me>

LecrisUT mentioned this pull request Jan 31, 2022

Support custom ACME provider go-gitea/gitea#18340

Merged

Changed the default TOS check to outside of the account creation and …

6cbe423

…improved the CheckAccountTOS

mholt requested changes Mar 1, 2022

View reviewed changes

Refactored variable name and removed TOS check when doing Revoke

28e22e3

mholt requested changes Mar 24, 2022

View reviewed changes

mholt closed this May 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TOS checker #165

Add TOS checker #165

LecrisUT commented Jan 31, 2022 •

edited

mholt commented Feb 2, 2022

LecrisUT commented Feb 2, 2022

mholt left a comment

mholt Feb 18, 2022

LecrisUT Mar 2, 2022 •

edited

mholt Mar 24, 2022

LecrisUT Mar 27, 2022

mholt left a comment

mholt Mar 24, 2022

LecrisUT Mar 27, 2022

mholt May 5, 2023

LecrisUT May 5, 2023

mholt May 5, 2023

mholt Mar 24, 2022

mholt commented Mar 25, 2022

LecrisUT commented May 5, 2023

mholt commented May 5, 2023

LecrisUT commented May 5, 2023

mholt commented May 5, 2023

LecrisUT commented May 5, 2023

mholt commented May 5, 2023

LecrisUT commented May 5, 2023

mholt commented May 5, 2023

LecrisUT commented May 5, 2023

mholt commented May 5, 2023

Add TOS checker #165

Add TOS checker #165

Conversation

LecrisUT commented Jan 31, 2022 • edited

mholt commented Feb 2, 2022

LecrisUT commented Feb 2, 2022

mholt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LecrisUT Mar 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mholt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mholt commented Mar 25, 2022

LecrisUT commented May 5, 2023

mholt commented May 5, 2023

LecrisUT commented May 5, 2023

mholt commented May 5, 2023

LecrisUT commented May 5, 2023

mholt commented May 5, 2023

LecrisUT commented May 5, 2023

mholt commented May 5, 2023

LecrisUT commented May 5, 2023

mholt commented May 5, 2023

LecrisUT commented Jan 31, 2022 •

edited

LecrisUT Mar 2, 2022 •

edited