-
Notifications
You must be signed in to change notification settings - Fork 572
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
Implement externalAccountBinding #270
base: master
Are you sure you want to change the base?
Conversation
@systemcrash can you please resolve the conflicts so tests can run? thanks! |
Sorry - pushed from an older branch copy 👍 |
ca79565
to
3a2c01f
Compare
OK - rebased and updated. |
@systemcrash looks like some tests weren't successful |
self.assertTrue(( # can be in either order
"Updated contact details:\nmailto:[email protected]\nmailto:[email protected]" in log_string.decode("utf8")
or "Updated contact details:\nmailto:[email protected]\nmailto:[email protected]" in log_string.decode("utf8")
)) My code amends one thing there: Edit: there is not much point to update contact details if this is already (implicitly) done because contact was already provided (at reg). So the only interesting thing to do is to update the contact at this point, if it actually changed after initial registration (as I understand the process). Edit 2: Summary: you can always provide contact, but the only time it is actually 'updated' (using this code) is when it has changed from the previous registration. So to test for this, something like this must happen: first, register, second, run again with different contact. Edit 3: I did it this way so we save a round-trip, and only 'update' if something has changed: |
A plausible fix for your test case is to add around L228 in result = acme_tiny.main([
"--account-key", self.KEYS['account_key'].name,
"--csr", self.KEYS['domain_csr'].name,
"--acme-dir", self.tempdir,
"--directory-url", self.DIR_URL,
"--check-port", self.check_port,
"--contact", "mailto:[email protected]",
]) |
e.g. systemcrash@d05a275 |
@systemcrash It looks like this pull request puts acme-tiny over the 200 line limit, so I'm not going to merge it for now. However, I'll keep this pull request open if I decide to increase the line limit. |
I just wanted to express support for this pull request. While I can respect the desire to keep acme-tiny short and auditable. I feel like there is merit to supporting other SSL providers that are not LetsEncrypt. In the (extremely unlikely) event of LetsEncrypt ever suffers from a prolonged outage or became no longer trustworthy. The ability to immediately switch to a different issuer would be valuable to many sites worldwide. Still, I expect if such an outage did come to pass this could be merged in a hurry. |
Well, I use other ACME providers, so this PR is actual. But if we want to exclude the other 90% of providers out there for the sake of 10-15 lines of code, then sure. |
@diafygi Althrought letsencrypt is a widely used provider and doesn't require EAB, but please consider that EAB is part of the ACME protocol, not a CA specific requirement. Now, this is the only obstacle on migrating from certbot cli automation. |
I'd like to be another voice in support of merging this PR! :) We're using the DFN PKI (sorry, German only) who use EAB for 'sub-accounts', and acme-tiny would be a great fit here since it is easily auditable in comparison to the likes of Certbot and acme.sh. |
Also +1 for EAB from here... I also appreciate desire to keep thing small, or like with this tool keeping it specific to what its function is, but I also don't quite understand the desire for arbitrarily decided line count as alone it doesn't measure much anything ... |
210 or 220 doesn't sound as straight forward as 200 I guess. But the
functionality this opens is pretty good. :)
|
200 may look/sound more relatable due to common sense, but this is base10ish concept. let's go with 256, it is less arbitrary (for the audience) and still nice. |
https://datatracker.ietf.org/doc/html/rfc8555#section-7.3.4 Renamed `account` variable to the more appropriate: `response`. That is what this variable holds.
https://datatracker.ietf.org/doc/html/rfc8555#section-7.3.3 (which will never happen on Letsencrypt, but may on other CAs)
Squeaked in eAB in under 200 lines. Skipped the |
Why no merge? Is project dead? |
You're welcome to try the copy in my repo:eAB :) |
Please... merge this, so no need for manual tracking of many sources... :) |
https://datatracker.ietf.org/doc/html/rfc8555#section-7.3.4
Also send contact at register time if available (some CAs mandate this)
This increases line-count, while maintaining readability of the bare minimum code necessary to achieve eAB. Tested and verified with some CAs that mandate eAB. ( Zerossl, Buypass )