Skip to content

Commit

Permalink
WFE: Use RA.GetAuthorization to filter out disabled challenges (letse…
Browse files Browse the repository at this point in the history
…ncrypt#7659)

Have the WFE ask the RA for authorizations, rather than asking the SA
directly. This extra layer of indirection allows us to filter out
challenges which have been disabled, so that clients don't think they
can attempt challenges that we have disabled.

Also shuffle the order of challenges within the authz objects rendered
by the API. We used to have code which does this at authz creation time,
but of course that was completely ineffectual once we stored the
challenges as just a bitmap in the database.

Update the WFE unit tests to mock RA.GetAuthorization instead of
SA.GetAuthorization2. This includes making the mock more accurate, so
that (e.g.) valid authorizations contain valid challenges, and the
challenges have their correct types (e.g. "http-01" instead of just
"http"). Also update the OTel tracing test to account for the new RPC.

Part of letsencrypt#5913
  • Loading branch information
aarongable authored Aug 22, 2024
1 parent c9be034 commit cac431c
Show file tree
Hide file tree
Showing 4 changed files with 243 additions and 160 deletions.
51 changes: 1 addition & 50 deletions mocks/sa.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"context"
"crypto/x509"
"errors"
"fmt"
"math/rand/v2"
"net"
"os"
Expand Down Expand Up @@ -501,57 +500,9 @@ func (sa *StorageAuthorityReadOnly) GetAuthorizations2(ctx context.Context, req
return &sapb.Authorizations{}, nil
}

var (
authzIdValid = int64(1)
authzIdPending = int64(2)
authzIdExpired = int64(3)
authzIdErrorResult = int64(4)
authzIdDiffAccount = int64(5)
)

// GetAuthorization2 is a mock
func (sa *StorageAuthorityReadOnly) GetAuthorization2(ctx context.Context, id *sapb.AuthorizationID2, _ ...grpc.CallOption) (*corepb.Authorization, error) {
authz := core.Authorization{
Status: core.StatusValid,
RegistrationID: 1,
Identifier: identifier.DNSIdentifier("not-an-example.com"),
Challenges: []core.Challenge{
{
Status: "pending",
Token: "token",
Type: "dns",
},
},
}

switch id.Id {
case authzIdValid:
exp := sa.clk.Now().AddDate(100, 0, 0)
authz.Expires = &exp
authz.ID = fmt.Sprintf("%d", authzIdValid)
return bgrpc.AuthzToPB(authz)
case authzIdPending:
exp := sa.clk.Now().AddDate(100, 0, 0)
authz.Expires = &exp
authz.ID = fmt.Sprintf("%d", authzIdPending)
authz.Status = core.StatusPending
return bgrpc.AuthzToPB(authz)
case authzIdExpired:
exp := sa.clk.Now().AddDate(0, -1, 0)
authz.Expires = &exp
authz.ID = fmt.Sprintf("%d", authzIdExpired)
return bgrpc.AuthzToPB(authz)
case authzIdErrorResult:
return nil, fmt.Errorf("unspecified database error")
case authzIdDiffAccount:
exp := sa.clk.Now().AddDate(100, 0, 0)
authz.RegistrationID = 2
authz.Expires = &exp
authz.ID = fmt.Sprintf("%d", authzIdDiffAccount)
return bgrpc.AuthzToPB(authz)
}

return nil, berrors.NotFoundError("no authorization found with id %q", id)
return &corepb.Authorization{}, nil
}

// GetSerialsByKey is a mock
Expand Down
12 changes: 8 additions & 4 deletions test/integration/otel_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -233,9 +233,11 @@ func TestTraces(t *testing.T) {
rpcSpan("sa.StorageAuthority/GetOrderForNames", ra, sa),
rpcSpan("sa.StorageAuthority/NewOrderAndAuthzs", ra, sa))),
httpSpan("/acme/authz-v3/",
rpcSpan("sa.StorageAuthorityReadOnly/GetAuthorization2", wfe, sa)),
rpcSpan("ra.RegistrationAuthority/GetAuthorization", wfe, ra,
rpcSpan("sa.StorageAuthority/GetAuthorization2", ra, sa))),
httpSpan("/acme/chall-v3/",
rpcSpan("sa.StorageAuthorityReadOnly/GetAuthorization2", wfe, sa),
rpcSpan("ra.RegistrationAuthority/GetAuthorization", wfe, ra,
rpcSpan("sa.StorageAuthority/GetAuthorization2", ra, sa)),
rpcSpan("ra.RegistrationAuthority/PerformValidation", wfe, ra,
rpcSpan("sa.StorageAuthority/GetRegistration", ra, sa))),
httpSpan("/acme/finalize/",
Expand All @@ -251,8 +253,10 @@ func TestTraces(t *testing.T) {
rpcSpan("ca.CertificateAuthority/IssueCertificateForPrecertificate", ra, ca),
redisPipelineSpan("set", ra),
rpcSpan("sa.StorageAuthority/FinalizeOrder", ra, sa))),
httpSpan("/acme/order/", rpcSpan("sa.StorageAuthorityReadOnly/GetOrder", wfe, sa)),
httpSpan("/acme/cert/", rpcSpan("sa.StorageAuthorityReadOnly/GetCertificate", wfe, sa)),
httpSpan("/acme/order/",
rpcSpan("sa.StorageAuthorityReadOnly/GetOrder", wfe, sa)),
httpSpan("/acme/cert/",
rpcSpan("sa.StorageAuthorityReadOnly/GetCertificate", wfe, sa)),
},
}

Expand Down
11 changes: 9 additions & 2 deletions wfe2/wfe.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"errors"
"fmt"
"math/big"
"math/rand/v2"
"net"
"net/http"
"slices"
Expand Down Expand Up @@ -1111,7 +1112,7 @@ func (wfe *WebFrontEndImpl) Challenge(
return
}
challengeID := slug[1]
authzPB, err := wfe.sa.GetAuthorization2(ctx, &sapb.AuthorizationID2{Id: authorizationID})
authzPB, err := wfe.ra.GetAuthorization(ctx, &rapb.GetAuthorizationRequest{Id: authorizationID})
if err != nil {
if errors.Is(err, berrors.NotFound) {
notFound()
Expand Down Expand Up @@ -1218,6 +1219,12 @@ func (wfe *WebFrontEndImpl) prepAuthorizationForDisplay(request *http.Request, a
for i := range authz.Challenges {
wfe.prepChallengeForDisplay(request, *authz, &authz.Challenges[i])
}

// Shuffle the challenges so no one relies on their order.
rand.Shuffle(len(authz.Challenges), func(i, j int) {
authz.Challenges[i], authz.Challenges[j] = authz.Challenges[j], authz.Challenges[i]
})

authz.ID = ""
authz.RegistrationID = 0

Expand Down Expand Up @@ -1552,7 +1559,7 @@ func (wfe *WebFrontEndImpl) Authorization(
return
}

authzPB, err := wfe.sa.GetAuthorization2(ctx, &sapb.AuthorizationID2{Id: authzID})
authzPB, err := wfe.ra.GetAuthorization(ctx, &rapb.GetAuthorizationRequest{Id: authzID})
if errors.Is(err, berrors.NotFound) {
wfe.sendError(response, logEvent, probs.NotFound("No such authorization"), nil)
return
Expand Down
Loading

0 comments on commit cac431c

Please sign in to comment.