diff --git a/ra/ra.go b/ra/ra.go index b7fe5b078fe..02f3938865c 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -1656,6 +1656,15 @@ func (ra *RegistrationAuthorityImpl) NewOrder(ctx context.Context, req *rapb.New continue } authz := nameToExistingAuthz[name] + // If the existing authz isn't valid, note that its missing and continue. + // Since we reuse whole pending orders there isn't any benefit to reusing + // individual pending authorizations between orders. Worse, it makes the + // process of invalidating an order when an authorization is invalidated + // confusing because multiple orders would be invalidated simultaneously. + if *authz.Status != string(core.StatusValid) { + missingAuthzNames = append(missingAuthzNames, name) + continue + } // If the identifier is a wildcard and the existing authz only has one // DNS-01 type challenge we can reuse it. In theory we will // never get back an authorization for a domain with a wildcard prefix diff --git a/ra/ra_test.go b/ra/ra_test.go index 6cbc55da5f7..3df1c5395bf 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -13,6 +13,7 @@ import ( "net" "net/url" "os" + "reflect" "sort" "strings" "sync" @@ -2166,7 +2167,14 @@ func TestNewOrder(t *testing.T) { // Abuse the order of the queries used to extract the reused authorizations existing := orderC.Authorizations[:3] sort.Strings(existing) - test.AssertDeepEquals(t, existing, orderA.Authorizations) + + // We expect the pending authorizations were not reused between separate + // orders + // NOTE(@cpu): There's no test.AssertNotDeepEquals to use here so we call + // reflect.DeepEqual ourselves. + if reflect.DeepEqual(existing, orderA.Authorizations) { + t.Fatal("existing authorizations matched orderA authorizations") + } _, err = ra.NewOrder(context.Background(), &rapb.NewOrderRequest{ RegistrationID: &id, @@ -2284,6 +2292,62 @@ func TestNewOrderReuse(t *testing.T) { } } +func TestNewOrderReuseInvalidAuthz(t *testing.T) { + if os.Getenv("BOULDER_CONFIG_DIR") != "test/config-next" { + return + } + + _, _, ra, _, cleanUp := initAuthorities(t) + defer cleanUp() + + ctx := context.Background() + regA := int64(1) + names := []string{"zombo.com"} + + // Create an initial request with regA and names + orderReq := &rapb.NewOrderRequest{ + RegistrationID: ®A, + Names: names, + } + + // First, add an order with `names` for regA + order, err := ra.NewOrder(ctx, orderReq) + // It shouldn't fail + test.AssertNotError(t, err, "Adding an initial order for regA failed") + // It should have an ID + test.AssertNotNil(t, order.Id, "Initial order had a nil ID") + // It should have one authorization + test.AssertEquals(t, len(order.Authorizations), 1) + + // Fetch the full authz by the ID + authzID := order.Authorizations[0] + authz, err := ra.SA.GetAuthorization(ctx, authzID) + test.AssertNotError(t, err, "Error getting order authorization") + + // Finalize the authz to an invalid status + authz.Status = core.StatusInvalid + err = ra.SA.FinalizeAuthorization(ctx, authz) + test.AssertNotError(t, err, fmt.Sprintf("Could not finalize authorization %q", authzID)) + + // The order associated with the authz should now be invalid + updatedOrder, err := ra.SA.GetOrder(ctx, &sapb.OrderRequest{Id: order.Id}) + test.AssertNotError(t, err, "Error getting order to check status") + test.AssertEquals(t, *updatedOrder.Status, "invalid") + + // Create a second order for the same names/regID + secondOrder, err := ra.NewOrder(ctx, orderReq) + // It shouldn't fail + test.AssertNotError(t, err, "Adding an initial order for regA failed") + // It should have a different ID than the first now-invalid order + test.AssertNotEquals(t, *secondOrder.Id, *order.Id) + // It should be status pending + test.AssertEquals(t, *secondOrder.Status, "pending") + // It should have one authorization + test.AssertEquals(t, len(secondOrder.Authorizations), 1) + // It should have a different authorization than the first order's now-invalid authorization + test.AssertNotEquals(t, secondOrder.Authorizations[0], order.Authorizations[0]) +} + func TestNewOrderWildcard(t *testing.T) { // Only run under test/config-next config where 20170731115209_AddOrders.sql // has been applied diff --git a/sa/sa.go b/sa/sa.go index cf2f9398646..ffb91d8fc9e 100644 --- a/sa/sa.go +++ b/sa/sa.go @@ -791,7 +791,7 @@ func (ssa *SQLStorageAuthority) UpdatePendingAuthorization(ctx context.Context, // FinalizeAuthorization converts a Pending Authorization to a final one. If the // Authorization is not found a berrors.NotFound result is returned. If the -// Authorization is not valid a berrors.InternalServer error is returned. +// Authorization is status pending an berrors.InternalServer error is returned. func (ssa *SQLStorageAuthority) FinalizeAuthorization(ctx context.Context, authz core.Authorization) error { tx, err := ssa.dbMap.Begin() if err != nil { @@ -832,6 +832,30 @@ func (ssa *SQLStorageAuthority) FinalizeAuthorization(ctx context.Context, authz return Rollback(tx, err) } + // When an authorization is being finalized to an invalid state we need to see + // if there is an order associated with this authorization that itself should + // now become invalid as a result of the authz being invalid. + if authz.Status == core.StatusInvalid { + // Try to find an order associated with this authz ID. There may not be one if + // this is a legacy V1 authorization from the new-authz endpoint. + orderID, err := ssa.orderIDForAuthz(tx, authz.ID) + // If there was an error, and it wasn't a no-result error, then we encountered + // something unexpected and must rollback + if err != nil && err != sql.ErrNoRows { + return Rollback(tx, err) + } + // If the err was nil then orderID is an associated order for this authz + if err == nil { + // Set the order to invalid + err := ssa.setOrderInvalid(ctx, tx, &corepb.Order{ + Id: &orderID, + }) + if err != nil { + return Rollback(tx, err) + } + } + } + return tx.Commit() } @@ -1456,6 +1480,29 @@ func (ssa *SQLStorageAuthority) SetOrderProcessing(ctx context.Context, req *cor return tx.Commit() } +// setOrderInvalid updates a provided *corepb.Order in pending status to be in +// invalid status by updating the status field of the corresponding row in the +// DB. +func (ssa *SQLStorageAuthority) setOrderInvalid(ctx context.Context, tx *gorp.Transaction, req *corepb.Order) error { + result, err := tx.Exec(` + UPDATE orders + SET status = ? + WHERE id = ? + AND status = ?`, + string(core.StatusInvalid), + *req.Id, + string(core.StatusPending)) + if err != nil { + return err + } + + n, err := result.RowsAffected() + if err != nil || n == 0 { + return berrors.InternalServerError("no order updated to invalid status") + } + return nil +} + // FinalizeOrder finalizes a provided *corepb.Order by persisting the // CertificateSerial and a valid status to the database. No fields other than // CertificateSerial and the order ID on the provided order are processed (e.g. @@ -1504,6 +1551,26 @@ func (ssa *SQLStorageAuthority) authzForOrder(orderID int64) ([]string, error) { return ids, nil } +// orderForAuthz finds an order ID associated with a given authz (If any). +func (ssa *SQLStorageAuthority) orderIDForAuthz(tx *gorp.Transaction, authzID string) (int64, error) { + var orderID int64 + + err := tx.SelectOne(&orderID, ` + SELECT orderID + FROM orderToAuthz + WHERE authzID = ?`, + authzID) + + // NOTE(@cpu): orderIDForAuthz does not handle the sql.ErrNoRows that could be + // returned by `SelectOne`. The caller must do so. + if err != nil { + // If there is an err, return it as-is. + return 0, err + } + + return orderID, nil +} + // namesForOrder finds all of the requested names associated with an order. The // names are returned in their reversed form (see `sa.ReverseName`). func (ssa *SQLStorageAuthority) namesForOrder(orderID int64) ([]string, error) { @@ -1631,8 +1698,16 @@ func (ssa *SQLStorageAuthority) GetOrderForNames( return nil, err } - // Get & return the order - return ssa.GetOrder(ctx, &sapb.OrderRequest{Id: &orderID}) + // Get the order + order, err := ssa.GetOrder(ctx, &sapb.OrderRequest{Id: &orderID}) + if err != nil { + return nil, err + } + // Only return a pending order + if *order.Status != string(core.StatusPending) { + return nil, berrors.NotFoundError("no order matching request found") + } + return order, nil } func (ssa *SQLStorageAuthority) getAuthorizations(ctx context.Context, table string, status string, diff --git a/sa/sa_test.go b/sa/sa_test.go index 8a43f1fb25e..7790af97e58 100644 --- a/sa/sa_test.go +++ b/sa/sa_test.go @@ -1864,3 +1864,74 @@ func TestGetOrderForNames(t *testing.T) { // already test.Assert(t, result == nil, "sa.GetOrderForNames returned non-nil result for finalized order case") } + +func TestUpdatePendingAuthorizationInvalidOrder(t *testing.T) { + if os.Getenv("BOULDER_CONFIG_DIR") != "test/config-next" { + return + } + + sa, fc, cleanUp := initSA(t) + defer cleanUp() + + expires := fc.Now().Add(time.Hour) + ctx := context.Background() + + // Create a registration to work with + reg := satest.CreateWorkingRegistration(t, sa) + + // Create a pending authz, not associated with any orders + authz := core.Authorization{ + RegistrationID: reg.ID, + Expires: &expires, + Status: core.StatusPending, + } + pendingAuthz, err := sa.NewPendingAuthorization(ctx, authz) + test.AssertNotError(t, err, "Couldn't create new pending authorization") + + // Update the pending authz to be invalid. This shouldn't error. + pendingAuthz.Status = core.StatusInvalid + err = sa.FinalizeAuthorization(ctx, pendingAuthz) + test.AssertNotError(t, err, "Couldn't finalize legacy pending authz to invalid") + + // Create a pending authz that will be associated with an order + authz = core.Authorization{ + RegistrationID: reg.ID, + Expires: &expires, + Status: core.StatusPending, + } + pendingAuthz, err = sa.NewPendingAuthorization(ctx, authz) + test.AssertNotError(t, err, "Couldn't create new pending authorization") + + // Add a new order that references the above pending authz + status := string(core.StatusPending) + expiresNano := expires.UnixNano() + order, err := sa.NewOrder(ctx, &corepb.Order{ + RegistrationID: ®.ID, + Expires: &expiresNano, + Status: &status, + Authorizations: []string{pendingAuthz.ID}, + Names: []string{"your.order.is.up"}, + }) + // It shouldn't error + test.AssertNotError(t, err, "sa.NewOrder failed") + // The order ID shouldn't be nil + test.AssertNotNil(t, *order.Id, "NewOrder returned with a nil Id") + // The order should be pending + test.AssertEquals(t, *order.Status, string(core.StatusPending)) + // The order should have one authz with the correct ID + test.AssertEquals(t, len(order.Authorizations), 1) + test.AssertEquals(t, order.Authorizations[0], pendingAuthz.ID) + + // Now finalize the authz to an invalid status. + pendingAuthz.Status = core.StatusInvalid + err = sa.FinalizeAuthorization(ctx, pendingAuthz) + test.AssertNotError(t, err, "Couldn't finalize pending authz associated with order to invalid") + + // Fetch the order to get its updated status + updatedOrder, err := sa.GetOrder( + context.Background(), + &sapb.OrderRequest{Id: order.Id}) + test.AssertNotError(t, err, "GetOrder failed") + // We expect the updated order status to be invalid + test.AssertEquals(t, *updatedOrder.Status, string(core.StatusInvalid)) +} diff --git a/test/integration-test-v2.py b/test/integration-test-v2.py index 0ed5b75c45f..9d47b984da6 100644 --- a/test/integration-test-v2.py +++ b/test/integration-test-v2.py @@ -34,6 +34,7 @@ def main(): test_overlapping_wildcard() test_wildcard_exactblacklist() test_wildcard_authz_reuse() + test_order_reuse_failed_authz() if not startservers.check(): raise Exception("startservers.check failed") @@ -118,6 +119,52 @@ def test_wildcard_authz_reuse(): raise Exception("order for %s included a non-pending authorization (status: %s) from a previous HTTP-01 order" % ((domains), str(authz.body.status))) +def test_order_reuse_failed_authz(): + """ + Test that creating an order for a domain name, failing an authorization in + that order, and submitting another new order request for the same name + doesn't reuse a failed authorizaton in the new order. + """ + + client = make_client(None) + domains = [ random_domain() ] + csr_pem = make_csr(domains) + + order = client.new_order(csr_pem) + firstOrderURI = order.uri + + # Pick the first authz's first challenge, doesn't matter what type it is + chall_body = order.authorizations[0].body.challenges[0] + # Answer it, but with nothing set up to solve the challenge request + client.answer_challenge(chall_body, chall_body.response(client.key)) + + # Wait for validation to occur + # TODO(@cpu): This should be handled better but it involves Python + # concurrency and I'm going to punt on that while working on other parts of + # this PR + import time + time.sleep(10) + + # Make another order with the same domains + order = client.new_order(csr_pem) + + # It should not be the same order as before + if order.uri == firstOrderURI: + raise Exception("new-order for %s returned a , now-invalid, order" % domains) + + # We expect all of the returned authorizations to be pending status + for authz in order.authorizations: + if authz.body.status != Status("pending"): + raise Exception("order for %s included a non-pending authorization (status: %s) from a previous order" % + ((domains), str(authz.body.status))) + + # We expect the new order can be fulfilled + cleanup = do_http_challenges(client, order.authorizations) + try: + order = client.poll_order_and_request_issuance(order) + finally: + cleanup() + if __name__ == "__main__": try: main()