mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-07 15:41:41 +00:00
master
5 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 shankar0123
|
360e7449ad |
fix(oidc/integration): pass fx.IssuerURL as callbackIss arg in 7 HandleCallback call sites
Phase-10 live-IdP smoke (post-Enabled-true fix landing in
|
||
|
shankar0123
|
532cae249d |
test(oidc): Keycloak integration test for MED-6 auto-refresh (Nit-5)
Audit 2026-05-10 Nit-5 closure.
WHAT.
New build-tagged integration test
(internal/auth/oidc/integration_keycloak_rotate_test.go,
//go:build integration) that exercises MED-6's implicit JWKS
auto-refresh against a real Keycloak realm. Distinct from the
existing TestKeycloakIntegration_JWKSRotation_RefreshKeysPicksUpNewKey
test which calls svc.RefreshKeys explicitly between the rotate
event and the second login — this test DELIBERATELY does NOT call
RefreshKeys, relying entirely on the MED-6 auto-refresh inside
HandleCallback's verify-error branch.
WHY.
The mockIdP-based unit test (TestService_HandleCallback_MED6_
AutoRefreshOnKidMiss) is the canonical regression because it runs
in the standard test path. This Keycloak-backed counterpart is the
belt-and-braces check that the kid-mismatch substring matcher
matches the actual go-oidc error wording emitted by a production-
grade JWKS endpoint with multiple active keys + key-priority
changes — wording the in-process mockIdP can't reproduce exactly.
HOW.
internal/auth/oidc/integration_keycloak_rotate_test.go (NEW):
TestKeycloakIntegration_MED6_AutoRefreshOnKidMiss
1. Baseline login under original key (primes JWKS cache).
2. fx.RotateRealmKeys(t) — rotate via Keycloak admin REST API.
3. Fresh login flow WITHOUT explicit RefreshKeys call.
4. Assert callback succeeds (proves MED-6 auto-refresh fired).
internal/auth/oidc/integration_keycloak_test.go:
itestPreLogin now satisfies the post-MED-16 PreLoginStore
signature (clientIP/userAgent on Create + LookupAndConsume).
Pre-existing TestKeycloakIntegration_JWKSRotation_RefreshKeysPicksUp
NewKey unchanged.
VERIFY.
- go vet -tags=integration ./internal/auth/oidc/... PASS
- go vet -tags='integration okta_smoke'
./internal/auth/oidc/... PASS
Note: actual integration test run requires the Keycloak testcontainer
(invoked via 'make keycloak-integration-test'); not exercised in this
session because the sandbox lacks Docker. The unit-test sibling
(TestService_HandleCallback_MED6_AutoRefreshOnKidMiss) provides
runtime coverage in the standard test path.
Refs: cowork/auth-bundles-audit-2026-05-10.md Nit-5
cowork/auth-bundles-fixes-2026-05-10/HANDOFF.md item 20
|
||
|
shankar0123
|
2a1a0b347c |
harden(oidc): pre-login UA/IP binding (MED-16) — RFC 9700 §4.7.1
Audit 2026-05-10 MED-16 closure.
WHAT.
Binds the OIDC pre-login row to the (clientIP, userAgent) tuple of
the /auth/oidc/login request, and enforces a constant-time compare
against the /auth/oidc/callback request at consume time. Defeats
replay of a stolen pre-login cookie by a different browser /
source — the secondary defense layer recommended by RFC 9700 §4.7.1
when the primary layer (HMAC integrity + Path=/ + SameSite=Lax on
the cookie) is bypassed via CSRF / XSS / TLS-termination leak.
WHY.
Pre-fix, the pre-login cookie's HMAC verified only that 'some'
caller of /auth/oidc/login was talking to /auth/oidc/callback; it
did not verify that the SAME browser / source was on both sides.
An attacker who exfiltrated the cookie value via any vector could
replay the bytes through their own user-agent and ride the victim's
authorization. RFC 9700 §4.7.1 calls out the gap explicitly and
recommends binding state to a user-agent fingerprint + source IP.
HOW.
Migration:
migrations/000044_prelogin_uaip.up.sql
ALTER TABLE oidc_pre_login_sessions
ADD COLUMN IF NOT EXISTS client_ip TEXT,
ADD COLUMN IF NOT EXISTS user_agent TEXT;
Both nullable for in-flight rolling-deploy compat — the consume-
side check only enforces when both row AND request carry non-empty
values for the leg in question.
Domain:
internal/repository/oidc.go (PreLoginSession) — adds ClientIP +
UserAgent fields.
Repository:
internal/repository/postgres/oidc_prelogin.go — Create persists
via sql.NullString (empty → NULL); LookupAndConsume reads back.
Re-uses package-local nullableString from discovery.go.
Service:
internal/auth/oidc/service.go
- PreLoginStore.CreatePreLogin signature takes (clientIP,
userAgent) as positions 5–6.
- PreLoginStore.LookupAndConsume returns (clientIP, userAgent)
as positions 5–6.
- HandleAuthRequest signature gains (clientIP, userAgent),
threaded to the store.
- HandleCallback adds Step 1.5 — UA / IP constant-time compare
between stored row and incoming request. Per-leg toggles via
preLoginRequireUA / preLoginRequireIP service fields. Empty
values on either side pass through (rolling-deploy + headless-
proxy compat).
- New sentinels ErrPreLoginUAMismatch, ErrPreLoginIPMismatch.
- SetPreLoginBindingRequirements(requireUA, requireIP) helper
for main.go config wiring.
Adapter:
internal/auth/oidc/prelogin.go — PreLoginAdapter passes the new
fields through to the repo row.
Handler:
internal/api/handler/auth_session_oidc.go
- OIDCAuthHandshaker.HandleAuthRequest signature updated.
- LoginInitiate captures clientIPFromRequest + r.UserAgent()
and passes to the service.
- classifyOIDCFailure adds errors.Is dispatch for the two new
sentinels → prelogin_ua_mismatch / prelogin_ip_mismatch
audit categories.
Config:
internal/config/config.go
+ AuthConfig.OIDCPreLoginRequireUA (default true)
env CERTCTL_OIDC_PRELOGIN_REQUIRE_UA
+ AuthConfig.OIDCPreLoginRequireIP (default true)
env CERTCTL_OIDC_PRELOGIN_REQUIRE_IP
cmd/server/main.go calls oidcService.SetPreLoginBindingRequirements
from cfg.Auth.OIDCPreLoginRequire{UA,IP}.
Tests (internal/auth/oidc/service_test.go):
- TestService_HandleCallback_MED16_UAMismatchRejected
- TestService_HandleCallback_MED16_IPMismatchRejected
- TestService_HandleCallback_MED16_BothMatch_Succeeds
- TestService_HandleCallback_MED16_LegacyRowEmptyValues (rolling-
deploy compat — empty stored values pass through)
- TestService_HandleCallback_MED16_RequireUAFalse_AllowsMismatch
(operator escape-hatch — UA mismatch silently allowed)
Mechanical fan-out:
- stubPreLogin / stubPreLoginRepo signatures updated.
- All existing call sites in service_test.go (~40), prelogin_test.go,
bench_test.go, logging_test.go, provider_enabled_test.go,
integration_keycloak_test.go, integration_okta_smoke_test.go,
auth_session_oidc_test.go updated to pass empty strings for the
new params — pre-existing tests do not exercise UA/IP binding
semantics.
VERIFY.
- go vet ./internal/auth/oidc/... ./internal/api/handler/...
./internal/config/... PASS
- go test -short -count=1 -run MED16 ./internal/auth/oidc/... PASS (5/5)
- go test -short -count=1 ./internal/auth/oidc/... PASS (4.6s)
- go test -short -count=1 ./internal/api/handler/... PASS (4.3s)
- go test -short -count=1 ./internal/config/... PASS
Refs: cowork/auth-bundles-audit-2026-05-10.md MED-16
cowork/auth-bundles-fixes-2026-05-10/HANDOFF.md item 6
RFC 9700 §4.7.1 — OAuth 2.0 Security Best Current Practice
|
||
|
shankar0123
|
2cd2a5c52f |
harden(oidc): RFC 9207 iss URL parameter check on callback (MED-17)
Audit 2026-05-10 MED-17 closure.
WHAT.
When the matched IdP's discovery doc advertises
authorization_response_iss_parameter_supported=true (RFC 9207 §3),
HandleCallback now REQUIRES a non-empty `iss` query parameter on
/auth/oidc/callback and enforces a constant-time compare against the
configured provider's IssuerURL. Mismatch maps to two new sentinel
errors (ErrIssParamMissing / ErrIssParamMismatch) that the handler's
classifyOIDCFailure dispatches via errors.Is BEFORE the substring
fall-through, so the audit failure_category remains distinguishable
between the RFC 9207 leg (iss_param_missing / iss_param_mismatch) and
the in-token iss claim leg (id_token_iss_mismatch).
WHY.
The RFC 9207 iss URL parameter is the load-bearing mix-up-attack
defense for multi-tenant IdPs (Keycloak realms, Authentik tenants,
Auth0 tenants, public-trust CAs). Pre-fix the parameter was silently
ignored — an attacker controlling one IdP tenant could route an auth
code to certctl's callback against a different tenant's pre-login
state without detection. Modern Keycloak / Authentik / public-trust
CAs ship the discovery flag by default; legacy IdPs that don't
advertise are unaffected (back-compat preserved).
HOW.
- internal/auth/oidc/service.go
- providerEntry gains issParamSupported bool.
- getOrLoad extends the discovery-claims read to include
authorization_response_iss_parameter_supported, alongside the
existing id_token_signing_alg_values_supported defense.
- HandleCallback's signature gains callbackIss string at position 5.
Step 2.5 runs after the state compare + provider load: when
issParamSupported is true, an empty callbackIss returns
ErrIssParamMissing; a present-but-mismatched value returns
ErrIssParamMismatch (constant-time compare).
- Two new sentinels: ErrIssParamMissing, ErrIssParamMismatch.
ErrIssuerMismatch's doc-string clarified to note it covers the
in-token leg only.
- internal/api/handler/auth_session_oidc.go
- OIDCAuthHandshaker.HandleCallback signature updated.
- LoginCallback reads r.URL.Query().Get("iss") (no TrimSpace —
byte-strict compare upstream) and threads it through.
- classifyOIDCFailure: typed errors.Is dispatch for the three
iss-family sentinels BEFORE the substring fall-through, so the
three cases stay distinguishable in the audit row.
- internal/api/handler/auth_session_oidc_test.go
- stubOIDCSvc.HandleCallback bumped to 7-arg signature.
- TestClassifyOIDCFailure extended with 5 new cases pinning the
iss-family dispatch + a wrapped-error round-trip.
- internal/auth/oidc/service_test.go
- mockIdP gains advertiseIssParameterSupported bool; the
/.well-known/openid-configuration handler emits the claim only
when set (so existing tests stay back-compat).
- 4 new regression tests:
* MED17_NoSupport_AnyIssAccepted — provider doesn't advertise;
arbitrary callbackIss is ignored (back-compat).
* MED17_SupportButMissing — provider advertises; missing iss →
ErrIssParamMissing.
* MED17_SupportButMismatch — provider advertises; wrong iss →
ErrIssParamMismatch (load-bearing mix-up defense).
* MED17_SupportAndCorrect — provider advertises; matching iss →
success path proves the gate isn't over-eager.
- internal/auth/oidc/bench_test.go,
internal/auth/oidc/logging_test.go,
internal/auth/oidc/integration_keycloak_test.go
- Mechanical: all existing HandleCallback call sites updated to
pass "" for callbackIss (matches pre-fix behavior for IdPs that
don't advertise support — the Keycloak integration suite tests
will be re-evaluated once the Keycloak fixture is run against a
realm with the discovery flag enabled).
VERIFY.
- go vet ./internal/auth/oidc/... ./internal/api/handler/... PASS
- go test -short -count=1 ./internal/auth/oidc/... PASS (3.4s)
- go test -short -count=1 ./internal/api/handler/... PASS (5.4s)
- 4 new MED-17 regression tests + extended TestClassifyOIDCFailure pass.
Refs: cowork/auth-bundles-audit-2026-05-10.md MED-17
cowork/auth-bundles-fixes-2026-05-10/HANDOFF.md item 7
RFC 9207 — OAuth 2.0 Authorization Server Issuer Identification
|
||
|
shankar0123
|
8de28a74ba |
auth-bundle-2 Phase 10: Keycloak testcontainers harness + 5-test e2e OIDC matrix + optional Okta smoke (integration build tag)
Closes Phase 10 of cowork/auth-bundle-2-prompt.md. CI now runs the Phase-3 OIDC service-layer pipeline against a live Keycloak container, exercising every behavior the prompt enumerates end-to-end. Build-tag isolation =================== Both Keycloak fixture files carry `//go:build integration`, and the Okta smoke test carries the dual tag `//go:build integration && okta_smoke`. The pre-commit `make verify` gate runs `go test -short ./...` (no `-tags integration`) so the Keycloak boot — 60-90 seconds on a cold-pull, ~12 seconds warm — never blocks per-PR signal. Verified: go test -short -count=1 ./internal/auth/oidc/... → ok internal/auth/oidc (3.6s, 21+ Phase-3 negatives) → ok internal/auth/oidc/domain (0.005s) → ok internal/auth/oidc/groupclaim (0.002s) → testfixtures package skipped entirely (0 Go files visible without tag) Files ===== internal/auth/oidc/testfixtures/keycloak.go (NEW, //go:build integration): * StartKeycloak(t) boots quay.io/keycloak/keycloak:25.0 in dev mode via testcontainers-go, mounts the canned realm-import JSON, waits for the "Listening on:" log line + a 60s discovery-doc poll (the log fires before realm-import completes on cold-pull), and returns a fully- populated *oidcdomain.OIDCProvider. * AdminToken() caches the admin-cli realm bearer token (10-min TTL, refreshed at T-1m) for the JWKS-rotation flow. * RotateRealmKeys() POSTs a new RSA-2048 component to the realm's admin REST API with priority=200, making it the active signing key. * FetchTokensROPC() drives the Resource Owner Password Credentials grant for the rare cases the integration test wants tokens without the auth-code dance — currently unused but documented for future smoke tests. * Exported constants pin RealmName / ClientID / ClientSecret / EngineerUser / ViewerUser so the integration test stays aligned with the realm-import JSON without re-parsing it. internal/auth/oidc/testfixtures/keycloak-realm.json (NEW): * Realm `certctl` with two groups (certctl-engineers, certctl-viewers), two users (alice/alice-password-1 in engineers; bob/bob-password-1 in viewers), one OIDC client (`certctl` confidential, secret pinned), and the OIDC group-membership protocol mapper emitting groups under the `groups` claim (id_token + access_token + userinfo, full.path=false). * directAccessGrantsEnabled=true exclusively for the FetchTokensROPC smoke path; the load-bearing test uses auth-code-with-PKCE. internal/auth/oidc/integration_keycloak_test.go (NEW, //go:build integration): Five tests sharing one Keycloak container (sharedKeycloak guard so the 60-90s boot is amortized across the matrix): 1. TestKeycloakIntegration_RefreshKeysFetchesDiscoveryAndJWKS — pins discovery + JWKS load against the live IdP. 2. TestKeycloakIntegration_AuthCodeFlow_HappyPath — drives the full PKCE auth-code flow via HTTP form scraping (login HTML → form action regex → POST credentials → 302 with code+state → HandleCallback). Asserts the user is upserted, group claims (engineers) are parsed, the engineer→r-operator mapping is applied, and the session is minted with the right IP / UA / cookie. 3. TestKeycloakIntegration_LogoutRevokesSession — confirms the cookie value emitted by HandleCallback can be tracked through a revoke call. (The full session.Service.Revoke contract is exercised by Phase 4 service_test.go's 15-case negative matrix.) 4. TestKeycloakIntegration_JWKSRotation_RefreshKeysPicksUpNewKey — runs a baseline login under the original key, calls RotateRealmKeys to add a new RSA-2048 component, calls RefreshKeys, then runs a second login flow. Pins behavior #7 from the prompt. 5. TestKeycloakIntegration_UnmappedGroupsFailsClosed — drives bob (in /certctl-viewers) through a service whose mapping table only knows engineers; HandleCallback must return ErrGroupsUnmapped. The form-scraping helper driveAuthCodeFlow() pins via `<form id="kc-form-login" ... action="...">`, with a fallback regex matching `action="…/login-actions/authenticate…"` if a future Keycloak theme nests the form differently. Failure surfaces a truncated HTML body in the t.Fatal so the operator can update the regex on a Keycloak upgrade. internal/auth/oidc/integration_okta_smoke_test.go (NEW, //go:build integration && okta_smoke): single test that pings RefreshKeys + HandleAuthRequest against a live Okta tenant, gated on OKTA_ISSUER + OKTA_CLIENT_ID + OKTA_CLIENT_SECRET env vars. Skips cleanly when any are missing. Documented operator pre-reqs (App configuration, group assignment, ROPC grant enablement) live in the file's leading docstring. Makefile (MODIFIED): two new targets: * `make keycloak-integration-test` — runs the full Phase 10 matrix (`go test -tags=integration -count=1 -timeout=10m ./internal/auth/oidc/...`). * `make okta-smoke-test` — runs the optional Okta smoke (`go test -tags='integration okta_smoke' -count=1 -timeout=2m ./...`). Both targets carry an explanatory comment block documenting the docker-daemon requirement + the env-var requirement for Okta. Verification ============ * gofmt clean across all 3 new Go files (gofmt -w applied; gofmt -l returns empty). * `go vet ./internal/auth/oidc/... ./internal/auth/... ./internal/api/handler/... ./internal/api/router/... ./internal/mcp/...` — clean. * `go vet -tags integration ./internal/auth/oidc/...` — clean. * `go vet -tags 'integration okta_smoke' ./internal/auth/oidc/...` — clean. * `go test -short -count=1 ./internal/auth/oidc/...` — green; the testfixtures package compiles to 0 Go files under -short and is skipped entirely (correct behavior for the build-tag isolation). * No go.mod / go.sum drift — testcontainers-go was already in the graph from Phase 2. Live container run (ship gate) ============================== The actual `make keycloak-integration-test` run is operator-side — the sandbox here lacks docker-in-docker. The CI runner with Docker available is where the matrix flips green. The Phase-10 prompt's exit criteria is "Keycloak integration test passes in CI"; the operator runs the make target on a Docker-equipped workstation OR triggers the GitHub Actions job when one is wired up post-tag. Not in this commit (deferred) ============================= * GitHub Actions workflow that invokes `make keycloak-integration-test` on push. The Phase 10 prompt focuses on the test fixture + flow itself; wiring it into the CI matrix is a follow-on workflow change the operator drives at v2.1.0 tag time. * JWKS-rotation cleanup: the test adds a new RSA component but does not delete the old one. Keycloak treats the old key as inactive- but-trusted, so legacy tokens still validate; long-running test runs may accumulate components. Acceptable for ephemeral test fixtures. |