From 655d8d31b356bd6f1e9cb7fb21d52007f78c5a2a Mon Sep 17 00:00:00 2001 From: tegwick Date: Sat, 31 Oct 2026 23:02:54 +0100 Subject: [PATCH] Review oidc-service-client 0.1 against KeyCape's client field rules Answers net-kingdom's NK-WP-0056-T14 review request (message dc359034). Field rules confirmed: the rendered record passes keycape check-config, and the token shape was observed on a local synthetic run (assurance is a fixed aal1/client_secret block). One correction: an excess scope returns invalid_profile_usage (HTTP 400, feature scope), not invalid_scope. Notes on the bind-env restart and its Secret precondition under Recreate, and on the rotation window. Co-Authored-By: Claude Opus 5.5 Assistant: claude-code Assistant-Model: opus Assistant-Process: 5502@bnt-lap001 Assistant-Session: 68b3b02f-5d0f-4e71-8421-796f62e50196 --- docs/oidc-service-client-type-review.md | 146 ++++++++++++++++++++++++ 1 file changed, 146 insertions(+) create mode 100644 docs/oidc-service-client-type-review.md diff --git a/docs/oidc-service-client-type-review.md b/docs/oidc-service-client-type-review.md new file mode 100644 index 0000000..27a14dc --- /dev/null +++ b/docs/oidc-service-client-type-review.md @@ -0,0 +1,146 @@ +# Review: SecurityCaseType oidc-service-client 0.1 + +Reviewed 2026-10-31 by key-cape, the owner of the client field rules, at net-kingdom's +request (NK-WP-0056-T14, message `dc359034`, hub decision `f8044f81`). Sources: +net-kingdom `security-case-types/oidc-service-client/type.yaml`, its README, the +example case `examples/nk-bind-executor-approval.yaml` and the renderer +`sso-mfa/k8s/keycape/keycape-client.py` (`service_client_from_params`), all at +net-kingdom `5d5f15d`. Compared with KeyCape `src/internal/config/validate.go`, +`src/internal/config/config.go`, `src/internal/domain/model.go`, +`src/internal/server/oidc/token.go` and `src/cmd/keycape/{main,checkconfig}.go` +at key-cape `a140a4a`. + +**Verdict: the field rules are correct; one correction to the type (R3) and two +notes for its attended steps (R4, R5).** No finding blocks KeyCape from starting. + +## How this was checked (no live system) + +- The renderer turned the example case into a client record. That record went into + a copy of `config/dev-config.yaml` as the only client. A locally built `keycape + check-config` accepted it (`valid: true`, `required_env: + [KEYCAPE_NK_BIND_EXECUTOR_APPROVAL_CLIENT_SECRET]`). With `--env-names` missing + that name, check-config refused it. +- The renderer reproduces `config/service-clients.example.yaml`'s + `secrets-engine-approval` record exactly from the equivalent parameters. +- A local KeyCape ran on the dev config with a synthetic secret in the env + variable. Three client_credentials exchanges were made against it. The results + are in R2 and R3. +- The same binary, started without the env variable, exited with `client + "nk-bind-executor-approval" secret environment variable ... is empty` / `failed to + build client registry`. + +## Findings + +**R1: the rendered client record matches KeyCape's model and passes check-config.** +(confirmed) +For the example case the renderer emits `clientId`, `displayName`, `audience`, +`allowedScopes` (a list), `grantTypes: [client_credentials]`, `clientType: +confidential`, `secretRef: env:KEYCAPE_NK_BIND_EXECUTOR_APPROVAL_CLIENT_SECRET`, +`serviceSubject`, `tenant`, `roles` (a list, omitted when empty) and `tokenLifetime`. +Every key is a field of `config.ClientConfig`. KeyCape requires these for +client_credentials: confidential, an `env:` secretRef, a non-empty `serviceSubject` +and `tenant`, and a lifetime between 1m and 1h. Redirect URIs are not required. The +type is stricter than KeyCape in each respect, which is allowed: +- resource scopes only; +- `service:` and `tenant:` prefixes; +- one audience token without whitespace; +- 5m, 10m or 15m only. +The env name maps `-` to `_` and admits no `_` in `client_id`, so no two ids share an +env name. The pattern `service:[a-z0-9-]+` does not fit two existing clients +(`secrets-engine-requester` uses `secrets-engine`, and `codex-railiance-platform` uses +`service:codex:railiance-platform`). That only matters if those clients are adopted +later. The README already limits the fit claim to `secrets-engine-approval`. + +**R2: shape of a client_credentials token.** (confirmed; this answers the +assurance question) +Observed from the local exchange for the example case: + +```json +{ + "iss": "", "sub": "service:nk-bind-executor", "aud": "approval-engine", + "iat": 1793479725, "exp": 1793480625, + "tenant": "tenant:platform", "tenant_source": "registration", + "principal_type": "service", "groups": [], "roles": ["bounded-executor"], + "scope": "approval:create approval:read approval:consume", + "assurance": {"level": "aal1", "methods": ["client_secret"], "mfa": false, + "source": "key-cape", "at": 1793479725} +} +``` + +- `aud` is a single string: the registered `audience`, or the `client_id` when no + audience is registered. The request cannot select it. +- `scope` is a space-separated string. With no `scope` parameter the token carries + all registered scopes; with one, exactly the requested subset. +- `exp - iat` and the response `expires_in` both equal the client's `tokenLifetime` + (900 for 15m). No refresh token is issued. +- `assurance` is a fixed literal for service tokens: + - `level` is always `aal1` and `methods` always `["client_secret"]`; + - `mfa` is always false; + - `at` is the mint time, since a service has no separate authentication event. + No configuration raises it. approval-engine's `identity_from_claims` needs only an + object here, so the token is admissible. Any approval-engine rule that requires + aal2 will, correctly, never be met by a service token. A consumer must use + `principal_type: service`, not the assurance level, to tell service tokens from + human ones. +- `jti` appears only when token status is enabled. `tenant_roles` appears only when + tenant-engine is configured and answers for the tenant. Neither is guaranteed. + +**R3: an excess scope returns `invalid_profile_usage`, not `invalid_scope`.** +(correction to the type) +KeyCape refuses any requested scope outside `allowedScopes` with HTTP 400 and the +body `{"error":"invalid_profile_usage","description":"requested scope is not +allowed","feature":"scope"}` (`token.go`, `serveClientCredentials`; the response was +observed for `scope=approval:approve`). The verification step `excess-scope-refused` +("KeyCape returns invalid_scope") and the README's threat model ("KeyCape refuses an +excess scope (`invalid_scope`)") name the wrong code. A literal check against them +would fail on a correct refusal. Fix: expect HTTP 400 with `error: +invalid_profile_usage` and `feature: scope`, and correct the README. The refusal +itself is real, and the whole request fails: no token is issued for the allowed +subset. KeyCape follows its profile error taxonomy here rather than RFC 6749's +`invalid_scope`. Changing that is a separate key-cape decision and is not needed for +this type. + +**R4: bind-env restarts KeyCape on its own, and its Secret must exist first.** +(note for the attended steps) +The type's order and its stated reason are correct. KeyCape resolves every `env:` +secretRef at start and exits if one is empty (`buildClientRegistryWith`). Running +register-client before bind-env therefore takes the only issuer down for every +client. The renderer guards against that: `keycape-client.py --check-config` passes +the Deployment's env names to `keycape check-config --env-names`, which refuses a +registry naming an unbound variable. Two points are missing from the attended +justification: +- Patching the Deployment env is itself a rollout. The Deployment has one replica + and `strategy: Recreate`, so bind-env and register-client each cause a restart that + drops in-flight logins for every client. That is two restarts per case unless both + changes go in one rollout. +- The env entry references Secret `sso/keycape--client` with no + `optional`. If deliver-secret is not Ready when bind-env runs, the new pod fails + with CreateContainerConfigError. Under Recreate the old pod is already gone, so + sign-in fails for every client. deliver-secret's ready_check must pass immediately + before bind-env in the same session, not just at some point beforehand. + +**R5: rotation breaks the consumer until KeyCape restarts.** (note on Known +limits) +The README says that a rotation also restarts KeyCape. The order matters: +- The consumer reads the new value from OpenBao on its next call. +- KeyCape keeps comparing against the value in its environment at start, until both + of these happen: ESO refreshes `sso/keycape--client`, and KeyCape + restarts. +- In between, every exchange by the consumer fails with 401 (`invalid client + authentication`). +Rotation should therefore be: mint, wait for the ESO refresh to report Ready, restart +KeyCape, and only then expect the consumer's exchanges to succeed. The window will +show up as consumer errors. KeyCape has no dual-secret overlap. + +## Not a finding + +- Identity scopes (`openid`, `profile`, `email`, `groups`) are accepted by KeyCape on + a client_credentials client, but they add nothing to the token. Excluding them in + the type is correct and costs nothing. +- `service-token-claims` checks sub, aud, tenant, scope and lifetime, and R2 confirms + each. Adding `principal_type: service` to the expected claims would be cheap and + matches what approval-engine checks. +- The client secret format is opaque to KeyCape. KeyCape form-decodes both halves of + `client_secret_basic`, so a base64url secret needs no escaping. +- Rollback order (registration removed before the env binding) mirrors the apply + order correctly. Removing the env first would hit the same start-up exit as in R4.