Skip to content

Drastically simplify changes to verify.go #1

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

Open
aarongable wants to merge 1 commit into
base: patch/issues-8-9-fresh-account-auth
Choose a base branch
from
+82 −110
Open

Drastically simplify changes to verify.go #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
115 changes: 45 additions & 70 deletions wfe2/verify.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,9 +300,9 @@ func (wfe *WebFrontEndImpl) validPOSTURL(
func (wfe *WebFrontEndImpl) matchJWSURLs(outer, inner jose.Header) error {
// Verify that the outer JWS has a non-empty URL header. This is strictly
// defensive since the expectation is that endpoints using `matchJWSURLs`
// have received at least one of their JWS from an account-authenticated
// verifier, which checks the outer JWS has the expected URL header before
// processing the inner JWS.
// have received at least one of their JWS from calling validPOSTForAccount(),
// which checks the outer JWS has the expected URL header before processing
// the inner JWS.
outerURL, ok := outer.ExtraHeaders[jose.HeaderKey("url")].(string)
if !ok || len(outerURL) == 0 {
wfe.stats.joseErrorCount.With(prometheus.Labels{"type": "KeyRolloverOuterJWSNoURL"}).Inc()
Expand Down Expand Up @@ -489,19 +489,11 @@ func (wfe *WebFrontEndImpl) acctIDFromURL(acctURL string, request *http.Request)
// authentication and does not contain an embedded JWK. Callers should have
// acquired headers from a bJSONWebSignature.
func (wfe *WebFrontEndImpl) lookupJWK(
header jose.Header,
ctx context.Context,
request *http.Request,
logEvent *web.RequestEvent) (*jose.JSONWebKey, *core.Registration, error) {
return wfe.lookupJWKUsing(header, ctx, request, logEvent, wfe.accountGetter)
}

func (wfe *WebFrontEndImpl) lookupJWKUsing(
header jose.Header,
ctx context.Context,
request *http.Request,
logEvent *web.RequestEvent,
accountGetter AccountGetter) (*jose.JSONWebKey, *core.Registration, error) {
accountGetter AccountGetter,
logEvent *web.RequestEvent) (*jose.JSONWebKey, *core.Registration, error) {
// We expect the request to be using an embedded Key ID auth type and to not
// contain the mutually exclusive embedded JWK.
if err := wfe.enforceJWSAuthType(header, embeddedKeyID); err != nil {
Expand Down Expand Up @@ -610,27 +602,13 @@ func (wfe *WebFrontEndImpl) validJWSForKey(
// JSONWebSignature, and a pointer to the JWK's associated account. If any of
// these conditions are not met or an error occurs only a error is returned.
func (wfe *WebFrontEndImpl) validJWSForAccount(
jws *bJSONWebSignature,
request *http.Request,
ctx context.Context,
logEvent *web.RequestEvent) ([]byte, *bJSONWebSignature, *core.Registration, error) {
return wfe.validJWSForAccountUsing(jws, request, ctx, logEvent, wfe.accountGetter)
}

func (wfe *WebFrontEndImpl) validJWSForAccountUsing(
jws *bJSONWebSignature,
request *http.Request,
ctx context.Context,
logEvent *web.RequestEvent,
accountGetter AccountGetter) ([]byte, *bJSONWebSignature, *core.Registration, error) {
accountGetter AccountGetter,
logEvent *web.RequestEvent) ([]byte, *bJSONWebSignature, *core.Registration, error) {
// Lookup the account and JWK for the key ID that authenticated the JWS
pubKey, account, err := wfe.lookupJWKUsing(
jws.Signatures[0].Header,
ctx,
request,
logEvent,
accountGetter,
)
pubKey, account, err := wfe.lookupJWK(ctx, jws.Signatures[0].Header, request, accountGetter, logEvent)
if err != nil {
return nil, nil, nil, err
}
Expand All @@ -644,65 +622,62 @@ func (wfe *WebFrontEndImpl) validJWSForAccountUsing(
return payload, jws, account, nil
}

// validPOSTForAccount checks a POST request against the configured account
// getter, which may be the WFE-local account cache. Use
// validPOSTForCurrentAccount before account-authenticated requests cross
// issuance or account mutation boundaries.
// validPOSTForAccount checks that a given POST request has a valid JWS using
// `validJWSForAccount` and the WFE's database/SA connection. If valid, the
// authenticated JWS body and the registration that authenticated the body are
// returned. Otherwise an error is returned.
//
// This function is not ideal for validating POST-as-GET requests; although it
// will work correctly, it will not validate that the request body is empty.
// Those requests should be validated via validPOSTAsGETForAccount.
func (wfe *WebFrontEndImpl) validPOSTForAccount(
request *http.Request,
ctx context.Context,
request *http.Request,
logEvent *web.RequestEvent) ([]byte, *bJSONWebSignature, *core.Registration, error) {
// Parse the JWS from the POST request
jws, err := wfe.parseJWSRequest(request)
if err != nil {
return nil, nil, nil, err
}
return wfe.validJWSForAccount(jws, request, ctx, logEvent)
}

// validPOSTForCurrentAccount checks a POST request against current SA account
// state, bypassing the WFE-local account cache. Use it before account-authenticated
// requests cross issuance or account mutation boundaries.
func (wfe *WebFrontEndImpl) validPOSTForCurrentAccount(
request *http.Request,
ctx context.Context,
logEvent *web.RequestEvent) ([]byte, *bJSONWebSignature, *core.Registration, error) {
return wfe.validPOSTForAccountUsing(request, ctx, logEvent, wfe.sa)
// Use wfe.sa (i.e. the real database) as the AccountGetter for all POST
// requests, as POSTs may modify the database. This prevents a stale account
// cache from letting an old key perform mutating operations, like rotating
// to a new key.
return wfe.validJWSForAccount(ctx, jws, request, wfe.sa, logEvent)
}

func (wfe *WebFrontEndImpl) validPOSTForAccountUsing(
request *http.Request,
// validPOSTAsGETForAccount checks that a given POST request is valid using
// `validJWSForAccount` and the wfe's read-through account cache. It
// additionally validates that the JWS request payload is empty. If a non empty
// payload is provided, it returns MalformedError.
//
// This function must only be used to validate POST-as-GET requests, it must
// not be used to validate mutating POST requests.
func (wfe *WebFrontEndImpl) validPOSTAsGETForAccount(
ctx context.Context,
logEvent *web.RequestEvent,
accountGetter AccountGetter) ([]byte, *bJSONWebSignature, *core.Registration, error) {
request *http.Request,
logEvent *web.RequestEvent) (*core.Registration, error) {
// Parse the JWS from the POST request
jws, err := wfe.parseJWSRequest(request)
if err != nil {
return nil, nil, nil, err
return nil, err
}
return wfe.validJWSForAccountUsing(jws, request, ctx, logEvent, accountGetter)
}

// validPOSTAsGETForAccount checks that a given POST request is valid using
// the configured account getter, which may be the WFE-local account cache. It
// additionally validates that the JWS request payload is empty, indicating
// that it is a POST-as-GET request per ACME draft 15+ section 6.3 "GET and
// POST-as-GET requests". If a non empty payload is provided in the JWS the
// invalidPOSTAsGETErr error is returned. This function is useful only for
// endpoints that do not need to handle both POSTs with a body and POST-as-GET
// requests (e.g. Order, Certificate).
func (wfe *WebFrontEndImpl) validPOSTAsGETForAccount(
request *http.Request,
ctx context.Context,
logEvent *web.RequestEvent) (*core.Registration, error) {
body, _, reg, err := wfe.validPOSTForAccount(request, ctx, logEvent)
// Use the account cache to look up the account. We are willing to accept
// idempotent read-only POST-as-GET requests authenticated by a key in the
// possibly-stale account cache, because such requests are relatively safe
// and relatively high-volume.
body, _, reg, err := wfe.validJWSForAccount(ctx, jws, request, wfe.accountCache, logEvent)
if err != nil {
return nil, err
}

// Verify the POST-as-GET payload is empty
if string(body) != "" {
return nil, berrors.MalformedError("POST-as-GET requests must have an empty payload")
}

// To make log analysis easier we choose to elevate the pseudo ACME HTTP
// method "POST-as-GET" to the logEvent's Method, replacing the
// http.MethodPost value.
Expand Down Expand Up @@ -793,9 +768,9 @@ type rolloverOperation struct {

// validKeyRollover checks if the innerJWS is a valid key rollover operation
// given the outer JWS that carried it. It is assumed that the outerJWS has
// already been validated per the normal ACME process. It is *critical* this is
// the case since `validKeyRollover` does not check the outerJWS signature. This
// function checks that:
// already been validated per the normal ACME process using `validPOSTForAccount`.
// It is *critical* this is the case since `validKeyRollover` does not check the
// outerJWS signature. This function checks that:
// 1) the inner JWS is valid and well formed
// 2) the inner JWS has the same "url" header as the outer JWS
// 3) the inner JWS is self-authenticated with an embedded JWK
Expand Down Expand Up @@ -844,8 +819,8 @@ func (wfe *WebFrontEndImpl) validKeyRollover(
return nil, berrors.MalformedError("Inner JWS does not verify with embedded JWK")
}
// NOTE(@cpu): we do not stomp the web.RequestEvent's payload here since that is set
// from the outerJWS by the account-authenticated verifier and contains the
// inner JWS and inner payload already.
// from the outerJWS in validPOSTForAccount and contains the inner JWS and inner
// payload already.

// Verify that the outer and inner JWS protected URL headers match
if err := wfe.matchJWSURLs(outerJWS.Signatures[0].Header, innerJWS.Signatures[0].Header); err != nil {
Expand Down
24 changes: 12 additions & 12 deletions wfe2/verify_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1178,7 +1178,7 @@ func TestLookupJWK(t *testing.T) {
in := tc.JWS.Signatures[0].Header
inputLogEvent := newRequestEvent()

gotJWK, gotAcct, gotErr := wfe.lookupJWK(in, context.Background(), tc.Request, inputLogEvent)
gotJWK, gotAcct, gotErr := wfe.lookupJWK(t.Context(), in, tc.Request, wfe.accountCache, inputLogEvent)
if tc.WantErrDetail == "" {
if gotErr != nil {
t.Fatalf("lookupJWK(%#v) = %#v, want nil", in, gotErr)
Expand Down Expand Up @@ -1395,7 +1395,7 @@ func TestValidPOSTForAccount(t *testing.T) {
wfe.stats.joseErrorCount.Reset()
inputLogEvent := newRequestEvent()

gotPayload, gotJWS, gotAcct, gotErr := wfe.validPOSTForAccount(tc.Request, context.Background(), inputLogEvent)
gotPayload, gotJWS, gotAcct, gotErr := wfe.validPOSTForAccount(t.Context(), tc.Request, inputLogEvent)
if tc.WantErrDetail == "" {
if gotErr != nil {
t.Fatalf("validPOSTForAccount(%#v) = %#v, want nil", tc.Request, gotErr)
Expand Down Expand Up @@ -1478,11 +1478,11 @@ func setupAuthOnlyWFE(
inmemNonceService := &inmemnonce.NonceService{Impl: nonceService}
freshGetter := &staticRegistrationGetter{registration: freshAccount}
wfe := WebFrontEndImpl{
sa: freshGetter,
rnc: inmemNonceService,
rncKey: rncKey,
accountGetter: rejectingRegistrationGetter{t: t},
stats: initStats(metrics.NoopRegisterer),
sa: freshGetter,
rnc: inmemNonceService,
rncKey: rncKey,
accountCache: rejectingRegistrationGetter{t: t},
stats: initStats(metrics.NoopRegisterer),
}

return wfe, requestSigner{t, inmemNonceService.AsSource()}, freshGetter
Expand All @@ -1497,7 +1497,7 @@ func TestValidPOSTForCurrentAccountRejectsCachedDeactivatedAccount(t *testing.T)
_, _, body := signer.byKeyID(1, nil, "http://localhost/test", "{}")
request := makePostRequestWithPath("test", body)

_, _, _, err := wfe.validPOSTForCurrentAccount(request, ctx, newRequestEvent())
_, _, _, err := wfe.validPOSTForAccount(ctx, request, newRequestEvent())
test.AssertErrorIs(t, err, berrors.Unauthorized)
test.AssertContains(t, err.Error(), `Account is not valid, has status "deactivated"`)
test.AssertEquals(t, freshGetter.calls, 1)
Expand All @@ -1512,7 +1512,7 @@ func TestValidPOSTForCurrentAccountRejectsCachedPreRolloverKey(t *testing.T) {
_, _, body := signer.byKeyID(1, nil, "http://localhost/test", "{}")
request := makePostRequestWithPath("test", body)

_, _, _, err := wfe.validPOSTForCurrentAccount(request, ctx, newRequestEvent())
_, _, _, err := wfe.validPOSTForAccount(ctx, request, newRequestEvent())
test.AssertErrorIs(t, err, berrors.Malformed)
test.AssertContains(t, err.Error(), "JWS verification error")
test.AssertEquals(t, freshGetter.calls, 1)
Expand Down Expand Up @@ -1557,7 +1557,7 @@ func TestValidPOSTAsGETForAccount(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.Name, func(t *testing.T) {
ev := newRequestEvent()
_, gotErr := wfe.validPOSTAsGETForAccount(tc.Request, context.Background(), ev)
_, gotErr := wfe.validPOSTAsGETForAccount(t.Context(), tc.Request, ev)
if tc.WantErrDetail == "" {
if gotErr != nil {
t.Fatalf("validPOSTAsGETForAccount(%#v) = %#v, want nil", tc.Request, gotErr)
Expand Down Expand Up @@ -1595,7 +1595,7 @@ func (sa mockSADifferentStoredKey) GetRegistration(_ context.Context, _ *sapb.Re
func TestValidPOSTForAccountSwappedKey(t *testing.T) {
wfe, _, signer := setupWFE(t)
wfe.sa = &mockSADifferentStoredKey{}
wfe.accountGetter = wfe.sa
wfe.accountCache = wfe.sa
event := newRequestEvent()

payload := `{"resource":"ima-payload"}`
Expand All @@ -1606,7 +1606,7 @@ func TestValidPOSTForAccountSwappedKey(t *testing.T) {
// Ensure that ValidPOSTForAccount produces an error since the
// mockSADifferentStoredKey will return a different key than the one we used to
// sign the request
_, _, _, err := wfe.validPOSTForAccount(request, ctx, event)
_, _, _, err := wfe.validPOSTForAccount(ctx, request, event)
test.AssertError(t, err, "No error returned for request signed by wrong key")
test.AssertErrorIs(t, err, berrors.Malformed)
test.AssertContains(t, err.Error(), "JWS verification error")
Expand Down
Loading