Repository navigation
acme: support standards-compliant order finalization (cherry-pick b4dcfb54b) - #9
Conversation
When an order finalization response is not immediately valid, Client.CreateOrderCert depends on the ACME server returning a Location header. The header value is used as the URL for polling the updated order status. RFC 8555 does not require this header, so CreateOrderCert does not interoperate with ACME servers that omit it (e.g. Pebble v2.9.0+). This commit adds Client.CreateCertFromOrder, which accepts an *Order and uses its URI for polling instead of relying on the finalization response's Location header. It also updates existing callers and documentation to use the new method and updates the integration tests to Pebble v2.10.1 (the latest available version), which omits the finalization Location header. Fixes golang/go#77704 Change-Id: I0852f19c5d18d6debc9c9686fa2c14a2e39663c2 Reviewed-on: https://go-review.googlesource.com/c/crypto/+/817000 LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Cherry Mui <cherryyz@google.com> Reviewed-by: Roland Shoemaker <roland@golang.org> (cherry picked from commit b4dcfb5) Signed-off-by: Richard Wall <richard.wall@cyberark.com>
bd5aad2 to
a0b1a2d
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The focused implementation matches the reviewed upstream change and includes unit and integration coverage; only a minor documentation typo remains.
1 open finding
What changed in this PR
Cherry-picks upstream ACME finalization support so clients poll the order URI without requiring a non-standard Location header.
Changes:
- Adds
CreateCertFromOrderand order-URI fallback handling. - Migrates internal callers to the standards-compliant API.
- Updates unit and Pebble integration coverage.
| File | Description |
|---|---|
acme/rfc8555.go |
Implements order-based certificate finalization. |
acme/rfc8555_test.go |
Tests missing Location headers and invalid orders. |
acme/pebble_test.go |
Updates Pebble and exercises the new API. |
acme/types.go |
Updates order documentation. |
acme/acme.go |
Updates deprecated API guidance. |
acme/autocert/autocert.go |
Migrates automatic issuance. |
acme/internal/acmeprobe/prober.go |
Migrates probe issuance flows. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
I compared the diff against upstream golang/crypto b4dcfb5. The code change is identical; only the hunk offsets differ because of our profiles, ARI and RetryAfter patches. No One thing for whoever merges: use "Create a merge commit". cert-manager/cert-manager#9430 pins the head commit a0b1a2d, and a squash or rebase would leave it unreachable from [Claude Fable 5.1] |
|
/label tide/merge-method-merge |
|
@hjoshi123: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/lgtm |
|
@hjoshi123: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: hjoshi123 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
This cherry-picks golang/crypto b4dcfb5 ("acme: support standards-compliant order finalization") onto our fork. It adds
Client.CreateCertFromOrder, which polls the order's own URI after finalize instead of the finalize response'sLocationheader. RFC 8555 does not require that header, and Pebble v2.9.0+ omits it.cert-manager needs this now. Its e2e Pebble omits the header, so every ACME finalize in e2e first fails with
Post "": unsupported protocol scheme ""and is retried. The retry sometimes finalizes the same order twice and fails the test. The cert-manager PR that consumes this commit is cert-manager/cert-manager#9430.The cherry-pick applied cleanly. A full sync with golang/crypto master is a bigger job, because upstream has since added its own ACME profiles support (4e01a84), which overlaps with this fork's profiles patch.
Please merge with a merge commit, not squash or rebase. cert-manager's
third_party/klone.yamlpins a0b1a2d, which must stay reachable frommaster.How this was tested
go test ./acme/...passes on this branch, includingTestWithPebbleagainst Pebble v2.10.1.On
masterwith only the Pebble version bumped to v2.10.1,TestWithPebblefails with the same error cert-manager sees:[with Claude]