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

[Account.orders] should be a Uri.t option, not a list #27

Closed
torinnd opened this issue Sep 17, 2021 · 2 comments · Fixed by #28
Closed

[Account.orders] should be a Uri.t option, not a list #27

torinnd opened this issue Sep 17, 2021 · 2 comments · Fixed by #28

Comments

@torinnd
Copy link

torinnd commented Sep 17, 2021

Re: https://github.com/mmaker/ocaml-letsencrypt/blob/eef905dceda483d6ea2b2c8d7c849887d9522128/src/acme_common.mli#L84

As per https://datatracker.ietf.org/doc/html/rfc8555#section-7.1.2 the Account object has a required field with a single uri where the listed orders are available. Letsencrypt doesn't implement this (see letsencrypt/boulder#3335) so the absence is handled by slamming the empty list in place (https://github.com/mmaker/ocaml-letsencrypt/blob/eef905dceda483d6ea2b2c8d7c849887d9522128/src/acme_common.ml#L307-L309). Other implementations (I've only tested https://smallstep.com/docs/step-ca/) may provide this field correctly as per the RFC, but the type mismatch will raise an error like
"couldn't find string list orders in <json_blob_containing_orders_field>" rather than proceeding.

I think the happy middle ground between RFC compliance and real-world functionality is to keep it optional, but correctly expect a [Uri.t] in the orders field. I'm not optimistic that I can offer a patch in the near future (my working copy uses [Async] instead of [Lwt], and I didn't take time to cleanly port the code), so I invite others to action this before I get to it.

@hannesm
Copy link
Collaborator

hannesm commented Sep 18, 2021

thanks for your report, I opened #28 that includes a fix as you suggested. could you look over the commit (f9bccd6) and let me know whether that is as you suggested?

@torinnd
Copy link
Author

torinnd commented Sep 18, 2021

+1

That looks perfect!

hannesm pushed a commit to hannesm/opam-repository that referenced this issue Sep 21, 2021
CHANGES:

* support EC (P-256, P-384, P-521) account keys (@reynir @hannesm)
  (reported in robur-coop/ocaml-letsencrypt#24 by @dinosaure)
* allow key_type to be passed into the alpn_solver (@hannesm)
* add RFC 7520 test cases (@reynir @hannesm)
* remove astring dependency (@hannesm)
* bugfix: "orders" field in account is Uri.t option, not a list (@hannesm)
  (reported in robur-coop/ocaml-letsencrypt#27 by @torinnd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants