From 10db7e9e32dfdf942be0c2460053d10ce555148d Mon Sep 17 00:00:00 2001 From: Daniel Date: Wed, 17 Jan 2018 12:43:58 -0500 Subject: [PATCH 1/2] Only reuse pending order, invalidate order on invalid authz. Prior to this commit the combination of: * Order reuse * Authz reuse within orders * Orders not being set invalid on a failed challenge/authz Meant that if a user created an order, failed (or deactivated) one of the associated authzs, and then tried to create another order for the same names they would get back the original order with the failed/deactivated authzs. Oops! This commit addresses this problem in three main ways: 1. Pending authz reuse within orders is removed. There's no advantage to reusing one pending authz across multiple orders when we have reuse at the order level and it complicates invalidating an order when a pending authz is finalized to invalid. 2. When an authz is finalized to an invalid state the SA now tries to find an associated order (if any) using the `orderToAuthz` join table. If an order is associated with the authz being finalized as invalid then the order is also updated to be invalid. 3. When NewOrder looks for an order to reuse it only reuses the order if the status is pending. The combination of the above means that calling NewOrder, failing (or deactivating an authz) and then calling NewOrder again will not result in the same "stuck" order being returned for the second call. The associated integration test was written ahead of the fixes and verifies that the change has the intended affect. Prior to the fixes it failed in the same manner as reported by users of the V2 staging API. --- ra/ra.go | 9 +++++ ra/ra_test.go | 66 +++++++++++++++++++++++++++++- sa/sa.go | 81 +++++++++++++++++++++++++++++++++++-- sa/sa_test.go | 71 ++++++++++++++++++++++++++++++++ test/integration-test-v2.py | 57 +++++++++++++++++++++++--- 5 files changed, 275 insertions(+), 9 deletions(-) diff --git a/ra/ra.go b/ra/ra.go index d5b400df24c..2e792ad52d0 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -1655,6 +1655,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 045e4fe5b5c..635dc311a23 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..7c47f7a8ffa 100644 --- a/test/integration-test-v2.py +++ b/test/integration-test-v2.py @@ -29,11 +29,12 @@ def main(): if not startservers.start(race_detection=True): raise Exception("startservers failed") - test_multidomain() - test_wildcardmultidomain() - test_overlapping_wildcard() - test_wildcard_exactblacklist() - test_wildcard_authz_reuse() + #test_multidomain() + #test_wildcardmultidomain() + #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() From e6b3207a6fd9fc3bb5367ac5b5397290c5810c2a Mon Sep 17 00:00:00 2001 From: Daniel Date: Fri, 19 Jan 2018 15:47:15 -0500 Subject: [PATCH 2/2] Reenable other integration tests --- test/integration-test-v2.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/integration-test-v2.py b/test/integration-test-v2.py index 7c47f7a8ffa..9d47b984da6 100644 --- a/test/integration-test-v2.py +++ b/test/integration-test-v2.py @@ -29,11 +29,11 @@ def main(): if not startservers.start(race_detection=True): raise Exception("startservers failed") - #test_multidomain() - #test_wildcardmultidomain() - #test_overlapping_wildcard() - #test_wildcard_exactblacklist() - #test_wildcard_authz_reuse() + test_multidomain() + test_wildcardmultidomain() + test_overlapping_wildcard() + test_wildcard_exactblacklist() + test_wildcard_authz_reuse() test_order_reuse_failed_authz() if not startservers.check():