Follow-ups: flow-serving bridge (gateway verification gate + review findings from #12-#20) #30

New issue

Open

opened 2026-09-15 17:38:41 +00:00 by ginjiruu · 0 comments

ginjiruu commented

2026-09-15 17:38:41 +00:00

Owner

Parent

#11 — Spec: The flow-serving bridge (gRPC ext_authz Check + OIDC/Exchange flows + callback/logout listener)

What this is

The single tracking ticket for the work that spec #11's implementation surfaced but did not include: one spec verification gate that was never performed, plus the code-review follow-ups from the nine build tickets (#12–#20, all merged). Broken down into individual tickets when scheduled.

Status (updated 21 Sep 2026)

Verified against the current tree:

Item 2 (shared token-endpoint POST helper): still open. tokenExchange (exchange.go), the refresh grant (oidc.go), and authorizationCodeExchange (callback.go) remain three separate form-POST skeletons; no shared helper.
Item 3 (OIDC TokenEndpoint not gated): still open. App.OIDCAvailable() (snapshot.go) checks the authorization endpoint, callback set, and JWKS, but not Shared.TokenEndpoint — the OIDC gate still misclassifies a never-resolved token endpoint as a failed refresh (302 loop) rather than the ADR 0011 503 unavailable class.
Item 4 (token cache TTL fallback bound): still open. defaultTokenTTL is still a fixed 60s with no subject-exp cap (overlaps #61 item 3).
Item 5 (callback edge cases): still open. matchCallbackPath still compares u.Path == reqPath, so a callback registered at the hostname root (https://host, empty path) never matches / (403); the multi-URL redirect_uri = CallbackURLs[0] vs match-by-path note still has no test/comment.
Item 1 (gateway verification gate) and Item 6 (gateway CRDs decision): pending — verification / decision items, no code change yet.

Items

1. Gateway verification gate (spec #11 — the only unperformed spec item)

The spec's verified gate: the lab gateway must honor denied_response (302 + Location + Set-Cookie) and response_headers_to_add (Set-Cookie), and the OIDC rule's allowedHeaders must list cookie (ADR 0003). If unmet, the OIDC Flow does not work on that gateway (the Exchange Flow still does — it needs no 302). Verify against the lab gateway; this is the difference between "the flows are built" and "the flows work end-to-end behind the gateway".

2. Shared token-endpoint POST helper (cross-cutting, from the #16/#17 reviews)

tokenExchange (internal/serve/exchange.go), tokenRefresh (internal/serve/oidc.go), and authorizationCodeExchange (internal/serve/callback.go) are three copies of the same form-POST skeleton: build form → NewRequestWithContext → Content-Type → Do → ReadAll → Unmarshal → status/error/missing-token checks. Extract a shared postFormToTokenEndpoint-style helper; each grant fills the form and defines its own success check. Related (optional): the unavailable-reason gates (unavailableReason / exchangeUnavailableReason / callbackUnavailableReason) share the same cascade shape but check different fields — a shared helper is possible but would need care to keep behavior identical.

3. Availability gate: `TokenEndpoint` not gated (from the #17 review)

unavailableReason (the OIDC Check's 5xx gate) checks the client secret, the OIDC section, the authorization endpoint, the callback URLs, and the JWKS — but not Shared.TokenEndpoint. An App whose token endpoint never resolved surfaces as a failed refresh (a 302 re-login loop) instead of the ADR 0011 503 "unavailable" class for broken config. Fail-closed, but misclassified. Fix: include TokenEndpoint == "" in the gate (the callback and exchange gates already check it).

4. Token cache TTL fallback bound (from the #18 review)

When the token endpoint omits expires_in, the per-replica cache falls back to a fixed 60s TTL, which can exceed the token's actual exp (the backend then 401s — not a mis-authorization, but it breaks the ADR 0009 "TTL-bounded ≤ the access token's exp" guarantee). Kanidm always sends expires_in, so this is an edge case; consider a smaller fallback or documenting the assumption.

5. Callback edge cases (from the #16 review — low priority)

A callback URL registered at the App's hostname root (https://host) never matches the request path / (always 403).
For multi-URL Apps, the auth request's redirect_uri is CallbackURLs[0] while the callback matches by path. Safe in the real IdP flow (the IdP only redirects to the requested redirect_uri; direct navigation to another registered path is 403'd — fail-closed), but worth a test or a comment.

6. Gateway CRDs are test-only (from the #20 review — decision needed)

The gateway-api CRDs are committed only under test/crds/gateway-api/ for envtest; the install bundle assumes a Gateway implementation (which ships the CRDs) is present in-cluster. Decide whether the bundle should be self-contained (ship the CRDs) or document the assumption.

Out of scope

Anything that changes the merged behavior of #12–#20 beyond the fixes above; new Flow kinds (ADR 0004's future work); the state model (spec #1).

## Parent #11 — Spec: The flow-serving bridge (gRPC ext_authz Check + OIDC/Exchange flows + callback/logout listener) ## What this is The single tracking ticket for the work that spec #11's implementation surfaced but did not include: one spec verification gate that was never performed, plus the code-review follow-ups from the nine build tickets (#12–#20, all merged). Broken down into individual tickets when scheduled. ## Status (updated 21 Sep 2026) Verified against the current tree: - **Item 2 (shared token-endpoint POST helper): still open.** `tokenExchange` (exchange.go), the refresh grant (oidc.go), and `authorizationCodeExchange` (callback.go) remain three separate form-POST skeletons; no shared helper. - **Item 3 (OIDC `TokenEndpoint` not gated): still open.** `App.OIDCAvailable()` (snapshot.go) checks the authorization endpoint, callback set, and JWKS, but not `Shared.TokenEndpoint` — the OIDC gate still misclassifies a never-resolved token endpoint as a failed refresh (302 loop) rather than the ADR 0011 503 unavailable class. - **Item 4 (token cache TTL fallback bound): still open.** `defaultTokenTTL` is still a fixed 60s with no subject-`exp` cap (overlaps #61 item 3). - **Item 5 (callback edge cases): still open.** `matchCallbackPath` still compares `u.Path == reqPath`, so a callback registered at the hostname root (`https://host`, empty path) never matches `/` (403); the multi-URL `redirect_uri = CallbackURLs[0]` vs match-by-path note still has no test/comment. - **Item 1 (gateway verification gate) and Item 6 (gateway CRDs decision): pending** — verification / decision items, no code change yet. ## Items ### 1. Gateway verification gate (spec #11 — the only unperformed spec item) The spec's verified gate: the lab gateway must honor `denied_response` (`302` + `Location` + `Set-Cookie`) and `response_headers_to_add` (`Set-Cookie`), and the OIDC rule's `allowedHeaders` must list `cookie` (ADR 0003). If unmet, the OIDC Flow does not work on that gateway (the Exchange Flow still does — it needs no `302`). Verify against the lab gateway; this is the difference between "the flows are built" and "the flows work end-to-end behind the gateway". ### 2. Shared token-endpoint POST helper (cross-cutting, from the #16/#17 reviews) `tokenExchange` (internal/serve/exchange.go), `tokenRefresh` (internal/serve/oidc.go), and `authorizationCodeExchange` (internal/serve/callback.go) are three copies of the same form-POST skeleton: build form → `NewRequestWithContext` → `Content-Type` → `Do` → `ReadAll` → `Unmarshal` → status/error/missing-token checks. Extract a shared `postFormToTokenEndpoint`-style helper; each grant fills the form and defines its own success check. Related (optional): the unavailable-reason gates (`unavailableReason` / `exchangeUnavailableReason` / `callbackUnavailableReason`) share the same cascade shape but check different fields — a shared helper is possible but would need care to keep behavior identical. ### 3. Availability gate: `TokenEndpoint` not gated (from the #17 review) `unavailableReason` (the OIDC Check's 5xx gate) checks the client secret, the OIDC section, the authorization endpoint, the callback URLs, and the JWKS — but not `Shared.TokenEndpoint`. An App whose token endpoint never resolved surfaces as a *failed refresh* (a 302 re-login loop) instead of the ADR 0011 `503` "unavailable" class for broken config. Fail-closed, but misclassified. Fix: include `TokenEndpoint == ""` in the gate (the callback and exchange gates already check it). ### 4. Token cache TTL fallback bound (from the #18 review) When the token endpoint omits `expires_in`, the per-replica cache falls back to a fixed 60s TTL, which can exceed the token's actual exp (the backend then 401s — not a mis-authorization, but it breaks the ADR 0009 "TTL-bounded ≤ the access token's exp" guarantee). Kanidm always sends `expires_in`, so this is an edge case; consider a smaller fallback or documenting the assumption. ### 5. Callback edge cases (from the #16 review — low priority) - A callback URL registered at the App's hostname root (`https://host`) never matches the request path `/` (always 403). - For multi-URL Apps, the auth request's `redirect_uri` is `CallbackURLs[0]` while the callback matches by path. Safe in the real IdP flow (the IdP only redirects to the requested `redirect_uri`; direct navigation to another registered path is 403'd — fail-closed), but worth a test or a comment. ### 6. Gateway CRDs are test-only (from the #20 review — decision needed) The gateway-api CRDs are committed only under `test/crds/gateway-api/` for envtest; the install bundle assumes a Gateway implementation (which ships the CRDs) is present in-cluster. Decide whether the bundle should be self-contained (ship the CRDs) or document the assumption. ## Out of scope Anything that changes the merged behavior of #12–#20 beyond the fixes above; new Flow kinds (ADR 0004's future work); the state model (spec #1).

ginjiruu added the

needs-triage

label

2026-09-15 17:38:44 +00:00

ginjiruu referenced this issue

2026-09-15 17:38:48 +00:00

Spec: The flow-serving bridge (gRPC ext_authz Check + OIDC/Exchange flows + callback/logout listener) #11

ginjiruu referenced this issue

2026-09-15 17:38:49 +00:00

Spec: The flow-serving bridge (gRPC ext_authz Check + OIDC/Exchange flows + callback/logout listener) #11

ginjiruu referenced this issue

2026-09-15 23:18:27 +00:00