Skip to content

Commit eea049d

Browse files
cpucpu
authored
Fix order reuse, calc order status by authz status (letsencrypt#3402)
This PR is a rework of what was originally letsencrypt#3382, integrating the design feedback proposed by @jsha: letsencrypt#3382 (comment) This PR removes the stored Order status field and replaces it with a value that is calculated on-the-fly by the SA when fetching an order, based on the order's associated authorizations. In summary (and order of precedence): * If any of the order's authorizations are invalid, the order is invalid. * If any of the order's authorizations are deactivated, the order is deactivated. * If any of the order's authorizations are pending, the order is pending. * If all of the order's authorizations are valid, and there is a certificate serial, the order is valid. * If all of the order's authorizations are valid, and we have began processing, but there is no certificate serial, the order is processing. * If all of the order's authorizations are valid, and we haven't processing, then the order is pending waiting a finalization request. This avoids having to explicitly update the order status when an associated authorization changes status. The RA's implementation of new-order is updated to only reuse an existing order if the calculated status is pending. This avoids giving back invalid or deactivated orders to clients. Resolves letsencrypt#3333
1 parent c325339 commit eea049d

File tree

11 files changed

+833
-142
lines changed

11 files changed

+833
-142
lines changed

‎core/proto/core.pb.go‎

Lines changed: 59 additions & 49 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎core/proto/core.proto‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@ message Order {
8585
repeated string authorizations = 6;
8686
optional string status = 7;
8787
repeated string names = 8;
88+
optional bool beganProcessing = 9;
8889
}
8990

9091
message Empty {}

‎grpc/pb-marshalling.go‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -407,19 +407,20 @@ func registrationValid(reg *corepb.Registration) bool {
407407
}
408408

409409
// orderValid checks that a corepb.Order is valid. In addition to the checks
410-
// from `newOrderValid` it ensures the order ID is not nil.
410+
// from `newOrderValid` it ensures the order ID and the BeganProcessing fields
411+
// are not nil.
411412
func orderValid(order *corepb.Order) bool {
412-
return order.Id != nil && newOrderValid(order)
413+
return order.Id != nil && order.BeganProcessing != nil && newOrderValid(order)
413414
}
414415

415416
// newOrderValid checks that a corepb.Order is valid. It allows for a nil
416417
// `order.Id` because the order has not been assigned an ID yet when it is being
417-
// created initially. It also allows `order.CertificateSerial` to be nil such
418-
// that it can be used in places where the order has not been finalized yet.
419-
// Callers must additionally ensure the `CertificateSerial` field is non-nil if
420-
// they intend to use it.
418+
// created initially. It allows `order.BeganProcessing` to be nil because
419+
// `sa.NewOrder` explicitly sets it to the default value. It also allows
420+
// `order.CertificateSerial` to be nil such that it can be used in places where
421+
// the order has not been finalized yet.
421422
func newOrderValid(order *corepb.Order) bool {
422-
return !(order.RegistrationID == nil || order.Expires == nil || order.Authorizations == nil || order.Status == nil || order.Names == nil)
423+
return !(order.RegistrationID == nil || order.Expires == nil || order.Authorizations == nil || order.Names == nil)
423424
}
424425

425426
func authorizationValid(authz *corepb.Authorization) bool {

‎grpc/pb-marshalling_test.go‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -339,6 +339,7 @@ func TestOrderValid(t *testing.T) {
339339
testID := int64(1)
340340
testExpires := int64(1)
341341
emptyString := ""
342+
falseBool := false
342343

343344
testCases := []struct {
344345
Name string
@@ -353,20 +354,20 @@ func TestOrderValid(t *testing.T) {
353354
Expires: &testExpires,
354355
CertificateSerial: &emptyString,
355356
Authorizations: []string{},
356-
Status: &emptyString,
357357
Names: []string{},
358+
BeganProcessing: &falseBool,
358359
},
359360
ExpectedValid: true,
360361
},
361362
{
362363
Name: "Serial nil",
363364
Order: &corepb.Order{
364-
Id: &testID,
365-
RegistrationID: &testID,
366-
Expires: &testExpires,
367-
Authorizations: []string{},
368-
Status: &emptyString,
369-
Names: []string{},
365+
Id: &testID,
366+
RegistrationID: &testID,
367+
Expires: &testExpires,
368+
Authorizations: []string{},
369+
Names: []string{},
370+
BeganProcessing: &falseBool,
370371
},
371372
ExpectedValid: true,
372373
},
@@ -381,8 +382,8 @@ func TestOrderValid(t *testing.T) {
381382
Expires: &testExpires,
382383
CertificateSerial: &emptyString,
383384
Authorizations: []string{},
384-
Status: &emptyString,
385385
Names: []string{},
386+
BeganProcessing: &falseBool,
386387
},
387388
},
388389
{
@@ -392,8 +393,8 @@ func TestOrderValid(t *testing.T) {
392393
Expires: &testExpires,
393394
CertificateSerial: &emptyString,
394395
Authorizations: []string{},
395-
Status: &emptyString,
396396
Names: []string{},
397+
BeganProcessing: &falseBool,
397398
},
398399
},
399400
{
@@ -403,8 +404,8 @@ func TestOrderValid(t *testing.T) {
403404
RegistrationID: &testID,
404405
CertificateSerial: &emptyString,
405406
Authorizations: []string{},
406-
Status: &emptyString,
407407
Names: []string{},
408+
BeganProcessing: &falseBool,
408409
},
409410
},
410411
{
@@ -414,12 +415,12 @@ func TestOrderValid(t *testing.T) {
414415
RegistrationID: &testID,
415416
Expires: &testExpires,
416417
CertificateSerial: &emptyString,
417-
Status: &emptyString,
418418
Names: []string{},
419+
BeganProcessing: &falseBool,
419420
},
420421
},
421422
{
422-
Name: "Status nil",
423+
Name: "BeganProcessing nil",
423424
Order: &corepb.Order{
424425
Id: &testID,
425426
RegistrationID: &testID,
@@ -437,7 +438,7 @@ func TestOrderValid(t *testing.T) {
437438
Expires: &testExpires,
438439
CertificateSerial: &emptyString,
439440
Authorizations: []string{},
440-
Status: &emptyString,
441+
BeganProcessing: &falseBool,
441442
},
442443
},
443444
}

‎ra/ra.go‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1585,12 +1585,10 @@ func (ra *RegistrationAuthorityImpl) DeactivateAuthorization(ctx context.Context
15851585
// NewOrder creates a new order object
15861586
func (ra *RegistrationAuthorityImpl) NewOrder(ctx context.Context, req *rapb.NewOrderRequest) (*corepb.Order, error) {
15871587
expires := ra.clk.Now().Add(ra.orderLifetime).UnixNano()
1588-
status := string(core.StatusPending)
15891588
order := &corepb.Order{
15901589
RegistrationID: req.RegistrationID,
15911590
Names: core.UniqueLowerNames(req.Names),
15921591
Expires: &expires,
1593-
Status: &status,
15941592
}
15951593

15961594
// Check that the registration ID in question has rate limit space for another

0 commit comments

Comments
 (0)