certmagic icon indicating copy to clipboard operation
certmagic copied to clipboard

Add TOS checker

Open LecrisUT opened this issue 2 years ago • 3 comments

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

LecrisUT avatar Jan 31 '22 02:01 LecrisUT

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!

mholt avatar Feb 02 '22 03:02 mholt

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.

LecrisUT avatar Feb 02 '22 04:02 LecrisUT

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?

mholt avatar Mar 25 '22 16:03 mholt

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.

LecrisUT avatar May 05 '23 14:05 LecrisUT

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.

mholt avatar May 05 '23 15:05 mholt

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

LecrisUT avatar May 05 '23 15:05 LecrisUT

You mean in gitea's non-interactive mode?

mholt avatar May 05 '23 15:05 mholt

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.

LecrisUT avatar May 05 '23 15:05 LecrisUT

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

mholt avatar May 05 '23 16:05 mholt

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.

LecrisUT avatar May 05 '23 17:05 LecrisUT

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

mholt avatar May 05 '23 17:05 mholt

You mean like when this is run while the site is in production :thinking:. 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 :thinking:?

LecrisUT avatar May 05 '23 17:05 LecrisUT

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 avatar May 05 '23 20:05 mholt