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

ACMEv2 - Expired authorizations break `GetOrder` / `statusForOrder`. #3499

Closed
cpu opened this issue Feb 28, 2018 · 0 comments

Comments

@cpu
Copy link
Member

commented Feb 28, 2018

In #3402 we implemented order status as a function of the authorizations associated with an order, and their respective statuses.

The sa.StatusForOrder function relies on sa.getAllOrderAuthorizations to load the full authorizations for an order:

boulder/sa/sa.go

Line 1673 in 8cf8c44

authzs, err := ssa.getAllOrderAuthorizations(ctx, *order.Id, *order.RegistrationID)

statusForOrder enforces the invariant that the # of authorization IDs in the order object (populated based on the orderToAuthz join table) should always be equal to the # of authorization objects returned by sa.getAllOrderAuthorizations or an InternalServerError is produced.

We overlooked the fact that sa.getAllOrderAuthorizations has a WHERE clause that excludes expired authorizations:

boulder/sa/sa.go

Line 1777 in 8cf8c44

authz.expires > ? AND

Therefore, if an order has an authorization that expires, the authorization ID will remain in the orderToAuthz join table used to find the order's authorization IDs, but it will not be present in the results returned from sa.getAllOrderAuthorizations, which results in sa.statusForOrder producing an internal server error, which results in sa.GetOrder failing, which in turn makes almost all ACME v2 operations fail because they rely on fetching order details and status through sa.GetOrder.

Worse, since our order expiry is currently set distinctly from authorization expiry (See #3498) order reuse will return the broken order for subsequent newOrder requests for the same identifiers.

To fix, sa.getAllOrderAuthorizations should return expired authorizations. In sa.StatusForOrder we should mark the order's overall status as invalid when one or more authorizations are expired.

@cpu cpu added this to the Sprint 2018-02-27 milestone Feb 28, 2018

@cpu cpu self-assigned this Feb 28, 2018

@jsha jsha closed this in #3500 Mar 1, 2018

jsha added a commit that referenced this issue Mar 1, 2018

Treat orders with expired authzs as invalid. (#3500)
This commit updates `sa.getAllOrderAuthorizations` to not exclude
expired authorizations. The expired authorizations are used in
`sa.statusForOrder` to set the overall order status to invalid when one
or more authorizations are expired.

Fixes #3499
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.