Skip to content

Implement latest draft-ietf-acme-ari spec#461

Merged
pgporada merged 21 commits into
mainfrom
pARtI
May 24, 2024
Merged

Implement latest draft-ietf-acme-ari spec#461
pgporada merged 21 commits into
mainfrom
pARtI

Conversation

@pgporada

@pgporada pgporada commented May 2, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

The draft spec version at the time of this PR was draft-ietf-acme-ari-03, but failed replacement order handling is from the yet-to-be-released draft-ietf-acme-ari-04.

  • Add a renewalInfo entry to the directory object which provides the base URL for ARI requests.
  • Add a new WFE handlefunc which parses incoming requests and returns reasonable renewalInfo for determining when the client should attempt renewal of a certificate.
  • Add support for marking orders as replaced. Replacement orders can be chained, but there can be no duplicate replacement of orders, just like boulder.
  • Restructured the asynchronous finalization anonymous go func to handle storing replaced orders. To be replaced, an order must previously have been finalized and have an issued certificate.

Fixes #403

@pgporada pgporada mentioned this pull request May 2, 2024
Closed
@pgporada

pgporada commented May 10, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

I've got a fork of eggsampler/acme that I've been working against. To test that against this branch:

# In the pebble dir
$ tmux

# Run challtestsrv, version doesn't particularly matter because we're not touching this code
$ docker rm challtestsrv 2>&1; docker run -p 5001:5001 -p 5002:5002 -p 5003:5003 -p 8053:8053 -p 8055:8055 -p 8443:8443 --name challtestsrv ghcr.io/letsencrypt/pebble-challtestsrv:latest

# Get the IP of that container
$ CHALLTESTSRV=$(docker inspect challtestsrv | jq -r '.[].NetworkSettings.Networks.bridge.IPAddress')

# Run pebble
$ go run cmd/pebble/main.go -config ./test/config/pebble-config.json -dnsserver ${CHALLTESTSRV}:8053

-----------------------------
# In the eggsampler repo
$ export PEBBLE_PATH=/path/to/pebble/on/your/computer
$ go test -test.run TestClient_IssueReplacementCert

@pgporada

pgporada commented May 14, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

Replacement orders are now supported

Pebble 2024/05/14 16:36:21 Order "6it4fspAVD-CNbSI06L4iPf9l29mNpL6rkQi-XnICBo" is not a replacement
Pebble 2024/05/14 16:36:21 There are now 1 orders in the db
Pebble 2024/05/14 16:36:28 Order "4gw9BYS8UnmQnymq8HnxSQA8hOs5le0SkB34Ux1cfDQ" is a replacement of "6it4fspAVD-CNbSI06L4iPf9l29mNpL6rkQi-XnICBo"
Pebble 2024/05/14 16:36:28 There are now 2 orders in the db
Pebble 2024/05/14 16:37:11 Order "vOKNgiDK8aoTvYWC8Q6GJp49UYK-iGPD34vBDEwhnTs" is not a replacement
Pebble 2024/05/14 16:37:11 There are now 3 orders in the db
Pebble 2024/05/14 16:37:20 Order "s7mJpikMeEy8OL57wlwB54_kakew2g6ns6nit8VWuKQ" is a replacement of "vOKNgiDK8aoTvYWC8Q6GJp49UYK-iGPD34vBDEwhnTs"
Pebble 2024/05/14 16:37:20 There are now 4 orders in the db

aarongable
aarongable reviewed May 14, 2024
View reviewed changes
Comment thread wfe/wfe.go Outdated
Comment thread wfe/wfe.go Outdated
Comment thread wfe/wfe.go Outdated
Comment thread wfe/wfe.go Outdated
Comment thread wfe/wfe.go Outdated
Comment thread wfe/wfe.go
@pgporada

pgporada commented May 17, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

There's a race condition in wfe.FinalizeOrder at existingOrder := wfe.db.GetOrderByID(orderID) causing an error during manual testing with my eggsample/acme fork.

=== RUN   TestClient_IssueReplacementCert
    ari_test.go:66: Issuing initial order
    ari_test.go:73: Issuing first replacement order
    ari_test.go:80: Issuing second replacement order
    ari_test.go:83: unexpected error: acme: error code 403 "urn:ietf:params:acme:error:orderNotReady": Order's status ("pending") was not ready

vs

=== RUN   TestClient_IssueReplacementCert
    ari_test.go:66: Issuing initial order
    ari_test.go:73: Issuing first replacement order
    ari_test.go:80: Issuing second replacement order
    ari_test.go:87: Should not be able to create a duplicate replacement
--- PASS: TestClient_IssueReplacementCert (19.05s)
PASS

EDIT: This whole entire detour was actually an implementation bug in my eggsampler fork. 🤦🏼

@pgporada pgporada changed the title Begin implementing draft-ietf-acme-ari-03 Implement draft-ietf-acme-ari-03 May 17, 2024
@pgporada pgporada marked this pull request as ready for review May 17, 2024 18:39
@pgporada pgporada requested review from a team, aarongable, beautifulentropy and jsha and removed request for a team May 17, 2024 18:39
@pgporada pgporada mentioned this pull request May 17, 2024
Merged
aarongable
aarongable reviewed May 20, 2024
View reviewed changes
Comment thread ca/ca.go Outdated
Comment thread core/types.go Outdated
Comment thread db/memorystore.go Outdated
Comment thread wfe/wfe.go Outdated
Comment thread wfe/wfe.go Outdated
Comment thread wfe/wfe.go Outdated
Comment thread wfe/wfe.go Outdated
aarongable
aarongable previously approved these changes May 20, 2024
View reviewed changes
Comment thread ca/ca.go Outdated
Comment thread va/va.go
@pgporada pgporada requested a review from aarongable May 21, 2024 14:13
@pgporada pgporada changed the title Implement draft-ietf-acme-ari-03 Implement latest draft-ietf-acme-ari spec May 21, 2024
aarongable
aarongable previously approved these changes May 23, 2024
View reviewed changes
beautifulentropy
beautifulentropy previously approved these changes May 23, 2024
View reviewed changes

@beautifulentropy beautifulentropy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to get this added to Pebble!

Some nits regarding contexts that we're dropping, but nothing that means we can't merge. I think these are just some contexts params from the Boulder implementation which aren't necessary in the Pebble implementation?

Comment thread wfe/wfe.go
Comment thread wfe/wfe.go Outdated
@pgporada pgporada dismissed stale reviews from beautifulentropy and aarongable via 5940490 May 23, 2024 23:09
@pgporada pgporada merged commit db1f587 into main May 24, 2024
@pgporada pgporada deleted the pARtI branch May 24, 2024 16:06
@orangepizza

orangepizza commented May 25, 2024

Copy link
Copy Markdown
Contributor

it's sad that this mission latest release by just a day 😢

@pgporada

pgporada commented May 25, 2024 via email

Copy link
Copy Markdown
Member Author

kwatson added a commit to kwatson/letsencrypt-pebble that referenced this pull request Jun 9, 2025
@kwatson
Merge branch 'main' of https://github.com/letsencrypt/pebble
a44c67c 
* 'main' of https://github.com/letsencrypt/pebble: (35 commits)
  add overriding of ARI response (letsencrypt#501)
  wfe: fix a race in `orderForDisplay` (letsencrypt#500)
  Bump golang.org/x/ dependencies (letsencrypt#499)
  currectly triggers BadSignatureAlgorithmProblem at JWS parse time (letsencrypt#492)
  use newer validation subdomain for dns-account-01 (fix CI eggsampler/acme error) (letsencrypt#498)
  Orders don't have a "deactivated" status. (letsencrypt#301)
  Update golangci-lint (letsencrypt#488)
  build(deps): bump github.com/go-jose/go-jose/v4 from 4.0.4 to 4.0.5 (letsencrypt#487)
  Truncate ARI timestamps to millisecond resolution (letsencrypt#485)
  return logical and compliant ARI windows for expiring certs (letsencrypt#484)
  Update dependencies (letsencrypt#481)
  docs: rm mention of subproblems being unimpl'd (letsencrypt#479)
  Fix(NOISSUE): Fix docker compose file example in README.md (letsencrypt#475)
  Add support for ACME Profiles (letsencrypt#473)
  Simplify KU, EKU, and SKID fields of issued certs (letsencrypt#472)
  Update golangci-lint to 1.60.2 (letsencrypt#474)
  Update /x/net for compatibility with go1.23 (letsencrypt#470)
  Reject extra command line args and fix README invocation (letsencrypt#467)
  Document exposing API and management ports when not using docker-compose.yaml (letsencrypt#465)
  Implement latest draft-ietf-acme-ari spec (letsencrypt#461)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aarongable aarongable aarongable approved these changes

@beautifulentropy beautifulentropy beautifulentropy approved these changes

@jsha jsha Awaiting requested review from jsha jsha was automatically assigned from letsencrypt/boulder-developers

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ACME Renewal Information (ARI)

4 participants

@pgporada @orangepizza @aarongable @beautifulentropy