From 60384834fe1b74cca4e786da67f5d4874425ae05 Mon Sep 17 00:00:00 2001
From: tegwick <bernd.worsch@gmail.com>
Date: Sat, 10 Oct 2026 12:39:31 +0200
Subject: [PATCH] Review oidc-browser-relying-party 1.0 against KeyCape's
 client field rules

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
---
 .../oidc-browser-relying-party-type-review.md | 65 +++++++++++++++++++
 1 file changed, 65 insertions(+)
 create mode 100644 docs/oidc-browser-relying-party-type-review.md

diff --git a/docs/oidc-browser-relying-party-type-review.md b/docs/oidc-browser-relying-party-type-review.md
new file mode 100644
index 0000000..259f7e1
--- /dev/null
+++ b/docs/oidc-browser-relying-party-type-review.md
@@ -0,0 +1,65 @@
+# 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_scopes` enum accepts values without `groups` together with a
+  non-empty `access_groups`. A consumer that reads claims from UserInfo then never
+  sees the groups, and the gate fails. Add a cross-parameter constraint:
+  `access_groups` requires `groups` in `allowed_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_id` patterns: KeyCape requires only a non-empty `clientId`. The type's
+  `{1,62}` and the derived env and Secret names are compatible: the env name maps
+  `-` to `_`, and `_` is not allowed in a `client_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`.