search

mholt / acmez

Premier ACME client library for Go
https://pkg.go.dev/github.com/mholt/acmez/v2
Apache License 2.0
281 stars 35 forks source link

Set omitempty on Order.Status field #21

Closed commonism closed 1 year ago

commonism commented 1 year ago

Hi,

as mentioned in https://github.com/noahkw/acmetk/issues/64 / https://github.com/caddyserver/caddy/issues/3539

the new-order payload send by acmez is questionable as the status is ""

{"status":"","expires":"0001-01-01T00:00:00Z","identifiers":[{"type":"dns","value":"example.com"}],"authorizations":null,"finalize":"","certificate":""}

and considered invalid by acmetk.

I think this is correct due to https://datatracker.ietf.org/doc/html/rfc8555#section-7.1.3 https://datatracker.ietf.org/doc/html/rfc8555#section-7.1.6 "" is not a possible value.

This patch initializes order.status.

I confimed acmetk will accept the order and grant a certificate using the porcelain example client.

mholt commented 1 year ago

Despite the invalid syntax, I'm not sure the logic of this change is correct.

The spec says the client should check the status field, not set it:

Clients SHOULD check the "status" field of an order to determine whether they need to take any action.

Additionally:

Order objects are created in the "pending" state.

Order objects are created by the server, not by the client. Once the server has provisioned the Order resource, it returns the URL of the order along with the populated Order object including the status.

This logic checks out in multiple production environments with every CA: Let's Encrypt (staging + prod), ZeroSSL (Sectigo's ACME server), BuyPass, Google Trust Services, and even the pebble dev server.

commonism commented 1 year ago

The body of the POST is a JWS object whose JSON payload is a subset of the order object defined in Section 7.1.3, containing the fields that describe the certificate to be issued:

https://datatracker.ietf.org/doc/html/rfc8555#section-7.4 status is not one of them, therefore omitempty would do as well.

matches certbot https://github.com/certbot/certbot/blob/b1978ff18837e40d16eedf2090330af53d8ceaa5/acme/acme/messages.py#L628 (which is what acmetk uses to parse the message)

mholt commented 1 year ago

:thinking: That seems to be overly-strict JSON decoding IMO since a missing value will decode into a "" string type either way.

But I guess we can make this change without harm. I initially didn't use omitempty because the field is required, except, I guess, for the initial request.