diff --git a/SCOPE.md b/SCOPE.md index dcf2419..e9766ef 100644 --- a/SCOPE.md +++ b/SCOPE.md @@ -44,7 +44,13 @@ Keycloak interchangeability are not established. Password and MFA credential migration is not supplied. - Snapshot validation is a limited Go rule set, not full machine-readable schema enforcement. The canonical YAML model and discovery metadata lag newer runtime - capabilities. Protocol hardening gaps remain; see the assessment below. + capabilities. The authorization-code grant now binds the redirect URI, + enforces grant-type eligibility, authenticates confidential clients and + consumes codes atomically, and UserInfo enforces algorithm, issuer and + access-token purpose (KEY-WP-0016). Upstream provider tokens from Authelia are + still accepted on a transport-trust assumption without signature or + issuer/audience verification; that gap remains open. Enforcing these bindings + is not complete profile conformance. See the assessment below. - Tests cover local handlers, adapters, transformations and CLI protocol behavior. They do not establish complete replacement against a running Keycloak/full-LDAP stack. The Scenario B/C shell harnesses are incomplete. diff --git a/WORK-RECORDS.md b/WORK-RECORDS.md index a56ba78..a6913f5 100644 --- a/WORK-RECORDS.md +++ b/WORK-RECORDS.md @@ -95,6 +95,7 @@ | task | KEY-WP-0013-T01 | done | — | workplans/KEY-WP-0013-approval-engine-resource-audience.md | | task | KEY-WP-0013-T02 | wait | — | workplans/KEY-WP-0013-approval-engine-resource-audience.md | | task | KEY-WP-0013-T03 | done | — | workplans/KEY-WP-0013-approval-engine-resource-audience.md | +| task | KEY-WP-0013-T04 | done | — | workplans/KEY-WP-0013-approval-engine-resource-audience.md | | task | KEY-WP-0014-T01 | done | — | workplans/KEY-WP-0014-native-credential-lane-handoff.md | | task | KEY-WP-0014-T02 | done | — | workplans/KEY-WP-0014-native-credential-lane-handoff.md | | task | KEY-WP-0014-T03 | done | — | workplans/KEY-WP-0014-native-credential-lane-handoff.md | diff --git a/history/2026-09-05-011726-scope-intent-assessment.md b/history/2026-09-05-011726-scope-intent-assessment.md index c4d3e14..7458335 100644 --- a/history/2026-09-05-011726-scope-intent-assessment.md +++ b/history/2026-09-05-011726-scope-intent-assessment.md @@ -82,6 +82,17 @@ redirect/grant mismatch, issuer/token-purpose mismatch and concurrent code reuse Validate upstream provider tokens or explicitly establish and test the chosen transport/trust contract. This assessment does not claim a demonstrated attack. +**Status 2026-09-06 (KEY-WP-0016): partially closed.** The local protocol +surface is now enforced and covered by negative tests — redirect-URI binding, +grant-type eligibility on the browser path, confidential-client authentication +with a constant-time comparison, atomic single-use code consumption (the +Get/Delete race was reproduced first: 9 of 16 concurrent exchanges succeeded +before the fix), and UserInfo algorithm, issuer and access-token-purpose checks. +Still open: the Authelia adapter's unverified upstream ID-token claims and its +transport-trust assumption, which is a trust-contract decision rather than a +local binding. G01 is not fully closed until that is settled, and none of this +establishes complete profile conformance. + ### G02 — Machine-readable contract and discovery lag the runtime **Priority: high. Kind: contract drift.** diff --git a/src/internal/authclient/client_test.go b/src/internal/authclient/client_test.go index a813263..cd5f6a9 100644 --- a/src/internal/authclient/client_test.go +++ b/src/internal/authclient/client_test.go @@ -53,7 +53,9 @@ func provider(t *testing.T) (*Client, Discovery, *oidc.TokenHandler) { mux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { json.NewEncoder(w).Encode(d) }) mux.HandleFunc("/authorize", func(w http.ResponseWriter, r *http.Request) { q := r.URL.Query() - code := sessions.Create(&oidc.PKCESession{ClientID: q.Get("client_id"), Username: "test", Nonce: q.Get("nonce"), Scopes: strings.Fields(q.Get("scope")), PKCEChallenge: q.Get("code_challenge"), ExpiresAt: time.Now().Add(time.Minute)}) + // Record the redirect URI as the real /authorize does: the token + // endpoint binds the exchange to it (KEY-WP-0016-T02). + code := sessions.Create(&oidc.PKCESession{ClientID: q.Get("client_id"), Username: "test", Nonce: q.Get("nonce"), Scopes: strings.Fields(q.Get("scope")), PKCEChallenge: q.Get("code_challenge"), RedirectURI: q.Get("redirect_uri"), ExpiresAt: time.Now().Add(time.Minute)}) target, _ := url.Parse(q.Get("redirect_uri")) params := target.Query() params.Set("state", q.Get("state")) diff --git a/src/internal/server/oidc/hardening_test.go b/src/internal/server/oidc/hardening_test.go new file mode 100644 index 0000000..d73a3ff --- /dev/null +++ b/src/internal/server/oidc/hardening_test.go @@ -0,0 +1,334 @@ +package oidc_test + +import ( + "encoding/base64" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "sync" + "testing" + "time" + + "keycape/internal/domain" + "keycape/internal/server/oidc" +) + +// KEY-WP-0016 — negative coverage for the authorization-code protocol bindings +// and UserInfo token verification. Each case asserts the specific rejection, so +// a binding that stops being enforced fails here rather than passing silently. + +func codeExchange(t *testing.T, params url.Values) *http.Request { + t.Helper() + req := httptest.NewRequest(http.MethodPost, "/token", strings.NewReader(params.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + return req +} + +func TestCodeExchangeRejectsRedirectURIMismatchAndOmission(t *testing.T) { + for name, redirect := range map[string]string{ + "omitted": "", + "different": "https://attacker.example.com/callback", + "prefix": seededRedirectURI + "/../callback", + "trailing": seededRedirectURI + "/", + } { + t.Run(name, func(t *testing.T) { + sessions := oidc.NewSessionStore() + h, _ := newTokenHandler(t, sessions, &mockUserRepo{users: map[string]*domain.User{"alice": aliceUser()}}) + verifier := "test-verifier" + code := seededSession(sessions, verifier) + params := url.Values{"grant_type": {"authorization_code"}, "client_id": {"test-client"}, "code": {code}, "code_verifier": {verifier}} + if redirect != "" { + params.Set("redirect_uri", redirect) + } + w := httptest.NewRecorder() + h.ServeHTTP(w, codeExchange(t, params)) + if w.Code != http.StatusBadRequest { + t.Fatalf("status %d, want 400", w.Code) + } + if !strings.Contains(w.Body.String(), "redirect_uri") { + t.Fatalf("wrong rejection: %s", w.Body.String()) + } + }) + } +} + +func TestCodeExchangeRejectsServiceOnlyClient(t *testing.T) { + sessions := oidc.NewSessionStore() + h, _ := newTokenHandler(t, sessions, &mockUserRepo{}) + h.ClientConfig["test-client"].GrantTypes = []string{"client_credentials"} + code := seededSession(sessions, "verifier") + w := httptest.NewRecorder() + h.ServeHTTP(w, codeExchange(t, url.Values{"grant_type": {"authorization_code"}, "client_id": {"test-client"}, "code": {code}, "code_verifier": {"verifier"}, "redirect_uri": {seededRedirectURI}})) + if w.Code != http.StatusBadRequest || !strings.Contains(w.Body.String(), "grant_type") { + t.Fatalf("status %d: %s", w.Code, w.Body.String()) + } +} + +// An empty grantTypes stays an implicit authorization-code client, matching +// what config validation already assumes. +func TestCodeExchangeAllowsImplicitAuthorizationCodeClient(t *testing.T) { + sessions := oidc.NewSessionStore() + h, _ := newTokenHandler(t, sessions, &mockUserRepo{users: map[string]*domain.User{"alice": aliceUser()}}) + h.ClientConfig["test-client"].GrantTypes = nil + verifier := "test-verifier" + code := seededSession(sessions, verifier) + w := httptest.NewRecorder() + h.ServeHTTP(w, codeExchange(t, url.Values{"grant_type": {"authorization_code"}, "client_id": {"test-client"}, "code": {code}, "code_verifier": {verifier}, "redirect_uri": {seededRedirectURI}})) + if w.Code != http.StatusOK { + t.Fatalf("status %d: %s", w.Code, w.Body.String()) + } +} + +func TestConfidentialCodeClientRequiresItsSecret(t *testing.T) { + makeHandler := func(t *testing.T) (string, http.Handler) { + sessions := oidc.NewSessionStore() + h, _ := newTokenHandler(t, sessions, &mockUserRepo{users: map[string]*domain.User{"alice": aliceUser()}}) + h.ClientConfig["test-client"].ClientType = "confidential" + h.ClientConfig["test-client"].ClientSecret = "human-client-secret" + return seededSession(sessions, "test-verifier"), h + } + + base := func(code string) url.Values { + return url.Values{"grant_type": {"authorization_code"}, "client_id": {"test-client"}, "code": {code}, "code_verifier": {"test-verifier"}, "redirect_uri": {seededRedirectURI}} + } + + t.Run("no credentials", func(t *testing.T) { + code, h := makeHandler(t) + w := httptest.NewRecorder() + h.ServeHTTP(w, codeExchange(t, base(code))) + if w.Code != http.StatusUnauthorized { + t.Fatalf("status %d, want 401", w.Code) + } + }) + + t.Run("wrong secret", func(t *testing.T) { + code, h := makeHandler(t) + req := codeExchange(t, base(code)) + req.SetBasicAuth("test-client", "not-the-secret") + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + if w.Code != http.StatusUnauthorized { + t.Fatalf("status %d, want 401", w.Code) + } + }) + + t.Run("other client identity", func(t *testing.T) { + code, h := makeHandler(t) + req := codeExchange(t, base(code)) + req.SetBasicAuth("someone-else", "human-client-secret") + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + if w.Code != http.StatusUnauthorized { + t.Fatalf("status %d, want 401", w.Code) + } + }) + + t.Run("correct secret", func(t *testing.T) { + code, h := makeHandler(t) + req := codeExchange(t, base(code)) + req.SetBasicAuth("test-client", "human-client-secret") + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + if w.Code != http.StatusOK { + t.Fatalf("status %d: %s", w.Code, w.Body.String()) + } + }) +} + +// A public client must not become "authenticated" by presenting a secret, and a +// confidential registration with no configured secret must not accept an empty +// one. +func TestPublicClientSecretIsIgnoredAndEmptyConfidentialSecretIsRejected(t *testing.T) { + sessions := oidc.NewSessionStore() + h, _ := newTokenHandler(t, sessions, &mockUserRepo{users: map[string]*domain.User{"alice": aliceUser()}}) + code := seededSession(sessions, "test-verifier") + req := codeExchange(t, url.Values{"grant_type": {"authorization_code"}, "client_id": {"test-client"}, "code": {code}, "code_verifier": {"test-verifier"}, "redirect_uri": {seededRedirectURI}}) + req.SetBasicAuth("test-client", "irrelevant") + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + if w.Code != http.StatusOK { + t.Fatalf("public client exchange: status %d", w.Code) + } + + sessions2 := oidc.NewSessionStore() + h2, _ := newTokenHandler(t, sessions2, &mockUserRepo{users: map[string]*domain.User{"alice": aliceUser()}}) + h2.ClientConfig["test-client"].ClientType = "confidential" + code2 := seededSession(sessions2, "test-verifier") + req2 := codeExchange(t, url.Values{"grant_type": {"authorization_code"}, "client_id": {"test-client"}, "code": {code2}, "code_verifier": {"test-verifier"}, "redirect_uri": {seededRedirectURI}}) + req2.SetBasicAuth("test-client", "") + w2 := httptest.NewRecorder() + h2.ServeHTTP(w2, req2) + if w2.Code != http.StatusUnauthorized { + t.Fatalf("empty configured secret accepted: status %d", w2.Code) + } +} + +// Concurrent exchanges of one code must yield exactly one token: the store +// consumes the session atomically rather than reading and deleting separately. +func TestConcurrentCodeExchangeSucceedsExactlyOnce(t *testing.T) { + sessions := oidc.NewSessionStore() + h, _ := newTokenHandler(t, sessions, &mockUserRepo{users: map[string]*domain.User{"alice": aliceUser()}}) + verifier := "test-verifier" + code := seededSession(sessions, verifier) + + const attempts = 16 + var wg sync.WaitGroup + codes := make([]int, attempts) + start := make(chan struct{}) + for i := 0; i < attempts; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + req := codeExchange(t, url.Values{"grant_type": {"authorization_code"}, "client_id": {"test-client"}, "code": {code}, "code_verifier": {verifier}, "redirect_uri": {seededRedirectURI}}) + <-start + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + codes[i] = w.Code + }(i) + } + close(start) + wg.Wait() + + succeeded := 0 + for _, c := range codes { + if c == http.StatusOK { + succeeded++ + } + } + if succeeded != 1 { + t.Fatalf("%d concurrent exchanges succeeded, want exactly 1", succeeded) + } +} + +// A failed exchange must not leave the code replayable. +func TestFailedExchangeConsumesTheCode(t *testing.T) { + sessions := oidc.NewSessionStore() + h, _ := newTokenHandler(t, sessions, &mockUserRepo{users: map[string]*domain.User{"alice": aliceUser()}}) + code := seededSession(sessions, "test-verifier") + params := url.Values{"grant_type": {"authorization_code"}, "client_id": {"test-client"}, "code": {code}, "code_verifier": {"wrong-verifier"}, "redirect_uri": {seededRedirectURI}} + w := httptest.NewRecorder() + h.ServeHTTP(w, codeExchange(t, params)) + if w.Code != http.StatusBadRequest { + t.Fatalf("first attempt: status %d", w.Code) + } + params.Set("code_verifier", "test-verifier") + w = httptest.NewRecorder() + h.ServeHTTP(w, codeExchange(t, params)) + if w.Code != http.StatusBadRequest || !strings.Contains(w.Body.String(), "authorization code") { + t.Fatalf("code survived a failed exchange: %d %s", w.Code, w.Body.String()) + } +} + +// -------------------------------------------------------------------------- +// KEY-WP-0016-T03 — UserInfo token verification bindings. +// -------------------------------------------------------------------------- + +func TestUserinfoRejectsUnverifiedTokenShapes(t *testing.T) { + users := &mockUserRepo{users: map[string]*domain.User{ + "user-alice": aliceUser(), + "alice": aliceUser(), + }} + + valid := func() map[string]interface{} { + now := time.Now() + return map[string]interface{}{ + "iss": "https://auth.netkingdom.local", + "sub": "alice", + "aud": "test-client", + "exp": now.Add(10 * time.Minute).Unix(), + "iat": now.Unix(), + "scope": "openid profile", + } + } + + t.Run("accepts a well-formed access token", func(t *testing.T) { + h, key := newUserinfoHandler(t, users) + w := httptest.NewRecorder() + h.ServeHTTP(w, userinfoRequest(buildToken(t, valid(), key))) + if w.Code != http.StatusOK { + t.Fatalf("status %d: %s", w.Code, w.Body.String()) + } + }) + + t.Run("foreign issuer", func(t *testing.T) { + h, key := newUserinfoHandler(t, users) + claims := valid() + claims["iss"] = "https://auth.other.example" + w := httptest.NewRecorder() + h.ServeHTTP(w, userinfoRequest(buildToken(t, claims, key))) + if w.Code != http.StatusUnauthorized { + t.Fatalf("status %d, want 401", w.Code) + } + }) + + t.Run("missing issuer", func(t *testing.T) { + h, key := newUserinfoHandler(t, users) + claims := valid() + delete(claims, "iss") + w := httptest.NewRecorder() + h.ServeHTTP(w, userinfoRequest(buildToken(t, claims, key))) + if w.Code != http.StatusUnauthorized { + t.Fatalf("status %d, want 401", w.Code) + } + }) + + // An ID token is correctly signed by this issuer but is not an access + // token: it carries no scope claim. + t.Run("id token presented as access token", func(t *testing.T) { + h, key := newUserinfoHandler(t, users) + claims := valid() + delete(claims, "scope") + claims["nonce"] = "nonce1" + w := httptest.NewRecorder() + h.ServeHTTP(w, userinfoRequest(buildToken(t, claims, key))) + if w.Code != http.StatusUnauthorized { + t.Fatalf("status %d, want 401", w.Code) + } + }) +} + +// The JOSE header algorithm is checked before the signature, so a token +// claiming "none" or a symmetric algorithm can never reach the RSA check. +func TestUserinfoRejectsNonRS256Algorithms(t *testing.T) { + users := &mockUserRepo{users: map[string]*domain.User{ + "user-alice": aliceUser(), + "alice": aliceUser(), + }} + h, key := newUserinfoHandler(t, users) + now := time.Now() + payload, err := json.Marshal(map[string]interface{}{ + "iss": "https://auth.netkingdom.local", + "sub": "alice", + "exp": now.Add(10 * time.Minute).Unix(), + "scope": "openid", + }) + if err != nil { + t.Fatal(err) + } + encodedPayload := base64.RawURLEncoding.EncodeToString(payload) + + // Keep a genuine RS256 signature and only restate the algorithm, so the + // test isolates the header check rather than relying on a broken signature. + signed := buildToken(t, map[string]interface{}{ + "iss": "https://auth.netkingdom.local", + "sub": "alice", + "exp": now.Add(10 * time.Minute).Unix(), + "scope": "openid", + }, key) + genuineSignature := strings.Split(signed, ".")[2] + + for _, alg := range []string{"none", "HS256", "RS512", "rs256", ""} { + header, err := json.Marshal(map[string]interface{}{"alg": alg, "typ": "JWT", "kid": "key-1"}) + if err != nil { + t.Fatal(err) + } + token := base64.RawURLEncoding.EncodeToString(header) + "." + encodedPayload + "." + genuineSignature + w := httptest.NewRecorder() + h.ServeHTTP(w, userinfoRequest(token)) + if w.Code != http.StatusUnauthorized { + t.Fatalf("alg %q accepted: status %d", alg, w.Code) + } + } +} diff --git a/src/internal/server/oidc/session.go b/src/internal/server/oidc/session.go index 6bd2474..6fc9eb8 100644 --- a/src/internal/server/oidc/session.go +++ b/src/internal/server/oidc/session.go @@ -69,6 +69,28 @@ func (s *SessionStore) Get(code string) (*PKCESession, bool) { return sess, true } +// Consume retrieves a session by code and removes it in the same critical +// section, so exactly one caller can ever observe a given code. Returns false +// if the code is not present or has expired. The token endpoint must use this +// rather than Get/Delete: signing happens between those two calls, which is +// long enough for two concurrent exchanges to both observe the same session +// (KEY-WP-0016-T01). A consumed code is gone even if the exchange then fails, +// which is the intended single-use semantics -- a failed attempt must not leave +// a replayable code. +func (s *SessionStore) Consume(code string) (*PKCESession, bool) { + s.mu.Lock() + sess, ok := s.sessions[code] + if ok { + delete(s.sessions, code) + } + s.mu.Unlock() + + if !ok || time.Now().After(sess.ExpiresAt) { + return nil, false + } + return sess, true +} + // Delete removes a session by code. No-op if the code is not present. func (s *SessionStore) Delete(code string) { s.mu.Lock() diff --git a/src/internal/server/oidc/token.go b/src/internal/server/oidc/token.go index f64ed5e..0f9fcba 100644 --- a/src/internal/server/oidc/token.go +++ b/src/internal/server/oidc/token.go @@ -68,15 +68,44 @@ func (h *TokenHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - // 2. Validate client exists (basic check; secret auth delegated to future work). - if _, ok := h.ClientConfig[clientID]; !ok { + // 2. Validate client exists and may use this grant. + client, ok := h.ClientConfig[clientID] + if !ok { profileerrors.InvalidProfileUsage("unknown client_id", "client_id"). Write(w, http.StatusBadRequest) return } - // 3. Look up PKCE session. - sess, ok := h.Sessions.Get(code) + // Grant-type eligibility, enforced equivalently to the service path + // (KEY-WP-0016-T02). An empty grantTypes is an implicit authorization-code + // client, matching config validation; a client_credentials-only client must + // not reach the browser path. + if len(client.GrantTypes) > 0 && !containsString(client.GrantTypes, "authorization_code") { + profileerrors.InvalidProfileUsage( + "client is not registered for grant_type=authorization_code", + "grant_type", + ).Write(w, http.StatusBadRequest) + return + } + + // Confidential authorization-code clients authenticate with their secret, + // using the same credential sources as the service grant. A public client + // must not be able to present a secret and be treated as authenticated. + if client.ClientType == "confidential" { + presentedID, secret, ok := basicClientCredentials(r) + if !ok || presentedID != clientID || client.ClientSecret == "" || + !secretsEqual(secret, client.ClientSecret) { + profileerrors.InvalidProfileUsage( + "client authentication failed", + "Authorization", + ).Write(w, http.StatusUnauthorized) + return + } + } + + // 3. Consume the PKCE session. Single-use and atomic: see + // SessionStore.Consume (KEY-WP-0016-T01). + sess, ok := h.Sessions.Consume(code) if !ok { profileerrors.InvalidProfileUsage( "authorization code not found or expired", @@ -94,6 +123,18 @@ func (h *TokenHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } + // Bind the exchange to the redirect URI the code was issued for + // (RFC 6749 section 4.1.3, KEY-WP-0016-T02). /authorize always records an + // exactly-matched registered redirect, so the parameter is always required + // here and must be identical. + if redirectURI := r.FormValue("redirect_uri"); redirectURI != sess.RedirectURI { + profileerrors.InvalidProfileUsage( + "redirect_uri does not match the authorization request", + "redirect_uri", + ).Write(w, http.StatusBadRequest) + return + } + // Recheck grants in case the client registration changed after authorization. for _, scope := range sess.Scopes { if !containsString(h.ClientConfig[clientID].AllowedScopes, scope) { @@ -118,7 +159,6 @@ func (h *TokenHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } if isSuspended(user) { - h.Sessions.Delete(code) profileerrors.RejectedForSafety( "account is suspended", "account_lifecycle", @@ -190,10 +230,8 @@ func (h *TokenHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - // 8. Delete used PKCE session (prevent replay). - h.Sessions.Delete(code) - - // 9. Build response. + // 8. Build response. The session was already consumed at lookup, so no + // separate replay-prevention delete is needed here. resp := tokenResponse{ AccessToken: accessToken, TokenType: "Bearer", @@ -217,6 +255,31 @@ func (h *TokenHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { _ = json.NewEncoder(w).Encode(resp) } +// basicClientCredentials reads client_secret_basic credentials, applying the +// form-encoding decode RFC 6749 appendix B requires of both halves. Shared by +// the service grant and confidential authorization-code client authentication. +func basicClientCredentials(r *http.Request) (clientID, clientSecret string, ok bool) { + clientID, clientSecret, ok = r.BasicAuth() + if !ok { + return "", "", false + } + decodedID, idErr := url.QueryUnescape(clientID) + decodedSecret, secretErr := url.QueryUnescape(clientSecret) + if idErr != nil || secretErr != nil { + return "", "", false + } + return decodedID, decodedSecret, true +} + +// secretsEqual compares two secrets in constant time. Digesting first keeps the +// comparison length-independent, so a wrong-length secret is indistinguishable +// from a wrong-value one. +func secretsEqual(presented, expected string) bool { + presentedDigest := sha256.Sum256([]byte(presented)) + expectedDigest := sha256.Sum256([]byte(expected)) + return subtle.ConstantTimeCompare(presentedDigest[:], expectedDigest[:]) == 1 +} + func (h *TokenHandler) serveClientCredentials(w http.ResponseWriter, r *http.Request) { ctx := r.Context() clientID, clientSecret, ok := r.BasicAuth() diff --git a/src/internal/server/oidc/token_test.go b/src/internal/server/oidc/token_test.go index 751956c..42314ec 100644 --- a/src/internal/server/oidc/token_test.go +++ b/src/internal/server/oidc/token_test.go @@ -93,13 +93,32 @@ func newTokenHandler(t *testing.T, sessions *oidc.SessionStore, users domain.Use return h, key } +// seededRedirectURI is the redirect stored by seededSession, and the value the +// token endpoint now requires the exchange to repeat (KEY-WP-0016-T02). +const seededRedirectURI = "https://app.example.com/callback" + func tokenRequest(params url.Values) *http.Request { + // Supply the matching redirect_uri for code exchanges unless the test is + // deliberately exercising a mismatch, so each case still fails for the + // reason it is testing. + if params.Get("grant_type") == "authorization_code" && !params.Has("redirect_uri") { + params = cloneValues(params) + params.Set("redirect_uri", seededRedirectURI) + } req := httptest.NewRequest(http.MethodPost, "/token", strings.NewReader(params.Encode())) req.Header.Set("Content-Type", "application/x-www-form-urlencoded") return req } +func cloneValues(params url.Values) url.Values { + out := make(url.Values, len(params)+1) + for key, values := range params { + out[key] = append([]string(nil), values...) + } + return out +} + func seededSession(sessions *oidc.SessionStore, verifier string) (code string) { challenge := s256Challenge(verifier) sess := &oidc.PKCESession{ diff --git a/src/internal/server/oidc/userinfo.go b/src/internal/server/oidc/userinfo.go index 54d4bb5..c3d2014 100644 --- a/src/internal/server/oidc/userinfo.go +++ b/src/internal/server/oidc/userinfo.go @@ -39,8 +39,9 @@ func (h *UserinfoHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - // 2. Validate token (signature + expiry) and extract claims. - claims, err := validateJWT(tokenStr, h.SigningKey) + // 2. Validate token (algorithm, signature, expiry, issuer, purpose) and + // extract claims. + claims, err := validateAccessToken(tokenStr, h.SigningKey, h.Issuer) if err != nil { http.Error(w, `{"error":"invalid_token","description":"token validation failed"}`, http.StatusUnauthorized) return @@ -135,21 +136,45 @@ func lookupUserBySubject( // JWT validation (stdlib only — no external JWT library) // --------------------------------------------------------------------------- -// validateJWT parses and validates a JWT signed with RS256. -// It checks the signature using pubKey and verifies the exp claim. -// Returns the parsed claims on success. -func validateJWT(tokenStr string, pubKey *rsa.PublicKey) (map[string]interface{}, error) { +// validateAccessToken parses and validates a KeyCape-issued access token. +// +// Beyond signature and expiry it enforces the bindings the caller CLI already +// requires (KEY-WP-0016-T03): the JOSE header algorithm must be exactly RS256, +// so an "alg":"none" or HMAC-shaped token can never bypass the RSA check; the +// issuer claim must equal this issuer, so a correctly-signed token from another +// deployment is refused; and the token must be an access token. +// +// Purpose is decided on the presence of the `scope` claim, which the token +// endpoint sets on access tokens and never on ID tokens. That keeps the check +// verification-side: it does not add a claim to the issued token contract, which +// consumers pin exactly. +func validateAccessToken(tokenStr string, pubKey *rsa.PublicKey, issuer string) (map[string]interface{}, error) { parts := strings.Split(tokenStr, ".") if len(parts) != 3 { return nil, errors.New("malformed JWT: expected 3 parts") } + // Verify the JOSE header algorithm before trusting the signature check. + headerJSON, err := base64.RawURLEncoding.DecodeString(parts[0]) + if err != nil { + return nil, errors.New("malformed JWT: invalid header encoding") + } + var header struct { + Alg string `json:"alg"` + } + if err := json.Unmarshal(headerJSON, &header); err != nil { + return nil, errors.New("malformed JWT: header is not valid JSON") + } + if header.Alg != "RS256" { + return nil, errors.New("unsupported JWT algorithm") + } + // Verify signature over header.payload. signingInput := parts[0] + "." + parts[1] digest := sha256.Sum256([]byte(signingInput)) - sigBytes, err := base64.RawURLEncoding.DecodeString(parts[2]) - if err != nil { + sigBytes, decodeErr := base64.RawURLEncoding.DecodeString(parts[2]) + if decodeErr != nil { return nil, errors.New("malformed JWT: invalid signature encoding") } @@ -158,8 +183,8 @@ func validateJWT(tokenStr string, pubKey *rsa.PublicKey) (map[string]interface{} } // Decode payload. - payloadJSON, err := base64.RawURLEncoding.DecodeString(parts[1]) - if err != nil { + payloadJSON, payloadErr := base64.RawURLEncoding.DecodeString(parts[1]) + if payloadErr != nil { return nil, errors.New("malformed JWT: invalid payload encoding") } @@ -177,6 +202,20 @@ func validateJWT(tokenStr string, pubKey *rsa.PublicKey) (map[string]interface{} return nil, errors.New("JWT has expired") } + // Require this issuer. An empty configured issuer would make the check + // vacuous, so treat that as a misconfiguration rather than skipping it. + if issuer == "" { + return nil, errors.New("issuer is not configured") + } + if tokenIssuer, _ := claims["iss"].(string); tokenIssuer != issuer { + return nil, errors.New("JWT issuer mismatch") + } + + // Require an access token. ID tokens carry no scope claim. + if _, ok := claims["scope"]; !ok { + return nil, errors.New("JWT is not an access token") + } + return claims, nil } diff --git a/src/internal/server/oidc/userinfo_test.go b/src/internal/server/oidc/userinfo_test.go index ca372af..5a12f5e 100644 --- a/src/internal/server/oidc/userinfo_test.go +++ b/src/internal/server/oidc/userinfo_test.go @@ -304,10 +304,11 @@ func TestUserinfoHandler_EmitsTelemetry(t *testing.T) { now := time.Now() claims := map[string]interface{}{ - "iss": "https://auth.netkingdom.local", - "sub": "alice", - "exp": now.Add(10 * time.Minute).Unix(), - "iat": now.Unix(), + "iss": "https://auth.netkingdom.local", + "sub": "alice", + "exp": now.Add(10 * time.Minute).Unix(), + "iat": now.Unix(), + "scope": "openid", } token, _ := oidc.BuildJWT(claims, "key-1", key) diff --git a/src/tests/profile/profile_test.go b/src/tests/profile/profile_test.go index d3cb519..8b802f7 100644 --- a/src/tests/profile/profile_test.go +++ b/src/tests/profile/profile_test.go @@ -574,6 +574,8 @@ func TestCompleteTokenFlow(t *testing.T) { tokenForm.Set("client_id", "demo-app") tokenForm.Set("code", authCode) tokenForm.Set("code_verifier", verifier) + // The exchange must repeat the redirect URI the code was issued for. + tokenForm.Set("redirect_uri", "http://localhost:3000/callback") tokenResp, err := http.Post( ts.Server.URL+"/token", diff --git a/workplans/KEY-WP-0013-approval-engine-resource-audience.md b/workplans/KEY-WP-0013-approval-engine-resource-audience.md index b5ad245..9a70ee7 100644 --- a/workplans/KEY-WP-0013-approval-engine-resource-audience.md +++ b/workplans/KEY-WP-0013-approval-engine-resource-audience.md @@ -89,6 +89,7 @@ rather than an empty claim. Local issuance proof only, not live-rollout evidence id: KEY-WP-0013-T04 status: done priority: high +state_hub_task_id: "d93e21cc-a422-5e78-981f-9c00f3161375" ``` Source: glas-harness inbox message f487c63a-dd7e-4ad8-9188-eec9fbea59c6, diff --git a/workplans/KEY-WP-0016-authorization-code-protocol-hardening.md b/workplans/KEY-WP-0016-authorization-code-protocol-hardening.md new file mode 100644 index 0000000..afc788e --- /dev/null +++ b/workplans/KEY-WP-0016-authorization-code-protocol-hardening.md @@ -0,0 +1,118 @@ +--- +id: KEY-WP-0016 +type: workplan +title: "Authorization-code protocol hardening and token verification bindings" +domain: infotech +repo: key-cape +status: finished +owner: claude +topic_slug: authorization-code-protocol-hardening +created: "2026-09-06" +updated: "2026-09-06" +--- + +Closes gap G01 of `history/2026-09-05-011726-scope-intent-assessment.md`, the +assessment's first recommended item. The browser grant validates PKCE, client id +and scopes but does not bind the redirect URI, authenticate confidential clients, +enforce grant-type eligibility, or consume the authorization code atomically. +UserInfo verification is weaker than the caller CLI's contract. + +Scope is the local protocol surface only. Upstream provider-token verification +(the Authelia adapter's deliberate no-signature-check trust assumption) is a +separate trust-contract decision and is not addressed here. + +## Consume authorization codes atomically + +```task +id: KEY-WP-0016-T01 +status: done +priority: high +``` + +`SessionStore.Get` reads a code and `Delete` removes it in separate lock +acquisitions, with signing in between, so two simultaneous exchanges can both +observe the same session before either deletes it. Replace the token path's +lookup with a single-operation consume that removes the session under the same +lock that reads it, so exactly one concurrent exchange can succeed. A failed +exchange must not leave a replayable code. Cover with a concurrent-reuse test +that asserts exactly one success across parallel requests. + +Added `SessionStore.Consume`, which reads and deletes under one lock, and moved +the token path onto it. Verified the defect was real before claiming the fix: +with `Get`/`Delete` restored, 9 of 16 concurrent exchanges succeeded and a +PKCE-failed exchange left the code replayable. Both now fail closed — +`TestConcurrentCodeExchangeSucceedsExactlyOnce`, `TestFailedExchangeConsumesTheCode`. + +## Bind redirect URI, grant type and confidential client authentication + +```task +id: KEY-WP-0016-T02 +status: done +priority: high +``` + +On the authorization-code path: require `redirect_uri` and compare it for exact +equality with the value stored at authorization time; reject a client whose +registration does not permit `authorization_code` (an empty `grantTypes` stays an +implicit authorization-code client, matching config validation, but a +`client_credentials`-only client must be refused); and authenticate confidential +clients with a constant-time secret comparison, accepting the same +`client_secret_basic` and form-encoded credentials as the service grant. Public +clients continue to present no secret and must not be able to supply one to +impersonate a confidential registration. Negative tests per condition. + +Implemented all three in `token.go`, with `basicClientCredentials` and +`secretsEqual` shared with the service grant (digest-first comparison, so a +wrong-length secret is indistinguishable from a wrong value). Tests: +`TestCodeExchangeRejectsRedirectURIMismatchAndOmission` (omitted, different, +traversal-shaped and trailing-slash variants), +`TestCodeExchangeRejectsServiceOnlyClient`, +`TestCodeExchangeAllowsImplicitAuthorizationCodeClient`, +`TestConfidentialCodeClientRequiresItsSecret` and +`TestPublicClientSecretIsIgnoredAndEmptyConfidentialSecretIsRejected`. The +`keycape login` CLI already sent `redirect_uri`, so no caller change was needed; +the authclient test stub was recording no redirect and is now faithful to real +`/authorize`. + +## Harden UserInfo token verification + +```task +id: KEY-WP-0016-T03 +status: done +priority: high +``` + +UserInfo verifies an RSA signature and expiry but ignores its own `Issuer` field +and does not check the JOSE header algorithm, the issuer claim, or the token +purpose, so an ID token or a foreign-issuer token of the right shape is accepted +where an access token is required. Enforce `alg=RS256` from the header, reject +`none` and any other algorithm, require the configured issuer, and reject tokens +that are not access tokens. Negative tests per condition. + +`validateJWT` became `validateAccessToken`, checking the JOSE header before +trusting the signature, requiring the configured issuer (an empty configured +issuer is a misconfiguration, not a skipped check), and requiring the `scope` +claim the token endpoint sets only on access tokens. Purpose is decided +verification-side deliberately: adding a `token_use` claim would change the +issued-token contract that consumers pin exactly. Tests: +`TestUserinfoRejectsUnverifiedTokenShapes` (foreign issuer, missing issuer, ID +token presented as an access token) and `TestUserinfoRejectsNonRS256Algorithms`, +which restates the header algorithm over a genuine RS256 signature so it +isolates the header check. + +## Reconcile scope and assessment records + +```task +id: KEY-WP-0016-T04 +status: done +priority: medium +``` + +Update `SCOPE.md`'s protocol-hardening limit and G01's closure status in the +assessment to state exactly which bindings are now enforced and which remain +open, naming the upstream-trust item as still outstanding. Do not claim complete +profile conformance: G01 closure covers these bindings, not the whole profile. + +Updated `SCOPE.md`'s protocol-hardening limit and recorded a partial-closure +status under G01 in the assessment, naming the Authelia upstream-trust item as +still open. G01 is explicitly not fully closed.