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

ACME v2 #457

Closed
captncraig opened this issue Dec 22, 2017 · 66 comments
Closed

ACME v2 #457

captncraig opened this issue Dec 22, 2017 · 66 comments

Comments

@captncraig
Copy link
Contributor

I, along with many others, am super excited about Let's Encrypt supporting wildcard certs in 2018. According to their latest blog they should have a test endpoint up in early January.

I really like this library, as it has saved me from having to deal with acme directly at all. Is acme v2 and wildcard support on your radar at all? I am not even sure what the major differences are, or what is involved in upgrading. Will a single library be able to handle both, or will a separate package be needed?

If I had all my dreams come true I would love for lego to let me get wildcard certs asap, but I know this is may be a considerable undertaking. Thanks for everything.

@xenolf
Copy link
Member

xenolf commented Dec 22, 2017

Yes, I do plan on supporting the V2 API of Let's Encrypt along with ACMEv2. It will happen during my holidays in early January.

@mholt
Copy link
Contributor

mholt commented Dec 22, 2017

And I will be available on standby to help where needed!

@cpu
Copy link
Contributor

cpu commented Dec 22, 2017

I am not even sure what the major differences are, or what is involved in upgrading.

I left a short comment on a similar issue in the Dehydrated repo with a quick list of differences that might help folks get a rough idea of the work involved.

@emilevauge
Copy link

The staging endpoint for ACME v2 is out: https://community.letsencrypt.org/t/staging-endpoint-for-acme-v2/49605 😃

@xenolf
Copy link
Member

xenolf commented Jan 8, 2018

Yep. I'm already on it in a private branch. Will publish it to the repo once I'm happy with it so you people can have a look and give feedback.

@captncraig
Copy link
Contributor Author

At the risk of sounding ungrateful, is there a status update on this? I'd love to be able to integrate and test things out before LE goes live this month.

@ryanohoro
Copy link

Also eagerly waiting support so we can leverage it in Terraform!

@xenolf
Copy link
Member

xenolf commented Feb 13, 2018

Hey all! I'm sorry this is getting delayed but I had a lot more work in my day job than I anticipated during this time. Be assured that I'm working on this as much as possible though.

@xenolf
Copy link
Member

xenolf commented Feb 13, 2018

So another question just popped up and I wanted to see what all of you think.
The issue is package naming.
ACMEv2 is largely incompatible with V1 so I developed it in a separate package. Seeing that it will probably be the default version going forward I'd move the V1 implementation into a new package to archive it and have the V2 implementation in the acme package.
What do you think?

@captncraig
Copy link
Contributor Author

Do you think the client api will need to change though? My code uses Register, ObtainCertificate, and RenewCertificate. I don't see much in those signatures that would change from v1 to v2, even if all of the code underneath changes.

Possibly my saved json files for accounts and certificates would be invalid moving from v1 to v2? But I hope not.

In my dream scenario, the package recognizes automatically which version to use based on the directory endpoint you give it. Maybe have explicit v1 and v2 packages, but the acme package does some kind of auto-discovery dispatch thing? Does the protocol have an easy version identifier endpoint?

@xenolf
Copy link
Member

xenolf commented Feb 13, 2018

Do you think the client api will need to change though?

No, there are currently no changes to the existing API, only internal changes and new functions as V2 offers more endpoints.

Does the protocol have an easy version identifier endpoint?

No. The protocol is not versioned at all, the whole "V2" is just because LE chose to name their endpoint that way. The ACME protocol itself was never standardized and so did not have a V1.

Possibly my saved json files for accounts and certificates would be invalid moving from v1 to v2? But I hope not.

Most likely. All messages are different and existing AuthZ are not transferred to the V2 endpoint, accounts are though so we may look at transferring them.

@captncraig
Copy link
Contributor Author

Looks like they delayed their live rollout a little bit. https://community.letsencrypt.org/t/acmev2-and-wildcard-launch-delay/53654

@nmengin
Copy link
Contributor

nmengin commented Feb 22, 2018

Hello @xenolf,

Seeing that it will probably be the default version going forward I'd move the V1 implementation into a new package to archive it and have the V2 implementation in the acme package

If the API does not change, maybe the best solution would be to use type aliases?
Thus, you can define two packages as acmev1 and acmev2 and then using type aliases to define all the acme package types by linking them to acmev1 types and mark all the acme types as deprecated.

WDYT?

@lenovouser
Copy link
Contributor

community.letsencrypt.org/acme-v2-and-wildcard-certificate-support-is-live

@captncraig
Copy link
Contributor Author

That's awesome news. Seems we're waiting on @xenolf here, and maybe there are some time demands outside his control.

I've got a dev environment set up and I'll try giving it a shot at converting to v2 myself.

@xenolf
Copy link
Member

xenolf commented Mar 13, 2018

Should only be a couple of hours :)

@xenolf
Copy link
Member

xenolf commented Mar 14, 2018

You can find initial ACMEv2 support in the acmev2 branch.
Please keep in mind that this is not completely done yet as some things like docs and final naming on the library are missing. I put it into its own folder for now because it was easier to write this way. I'm not sure if it will stay like that though - I'm open for suggestions.

Anyways, you should be able to get a certificate from the V2 endpoint (yes, wildcards) and it should not need many changes to your code.
For users of the CLI binary, it will attempt to convert your account to the new format on first run. Certificates can't be transferred and need to be requested again.

Please report any problems you encounter with it and also please let me know your thoughts about the naming - I spent a considerable amount of time on thinking about that and didn't come up with a really good solution.

@stephenjamieson
Copy link

Had to remove Azure as a dns provider, seems like they removed the sdk for dns or something from github, didn't investigate too much.

When I try a run, I get the following error:

2018/03/14 01:45:12 Could not load account for<emailHere>. Registration is nil -> &errors.errorString{s:"directory missing new registration URL"}

@shaunbro
Copy link

I replaced the import string in the azure.go providers directory with
"github.com/Azure/azure-sdk-for-go/services/dns/mgmt/2017-09-01/dns"

acmev2 branch has
"github.com/Azure/azure-sdk-for-go/arm/dns" <-- which doesn't exist

the go update still broke after that so I just removed azure and updated.
@stephenjamieson I got the same error using the non acmev2 branch against the acmev2 endpoint

Once the go update completed (-azure) the process worked flawlessly.

@stephenjamieson
Copy link

stephenjamieson commented Mar 14, 2018

Hrm, @shaunbro I did switch to the acmev2 branch, not sure what else I'm missing.

edit: nevermind it worked, didn't occur to me I needed to add --server="https://acme-v02.api.letsencrypt.org/directory"

@captncraig
Copy link
Contributor Author

Awesome. Super excited to test this tomorrow. One thing I noticed: the v1 and v2 directory endpoints have a noticeably different structure. Field names moved from underscore_case to camelCase, and some things like that.

What if...

  • We detect from the directory endpoint given which version of the protocol to use.
  • acmev1 and acmev2 packages implement a thin client interface, and the acme package does the higher level orchestration to get certificates and perform bigger actions in a version-independent way.

@mholt
Copy link
Contributor

mholt commented Mar 14, 2018

ACME v1 was never standardized and will be phased out. I don't really see a point in trying to keep it alive, other than to have it available for a while if people still need it; but I think everyone will be moving to v2. And if not, they'll have to anyway as v1 will be discontinued. In other words, I would rather we not make design decisions with the hope of preserving v1... but that's just me. 😇

@captncraig
Copy link
Contributor Author

Wow! this is working great. I managed to issue some certs with multiple wildcards and everything.

I did manage to get it to crash a few times. I think I failed some validations and then ran again, and it gave some super odd output and paniced:

2018/03/14 11:33:27 client.go:40: [INFO][teststackoverflow.com, www.teststackoverflow.com, *.teststackoverflow.com, *.ww33w.teststackoverflow.com] acme: Obtaining bundled SAN certificate
2018/03/14 11:33:27 client.go:40: [INFO][teststackoverflow.com, www.teststackoverflow.com, *.teststackoverflow.com, *.ww33w.teststackoverflow.com] acme: Validations succeeded; requesting certificates
panic: runtime error: index out of range

goroutine 1 [running]:
github.com/StackExchange/dnscontrol/vendor/github.com/xenolf/lego/acmev2.(*Client).requestCertificateForOrder(0xc0420bc900, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
        D:/gopath/src/github.com/StackExchange/dnscontrol/vendor/github.com/xenolf/lego/acmev2/client.go:591 +0x490
github.com/StackExchange/dnscontrol/vendor/github.com/xenolf/lego/acmev2.(*Client).ObtainCertificate(0xc0420bc900, 0xc04252e600, 0x4, 0x4, 0x49d801, 0x0, 0x0, 0xc0425f9401, 0x0, 0x0, ...)
        D:/gopath/src/github.com/StackExchange/dnscontrol/vendor/github.com/xenolf/lego/acmev2/client.go:372 +0x453

This is against the staging LE server. It seems to be pretty consistent, so maybe I can help get more output to debug with.

@xenolf
Copy link
Member

xenolf commented Mar 14, 2018

Wow that's a weird error. I am sure it didn't do any validations in under a second. Were there some odd circumstances you could identify? If a challenge fails to verify, it should have been logged.

@captncraig
Copy link
Contributor Author

The previous run to the panics ended with acme: Error 400 - urn:ietf:params:acme:error:connection - DNS problem: NXDOMAIN looking up TXT for _acme-challenge.teststackoverflow.com

@xenolf
Copy link
Member

xenolf commented Mar 14, 2018

I will see if I can reproduce when I get home today.

@xenolf
Copy link
Member

xenolf commented Mar 14, 2018

@captncraig I pushed the missing return statements. The process should now properly abort on error.

@xenolf
Copy link
Member

xenolf commented Mar 14, 2018

@captncraig It seems like your issue is because of overlapping wildcard domains. An issue will be opened in boulder to improve the error message that is returned.

@captncraig
Copy link
Contributor Author

What do you mean overlapping? *.example.com and *.foo.example.com don't overlap.

It does seem related to boulder though. If I request without any wildcards it proceeds. If I add any wildcards now it gives the "error creating order". I think I may have messed up something in my account.

@captncraig
Copy link
Contributor Author

Oh, the wildcard name conflicts with a non-wildcard name. Gotcha. Can't do foo.example.com and *.example.com on one cert. Thanks!

@cpu
Copy link
Contributor

cpu commented Mar 14, 2018

@captncraig It seems like your issue is because of overlapping wildcard domains. An issue will be opened in boulder to improve the error message that is returned.

letsencrypt/boulder#3558 is the Boulder issue to track

(edit: Thanks for the bug report folks!!!)

@mholt
Copy link
Contributor

mholt commented Mar 14, 2018

(Another good reason Caddy doesn't try to put multiple names on a cert! 🕺 - keeps things simpler)

This is working pretty great in Caddy. I've just about figured out the updated flow as far as user accounts and wildcards go. Great work, @xenolf!

@Knight1
Copy link
Contributor

Knight1 commented Mar 14, 2018

When I use renew lego is using *.domain instead of _. it is also another domain from many i use :)
Error while loading the certificate for domain *. (..) no such file or directory

@xenolf
Copy link
Member

xenolf commented Mar 14, 2018

@Knight1 As I wrote above, old certificate descriptors are not compatible with the new format. As such you need to use run with all your domains instead of renew once

@Knight1
Copy link
Contributor

Knight1 commented Mar 14, 2018

I used run many times but i have an idea. I switched the commonName from .us Domain to .de. And he used the .us Domain. Maybe this is the case here. Will test it tomorrow but you might have an idea :)

@jahmad
Copy link

jahmad commented Mar 15, 2018

@xenolf in the master branch, lego will use the first domains value mentioned by user as certificate's CN and file name. in this version it uses the first domains in alphabetical order (i think) for run only. for renew it still try to use master's behavior, so it can't find the files.

FWIW, I still prefer the old behavior so I can choose which domain as CN.

@xenolf
Copy link
Member

xenolf commented Mar 15, 2018

@jahmad That is a regression and not expected. Boulder seems to return the identifiers in alphabetical order in the response to the order creation request and not in the same order we submit them. I'll take a closer look at that.

@cpu
Copy link
Contributor

cpu commented Mar 15, 2018

@captncraig It seems like your issue is because of overlapping wildcard domains. An issue will be opened in boulder to improve the error message that is returned.

letsencrypt/boulder#3558 is the Boulder issue to track

As a quick update: letsencrypt/boulder#3558 is fixed in master, and the staging environment. The bug was not present in production. There shouldn't be any further staging newOrder 500s. Please open a Boulder issue if you find any!

Thanks!

@xenolf
Copy link
Member

xenolf commented Mar 15, 2018

@cpu Do you know if it is intentional that boulder returns the identifiers you request in an order in alphabetical order in the response to "new-order"?

@cpu
Copy link
Contributor

cpu commented Mar 15, 2018

@cpu Do you know if it is intentional that boulder returns the identifiers you request in an order in alphabetical order in the response to "new-order"?

It's intentional. This happens at the RA level. We normalize the names before storing the order with core.UniqueLowerNames:
https://github.com/letsencrypt/boulder/blob/7e5f22dd8d35c8f48dac8ff241b61cee41296dac/ra/ra.go#L1715

That function returns the names in alphabetical order: https://github.com/letsencrypt/boulder/blob/7e5f22dd8d35c8f48dac8ff241b61cee41296dac/core/util.go#L203:L218

The specification doesn't say anything about identifier order in the order (wow that's confusing to say) so I think the safest bet is to not assume anything about the order of the identifiers in the order returned by the server. Boulder sorts them but another ACME server may not. If pressed to change I would probably advocate for Boulder to move towards randomizing the identifier order in the order response to emphasize that it shouldn't be relied upon (Sounds like a good idea for Pebble actually! - letsencrypt/pebble#104).

@xenolf
Copy link
Member

xenolf commented Mar 15, 2018

@cpu thanks for the clarification!

@arnested
Copy link

FWIW, I still prefer the old behavior so I can choose which domain as CN.

As far as I can tell common name usage is deprecated and will probably be removed from browsers: https://groups.google.com/a/chromium.org/forum/#!topic/security-dev/IGT2fLJrAeo

@jahmad
Copy link

jahmad commented Mar 15, 2018

@arnested my main concern is seeing my typo domain as CN, it just feels ugly :)
that link You provided seems only talk about requiring SAN, which I believe won't affect any LE's certificates.

@zatricky
Copy link

zatricky commented Mar 22, 2018

@arnested @jahmad CN deprecation is at the "you can add it but nobody will listen anyway" stage. Only took 17 years for reality to catch up to the RFC. Heh.

@jahmad
Copy link

jahmad commented Mar 22, 2018

@zatricky nobody disputing that. Yes, browser will only use SAN and ignore CN for verification. There is no problem whatsoever with that reality. None. Zilch.
I'm only talking about cosmetic change, what's displayed first time when users check the certificate. and how I actually manage my own certs. and @xenolf already recognize it as regression anyway...

@tamalsaha
Copy link

@xenolf , is there an update when acmev2 branch will be merged? Is there any blocker other than the CN order issue?

@aramase
Copy link

aramase commented Apr 25, 2018

@xenolf Any update on when the acmev2 branch will be merged to master?

@winteraz
Copy link

any update?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests