From 61ab6bcc33e0b2ac3c5706aa34d6f0dd5f7463a0 Mon Sep 17 00:00:00 2001 From: tegwick Date: Sat, 10 Oct 2026 13:18:11 +0200 Subject: [PATCH] NK-WP-0048: key-cape type review R1-R4 as a 1.0 patch R1: redirect URIs refuse '?' and '#' in the type and keycape-client.py. R2: allowed_scopes always includes groups; access_groups is enforced by the consumer, not KeyCape. R3/R4: mfa_required false and restart-for-all-clients documented. The consumer-enforcement verification step (1.1) awaits a decision. Co-Authored-By: Claude Opus 5.5 Assistant: claude-code Assistant-Model: opus Assistant-Process: 736651@bnt-lap001 Assistant-Session: f6189624-fd64-4109-b53e-f8a59ca8a2d8 --- .../examples/example-public-portal.yaml | 2 +- .../tests/test_oidc_browser_relying_party.py | 9 ++++- .../oidc-browser-relying-party/type.yaml | 35 +++++++++++++++---- sso-mfa/k8s/keycape/keycape-client.py | 6 +++- sso-mfa/k8s/keycape/test_keycape_client.py | 12 +++++++ workplans/NK-WP-0048-security-case-types.md | 19 ++++++++++ 6 files changed, 73 insertions(+), 10 deletions(-) diff --git a/security-case-types/oidc-browser-relying-party/examples/example-public-portal.yaml b/security-case-types/oidc-browser-relying-party/examples/example-public-portal.yaml index 121bc84..c76ddb3 100644 --- a/security-case-types/oidc-browser-relying-party/examples/example-public-portal.yaml +++ b/security-case-types/oidc-browser-relying-party/examples/example-public-portal.yaml @@ -12,7 +12,7 @@ spec: client_id: example-portal display_name: Example portal redirect_uris: https://portal.example.invalid/auth/callback - allowed_scopes: openid,profile,email + allowed_scopes: openid,profile,email,groups client_type: public mfa_required: "false" access_groups: example-portal-users diff --git a/security-case-types/oidc-browser-relying-party/tests/test_oidc_browser_relying_party.py b/security-case-types/oidc-browser-relying-party/tests/test_oidc_browser_relying_party.py index dc3b2fc..431a3bf 100644 --- a/security-case-types/oidc-browser-relying-party/tests/test_oidc_browser_relying_party.py +++ b/security-case-types/oidc-browser-relying-party/tests/test_oidc_browser_relying_party.py @@ -69,10 +69,17 @@ def test_forgejo_case_matches_the_accepted_request(): def test_redirects_must_be_exact_https(): t = as_implemented() for bad in ["https://forgejo.coulomb.social/*", "http://forgejo.coulomb.social/cb", - "https://a.example/cb, https://b.example/cb"]: + "https://a.example/cb, https://b.example/cb", + "https://forgejo.coulomb.social/cb?x=1", "https://forgejo.coulomb.social/cb#f"]: assert any("redirect_uris" in m for m in msgs(v.validate_case(with_params(FORGEJO, redirect_uris=bad), t))), bad + +def test_allowed_scopes_always_include_groups(): + t = as_implemented() + assert all("groups" in e.split(",") for e in t["spec"]["parameters"][3]["constraints"]["enum"]) + assert any("allowed_scopes" in m for m in msgs(v.validate_case(with_params(FORGEJO, allowed_scopes="openid,profile,email"), t))) + def test_custody_path_is_derived_not_case_settable(): t = as_implemented() assert msgs(v.validate_case(FORGEJO, t)) == [] diff --git a/security-case-types/oidc-browser-relying-party/type.yaml b/security-case-types/oidc-browser-relying-party/type.yaml index d547884..ea471f6 100644 --- a/security-case-types/oidc-browser-relying-party/type.yaml +++ b/security-case-types/oidc-browser-relying-party/type.yaml @@ -28,15 +28,21 @@ spec: - name: redirect_uris type: string case_settable: true - constraints: {pattern: "^https://[^*\\s,]+(,https://[^*\\s,]+)*$"} + constraints: {pattern: "^https://[^*?#\\s,]+(,https://[^*?#\\s,]+)*$"} sensitivity: security_sensitive - description: Comma-separated exact https redirect URIs. No wildcards, no plain http. + description: >- + Comma-separated exact https redirect URIs. No wildcards, no plain http, no + query and no fragment: KeyCape refuses '*' and '?' at startup, and OAuth + 2.0 forbids fragments (key-cape type review R1). - name: allowed_scopes type: string case_settable: true - constraints: {enum: ["openid", "openid,profile", "openid,profile,email", "openid,profile,email,groups", "openid,email", "openid,email,groups", "openid,profile,groups", "openid,groups"]} + constraints: {enum: ["openid,profile,email,groups", "openid,email,groups", "openid,profile,groups", "openid,groups"]} sensitivity: security_sensitive - description: Scopes the client may request, openid first. + description: >- + Scopes the client may request, openid first. Always includes groups, + because access_groups is required and KeyCape's UserInfo returns groups + only for that scope (key-cape type review R2). - name: client_type type: string case_settable: true @@ -48,13 +54,23 @@ spec: case_settable: true constraints: {enum: ["true", "false"]} sensitivity: security_sensitive - description: Pin MFA on this client. Required "true" when the service grants administrative or owner rights. + description: >- + "true" pins MFA on this client; required when the service grants + administrative or owner rights. "false" is not "no MFA": the client then + carries no mfaRequired field and KeyCape applies the per-user privacyIDEA + policy. A renderer must never emit an explicit mfaRequired false, which + would skip that policy (key-cape type review R3). - name: access_groups type: string case_settable: true constraints: {pattern: "^[a-z][a-z0-9-]{1,62}(,[a-z][a-z0-9-]{1,62})*$"} sensitivity: operational - description: Comma-separated LLDAP groups that gate sign-in and that the service maps to its roles. Created if absent. + description: >- + Comma-separated LLDAP groups that the service maps to its roles. Created if + absent. KeyCape has no per-client group gate: it issues tokens to any + authenticated user and only carries the groups claim, so the consuming + service enforces the gate itself (for Forgejo, its required-claim setting; + key-cape type review R2). - name: initial_members type: string required: false @@ -214,7 +230,12 @@ spec: not read or write Secrets. Until KeyCape loads clients from a non-secret registry with secrets referenced from custody, one attended operator session applies the rendered registration. ops-warden groups - all attended steps of a run into that one session. + all attended steps of a run into that one session. KeyCape loads clients + at startup and keeps login, authorization and handoff state in memory, so + the restart drops in-flight logins and sessions for every client; runs go + in a quiet window (key-cape type review R4). The rendered registry must + pass KeyCape's own client validation before the restart: a refused entry + stops the single replica and every client loses sign-in (R1). - id: hand-over owner: "{consumer_owner_repo}" entry_point: "configure {consuming_service} with issuer {issuer}, client {client_id} and custody bao:platform/workloads/keycape/{client_id}" diff --git a/sso-mfa/k8s/keycape/keycape-client.py b/sso-mfa/k8s/keycape/keycape-client.py index 35a0010..64a0dae 100644 --- a/sso-mfa/k8s/keycape/keycape-client.py +++ b/sso-mfa/k8s/keycape/keycape-client.py @@ -57,7 +57,8 @@ require = lane.require CASE_TYPE = re.compile(r'^oidc-browser-relying-party@1\.\d+$') CLIENT_ID = re.compile(r'^[a-z0-9][a-z0-9-]{1,62}$') -REDIRECT = re.compile(r'^https://[^*\s,]+$') +# KeyCape refuses '*' and '?' at startup; OAuth 2.0 forbids fragments. +REDIRECT = re.compile(r'^https://[^*?#\s,]+$') SCOPES = {'openid', 'profile', 'email', 'groups'} @@ -78,6 +79,9 @@ def client_from_params(params): scopes = params['allowed_scopes'].split(',') require(scopes[0] == 'openid' and set(scopes) <= SCOPES and len(set(scopes)) == len(scopes), 'allowed_scopes_invalid') + # KeyCape has no per-client group gate; the consumer reads groups, and UserInfo + # returns them only for the groups scope. + require(not params.get('access_groups') or 'groups' in scopes, 'access_groups_require_groups_scope') require(params['client_type'] in ('confidential', 'public'), 'client_type_invalid') # The type derives custody at platform/workloads/keycape/; a case cannot choose it. require('secret_ref' not in params, 'secret_ref_is_derived_by_the_type') diff --git a/sso-mfa/k8s/keycape/test_keycape_client.py b/sso-mfa/k8s/keycape/test_keycape_client.py index 1cf3d62..d20523d 100644 --- a/sso-mfa/k8s/keycape/test_keycape_client.py +++ b/sso-mfa/k8s/keycape/test_keycape_client.py @@ -84,6 +84,18 @@ class ClientRecordTests(unittest.TestCase): with self.assertRaises(kc.Refused): kc.client_from_case({'spec': {'type': 'other@1.0', 'parameters': FORGEJO_PARAMS}}) + def test_redirects_keycape_would_refuse_at_startup_are_refused(self): + # key-cape type review R1: a refused entry stops the single replica. + for bad in ['https://forgejo.example/cb?x=1', 'https://forgejo.example/cb#frag']: + with self.subTest(redirect=bad), self.assertRaises(kc.Refused): + kc.client_from_params(dict(FORGEJO_PARAMS, redirect_uris=bad)) + + def test_access_groups_require_groups_scope(self): + # key-cape type review R2: UserInfo returns groups only for that scope. + with self.assertRaises(kc.Refused): + kc.client_from_params(dict(FORGEJO_PARAMS, access_groups='forge-admins', + allowed_scopes='openid,profile,email')) + class MergeTests(unittest.TestCase): def test_add_appends_only_this_client_and_preserves_bytes(self): diff --git a/workplans/NK-WP-0048-security-case-types.md b/workplans/NK-WP-0048-security-case-types.md index 1e28ab0..ff0802f 100644 --- a/workplans/NK-WP-0048-security-case-types.md +++ b/workplans/NK-WP-0048-security-case-types.md @@ -388,6 +388,25 @@ register-client session. `forgejo/netkingdom-oidc-client` proposal is withdrawn. This matches the type and the Forgejo example; no change needed here. +2026-10-10: key-cape's type-composition review (key-cape 6038483, message +`8e5ed5f6`) found the composition sound with two blocking findings, taken as a +1.0 patch (stricter validation and documentation, canon section 6): +R1, redirect URIs now refuse `?` and `#` in the type pattern and in +`keycape-client.py`'s `REDIRECT` (KeyCape refuses `?` at startup and the single +replica would stop); R2, `allowed_scopes` always includes `groups`, the renderer +refuses `access_groups` without it, and `access_groups` now says the consumer +enforces the gate because KeyCape has none; R3, `mfa_required` "false" means the +per-user privacyIDEA policy; R4, register-client's justification records the +restart for every client, the quiet window, and that the rendered registry must +pass KeyCape's client validation before it. The accepted Forgejo case and both +examples still validate; 116 tests pass. +Open, needs a decision: R2's verification step that the consumer enforces the +group gate is a new verification step, hence 1.1 (minor), and the validator pins +cases to the exact type version, so the accepted 1.0 Forgejo case would need +re-pinning. Also open with key-cape: a KeyCape entry point that validates a +rendered client registry without starting the server; until then +`keycape-client.py` mirrors the redirect rule. + ```task id: NK-WP-0048-T07 status: wait