Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify GetValidOrderAuthorizations2 #7646

Merged
merged 4 commits into from
Aug 8, 2024
+1,293 −1,395
Merged

Simplify GetValidOrderAuthorizations2 #7646

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e3ec249
Simplify GetValidOrderAuthorizations2
aarongable Aug 5, 2024
0d37e2d
Fixup TODO comments
aarongable Aug 6, 2024
8362297
Merge branch main into better-check-finalize
aarongable Aug 7, 2024
d47c17f
Merge branch main into better-check-finalize
aarongable Aug 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 19 additions & 7 deletions grpc/pb-marshalling.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1774,8 +1774,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 @@ -1787,7 +1787,7 @@ func TestRecheckCAADates(t *testing.T) {
},
},
},
"older.com": {
identifier.DNSIdentifier("older.com"): {
Identifier: makeIdentifier("older.com"),
Expires: &olderExpires,
Challenges: []core.Challenge{
Expand All @@ -1799,7 +1799,7 @@ func TestRecheckCAADates(t *testing.T) {
},
},
},
"older2.com": {
identifier.DNSIdentifier("older2.com"): {
Identifier: makeIdentifier("older2.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,7 @@ func TestRecheckCAADates(t *testing.T) {
},
},
},
"*.wildcard.com": {
identifier.DNSIdentifier("*.wildcard.com"): {
Identifier: makeIdentifier("*.wildcard.com"),
Expires: &olderExpires,
Challenges: []core.Challenge{
Expand All @@ -1835,7 +1835,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 @@ -1854,13 +1856,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 @@ -1877,23 +1883,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 @@ -2909,7 +2914,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 @@ -2922,7 +2927,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 @@ -2935,7 +2940,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 @@ -2985,7 +2990,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