certmagic
certmagic copied to clipboard
Add TOS checker
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 ofNewAccountFunc
? 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 asTermsOfServiceAgreed == 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
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!
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.
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?
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.
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.
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
You mean in gitea's non-interactive mode?
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.
So to make sure I understand, the change you're proposing would force noninteractive environments to become interactive?
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.
I don't think we can reliably fail in non-interactive mode, that would take sites offline.
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:?
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.