feat(oidc): OIDC authorization-code login + group→role mapping (bookshelf-4op.6) #605
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-4op.6"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Test plan
Closes bead bookshelf-4op.6 on merge.
CODE REVIEW — bookshelf-4op.6 (#605)
Phase 0: DEMO Verification
No DEMO block was provided in the bead comments (bead is a service-side auth feature with no CLI output to capture). This is a valid case for a browser-only flow — proceeding to Phases 1 and 2.
Phase 1 + 2: Findings
[MAJOR] internal/users/oidc_service.go:340 —
syncOIDCGroupMappingssilently no-ops whenuserGroupsis emptyWhen the ID-token's groups claim is absent or an empty array,
groupsFromClaimsreturns nil andsyncOIDCGroupMappingsreturns nil immediately without touchinguser_permissionsoruser_library_mapping. A user who was previously an admin and is later removed from every group will still log in with their old elevated permissions — the sync is add-only when zero groups are present. The bead spec requires "A user dropped from an admin group LOSES admin on next login." This is violated.Similarly, at line 201–204 the group sync failure is non-fatal: a transient DB error during
upsertPermissionslets the login proceed with stale (potentially elevated) permissions. For a permissions-resetting path, this should be a fatal error that aborts login rather than a silent continue.Suggested fix: when
anyMatched == false(oruserGroupsis empty), perform a zero-permissions upsert (all bools false, empty library set) to explicitly revoke any previously-granted access. Group sync failures should return an error soOIDCHandleCallbackpropagates them and blocks login rather than silently continuing.[MAJOR] internal/users/oidc_jwks.go:42–46 — Discovery document
issuerfield is never validated against configured issuerDiscoverOIDCMetafetches the OIDC well-known document, parses it, checks thatauthorization_endpoint/token_endpoint/jwks_uriare non-empty, but never asserts thatmeta.Issuer == configuredIssuer. OIDC Discovery §4.3 and RFC 8414 §3.3 both require this exact comparison. Without it, a DNS hijack or open-redirect on the discovery path could substitute a different provider's metadata (including a different JWKS URI), permitting an attacker-controlled key to sign tokens that pass verification against the configured issuer inverifyIDToken.The
oidcProviderMetastruct already captures theissuerfield — it just is never checked. Addif meta.Issuer != issuer { return oidcProviderMeta{}, fmt.Errorf("oidc discover: issuer mismatch ...") }before returning.[MINOR] internal/users/oidc_service.go:191 — Bare type assertion
claims["sub"].(string)foroidcSessionIDAfter
provisionOIDCUserreturns successfullyclaims["sub"]is guaranteed to be a non-empty string (the function returns an error otherwise), so this cannot panic in practice. However, the assertion is stylistically inconsistent with the surrounding code — thesidextraction immediately above uses the safe two-value formclaims["sid"].(string). PreferclaimString(claims, "sub")or the safe form for consistency and future-proofing.Summary of claims verification
verifyIDToken, method guard line 110issverified against configured issuerverifyIDTokenline 134; MISSING inDiscoverOIDCMeta(see MAJOR above)audverified against client_idexp/iatvalidatedParseWithClaimsGetUserByOIDCSubjectIssuerkeyed on both; no email-match pathoidc_session,oidc_group_mapping, and existing nullableoidc_subject/oidc_issuercolumns onusersLoginResult/setAuthCookiespathoidc_coverage_test.gois real coverage with concrete assertions against JWKS/claims/provision/sync pathsREVIEW VERDICT: 0 blocker, 2 major, 1 minor
Security Review — OIDC Login (bookshelf-4op.6)
Verdict
REVIEW VERDICT: 0 blocker, 1 major, 3 minor
Detailed Findings
[MAJOR] internal/config/config.go (OIDCIssuer validation) — HTTPS scheme not enforced
The config comment reads
// Must be an HTTPS URLbutValidate()only checks thatOIDCIssuer != ""— it does not reject anhttp://issuer. If an admin misconfigures the issuer to use plain HTTP (e.g. an internal IdP during testing that accidentally gets deployed), an on-path attacker can MITM the discovery document fetch and replaceauthorization_endpoint,token_endpoint, andjwks_uriwith their own. Because the JWKS is fetched from the (attacker-controlled)jwks_uriin the discovery doc, the attacker can supply their own public key and sign ID tokens that passverifyIDToken. The entire auth chain is then bypassed: attacker issues a token for anysub, which provisions a new user (or matches an existing one) without touching the real IdP.Fix: add to
Validate()after theOIDCIssuer != ""check:[MINOR] internal/users/oidc_jwks.go (verifyIDToken) — discovery document issuer field not cross-validated
RFC 8414 §3.3 requires that
metadata.issuerin the discovery document MUST exactly match the Issuer URL used to fetch it. The code trustsmeta.AuthorizationEndpoint/meta.TokenEndpoint/meta.JWKSURIfrom the discovery doc but never checksmeta.Issuer == cfg.Issuer. This is low risk because the id_tokenissclaim IS validated againstcfg.Issuer, so a rogue discovery doc cannot escalate to a successful token verification on its own — but the spec compliance gap is worth closing.Fix: in
OIDCHandleCallback(orDiscoverOIDCMeta), after parsing the document, assertmeta.Issuer == cfg.Issuerand return an error if not.[MINOR] internal/users/oidc_jwks.go — EC (P-256/P-384) keys silently skipped
The JWKS parser skips any key where
kty != "RSA", and the keyfunc rejects any signing method other than*jwt.SigningMethodRSA. Providers that issue ES256 tokens by default (Dex, some Keycloak configs) will silently fail at theno matching JWK for kidstep, producing an opaque login failure. This is a functionality gap, not a security one (silently skipping EC keys means EC-signed tokens fail closed — no bypass), but it limits compatibility.Fix: add EC key (
kty == "EC") parsing alongside RSA and accept*jwt.SigningMethodECDSAin the keyfunc.[MINOR] internal/users/oidc_service.go:191 — unchecked type assertion on
claims["sub"]claims["sub"].(string)is an unchecked assertion. It is safe-by-ordering:provisionOIDCUserruns first and validates thatsubis a non-empty string (returning an error otherwise), so control never reaches line 191 with a non-stringsub. But the code reads as a panic hazard. Replace with the safe pattern already used elsewhere:Clean Areas (checklist)
verifyIDTokenusesgolang-jwt/jwt v5with an RSA-only keyfunc that explicitly rejects any non-RSA signing method.alg:nonetokens are rejected. HMAC confusion is rejected. PASS.crypto/rand) → base64url = 32-char unguessable token; bound to browser session viaHttpOnly; Secure; SameSite=Laxcookie; single-use (cookie cleared at callback entry regardless of outcome); compared byte-for-byte before code exchange. PASS.codeis single-use at the provider. PASS."/". Nonext/redirect_uriparameter accepted. PASS.(oidc_subject, oidc_issuer)pair —GetUserByOIDCSubjectIssuer. Email is stored but never used for lookup. No email-based account linking path exists. PASS.merged.IsAdminand permissions are derived entirely from the verified ID tokengroupsclaim matched against the server-sideoidc_group_mappingtable. No client-supplied claim influences role outside the verified token. PASS.ClientSecretis never passed to any logger. TheerrfromoauthCfg.Exchangeis logged atWarnbut does not contain the secret (oauth2 library wraps the provider error response, not the request). Rawid_token_hintis stored inoidc_sessionfor logout hint use — acceptable.refresh_tokenis stored ashashToken(refreshToken)(SHA-256). PASS.?placeholders andExecContext/QueryRowContext. PASS.http.Clientwith noInsecureSkipVerify— standard TLS validation applies.io.LimitReadercaps discovery at 64 KiB and JWKS at 256 KiB. PASS.Security Re-Review — PR #605 OIDC (bd-bookshelf-4op.6)
Re-review of the fix commit (
62eee5e6). Verifying all 4 prior items and re-checking the previously-clean protections.Prior findings — status
1. [MAJOR — https issuer] — PARTIALLY FIXED, NEW BYPASS INTRODUCED
internal/config/config.go:443— The https enforcement is present and fires inValidate():However, the localhost exception uses a bare prefix match
"http://localhost"which is too broad. The following inputs all bypass the HTTPS check:http://localhost.evil.com— matchesHasPrefix("http://localhost")http://localhostevil.com— matchesHasPrefix("http://localhost")http://localhost:8080@evil.com— matchesHasPrefix("http://localhost")The intended localhost carve-out should match
http://localhostfollowed only by:,/, or end-of-string. The fix is to tighten the check:No test covers
http://localhost.evil.com— the bypass is untested as well as unblocked.2. [MINOR — issuer cross-validation] — FIXED
internal/users/oidc_jwks.go:57—DiscoverOIDCMetanow validatesmeta.Issuer != strings.TrimRight(issuer, "/")and returns an error on mismatch. The check is present and correct.3. [MINOR — EC/ES256 keys] — FIXED
internal/users/oidc_jwks.go:219-261—jwkToECnow supports P-256 (ES256), P-384 (ES384), and P-521 (ES512). Point-on-curve validation is done viacrypto/ecdh.NewPublicKey()using the uncompressed-point format, which is the correct approach. The signing method allowlist at line 143 (*jwt.SigningMethodRSA, *jwt.SigningMethodECDSA) admits all three.4. [MINOR —
subsafe helper] — FIXEDinternal/users/oidc_service.go:262—subis extracted viaclaimString(claims, "sub")which does a safe type-assertionclaims[key].(string)— never amap[string]anylookup that could accept non-string types.sub == ""is also explicitly checked and rejected (line 270-272).Previously-clean protections — re-confirmed intact
verifyIDTokenkeyfunc at line 143 accepts only*jwt.SigningMethodRSAand*jwt.SigningMethodECDSA; any other method (includingnoneand HS256/HMAC) is rejected.parseAccessTokenintoken.go:68independently checks*jwt.SigningMethodHMACfor the local-JWT path. No cross-contamination possible.crypto/randbase64url (generateOIDCState). It is bound to a short-lived (10 min),HttpOnly,SameSite=Laxcookie.OIDCHandleCallbackrejects onstate == "" || state != cookieState. Single-use (cookie is cleared on every callback, success or failure).oidcCallbackHandlerline 116 hardcodes"/"as the redirect target post-login. No attacker-controllednextorredirect_toparameter.GetUserByOIDCSubjectIssuerquery uses bothoidc_subjectandoidc_issuercolumns;CreateOIDCUserstores both. No email-only or sub-only lookup path.OIDCClientSecretis never passed to the logger.rawIDTokenis stored inoidc_session.id_token_hint(nullable, intended for backchannel logout) but never logged. Refresh token is SHA-256 hashed before DB storage (hashToken).internal/db/queries/oidc.sqluse positional?placeholders. No string interpolation.[MAJOR] internal/config/config.go:443 — localhost prefix bypass allows http://localhost.evil.com and similar as OIDC issuer
strings.HasPrefix(c.OIDCIssuer, "http://localhost")matches any domain beginning with "localhost", not just the loopback address. An operator configuringhttp://localhost.corp-idp.example.comas an issuer would silently bypass the HTTPS enforcement, making the MITM-discovery attack possible. Fix: replace with an explicit equality + allowed-suffix check (== "http://localhost", or has prefix"http://localhost/"/"http://localhost:").REVIEW VERDICT: 0 blocker, 1 major, 0 minor
CODE RE-REVIEW — bookshelf-4op.6 (#605)
Re-review of fixes committed since comment 7131 (2 MAJOR + 1 MINOR).
Finding 1 — [MAJOR] Group revoke: RESOLVED WITH CAVEAT
The prior finding required an explicit zero-permissions upsert when
userGroupsis empty or no group matches, and that group-sync failure be fatal (blocks login).Both are addressed:
syncOIDCGroupMappings(oidc_service.go:397) always callsupsertPermissions(zero-value struct when no match), so stale admin/perms are revoked on every login regardless of whether any groups matched.OIDCHandleCallback(oidc_service.go:201–207) returns the sync error rather than logging and continuing.Residual gap — [MAJOR]
oidc_service.go:403–411—setLibrariesnot called whenanyMatched=truebut no library IDsThe library-revocation conditional has three states but only two branches:
When a user belongs to a group that grants
IsAdminbut has nolibrary_idsassigned,anyMatched=trueandlibIDsis empty. Neither branch executes. Stale library assignments from a previous login (e.g., a library access that was later removed from the mapping row) are NOT cleared. The comment says "Always replace library access" but the code does not always do it. A user removed from a library by removing the library ID from the group mapping will retain access until they are moved to the!anyMatchedpath.Fix: collapse to
if anyMatched { setLibraries(ctx, userID, libIDs) }sosetLibrariesis always called when a group matched, passing an empty slice when the matched groups carry no library IDs.Finding 2 — [MAJOR] Discovery issuer: RESOLVED
DiscoverOIDCMeta(oidc_jwks.go:55–58) now checksmeta.Issuer != strings.TrimRight(issuer, "/")and returns an error on mismatch. TestDiscoverOIDCMeta issuer mismatchinoidc_jwks_test.goassertsMatchError(ContainSubstring("issuer mismatch")). Resolved.Finding 3 — [MINOR] Bare
claims["sub"].(string): RESOLVEDNo bare type assertion on
claims["sub"]anywhere in the file. The session insertion usesclaimString(claims, "sub")(oidc_service.go,InsertOIDCSessionParams). Resolved.Test coverage of requested scenarios
syncOIDCGroupMappings authoritative revoke on group removal@oidc_coverage_test.go:2233syncOIDCGroupMappingsuserGroups is empty@oidc_service_test.go+oidc_coverage_test.go:2205OIDCHandleCallback fatal syncOIDCGroupMappings error@oidc_coverage_test.go:1346DiscoverOIDCMeta issuer mismatch@oidc_jwks_test.gooidc_coverage_test.goNote: no test covers the
anyMatched=true && libIDs emptygap described above — the existing revocation tests usenil/[]string{"unknown_group"}(triggering the!anyMatchedpath), not a group that matches but has no library IDs.Verdict
[MAJOR] internal/users/oidc_service.go:403–411 —
setLibrariesnot called when a group matches but has no library IDs — stale library access not revoked in this case.REVIEW VERDICT: 0 blocker, 1 major, 0 minor
Security Re-Review — bookshelf-4op.6 (commit
5e92ec91)Scope: Full adversarial security pass on the OIDC authorization-code flow, with specific verification of the prior open major.
Prior major: config.go OIDC issuer HTTPS enforcement bypass — RESOLVED ✓
Commit
b70166e6replaces the string-prefix check withisLoopbackHTTP(u string) boolwhich usesurl.Parse+Hostname(). All four attack vectors from the prior finding are now correctly rejected and regression-tested:http://localhost.evil.com→ rejected (host ≠ loopback)http://localhostevil.com→ rejectedhttp://localhost:8080@evil.com→ rejected (url.Parse sees host=evil.com, userinfo=localhost:8080)http://localhost.attacker.com→ rejectedSix valid loopback cases (bare localhost, with port, with path, 127.0.0.1, ::1) are accepted. The prior open major is fully resolved.
Full adversarial pass results
Signature verification (alg:none / alg-confusion)
internal/users/oidc_jwks.go:141–148—jwt.ParseWithClaimskeyfunc gate rejects any method that is not*jwt.SigningMethodRSAor*jwt.SigningMethodECDSA. Thealg:none/ HMAC-confusion vector is blocked. ✓Issuer + audience + expiry
issvalidated post-parse againstcfg.Issuer;audvalidated for both string and[]anyforms;exp/nbf/iatvalidated byjwt/v5 MapClaims.Valid()insideParseWithClaims. ✓Discovery-document issuer cross-check
DiscoverOIDCMetaenforcesmeta.Issuer == strings.TrimRight(issuer, "/")per RFC 8414 §3.3. A substituted document is rejected. ✓State parameter CSRF
24-byte crypto-random state, stored in an
HttpOnly; Secure; SameSite=Lax; MaxAge=600; Path=/auth/oidccookie, cleared unconditionally on callback entry before code exchange. CSRF check (state == "" || state != cookieState) runs before the code exchange. ✓Account provisioning identity key
Lookup is
(oidc_subject, oidc_issuer)— not email. Email-based linking (account-takeover vector) is absent. The DB has aUNIQUE KEY idx_users_oidc_issuer_subject (oidc_issuer, oidc_subject). ✓Refresh-token storage
Pergamum's own refresh token is stored as
hashToken(refreshToken)(hashed, not plaintext). The provider's OAuth2 access/refresh tokens are not persisted. ✓Group→role escalation
syncOIDCGroupMappingsis authoritative-replace on every login (not additive). Zero-permissions upsert is issued even when no groups match, revoking stale admin. Theb70166e6fix also closes thelen(libIDs) > 0guard hole that left stale library memberships when a mapping had onlyIsAdminand nolibrary_ids. ✓Open redirect on callback
All redirects in
oidcCallbackHandlergo to fixed paths (/login?error=oidc,/). No user-suppliednextparameter is followed. ✓PII/token logging
OIDCClientSecretis never logged.rawIDTokenis never logged (only the error fromverifyIDToken). Log lines emituser_id,username,issueronly. ✓EC on-curve validation
jwkToECusescrypto/ecdh.NewPublicKeyto validate the point before constructing the*ecdsa.PublicKey, blocking invalid-curve attacks. ✓Findings
[MINOR] internal/users/oidc_service.go:97 — Nonce not sent or verified in OIDC authorization-code flow
OpenID Core §3.1.2.1 RECOMMENDS sending a
noncein the auth request and verifying it in the returned ID token'snonceclaim. Its absence allows a compromised or misconfigured provider to replay the same ID token across multiple unrelated authorization requests to this relying party. For a confidential server-side client (client secret bound to a specific redirect URI), the practical exploit window is narrow — the attacker would also need to intercept the code-exchange response — but the nonce provides defense-in-depth at zero cost. Suggested fix: generate a second crypto-random value alongsidestate, store it in a second short-lived cookie (or extend the state cookie), pass it toAuthCodeURLviaoauth2.SetAuthURLParam("nonce", nonce), and assertclaims["nonce"] == storedNonceinverifyIDToken.REVIEW VERDICT: 0 blocker, 0 major, 1 minor
CODE RE-REVIEW — PR #605 (bookshelf-4op.6, OIDC login + group→role mapping)
Head:
5e92ec91. CI green. Prior open items verified.Prior open items — BOTH RESOLVED
1. isLoopbackHTTP localhost prefix bypass — FIXED.
internal/config/config.go:466— isLoopbackHTTP now uses url.Parse + Hostname() correctly. Rejects http://localhost.evil.com (hostname is localhost.evil.com, not in allowlist) and http://localhost:8080@evil.com (hostname is evil.com, not in allowlist). Only localhost, 127.0.0.1, and ::1 pass. The call site at line 444 is correct.2. setLibraries not called when group matched but had no library IDs — FIXED.
internal/users/oidc_service.go:408-414— both the anyMatched and !anyMatched branches now call setLibraries, so stale library access is revoked authoritatively on every login regardless of path.Holistic security pass findings
[MINOR] internal/users/oidc_handler.go:95 — errDesc from provider error callback is silently dropped despite comment claiming it is logged
The comment
_ = errDesc // logged belowis stale — errDesc is never logged. This loses the provider-supplied error description on OIDC error redirects, making debugging provider-side rejections harder.[MINOR] internal/users/oidc_jwks.go:195 — no minimum RSA key size check in jwkToRSA
A JWKS document with a sub-2048-bit RSA key is accepted. In practice the operator controls the configured issuer so a weak key would be a provider misconfiguration, not an attacker-controlled bypass. Low practical risk in this threat model.
[MINOR] internal/config/config.go — OIDCIssuer not normalized (trailing slash stripped) before use
Validate() does not strip a trailing / from OIDCIssuer. If the operator configures https://provider.example.com/, verifyIDToken receives expectedIssuer with the trailing slash but the token's iss claim will not have one — the comparison fails and every login is rejected with a confusing error. Not a security issue but a footgun. Suggest strings.TrimRight(c.OIDCIssuer, "/") in Validate() before the HTTPS check.
No BLOCKERs or MAJORs found. All prior blockers/majors are resolved.
REVIEW VERDICT: 0 blocker, 0 major, 3 minor
5e92ec91c11db577b7b6