Skip to content

Commit

Permalink
Simplify GetValidOrderAuthorizations2 (#7646)
Browse files Browse the repository at this point in the history 
Simplify SA.GetValidOrderAuthorizations2 so that it no longer conditions
the query on the status, expiry, or registration ID of the authorization
rows. This gives the query much better performance, because it no longer
tries to use an overly-large index, and fall back to large row-scans
when the query planner decides the index is too large.

While we're here, also improve the return type of
GetValidOrderAuthorizations2, so that instead of returning a map of
names to authorizations, it simply returns a list of authzs. This both
reduces the size of the gRPC message (once the old map is fully
removed), and improves its correctness because we cannot count on names
to be unique across multiple identifier types.

Finally, improve the RA code which calls SA.GetValidOrderAuthorizations2
to handle this improved return type, to make fewer assumptions about
identifier types, and to separate static authorization-checking from CAA
rechecking.

Fixes #7645
  • Loading branch information
@aarongable
aarongable authored Aug 8, 2024
1 parent 35b0b55 commit 28f0934
Show file tree
Hide file tree
Showing 7 changed files with 1,293 additions and 1,395 deletions.
26 changes: 19 additions & 7 deletions grpc/pb-marshalling.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -421,14 +421,26 @@ func PBToCertStatus(pb *corepb.CertificateStatus) core.CertificateStatus {

// PBToAuthzMap converts a protobuf map of domains mapped to protobuf authorizations to a
// golang map[string]*core.Authorization.
func PBToAuthzMap(pb *sapb.Authorizations) (map[string]*core.Authorization, error) {
m := make(map[string]*core.Authorization, len(pb.Authz))
for _, v := range pb.Authz {
authz, err := PBToAuthz(v.Authz)
if err != nil {
return nil, err
func PBToAuthzMap(pb *sapb.Authorizations) (map[identifier.ACMEIdentifier]*core.Authorization, error) {
m := make(map[identifier.ACMEIdentifier]*core.Authorization, len(pb.Authz))
if len(pb.Authzs) != 0 {
// Prefer the new field, if it is populated.
for _, v := range pb.Authzs {
authz, err := PBToAuthz(v)
if err != nil {
return nil, err
}
m[authz.Identifier] = &authz
}
} else {
// Fall back to the old field, if necessary.
for _, v := range pb.Authz {
authz, err := PBToAuthz(v.Authz)
if err != nil {
return nil, err
}
m[authz.Identifier] = &authz
}
m[v.Domain] = &authz
}
return m, nil
}
126 changes: 74 additions & 52 deletions ra/ra.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -783,9 +783,10 @@ func (ra *RegistrationAuthorityImpl) matchesCSR(parsedCertificate *x509.Certific
// will be of type BoulderError.
func (ra *RegistrationAuthorityImpl) checkOrderAuthorizations(
ctx context.Context,
names []string,
orderID orderID,
acctID accountID,
orderID orderID) (map[string]*core.Authorization, error) {
names []string,
now time.Time) (map[identifier.ACMEIdentifier]*core.Authorization, error) {
// Get all of the valid authorizations for this account/order
req := &sapb.GetValidOrderAuthorizationsRequest{
Id: int64(orderID),
Expand All @@ -800,23 +801,68 @@ func (ra *RegistrationAuthorityImpl) checkOrderAuthorizations(
return nil, err
}

// Ensure the names from the CSR are free of duplicates & lowercased.
names = core.UniqueLowerNames(names)

// Check the authorizations to ensure validity for the names required.
err = ra.checkAuthorizationsCAA(ctx, int64(acctID), names, authzs, ra.clk.Now())
if err != nil {
return nil, err
}
// Ensure that every identifier has a matching authz, and vice-versa.
var missing []string
var invalid []string
var expired []string
for _, name := range names {
// TODO(#7647): Iterate directly over identifiers here, once the rest of the
// finalization flow supports non-dnsName identifiers.
ident := identifier.DNSIdentifier(name)

// Check the challenges themselves too.
for _, authz := range authzs {
authz, ok := authzs[ident]
if !ok || authz == nil {
missing = append(missing, ident.Value)
continue
}
if authz.Status != core.StatusValid {
invalid = append(invalid, ident.Value)
continue
}
if authz.Expires.Before(now) {
expired = append(expired, ident.Value)
continue
}
err = ra.PA.CheckAuthz(authz)
if err != nil {
return nil, err
invalid = append(invalid, ident.Value)
continue
}
}

if len(missing) > 0 {
return nil, berrors.UnauthorizedError(
"authorizations for these identifiers not found: %s",
strings.Join(missing, ", "),
)
}

if len(invalid) > 0 {
return nil, berrors.UnauthorizedError(
"authorizations for these identifiers not valid: %s",
strings.Join(invalid, ", "),
)
}
if len(expired) > 0 {
return nil, berrors.UnauthorizedError(
"authorizations for these identifiers expired: %s",
strings.Join(expired, ", "),
)
}

// Even though this check is cheap, we do it after the more specific checks
// so that we can return more specific error messages.
if len(names) != len(authzs) {
return nil, berrors.UnauthorizedError("incorrect number of names requested for finalization")
}

// Check that the authzs either don't need CAA rechecking, or do the
// necessary CAA rechecks right now.
err = ra.checkAuthorizationsCAA(ctx, int64(acctID), authzs, now)
if err != nil {
return nil, err
}

return authzs, nil
}

Expand All @@ -833,19 +879,15 @@ func validatedBefore(authz *core.Authorization, caaRecheckTime time.Time) (bool,
return authz.Challenges[0].Validated.Before(caaRecheckTime), nil
}

// checkAuthorizationsCAA implements the common logic of validating a set of
// authorizations against a set of names that is used by both
// `checkAuthorizations` and `checkOrderAuthorizations`. If required CAA will be
// rechecked for authorizations that are too old.
// If it returns an error, it will be of type BoulderError.
// checkAuthorizationsCAA ensures that we have sufficiently-recent CAA checks
// for every input identifier/authz. If any authz was validated too long ago, it
// kicks off a CAA recheck for that identifier If it returns an error, it will
// be of type BoulderError.
func (ra *RegistrationAuthorityImpl) checkAuthorizationsCAA(
ctx context.Context,
acctID int64,
names []string,
authzs map[string]*core.Authorization,
authzs map[identifier.ACMEIdentifier]*core.Authorization,
now time.Time) error {
// badNames contains the names that were unauthorized
var badNames []string
// recheckAuthzs is a list of authorizations that must have their CAA records rechecked
var recheckAuthzs []*core.Authorization

Expand All @@ -858,15 +900,8 @@ func (ra *RegistrationAuthorityImpl) checkAuthorizationsCAA(
// Set the recheck time to 7 hours ago.
caaRecheckAfter := now.Add(caaRecheckDuration)

for _, name := range names {
authz := authzs[name]
if authz == nil {
badNames = append(badNames, name)
} else if authz.Expires == nil {
return berrors.InternalServerError("found an authorization with a nil Expires field: id %s", authz.ID)
} else if authz.Expires.Before(now) {
badNames = append(badNames, name)
} else if staleCAA, err := validatedBefore(authz, caaRecheckAfter); err != nil {
for _, authz := range authzs {
if staleCAA, err := validatedBefore(authz, caaRecheckAfter); err != nil {
return berrors.InternalServerError(err.Error())
} else if staleCAA {
// Ensure that CAA is rechecked for this name
Expand All @@ -881,13 +916,6 @@ func (ra *RegistrationAuthorityImpl) checkAuthorizationsCAA(
}
}

if len(badNames) > 0 {
return berrors.UnauthorizedError(
"authorizations for these names not found or expired: %s",
strings.Join(badNames, ", "),
)
}

caaEvent := &finalizationCAACheckEvent{
Requester: acctID,
Reused: len(authzs) - len(recheckAuthzs),
Expand Down Expand Up @@ -1155,16 +1183,9 @@ func (ra *RegistrationAuthorityImpl) validateFinalizeRequest(
csrNames := csrlib.NamesFromCSR(csr).SANs
orderNames := core.UniqueLowerNames(req.Order.Names)

// Immediately reject the request if the number of names differ
if len(orderNames) != len(csrNames) {
return nil, berrors.UnauthorizedError("Order includes different number of names than CSR specifies")
}

// Check that the order names and the CSR names are an exact match
for i, name := range orderNames {
if name != csrNames[i] {
return nil, berrors.UnauthorizedError("CSR is missing Order domain %q", name)
}
if !slices.Equal(csrNames, orderNames) {
return nil, berrors.UnauthorizedError(("CSR does not specify same identifiers as Order"))
}

// Get the originating account for use in the next check.
Expand All @@ -1183,9 +1204,10 @@ func (ra *RegistrationAuthorityImpl) validateFinalizeRequest(
return nil, berrors.MalformedError("certificate public key must be different than account key")
}

// Double-check that all authorizations on this order are also associated with
// the same account as the order itself.
authzs, err := ra.checkOrderAuthorizations(ctx, csrNames, accountID(req.Order.RegistrationID), orderID(req.Order.Id))
// Double-check that all authorizations on this order are valid, are also
// associated with the same account as the order itself, and have recent CAA.
authzs, err := ra.checkOrderAuthorizations(
ctx, orderID(req.Order.Id), accountID(req.Order.RegistrationID), csrNames, ra.clk.Now())
if err != nil {
// Pass through the error without wrapping it because the called functions
// return BoulderError and we don't want to lose the type.
Expand All @@ -1195,11 +1217,11 @@ func (ra *RegistrationAuthorityImpl) validateFinalizeRequest(
// Collect up a certificateRequestAuthz that stores the ID and challenge type
// of each of the valid authorizations we used for this issuance.
logEventAuthzs := make(map[string]certificateRequestAuthz, len(csrNames))
for name, authz := range authzs {
for _, authz := range authzs {
// No need to check for error here because we know this same call just
// succeeded inside ra.checkOrderAuthorizations
solvedByChallengeType, _ := authz.SolvedBy()
logEventAuthzs[name] = certificateRequestAuthz{
logEventAuthzs[authz.Identifier.Value] = certificateRequestAuthz{
ID: authz.ID,
ChallengeType: solvedByChallengeType,
}
Expand Down
41 changes: 23 additions & 18 deletions ra/ra_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1762,8 +1762,8 @@ func TestRecheckCAADates(t *testing.T) {
}
}

authzs := map[string]*core.Authorization{
"recent.com": {
authzs := map[identifier.ACMEIdentifier]*core.Authorization{
identifier.DNSIdentifier("recent.com"): {
Identifier: makeIdentifier("recent.com"),
Expires: &recentExpires,
Challenges: []core.Challenge{
Expand All @@ -1775,7 +1775,7 @@ func TestRecheckCAADates(t *testing.T) {
},
},
},
"older.com": {
identifier.DNSIdentifier("older.com"): {
Identifier: makeIdentifier("older.com"),
Expires: &olderExpires,
Challenges: []core.Challenge{
Expand All @@ -1787,7 +1787,7 @@ func TestRecheckCAADates(t *testing.T) {
},
},
},
"older2.com": {
identifier.DNSIdentifier("older2.com"): {
Identifier: makeIdentifier("older2.com"),
Expires: &olderExpires,
Challenges: []core.Challenge{
Expand All @@ -1799,7 +1799,7 @@ func TestRecheckCAADates(t *testing.T) {
},
},
},
"wildcard.com": {
identifier.DNSIdentifier("wildcard.com"): {
Identifier: makeIdentifier("wildcard.com"),
Expires: &olderExpires,
Challenges: []core.Challenge{
Expand All @@ -1811,7 +1811,7 @@ func TestRecheckCAADates(t *testing.T) {
},
},
},
"*.wildcard.com": {
identifier.DNSIdentifier("*.wildcard.com"): {
Identifier: makeIdentifier("*.wildcard.com"),
Expires: &olderExpires,
Challenges: []core.Challenge{
Expand All @@ -1823,7 +1823,9 @@ func TestRecheckCAADates(t *testing.T) {
},
},
},
"twochallenges.com": {
}
twoChallenges := map[identifier.ACMEIdentifier]*core.Authorization{
identifier.DNSIdentifier("twochallenges.com"): {
ID: "twochal",
Identifier: makeIdentifier("twochallenges.com"),
Expires: &recentExpires,
Expand All @@ -1842,13 +1844,17 @@ func TestRecheckCAADates(t *testing.T) {
},
},
},
"nochallenges.com": {
}
noChallenges := map[identifier.ACMEIdentifier]*core.Authorization{
identifier.DNSIdentifier("nochallenges.com"): {
ID: "nochal",
Identifier: makeIdentifier("nochallenges.com"),
Expires: &recentExpires,
Challenges: []core.Challenge{},
},
"novalidationtime.com": {
}
noValidationTime := map[identifier.ACMEIdentifier]*core.Authorization{
identifier.DNSIdentifier("novalidationtime.com"): {
ID: "noval",
Identifier: makeIdentifier("novalidationtime.com"),
Expires: &recentExpires,
Expand All @@ -1865,23 +1871,22 @@ func TestRecheckCAADates(t *testing.T) {

// NOTE: The names provided here correspond to authorizations in the
// `mockSAWithRecentAndOlder`
names := []string{"recent.com", "older.com", "older2.com", "wildcard.com", "*.wildcard.com"}
err := ra.checkAuthorizationsCAA(context.Background(), Registration.Id, names, authzs, fc.Now())
err := ra.checkAuthorizationsCAA(context.Background(), Registration.Id, authzs, fc.Now())
// We expect that there is no error rechecking authorizations for these names
if err != nil {
t.Errorf("expected nil err, got %s", err)
}

// Should error if a authorization has `!= 1` challenge
err = ra.checkAuthorizationsCAA(context.Background(), Registration.Id, []string{"twochallenges.com"}, authzs, fc.Now())
err = ra.checkAuthorizationsCAA(context.Background(), Registration.Id, twoChallenges, fc.Now())
test.AssertEquals(t, err.Error(), "authorization has incorrect number of challenges. 1 expected, 2 found for: id twochal")

// Should error if a authorization has `!= 1` challenge
err = ra.checkAuthorizationsCAA(context.Background(), Registration.Id, []string{"nochallenges.com"}, authzs, fc.Now())
err = ra.checkAuthorizationsCAA(context.Background(), Registration.Id, noChallenges, fc.Now())
test.AssertEquals(t, err.Error(), "authorization has incorrect number of challenges. 1 expected, 0 found for: id nochal")

// Should error if authorization's challenge has no validated timestamp
err = ra.checkAuthorizationsCAA(context.Background(), Registration.Id, []string{"novalidationtime.com"}, authzs, fc.Now())
err = ra.checkAuthorizationsCAA(context.Background(), Registration.Id, noValidationTime, fc.Now())
test.AssertEquals(t, err.Error(), "authorization's challenge has no validated timestamp for: id noval")

// We expect that "recent.com" is not checked because its mock authorization
Expand Down Expand Up @@ -2897,7 +2902,7 @@ func TestFinalizeOrder(t *testing.T) {
},
Csr: oneDomainCSR,
},
ExpectedErrMsg: "Order includes different number of names than CSR specifies",
ExpectedErrMsg: "CSR does not specify same identifiers as Order",
},
{
Name: "CSR and Order with diff number of names (other way)",
Expand All @@ -2910,7 +2915,7 @@ func TestFinalizeOrder(t *testing.T) {
},
Csr: twoDomainCSR,
},
ExpectedErrMsg: "Order includes different number of names than CSR specifies",
ExpectedErrMsg: "CSR does not specify same identifiers as Order",
},
{
Name: "CSR missing an order name",
Expand All @@ -2923,7 +2928,7 @@ func TestFinalizeOrder(t *testing.T) {
},
Csr: oneDomainCSR,
},
ExpectedErrMsg: "CSR is missing Order domain \"foobar.com\"",
ExpectedErrMsg: "CSR does not specify same identifiers as Order",
},
{
Name: "CSR with policy forbidden name",
Expand Down Expand Up @@ -2973,7 +2978,7 @@ func TestFinalizeOrder(t *testing.T) {
},
Csr: twoDomainCSR,
},
ExpectedErrMsg: "authorizations for these names not found or expired: a.com, b.com",
ExpectedErrMsg: "authorizations for these identifiers not found: a.com, b.com",
},
{
Name: "Order with correct authorizations, ready status",
Expand Down
Loading

0 comments on commit 28f0934

Please sign in to comment.