auth: lift OIDC into reusable pkg/auth provider package#829
Open
alvarofraguas wants to merge 4 commits into
Open
auth: lift OIDC into reusable pkg/auth provider package#829alvarofraguas wants to merge 4 commits into
alvarofraguas wants to merge 4 commits into
Conversation
Scaffolds the protocol-agnostic federated-identity surface for
osctrl-api. No concrete provider yet — that lands on the next branch
(auth/01-oidc-provider).
== types.go ==
Defines:
- Provider interface: Type / LoginURL / HandleCallback. Context-aware
on every method so a slow IdP cannot wedge a handler.
- ResolvedIdentity: protocol-neutral output of HandleCallback. Subject
is the stable identifier; PreferredUsername is the display/match
field; Email is informational (mutable, spoofable — threat T24);
Groups carries IdP group memberships for the RequiredGroups gate.
- State: per-login data the caller must round-trip from LoginURL to
HandleCallback. Contains EnvUUID (locks to one env — threat T18),
Nonce (256-bit cryptorandom), and PKCE Verifier.
- Type constants: TypeOIDC ("oidc") + reserved TypeSAML ("saml").
== state.go ==
State-as-JWT cookie implementation. HS256-signed with the api's
existing JWT secret; audience-scoped to "osctrl-auth-state" so a
state JWT cannot be used as a user-auth JWT and vice versa
(threat T19). Cookie attributes:
- Name: osctrl_auth_state
- Path: /api/v1/auth/ (scoped so the cookie doesn't leak to
other endpoints)
- HttpOnly + Secure + SameSite=Lax
- MaxAge: 10 minutes
Verification enforces in order: cookie present, HS256 pinned
(WithValidMethods), signature valid, iss == osctrl-api, aud ==
osctrl-auth-state, exp/nbf valid, EnvUUID + Nonce non-empty.
Single ErrStateInvalid for all failure modes — external observers
cannot distinguish "tampered" from "expired" from "wrong audience"
(threat T31).
NewNonce returns 256-bit cryptorandom base64url-encoded values.
ClearStateCookie deletes the cookie immediately after a successful
parse so a callback URL cannot be replayed (threat T9).
== state_test.go ==
15 tests covering each threat vector in scope for this commit:
TestNonceUnique — sanity on cryptorandom (T7)
TestStateRoundTripHappy — canonical good path
TestStateMissing — no cookie → ErrStateMissing (T6)
TestStateEmptyValue — defensive: empty cookie value
TestStateSignatureTampered — flipped sig byte → ErrStateInvalid (T31)
TestStateExpired — past-exp token rejected (T9)
TestStateWrongAudience — user-auth aud rejected (T19)
TestStateWrongIssuer — wrong iss rejected (defense in depth)
TestStateWrongSecret — attacker-signed rejected (T27)
TestStateAlgNoneRejected — alg=none rejected (alg-confusion)
TestIssueRejectsEmptyEnv — empty EnvUUID refused (T18 groundwork)
TestIssueRejectsEmptyNonce — empty Nonce refused
TestCookieAttributes — HttpOnly+Secure+SameSite+Path (T11/T12)
TestClearStateCookie — path matches for browser-side delete
TestStateIsZero — IsZero helper
All 15 pass. `go vet ./pkg/auth/...` clean. `go build ./...` clean.
Lifts the OIDC protocol code from cmd/admin/oidc.go into a reusable
pkg/auth/oidc package implementing the auth.Provider interface from
auth/00-shared-pkg. Legacy admin is refactored to consume the shared
package; behavior is preserved (backwards-compat audit in spec).
== pkg/auth/oidc/ ==
config.go — Config struct decoupled from YAML/CLI bindings, with
Validate() and effective*() helpers. New field
LegacyPermissiveUsername for the backwards-compat shim.
claims.go — pickUsername (claim selection), sanitizeUsername
(strict regex), hasRequiredGroup (group gate with
malformed-claim defense).
provider.go — NewOIDCProvider, LoginURL, HandleCallback. Implements
the auth.Provider interface (compile-time check).
HandleCallback runs the full 10-step verification
chain documented inline.
Test coverage (28 oidc subtests; 15 pkg/auth subtests = 43 total):
TestT1ForgedSignature — sig mint-by-attacker rejected
TestT2WrongIssuer — iss claim mismatch rejected
TestT3WrongAudience — aud claim mismatch rejected
TestT4Expired — exp in the past rejected
TestNonceMismatch — id_token nonce != state nonce
TestT10PKCEVerifierMissing — PKCE-on + no verifier rejected
TestT17RequiredGroupMissing — gate denies wrong group
TestT17GroupsClaimMalformed — string-shaped claim denies
TestRequiredGroupSatisfied — happy-path group OK
TestT18StateEnvMismatch — cross-env state replay rejected
TestT23UsernameInjection (6 sub) — SQL/newline/NUL/etc usernames rejected
TestIdPErrorParam — IdP error_description NOT leaked
TestMissingCode — empty code rejected
TestLegacyPermissiveUsername(2) — shim accepts email-format,
still rejects empty/whitespace
TestHandleCallbackHappy — full successful exchange
TestNewOIDCProviderDiscoveryFails — bad discovery rejected at init
TestLoginURLValidatesState — empty State.EnvUUID/Nonce rejected
TestLoginURLPKCERequired — verifier-missing rejected at LoginURL
+ claim/config unit tests
The in-process fakeIdP test harness boots a real httptest.Server that
serves discovery + JWKS + token endpoints. id_tokens are
RS256-signed with an in-memory RSA key, so the signature/iss/aud/exp
checks all exercise the genuine go-oidc.Verifier path, not a mock.
== cmd/admin/oidc.go refactor ==
The file shrinks from 360 LoC to ~180 LoC by replacing the inline
protocol implementation with calls into pkg/auth/oidc. Preserved:
- Route names (/login, /oidc/callback)
- YAML config field names
- Session-cookie + session-manager flow
- JIT-provisioning policy (zero permissions)
- Audit-log behavior
State-cookie storage switches from gorilla/securecookie to
pkg/auth.IssueStateCookie (HMAC JWT signed with the existing
flagParams.Admin.SessionKey). State cookie name changes from
"osctrl-admin-oidc-state" to "osctrl_auth_state" — operators
mid-OIDC-login during a deploy will need one retry. Documented in
the spec's "accepted minor break" section.
Username validation: pre-refactor cmd/admin did no character-class
check, so deployments where IdPs emit email-format
preferred_username values rely on permissive acceptance. The
Config.LegacyPermissiveUsername flag set by cmd/admin's
fromYAMLConfig preserves that behavior. New callers (cmd/api in a
later branch) leave the flag false and get the strict default.
== cmd/admin/main.go ==
Drops the no-longer-needed `var oidcRT *oidcRuntime` global and
updates initOIDC's call site to match the new (error)-only return.
== Verified ==
- go build ./... clean
- go vet ./... clean
- go test ./... all 16 packages green including the new auth tests
- Backwards-compat audit documented in spec § "Backwards compatibility
for cmd/admin"
Closes hardening item 5A from the auth spec's pentest plan.
Before: LoginURL passed state.EnvUUID as the OAuth2 `state` parameter,
and HandleCallback compared the returned URL state against
state.EnvUUID. Env identifiers are operator-visible (URLs, logs, UI)
and in many deployments are short, guessable strings ("admin",
"prod", "default"). An attacker who knows or guesses the target's
EnvUUID could craft a callback URL whose state parameter survives
step 2 of the verification chain. The defense was relying entirely
on the cookie's HMAC signature for CSRF protection — a single
mistake in cookie handling would have opened a CSRF hole.
After: LoginURL passes state.Nonce — the 256-bit cryptorandom value
already minted for the id_token nonce — as the OAuth2 state param.
HandleCallback checks `url.state == cookie.Nonce`. The value is now
unguessable, so step 2 alone gates CSRF whether or not the cookie's
HMAC happens to verify. Cookie HMAC remains the second layer.
State.EnvUUID stays in the cookie shape (LoginURL still rejects an
empty one) but becomes informational at the protocol layer. Callers
that want env-scoping above the protocol use it out-of-band:
cmd/admin sets it to "admin"; cmd/api will set it to a global
sentinel.
Test changes:
- TestT18StateEnvMismatch repurposed as TestT6CSRFStateTampering
with a more accurate threat label: env-replay isn't a threat at
this layer; CSRF/state-tampering is.
- TestLoginURLValidatesState now asserts state=<nonce> in the
authorize URL AND asserts that state=<env-uuid> does NOT appear.
- fakeCallback's stateParam argument documented as the nonce.
- TestMissingCode's state param updated so the test reaches step 3
(missing code) rather than failing at step 2 (state mismatch).
All 28 oidc subtests pass. `go build ./...` and `go vet ./...` clean.
Bug found during Auth0 manual smoke: operators configuring
--oidc-username-claim with a non-standard name (nickname, upn,
login, etc.) silently got the subject claim back instead. The old
switch statement only covered preferred_username/email/sub; any
other configured name fell through to subject — which on Auth0 is
"auth0|hex…" containing the pipe character the strict username
regex rejects.
After: pickUsername now consults a generic raw claim map after the
typed-struct fast path. Any string-valued claim wins; non-string
values (arrays, objects, missing) fall back to subject. The typed
struct fast path stays for the three known claims since it avoids
a map lookup in the hot path.
== Files ==
pkg/auth/oidc/claims.go
* pickUsername(c, raw, claim) signature; reads raw[claim] for
claim names not in the typed struct
pkg/auth/oidc/provider.go
* HandleCallback decodes raw map ONCE (was duplicated — earlier
decode for ResolvedIdentity.Raw moved up so pickUsername can
reuse it)
pkg/auth/oidc/claims_test.go
+ TestPickUsernameCustomClaim covers nickname/upn/missing/
non-string/nil-map paths
== Why this matters ==
Auth0's default sub format (auth0|hex) means operators MUST set a
non-default username claim — and the only practical one in the
profile scope is `nickname`. Without this fix the manual smoke test
against Auth0 silently fails 100% of the time, regardless of how the
operator configures their app.
The fix also unblocks Entra ID (which uses `upn`) and any custom
claim shape operators have set up via Auth0 Actions or Keycloak
mappers.
== Verified ==
- go build ./... clean
- go test ./pkg/auth/... -count=1 — all pass (includes the new
3-subtest TestPickUsernameCustomClaim covering nickname, upn,
missing claim, non-string value, nil raw map)
6 tasks
javuto
added
osctrl-admin
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pkg/auth/with a sharedauth.Providerinterface (Type(),LoginURL(),HandleCallback()) and JWT-based state/nonce cookie helperspkg/auth/oidc/from the legacy admin's inline OIDC handling into a reusable provider implementationcmd/admin/handlers/to consume the shared package — legacy admin OIDC behavior preserved end-to-endpickUsername()reads arbitrary claim names from the raw ID token (fixes Auth0nicknameclaim handling)Depends on
Test plan
go test ./pkg/auth/... ./pkg/users/...passesgo vet ./...clean