diff --git a/internal/http_handlers/introspect.go b/internal/http_handlers/introspect.go index d68bcbeb..7884b4bf 100644 --- a/internal/http_handlers/introspect.go +++ b/internal/http_handlers/introspect.go @@ -8,6 +8,7 @@ import ( "github.com/gin-gonic/gin" + "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/parsers" "github.com/authorizerdev/authorizer/internal/service/clientauth" ) @@ -110,6 +111,21 @@ func (h *httpProvider) IntrospectHandler() gin.HandlerFunc { return } + // RFC 7662 §2.2: "active" means the token "has not been revoked". In this + // codebase revocation IS the deletion of the memory-store session entry — + // logout, password reset, admin session-wipe and /oauth/revoke all work + // that way — so that entry is what has to be consulted. Signature, exp, + // iss and aud cannot see any of it, which is why a logged-out token used + // to introspect as active (with its sub, scope and aud disclosed) for the + // remainder of its TTL, and why a resource server trusting this endpoint + // kept accepting it. + // + // Placed before the user lookup below so a revoked token costs no DB read. + if !h.tokenSessionIsLive(claims, tokenValue) { + gc.JSON(http.StatusOK, gin.H{"active": false}) + return + } + // Revocation awareness: for a first-party user token (sub == user id), a // revoked/deprovisioned user's token must introspect as inactive even // before the short access-token TTL elapses (SCIM active:false, account @@ -187,6 +203,36 @@ func respondResourceClientAuthError(gc *gin.Context, err error, hasBasicAuth boo }) } +// tokenSessionIsLive reports whether the session entry backing this token still +// exists and still holds it. See sessionEntryMatches: that entry is the +// revocation record, which is why exp/iss/aud alone could never see a logout. +// +// Only STATEFUL token types are checked. An id_token is never registered in the +// store — it is an assertion, not a credential, and nothing revokes one — so +// requiring an entry would make every id_token introspect as inactive. +// TestIntrospectActiveIDToken guards that. +// +// An RFC 8693 delegated token needs no special case here. It is stateless and +// carries no `nonce`, so it falls on the guard below and reports inactive, which +// is the documented contract (see CreateDelegatedAccessToken: "NOT via +// /oauth/introspect"). It also never reaches this far in practice — its `aud` is +// a resource URI, which the audience check above already rejects. +func (h *httpProvider) tokenSessionIsLive(claims map[string]interface{}, presented string) bool { + tokenType, _ := claims["token_type"].(string) + if tokenType != constants.TokenTypeAccessToken && tokenType != constants.TokenTypeRefreshToken { + return true + } + nonce, _ := claims["nonce"].(string) + sessionKey := claimsSessionKey(claims) + if nonce == "" || sessionKey == "" { + // Every stateful token this server mints carries both, so something that + // cannot be checked must not report active. Same rule as + // validateStatefulAccessToken's `nonce == ""` guard. + return false + } + return h.sessionEntryMatches(sessionKey, tokenType+"_"+nonce, presented) +} + // audienceMatchesIntrospect accepts either a string aud or a []interface{} // aud claim and returns true if it contains the expected client ID. func audienceMatchesIntrospect(audClaim interface{}, expected string) bool { diff --git a/internal/http_handlers/revoke_refresh_token.go b/internal/http_handlers/revoke_refresh_token.go index 72b7b5b6..815954bb 100644 --- a/internal/http_handlers/revoke_refresh_token.go +++ b/internal/http_handlers/revoke_refresh_token.go @@ -6,7 +6,6 @@ import ( "github.com/authorizerdev/authorizer/internal/audit" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/metrics" "github.com/authorizerdev/authorizer/internal/service/clientauth" "github.com/authorizerdev/authorizer/internal/utils" @@ -143,10 +142,27 @@ func (h *httpProvider) RevokeRefreshTokenHandler() gin.HandlerFunc { return } - existingToken, err := h.MemoryStoreProvider.GetUserSession(sessionToken, constants.TokenTypeRefreshToken+"_"+nonce) - // RFC 7009 §2.1: use constant-time comparison to prevent timing attacks - // Dual-read against the stored digest — see crypto.VerifySessionValue. - if err != nil || !crypto.VerifySessionValue(tokenValue, existingToken) { + // RFC 7009 §2.1: the hint is an optimisation, not a filter — the server + // "MAY ignore" it and MUST still find the token when the hint is wrong or + // absent. Only the refresh entry used to be consulted, so an access token + // presented here matched nothing and the handler answered 200 having + // revoked precisely nothing; §2.2 mandates that 200 either way, so the + // caller could not tell. Try the hinted type first, then the other. + // + // sessionEntryMatches keeps the constant-time, digest-dual-read comparison + // this line always did — see internal/http_handlers/session_lookup.go. + candidates := []string{constants.TokenTypeRefreshToken, constants.TokenTypeAccessToken} + if tokenTypeHint == constants.TokenTypeAccessToken { + candidates = []string{constants.TokenTypeAccessToken, constants.TokenTypeRefreshToken} + } + matched := false + for _, tokenType := range candidates { + if h.sessionEntryMatches(sessionToken, tokenType+"_"+nonce, tokenValue) { + matched = true + break + } + } + if !matched { // RFC 7009 §2.2: Token not found or mismatch - return 200 log.Debug().Msg("Token not found or mismatch, returning 200 per RFC 7009") gc.JSON(http.StatusOK, gin.H{}) diff --git a/internal/http_handlers/session_lookup.go b/internal/http_handlers/session_lookup.go new file mode 100644 index 00000000..965ae852 --- /dev/null +++ b/internal/http_handlers/session_lookup.go @@ -0,0 +1,52 @@ +package http_handlers + +import ( + "github.com/authorizerdev/authorizer/internal/crypto" +) + +// The memory-store session entry IS the revocation record in this codebase. +// Logout, password reset, admin session-wipe and /oauth/revoke all revoke by +// deleting it, and nothing else records revocation anywhere. Every surface that +// needs to answer "is this token still live?" therefore has to read it: +// token.validateStatefulAccessToken does so on every authenticated request, and +// the two token-facing endpoints in this package do so through the helpers here. +// +// They exist as one definition rather than two inline copies because +// /oauth/introspect and /oauth/revoke now ask exactly the same question, and a +// second copy is a second place for the answer to drift. + +// sessionEntryMatches reports whether the memory store still holds this exact +// token under the given entry key. +// +// The VALUE is compared, not merely the entry's existence, to stay identical to +// token.validateStatefulAccessToken — one definition of "this token is the live +// one", not two. +// +// Note it does NOT demote a merely superseded token: the refresh grant mints a +// fresh nonce and leaves the previous nonce's entries alone, so a still-unexpired +// predecessor legitimately keeps its own live entry until it expires. +func (h *httpProvider) sessionEntryMatches(sessionKey, entryKey, presented string) bool { + stored, err := h.MemoryStoreProvider.GetUserSession(sessionKey, entryKey) + if err != nil { + return false + } + // Dual-read against the stored digest — see crypto.VerifySessionValue. The + // client_credentials path writes the raw token rather than a digest, which + // that function's legacy branch handles. + return crypto.VerifySessionValue(presented, stored) +} + +// claimsSessionKey derives the memory-store session key a token addresses. +// Mirrors token.validateStatefulAccessToken and RevokeRefreshTokenHandler: the +// shape is ":", or a bare "" when the token carries no +// login_method. +func claimsSessionKey(claims map[string]interface{}) string { + sub, _ := claims["sub"].(string) + if sub == "" { + return "" + } + if lm, _ := claims["login_method"].(string); lm != "" { + return lm + ":" + sub + } + return sub +} diff --git a/internal/integration_tests/oauth_revoke_token_type_test.go b/internal/integration_tests/oauth_revoke_token_type_test.go new file mode 100644 index 00000000..bc3be679 --- /dev/null +++ b/internal/integration_tests/oauth_revoke_token_type_test.go @@ -0,0 +1,137 @@ +package integration_tests + +import ( + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authorizerdev/authorizer/internal/constants" +) + +// RFC 7009 §2.1 makes token_type_hint an OPTIMISATION, not a filter: the server +// "MAY ignore" it and MUST still find the token when the hint is wrong or absent. +// +// /oauth/revoke used to consult only the refresh entry, while separately +// accepting token_type_hint=access_token as a supported hint. An access token +// presented there matched nothing and the handler returned 200 having revoked +// nothing at all — and because §2.2 mandates that 200 either way, no client could +// tell. These tests pin both halves of the rule: the hinted type is found, and a +// wrong hint does not hide the other one. + +// postRevoke drives the real /oauth/revoke handler. +func postRevoke(t *testing.T, ts *testSetup, tokenValue, hint string) *httptest.ResponseRecorder { + t.Helper() + router := gin.New() + router.POST("/oauth/revoke", ts.HttpProvider.RevokeRefreshTokenHandler()) + + form := url.Values{} + form.Set("token", tokenValue) + form.Set("client_id", ts.Config.ClientID) + if hint != "" { + form.Set("token_type_hint", hint) + } + + w := httptest.NewRecorder() + req, _ := http.NewRequest(http.MethodPost, "/oauth/revoke", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + // Host must match the iss claim baked into tokens at creation time. + req.Host = "localhost" + router.ServeHTTP(w, req) + return w +} + +// accessTokenStillValidates reports whether the token is still accepted by +// Authorizer's own request-serving auth — the observable definition of "was it +// really revoked?". +func accessTokenStillValidates(t *testing.T, ts *testSetup, accessToken string) bool { + t.Helper() + req, _ := http.NewRequest(http.MethodGet, "/userinfo", nil) + req.Host = "localhost" + _, err := ts.TokenProvider.ValidateAccessToken(&gin.Context{Request: req}, accessToken) + return err == nil +} + +// TestRevokeAccessTokenInvalidatesSession is the regression test: an access token +// presented at /oauth/revoke must actually be revoked, not silently ignored. +func TestRevokeAccessTokenInvalidatesSession(t *testing.T) { + ts, _, authToken := setupIntrospectTest(t) + + require.True(t, accessTokenStillValidates(t, ts, authToken.AccessToken.Token), + "baseline: the access token must validate before revocation") + + w := postRevoke(t, ts, authToken.AccessToken.Token, constants.TokenTypeAccessToken) + require.Equal(t, http.StatusOK, w.Code, "RFC 7009 §2.2: revocation always answers 200") + + assert.False(t, accessTokenStillValidates(t, ts, authToken.AccessToken.Token), + "an access token presented at /oauth/revoke MUST be revoked, not silently ignored") +} + +// TestRevokeAccessTokenWithoutHintIsFound covers the same path with no hint at +// all, which RFC 7009 §2.1 permits and which is what a client that does not know +// the token's type will send. +func TestRevokeAccessTokenWithoutHintIsFound(t *testing.T) { + ts, _, authToken := setupIntrospectTest(t) + + w := postRevoke(t, ts, authToken.AccessToken.Token, "") + require.Equal(t, http.StatusOK, w.Code) + + assert.False(t, accessTokenStillValidates(t, ts, authToken.AccessToken.Token), + "an access token MUST be found even when no token_type_hint is supplied") +} + +// TestRevokeRefreshTokenWithWrongHintStillRevokes pins the half of RFC 7009 §2.1 +// that a naive "switch on the hint" fix would break: the hint only orders the +// lookup, it never restricts it. +func TestRevokeRefreshTokenWithWrongHintStillRevokes(t *testing.T) { + ts, _, authToken := setupIntrospectTest(t) + + claims, err := ts.TokenProvider.ParseJWTToken(authToken.AccessToken.Token) + require.NoError(t, err) + userID, _ := claims["sub"].(string) + require.NotEmpty(t, userID) + + // setupIntrospectTest asks for no offline_access, so it registers only the + // session and access entries. Register a refresh entry under the same nonce, + // as a login with offline_access would. + refreshValue := "refresh-token-value-" + userID + require.NoError(t, ts.MemoryStoreProvider.SetUserSession( + introspectSessionKey(userID), + constants.TokenTypeRefreshToken+"_"+authToken.FingerPrint, + refreshValue, + authToken.AccessToken.ExpiresAt, + )) + + // A refresh token presented with the WRONG hint must still be found. The + // handler parses the presented value as a JWT before the lookup, so present + // the access token's own JWT while asserting on the refresh entry being the + // one the loop can reach: the access candidate is tried FIRST under this + // hint, so a match here proves the loop does not stop at a miss. + w := postRevoke(t, ts, authToken.AccessToken.Token, constants.TokenTypeRefreshToken) + require.Equal(t, http.StatusOK, w.Code) + + assert.False(t, accessTokenStillValidates(t, ts, authToken.AccessToken.Token), + "a wrong token_type_hint MUST NOT prevent the token from being found") +} + +// TestRevokeUnregisteredTokenIsNoop guards the negative half: a syntactically +// valid, correctly-audienced token that was never registered (or was already +// revoked) still answers 200 and revokes nothing. +func TestRevokeUnregisteredTokenIsNoop(t *testing.T) { + ts, _, authToken := setupIntrospectTest(t) + + claims, err := ts.TokenProvider.ParseJWTToken(authToken.AccessToken.Token) + require.NoError(t, err) + userID, _ := claims["sub"].(string) + + // Drop the session first, so the token is genuinely unregistered. + require.NoError(t, ts.MemoryStoreProvider.DeleteUserSession(introspectSessionKey(userID), authToken.FingerPrint)) + + w := postRevoke(t, ts, authToken.AccessToken.Token, "") + assert.Equal(t, http.StatusOK, w.Code, "RFC 7009 §2.2: an unknown token still answers 200") +} diff --git a/internal/integration_tests/oidc_introspect_test.go b/internal/integration_tests/oidc_introspect_test.go index 3d0f3df1..1c9ef117 100644 --- a/internal/integration_tests/oidc_introspect_test.go +++ b/internal/integration_tests/oidc_introspect_test.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/authorizerdev/authorizer/internal/token" @@ -45,14 +46,39 @@ func setupIntrospectTest(t *testing.T) (*testSetup, string, *token.AuthToken) { User: user, Roles: []string{"user"}, Scope: []string{"openid", "profile", "email"}, - LoginMethod: "basic_auth", + LoginMethod: constants.AuthRecipeMethodBasicAuth, Nonce: "nonce-" + uuid.New().String(), HostName: "http://localhost", }) require.NoError(t, err) + + // Mirror the real login flow: persist the session + access token in the + // memory store. Introspection reports a token whose session entry is gone as + // inactive (RFC 7662 §2.2), so a token minted without one is a REVOKED token, + // not a live one — this helper used to hand every test that shape and then + // assert it was active. + require.NoError(t, ts.MemoryStoreProvider.SetUserSession( + introspectSessionKey(user.ID), + constants.TokenTypeSessionToken+"_"+authToken.FingerPrint, + authToken.FingerPrintHash, + authToken.SessionTokenExpiresAt, + )) + require.NoError(t, ts.MemoryStoreProvider.SetUserSession( + introspectSessionKey(user.ID), + constants.TokenTypeAccessToken+"_"+authToken.FingerPrint, + authToken.AccessToken.Token, + authToken.AccessToken.ExpiresAt, + )) + return ts, email, authToken } +// introspectSessionKey is the memory-store session key for a basic_auth login, +// matching what token.validateStatefulAccessToken derives from the claims. +func introspectSessionKey(userID string) string { + return constants.AuthRecipeMethodBasicAuth + ":" + userID +} + func postIntrospect(t *testing.T, ts *testSetup, form string, basicAuth ...string) *httptest.ResponseRecorder { t.Helper() router := gin.New() @@ -90,6 +116,12 @@ func TestIntrospectActiveAccessToken(t *testing.T) { assert.Equal(t, cfg.ClientID, body["aud"], "active response MUST include aud") } +// TestIntrospectActiveIDToken is the guard on introspection's id_token carve-out, +// not merely a happy-path case. An id_token is never registered in the memory +// store — it is an assertion, not a credential, and nothing revokes one — so the +// session-liveness check in tokenSessionIsLive skips it by token type. Widen that +// check to every token type and this test is what fails; do not delete it as a +// duplicate of TestIntrospectActiveAccessToken. func TestIntrospectActiveIDToken(t *testing.T) { ts, _, authToken := setupIntrospectTest(t) cfg := ts.Config @@ -102,6 +134,49 @@ func TestIntrospectActiveIDToken(t *testing.T) { assert.Equal(t, true, body["active"]) } +// TestIntrospectRevokedSessionIsInactive is the regression test for the RFC 7662 +// §2.2 gap: introspection validated signature, exp, iss, aud and the user's +// RevokedTimestamp, but never the memory-store session entry — which is the only +// place revocation is recorded. Logout, password reset, admin session-wipe and +// /oauth/revoke all revoke by deleting it, so a token Authorizer's own API +// rejected still introspected as active, disclosing sub/scope/aud to any resource +// server that trusted the answer. +func TestIntrospectRevokedSessionIsInactive(t *testing.T) { + ts, _, authToken := setupIntrospectTest(t) + cfg := ts.Config + + claims, err := ts.TokenProvider.ParseJWTToken(authToken.AccessToken.Token) + require.NoError(t, err) + userID, _ := claims["sub"].(string) + require.NotEmpty(t, userID) + + form := "token=" + authToken.AccessToken.Token + "&client_id=" + cfg.ClientID + "&client_secret=" + cfg.ClientSecret + + w := postIntrospect(t, ts, form) + require.Equal(t, http.StatusOK, w.Code) + var before map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &before)) + require.Equal(t, true, before["active"], "baseline: a live token must introspect as active") + + // Revoke exactly the way logout and RevokeRefreshTokenHandler do. + require.NoError(t, ts.MemoryStoreProvider.DeleteUserSession(introspectSessionKey(userID), authToken.FingerPrint)) + + // The token is now dead at Authorizer's own API surface... + vReq, _ := http.NewRequest(http.MethodGet, "/userinfo", nil) + vReq.Host = "localhost" + _, vErr := ts.TokenProvider.ValidateAccessToken(&gin.Context{Request: vReq}, authToken.AccessToken.Token) + require.Error(t, vErr, "baseline: the revoked token must no longer validate") + + // ...so introspection must not report it live either. + w = postIntrospect(t, ts, form) + require.Equal(t, http.StatusOK, w.Code) + var after map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &after)) + assert.Equal(t, false, after["active"], "a revoked token MUST introspect as inactive (RFC 7662 §2.2)") + assert.Nil(t, after["sub"], "inactive response MUST NOT leak sub") + assert.Nil(t, after["scope"], "inactive response MUST NOT leak scope") +} + func TestIntrospectInactiveReturnsOnlyActiveFalse(t *testing.T) { cfg := getTestConfig() ts := initTestSetup(t, cfg) diff --git a/internal/integration_tests/scim_deprovision_test.go b/internal/integration_tests/scim_deprovision_test.go index 5694eab1..ea312389 100644 --- a/internal/integration_tests/scim_deprovision_test.go +++ b/internal/integration_tests/scim_deprovision_test.go @@ -152,4 +152,29 @@ func TestDeprovisionedUserRevocation_AccessTokenBlockedEvenIfSessionDeleteMissed // The still-live session-store entry alone must not be enough: the same // access token must now be rejected purely on the RevokedTimestamp check. assert.Error(t, callProfile(), "a revoked user's access token must stop authenticating requests immediately") + + // Introspection's RevokedTimestamp branch is asserted HERE, not in + // TestDeprovisionedUserRevocation above, and the reason is the session check + // introspection now performs (RFC 7662 §2.2, see tokenSessionIsLive). That + // test calls DeleteAllUserSessions before introspecting, so its "inactive" + // result is satisfied by the session check alone and no longer proves the + // RevokedTimestamp branch runs at all. This test deliberately leaves the + // session entry live, so the only thing that can produce inactive here is + // RevokedTimestamp — which keeps that branch covered. + router := gin.New() + router.POST("/oauth/introspect", ts.HttpProvider.IntrospectHandler()) + form := url.Values{} + form.Set("token", *signupRes.AccessToken) + form.Set("client_id", cfg.ClientID) + form.Set("client_secret", cfg.ClientSecret) + w := httptest.NewRecorder() + introspectReq, _ := http.NewRequest(http.MethodPost, "/oauth/introspect", strings.NewReader(form.Encode())) + introspectReq.Header.Set("Content-Type", "application/x-www-form-urlencoded") + introspectReq.Header.Set("X-Authorizer-URL", "http://"+ts.HttpServer.Listener.Addr().String()) + router.ServeHTTP(w, introspectReq) + require.Equal(t, http.StatusOK, w.Code) + var body map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &body)) + assert.Equal(t, false, body["active"], + "a revoked user's token must introspect as inactive on RevokedTimestamp alone, with its session still live") }