Answers net-kingdom's NK-WP-0048-T04 review request. Two blocking findings: redirect URIs with '?' pass the type but make KeyCape exit at startup, and access_groups is enforced only by the consumer, never by KeyCape. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Assistant: claude-code Assistant-Model: opus Assistant-Process: 779409@bnt-lap001 Assistant-Session: c658ca06-19b5-468d-be64-2985dc20bfd3
3.8 KiB
Review: SecurityCaseType oidc-browser-relying-party 1.0
Reviewed 2026-10-10 by key-cape, the owner of the client field rules, at net-kingdom's
request (NK-WP-0048-T04, messages c0eb8a9a and f4425dc4). Sources:
net-kingdom security-case-types/oidc-browser-relying-party/type.yaml and the
renderer sso-mfa/k8s/keycape/keycape-client.py (client_from_params), compared
with KeyCape src/internal/config/validate.go and src/cmd/keycape/main.go.
The composition is sound. The derived custody path, the ESO-only reader, the order
bind-env → register-client, the reverse rollback order and the value-free
verification all match how KeyCape resolves and checks clients. The mapping of
mfa_required is correct: "true" writes mfaRequired: true, and "false" omits
the field (it never writes an explicit false). That distinction matters, as R3
explains. The renderer has a test for it (test_keycape_client.py).
Findings
R1 — A redirect URI with ? passes the type and the renderer, and then stops
KeyCape from starting. (blocking)
The type's pattern ^https://[^*\s,]+…$ and the renderer's REDIRECT both allow ?.
KeyCape's validator rejects any redirect URI that contains * or ?
(validate.go, the wildcard check), and main.go exits on any validation error.
After register-client restarts KeyCape, a case with a query in a redirect URI leaves
the single replica unable to start, so every client loses sign-in, not just the new
one. Fix: exclude ? and # from both patterns (OAuth 2.0 also forbids fragments
in redirect URIs). Separately from this case, register-client should run KeyCape's
own config validation on the rendered registry before it restarts anything.
R2 — access_groups does not gate sign-in at KeyCape. (blocking for the stated
purpose)
KeyCape has no per-client group gate, and access_groups is not bound to the
registration. KeyCape emits the user's groups in the ID token and access token
regardless of scope (token.go, core claims). /userinfo returns them only when
groups was granted (userinfo.go). Two consequences:
- The description should say that the consumer enforces the gate. For Forgejo
that is the required-claim setting in
docs/forgejo-oidc-client-request.yaml. A verification step should confirm that the consumer is actually configured to enforce it, since KeyCape cannot. - The
allowed_scopesenum accepts values withoutgroupstogether with a non-emptyaccess_groups. A consumer that reads claims from UserInfo then never sees the groups, and the gate fails. Add a cross-parameter constraint:access_groupsrequiresgroupsinallowed_scopes.
R3 — mfa_required: "false" means "the per-user policy", not "no MFA".
(documentation)
With no mfaRequired set, KeyCape asks privacyIDEA whether that user needs MFA. An
explicit mfaRequired: false skips that check (authorize.go). The renderer already
does the right thing. State it in the parameter description so that no later
renderer emits an explicit false.
R4 — register-client restarts the identity provider for everyone. (operational note) KeyCape loads clients at startup and keeps login, authorization and handoff state in process memory (SCOPE.md, "Material limits"). Each apply or remove therefore drops in-flight logins and browser sessions for every client. Record this in the attended justification, and schedule runs in a quiet window.
Not a finding
client_idpatterns: KeyCape requires only a non-emptyclientId. The type's{1,62}and the derived env and Secret names are compatible: the env name maps-to_, and_is not allowed in aclient_id, so two ids cannot produce the same env name.- The client secret format (48 random bytes, base64url, 64 characters) is opaque to
KeyCape. It needs no escaping under
client_secret_basic.