From 21a17f0bafd4154d4e2d57c435976aac91a7d886 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Fri, 16 Mar 2018 16:42:21 -0400 Subject: [PATCH] Harmonize order expiry with assoc. authz expiry. (#3567) Prior to this commit an order's expiry was set based on ra.orderLiftime while pending and valid authorization expiry was set based on ra.pendingAuthorizationLifetime and ra.authorizationLifetime. Since orders reused existing valid/pending authorizations this can lead to a case where an order has an expiry beyond the associated authorization expiries. In this case when an authorization expired the order becomes inactionable and the extra order lifetime is not useful. This commit addresses this problem in two ways: 1. The SA GetAuthorizations function used to find authzs to reuse for ra.NewOrder is adjusted to only return authorizations at min 24hr away from expiry. 2. Order expiry is now calculated by the RA in newOrder as the min of the order's own expiry or the soonest authorization expiry. This properly reflects the order's true lifetime based on the authorization lifetime. The RA/SA unit tests are updated accordingly. Resolves #3498 --- ra/ra.go | 51 +++++++++++++++++++++--- ra/ra_test.go | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++ sa/sa_test.go | 69 +++++++++++++++++++++----------- 3 files changed, 199 insertions(+), 29 deletions(-) diff --git a/ra/ra.go b/ra/ra.go index 16c561dd1..ef16e5c0d 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -1709,11 +1709,9 @@ func (ra *RegistrationAuthorityImpl) DeactivateAuthorization(ctx context.Context // NewOrder creates a new order object func (ra *RegistrationAuthorityImpl) NewOrder(ctx context.Context, req *rapb.NewOrderRequest) (*corepb.Order, error) { - expires := ra.clk.Now().Add(ra.orderLifetime).UnixNano() order := &corepb.Order{ RegistrationID: req.RegistrationID, Names: core.UniqueLowerNames(req.Names), - Expires: &expires, } // Validate that our policy allows issuing for each of the names in the order @@ -1757,15 +1755,21 @@ func (ra *RegistrationAuthorityImpl) NewOrder(ctx context.Context, req *rapb.New return nil, err } - // Check whether there are existing non-expired authorizations for the set of - // order names - now := ra.clk.Now().UnixNano() + // An order's lifetime is effectively bound by the shortest remaining lifetime + // of its associated authorizations. For that reason it would be Uncool if + // `sa.GetAuthorizations` returned an authorization that was very close to + // expiry. The resulting pending order that references it would itself end up + // expiring very soon. + // To prevent this we only return authorizations that are at least 1 day away + // from expiring. + authzExpiryCutoff := ra.clk.Now().AddDate(0, 0, 1).UnixNano() + // We do not want any legacy V1 API authorizations not associated with an // order to be returned from the SA so we set requireV2Authzs to true requireV2Authzs := true existingAuthz, err := ra.SA.GetAuthorizations(ctx, &sapb.GetAuthorizationsRequest{ RegistrationID: order.RegistrationID, - Now: &now, + Now: &authzExpiryCutoff, Domains: order.Names, RequireV2Authzs: &requireV2Authzs, }) @@ -1806,6 +1810,8 @@ func (ra *RegistrationAuthorityImpl) NewOrder(ctx context.Context, req *rapb.New continue } + // Delete the authz from the nameToExistingAuthz map since we are not reusing it. + delete(nameToExistingAuthz, name) // If we reached this point then the existing authz was not acceptable for // reuse and we need to mark the name as requiring a new pending authz missingAuthzNames = append(missingAuthzNames, name) @@ -1837,14 +1843,47 @@ func (ra *RegistrationAuthorityImpl) NewOrder(ctx context.Context, req *rapb.New newAuthzs = append(newAuthzs, pb) } + // Start with the order's own expiry as the minExpiry. We only care + // about authz expiries that are sooner than the order's expiry + minExpiry := ra.clk.Now().Add(ra.orderLifetime) + + // Check the reused authorizations to see if any have an expiry before the + // minExpiry (the order's lifetime) + for _, authz := range nameToExistingAuthz { + // An authz without an expiry is an unexpected internal server event + if authz.Expires == nil { + return nil, berrors.InternalServerError( + "SA.GetAuthorizations returned an authz (%d) with nil expiry", + *authz.Id) + } + // If the reused authorization expires before the minExpiry, it's expiry + // is the new minExpiry. + authzExpiry := time.Unix(0, *authz.Expires) + if authzExpiry.Before(minExpiry) { + minExpiry = authzExpiry + } + } + + // If new authorizations are needed, call AddPendingAuthorizations. Also check + // whether the newly created pending authz's have an expiry lower than minExpiry if len(newAuthzs) > 0 { authzIDs, err := ra.SA.AddPendingAuthorizations(ctx, &sapb.AddPendingAuthorizationsRequest{Authz: newAuthzs}) if err != nil { return nil, err } order.Authorizations = append(order.Authorizations, authzIDs.Ids...) + + // If the newly created pending authz's have an expiry closer than the + // minExpiry the minExpiry is the pending authz expiry. + newPendingAuthzExpires := ra.clk.Now().Add(ra.pendingAuthorizationLifetime) + if newPendingAuthzExpires.Before(minExpiry) { + minExpiry = newPendingAuthzExpires + } } + // Set the order's expiry to the minimum expiry + minExpiryTS := minExpiry.UnixNano() + order.Expires = &minExpiryTS storedOrder, err := ra.SA.NewOrder(ctx, order) if err != nil { return nil, err diff --git a/ra/ra_test.go b/ra/ra_test.go index ea9beab4f..ba8a4d4cf 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -2636,6 +2636,114 @@ func TestNewOrderWildcard(t *testing.T) { test.AssertEquals(t, dupeOrder.Authorizations[0], order.Authorizations[0]) } +// mockSANearExpiredAuthz is a mock SA that always returns an authz near expiry +// to test orders expiry calculations +type mockSANearExpiredAuthz struct { + mocks.StorageAuthority + expiry time.Time +} + +// GetAuthorizations is a mock that always returns a valid authorization for +// "zombo.com" very near to expiry +func (sa *mockSANearExpiredAuthz) GetAuthorizations( + ctx context.Context, + req *sapb.GetAuthorizationsRequest) (*sapb.Authorizations, error) { + authzs := map[string]*core.Authorization{ + "zombo.com": &core.Authorization{ + // A static fake ID we can check for in a unit test + ID: "near-expired-authz", + Identifier: core.AcmeIdentifier{Type: "dns", Value: "zombo.com"}, + RegistrationID: *req.RegistrationID, + Expires: &sa.expiry, + Status: "valid", + Challenges: []core.Challenge{ + core.Challenge{ + Type: core.ChallengeTypeHTTP01, + Status: core.StatusValid, + }, + }, + Combinations: [][]int{{0}, {1}}, + }, + } + // We can't easily access sa.authzMapToPB so we "inline" it for the mock :-) + resp := &sapb.Authorizations{} + for k, v := range authzs { + authzPB, err := sagrpc.AuthzToPB(*v) + if err != nil { + return nil, err + } + // Make a copy of k because it will be reassigned with each loop. + kCopy := k + resp.Authz = append(resp.Authz, &sapb.Authorizations_MapElement{Domain: &kCopy, Authz: authzPB}) + } + return resp, nil +} + +// AddPendingAuthorizations is a mock that just returns a fake pending authz ID +// that is != "near-expired-authz" +func (sa *mockSANearExpiredAuthz) AddPendingAuthorizations( + _ context.Context, + _ *sapb.AddPendingAuthorizationsRequest) (*sapb.AuthorizationIDs, error) { + return &sapb.AuthorizationIDs{ + Ids: []string{ + "abcdefg", + }, + }, nil +} + +func TestNewOrderExpiry(t *testing.T) { + _, _, ra, clk, cleanUp := initAuthorities(t) + defer cleanUp() + + ctx := context.Background() + regA := int64(1) + names := []string{"zombo.com"} + + // Set the order lifetime to 48 hours. + ra.orderLifetime = 48 * time.Hour + + // Use an expiry that is sooner than the configured order expiry but greater + // than 24 hours away. + fakeAuthzExpires := clk.Now().Add(35 * time.Hour) + + // Use a mock SA that always returns a soon-to-be-expired valid authz for + // "zombo.com". + ra.SA = &mockSANearExpiredAuthz{expiry: fakeAuthzExpires} + + // Create an initial request with regA and names + orderReq := &rapb.NewOrderRequest{ + RegistrationID: ®A, + Names: names, + } + + // Create an order for that request + order, err := ra.NewOrder(ctx, orderReq) + // It shouldn't fail + test.AssertNotError(t, err, "Adding an order for regA failed") + // There should be one authorization + test.AssertEquals(t, len(order.Authorizations), 1) + // It should be the fake near-expired-authz authz + test.AssertEquals(t, order.Authorizations[0], "near-expired-authz") + // The order's expiry should be the fake authz's expiry since it is sooner + // than the order's own expiry. + test.AssertEquals(t, *order.Expires, fakeAuthzExpires.UnixNano()) + + // Set the order lifetime to be lower than the fakeAuthzLifetime + ra.orderLifetime = 12 * time.Hour + expectedOrderExpiry := clk.Now().Add(ra.orderLifetime).UnixNano() + // Create the order again + order, err = ra.NewOrder(ctx, orderReq) + // It shouldn't fail + test.AssertNotError(t, err, "Adding an order for regA failed") + // There should be one authorization + test.AssertEquals(t, len(order.Authorizations), 1) + // It should be the fake near-expired-authz authz + test.AssertEquals(t, order.Authorizations[0], "near-expired-authz") + // The order's expiry should be the order's own expiry since it is sooner than + // the fake authz's expiry. + test.AssertEquals(t, *order.Expires, expectedOrderExpiry) +} + func TestFinalizeOrder(t *testing.T) { _, sa, ra, _, cleanUp := initAuthorities(t) defer cleanUp() diff --git a/sa/sa_test.go b/sa/sa_test.go index e13bdec59..4792003ef 100644 --- a/sa/sa_test.go +++ b/sa/sa_test.go @@ -1488,7 +1488,6 @@ func TestOrder(t *testing.T) { }) test.AssertNotError(t, err, "Couldn't create test registration") - // Add one pending authz authzExpires := fc.Now().Add(time.Hour) newAuthz := core.Authorization{ Identifier: core.AcmeIdentifier{Type: core.IdentifierDNS, Value: "example.com"}, @@ -1499,7 +1498,8 @@ func TestOrder(t *testing.T) { authz, err := sa.NewPendingAuthorization(ctx, newAuthz) test.AssertNotError(t, err, "Couldn't create new pending authorization") - expires := time.Now().Truncate(time.Second).UnixNano() + // Set the order to expire in two hours + expires := fc.Now().Add(2 * time.Hour).UnixNano() empty := "" inputOrder := &corepb.Order{ @@ -1509,31 +1509,37 @@ func TestOrder(t *testing.T) { Authorizations: []string{authz.ID}, } + // Create the order order, err := sa.NewOrder(context.Background(), inputOrder) test.AssertNotError(t, err, "sa.NewOrder failed") - test.AssertEquals(t, *order.Id, int64(1)) pendingStatus := string(core.StatusPending) falseBool := false one := int64(1) nowTS := sa.clk.Now().UnixNano() + // The Order from GetOrder should match the following expected order expectedOrder := &corepb.Order{ - // The expected order matches the input order + // The registration ID, authorizations, expiry, and names should match the + // input to NewOrder RegistrationID: inputOrder.RegistrationID, - Expires: inputOrder.Expires, - Names: inputOrder.Names, Authorizations: inputOrder.Authorizations, - // And should also have an empty serial and the correct status and - // processing state, and an ID of 1, and a created timestamp - Id: &one, + Names: inputOrder.Names, + Expires: inputOrder.Expires, + // The ID should have been set to 1 by the SA + Id: &one, + // The status should be pending + Status: &pendingStatus, + // The serial should be empty since this is a pending order CertificateSerial: &empty, - BeganProcessing: &falseBool, - Status: &pendingStatus, - Created: &nowTS, + // We should not be processing it + BeganProcessing: &falseBool, + // The created timestamp should have been set to the current time + Created: &nowTS, } + // Fetch the order by its ID and make sure it matches the expected storedOrder, err := sa.GetOrder(context.Background(), &sapb.OrderRequest{Id: order.Id}) - test.AssertNotError(t, err, "sa.Order failed") + test.AssertNotError(t, err, "sa.GetOrder failed") test.AssertDeepEquals(t, storedOrder, expectedOrder) } @@ -1623,11 +1629,13 @@ func TestGetAuthorizations(t *testing.T) { defer cleanup() reg := satest.CreateWorkingRegistration(t, sa) - exp := fc.Now().AddDate(0, 0, 1) + exp := fc.Now().AddDate(0, 0, 10) identA := "aaa" identB := "bbb" - idents := []string{identA, identB} + identC := "ccc" + identD := "ddd" + idents := []string{identA, identB, identC} // Create an authorization template for a pending authorization with a dummy identifier pa := core.Authorization{ @@ -1655,33 +1663,48 @@ func TestGetAuthorizations(t *testing.T) { err = sa.FinalizeAuthorization(ctx, paB) test.AssertNotError(t, err, "Couldn't finalize pending authorization with ID "+paB.ID) + // Adjust the template to have an expiry in 1 hour from now. + nearbyExpires := fc.Now().Add(time.Hour) + pa.Expires = &nearbyExpires + pa.Identifier.Value = identC + // Add the template to create pending authorization C + paC, err := sa.NewPendingAuthorization(ctx, pa) + // There should be no error + test.AssertNotError(t, err, "Couldn't create new pending authorization") + test.Assert(t, paC.ID != "", "ID shouldn't be blank") + // Don't require V2 authorizations because the above pending authorizations // aren't associated with orders, and therefore are seen as legacy V1 // authorizations. requireV2Authzs := false - now := fc.Now().UnixNano() + + // Set an expiry cut off of 1 day in the future similar to `RA.NewOrder`. This + // should exclude pending authorization C based on its nearbyExpires expiry + // value. + expiryCutoff := fc.Now().AddDate(0, 0, 1).UnixNano() // Get authorizations for the names used above. authz, err := sa.GetAuthorizations(context.Background(), &sapb.GetAuthorizationsRequest{ RegistrationID: ®.ID, Domains: idents, - Now: &now, + Now: &expiryCutoff, RequireV2Authzs: &requireV2Authzs, }) // It should not fail test.AssertNotError(t, err, "sa.GetAuthorizations failed") - // We should get back two authorizations + // We should get back two authorizations since one of the three authorizations + // created above expires too soon. test.AssertEquals(t, len(authz.Authz), 2) // Get authorizations for the names used above, and one name that doesn't exist authz, err = sa.GetAuthorizations(context.Background(), &sapb.GetAuthorizationsRequest{ RegistrationID: ®.ID, - Domains: append(idents, "ccc"), - Now: &now, + Domains: append(idents, identD), + Now: &expiryCutoff, RequireV2Authzs: &requireV2Authzs, }) // It should not fail test.AssertNotError(t, err, "sa.GetAuthorizations failed") - // It should return two authorizations + // It should still return only two authorizations test.AssertEquals(t, len(authz.Authz), 2) // Get authorizations for the names used above, but this time enforce that no @@ -1690,7 +1713,7 @@ func TestGetAuthorizations(t *testing.T) { authz, err = sa.GetAuthorizations(context.Background(), &sapb.GetAuthorizationsRequest{ RegistrationID: ®.ID, Domains: idents, - Now: &now, + Now: &expiryCutoff, RequireV2Authzs: &requireV2Authzs, }) // It should not fail @@ -1714,7 +1737,7 @@ func TestGetAuthorizations(t *testing.T) { authz, err = sa.GetAuthorizations(context.Background(), &sapb.GetAuthorizationsRequest{ RegistrationID: ®.ID, Domains: idents, - Now: &now, + Now: &expiryCutoff, RequireV2Authzs: &requireV2Authzs, }) // It should not fail