diff --git a/docs/approval-engine-auth-contract.md b/docs/approval-engine-auth-contract.md index a78b962..c5d4677 100644 --- a/docs/approval-engine-auth-contract.md +++ b/docs/approval-engine-auth-contract.md @@ -15,9 +15,43 @@ with RS256 and publishes its public key through `/jwks`. Human approvers need a separate authorization-code/PKCE registration with an exact deployment-owned callback, `audience: approval-engine`, -`allowedScopes: [openid, approval:approve]`, and `mfaRequired: true`. Do not add -consume or other approval grants to that client. No callback is invented here. -The ID token is for the login client; present the access token to approval-engine. +`allowedScopes: [openid, approval:read, approval:approve]`, and +`mfaRequired: true`. Do not add consume or other approval grants to that client. +No callback is invented here. The ID token is for the login client; present the +access token to approval-engine. + +`approval:read` is present because approval-engine showed the surface cannot +render a decision without it: `GET /v1/approvals/{id}` and `/claim` both require +it, so the earlier `[openid, approval:approve]` would have let an approver submit +an entry they were never able to display. Reading through the owning component's +own service identity would also work, but it weakens the one claim that surface +exists to make — evidence of what *this person* was shown — so the read is +granted to the human principal instead. `approval:consume` stays excluded: human +principals are refused consume in approval-engine's code regardless, and +consumption belongs to the PEP causing the side effect. + +### The assurance object + +KeyCape emits `assurance` on every human token, unscoped. approval-engine stores +it verbatim into the approval entry, where it is the only place `mfaRequired: +true` survives into the record, so its shape is a contract: + +| Field | Type | Meaning | +| --- | --- | --- | +| `level` | string | `aal1` or `aal2`. `aal2` exactly when MFA was verified in this authorization. The closest thing to `acr`. | +| `methods` | string[] | `["pwd"]`, or `["pwd","otp"]` when MFA was verified. The closest thing to `amr`. | +| `mfa` | bool | Whether MFA was verified. Redundant with `level` by construction, and kept because a consumer asserting on one should not have to know the mapping. | +| `source` | string | Always `key-cape`. Names which issuer made the assertion. | +| `at` | number | Unix seconds at which the user **authenticated** — not when the token was minted. | + +`at` is authentication time on purpose. A reused browser session can be hours +old, and a record saying MFA happened at mint time would overstate how recently +the person proved anything. Where an authorization rides an existing session, the +original login instant is carried through. + +The level is derived from what happened in *this* authorization, never from +enrollment state: a user with MFA enrolled who was not challenged gets `aal1`. +A consumer that needs a maximum age should compare `at`, not assume freshness. These fragments are not live registrations. The two service registrations require custody-managed values for the named environment references and a reviewed diff --git a/docs/approval-engine-provisioning-request.yaml b/docs/approval-engine-provisioning-request.yaml index e5b58ff..22c6cb2 100644 --- a/docs/approval-engine-provisioning-request.yaml +++ b/docs/approval-engine-provisioning-request.yaml @@ -36,10 +36,27 @@ requests: human_registration: status: awaiting-exact-callback blocks_service_client_rollout: false - owner: unassigned-approver-ui - scopes: [openid, approval:approve] + # Owner identified 2026-09-09: informed-decision (hub repo cf4c7da8). It + # supplies client_id and callback URI from INFD-WP-0001-T07 once it has a + # deployed origin. approval-engine is a bearer-only resource server and never + # owned those strings. + owner: informed-decision + # approval:read added 2026-09-09 on approval-engine's finding: the surface + # cannot render a decision without GET /v1/approvals/{id} and /claim, so the + # earlier scope set let an approver submit what they could not display. + scopes: [openid, approval:read, approval:approve] mfa_required: true client_type: public + grant: authorization_code + S256 PKCE + never: [approval:consume] + blocked_on_keycape_side: | + A human access token cannot carry tenant:platform today. The tenant claim on + a human token is resolved from the directory user (effectiveTenant in + src/internal/server/oidc/token.go), not from the client registration, and no + adapter populates User.Tenant -- so every human token defaults to + tenant:coulomb. approval-engine compares tenant by exact string equality and + refuses near-miss spellings, so an approver token would be rejected. This + must be resolved before the registration is issued, not after. verification: # The first two lines are now one runnable command per client; see # docs/native-authentication.md, "Verifying a live registration". It writes diff --git a/src/internal/server/oidc/assurance_time_test.go b/src/internal/server/oidc/assurance_time_test.go new file mode 100644 index 0000000..72ad579 --- /dev/null +++ b/src/internal/server/oidc/assurance_time_test.go @@ -0,0 +1,54 @@ +package oidc + +import ( + "testing" + "time" +) + +// approval-engine persists the assurance object verbatim and it is the only +// downstream evidence that MFA happened (KEY-WP-0013-T05). A reused browser +// session can be hours old, so reporting mint time would overstate how +// recently the person actually proved anything. +func TestAssuranceReportsAuthenticationTimeNotMintTime(t *testing.T) { + authenticated := time.Now().Add(-4 * time.Hour) + minted := time.Now() + + claim := assuranceClaim(true, authenticated, minted) + if got := claim["at"].(int64); got != authenticated.Unix() { + t.Errorf("assurance.at = %d, want the authentication time %d", got, authenticated.Unix()) + } + if claim["level"] != "aal2" || claim["mfa"] != true { + t.Errorf("MFA-verified authorization did not report aal2: %v", claim) + } +} + +// A session stored before AuthTime existed has a zero value; falling back to +// mint time keeps the claim present rather than emitting a 1970 timestamp. +func TestAssuranceFallsBackToMintTimeWhenAuthTimeIsUnset(t *testing.T) { + minted := time.Now() + claim := assuranceClaim(false, time.Time{}, minted) + if got := claim["at"].(int64); got != minted.Unix() { + t.Errorf("assurance.at = %d, want the mint-time fallback %d", got, minted.Unix()) + } + if claim["level"] != "aal1" || claim["mfa"] != false { + t.Errorf("unverified authorization did not report aal1: %v", claim) + } +} + +// The shape approval-engine and informed-decision consume. A missing key here +// breaks a downstream record that cannot be reconstructed later. +func TestAssuranceCarriesTheDocumentedShape(t *testing.T) { + claim := assuranceClaim(true, time.Now(), time.Now()) + for _, key := range []string{"level", "methods", "mfa", "source", "at"} { + if _, ok := claim[key]; !ok { + t.Errorf("assurance object is missing %q: %v", key, claim) + } + } + methods, ok := claim["methods"].([]string) + if !ok || len(methods) != 2 || methods[0] != "pwd" || methods[1] != "otp" { + t.Errorf("aal2 methods = %v, want [pwd otp]", claim["methods"]) + } + if claim["source"] != "key-cape" { + t.Errorf("source = %v, want key-cape", claim["source"]) + } +} diff --git a/src/internal/server/oidc/authorize.go b/src/internal/server/oidc/authorize.go index 1fe5a37..322cdce 100644 --- a/src/internal/server/oidc/authorize.go +++ b/src/internal/server/oidc/authorize.go @@ -317,7 +317,8 @@ func (h *AuthorizeHandler) ServeHTTPCallback(w http.ResponseWriter, r *http.Requ return } - decision, err := h.decideAssurance(ctx, ps, result.Username, h.Logins.fromRequest(r)) + existingLogin := h.Logins.fromRequest(r) + decision, err := h.decideAssurance(ctx, ps, result.Username, existingLogin) if err != nil { h.Emitter.Emit(ctx, telemetry.Event{ Timestamp: time.Now(), @@ -441,6 +442,15 @@ func (h *AuthorizeHandler) completeAuthorization(w http.ResponseWriter, r *http. if mfaVerified { level = domain.AssuranceAAL2 } + + // When an existing browser session for this same user carried the + // authorization, the authentication happened when that session was + // issued, not now. Creating the new login session below resets IssuedAt, + // so the original instant has to be read before that. + authTime := time.Now() + if prior := h.Logins.fromRequest(r); prior != nil && prior.Username == username && !prior.IssuedAt.IsZero() { + authTime = prior.IssuedAt + } if login := h.Logins.Create(username, level); login != nil { writeLoginCookie(w, login, issuerIsHTTPS(h.Issuer)) } @@ -457,6 +467,7 @@ func (h *AuthorizeHandler) completeAuthorization(w http.ResponseWriter, r *http. Scopes: ps.Scopes, ExpiresAt: time.Now().Add(10 * time.Minute), MFAVerified: mfaVerified, + AuthTime: authTime, } authCode := h.Sessions.Create(sess) diff --git a/src/internal/server/oidc/session.go b/src/internal/server/oidc/session.go index 6fc9eb8..15d759c 100644 --- a/src/internal/server/oidc/session.go +++ b/src/internal/server/oidc/session.go @@ -26,6 +26,13 @@ type PKCESession struct { // completeAuthorization -- never re-derived from stale enrollment state // at token-exchange time. MFAVerified bool + // AuthTime is when the user actually authenticated, which is not the + // same instant the token is minted: a reused browser session can be + // hours old. approval-engine persists the assurance object verbatim as + // the only downstream evidence that MFA happened (KEY-WP-0013-T05), so + // reporting mint time there would misdate that evidence. Zero means + // "authenticated during this authorization". + AuthTime time.Time } // SessionStore is an in-memory PKCE session store. diff --git a/src/internal/server/oidc/token.go b/src/internal/server/oidc/token.go index 0f9fcba..0080249 100644 --- a/src/internal/server/oidc/token.go +++ b/src/internal/server/oidc/token.go @@ -201,7 +201,7 @@ func (h *TokenHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { claims["principal_type"] = "human" claims["groups"] = nonNilStrings(user.Groups) claims["roles"] = nonNilStrings(user.Roles) - claims["assurance"] = assuranceClaim(sess.MFAVerified, now) + claims["assurance"] = assuranceClaim(sess.MFAVerified, sess.AuthTime, now) // Optional cached tenant_roles claim (KEY-WP-0005-T02). Fails open -- // see internal/adapters/tenantengine's package doc for why this is the @@ -412,7 +412,18 @@ func nonNilStrings(s []string) []string { // was actually verified during this authorization (session.MFAVerified), // not from static enrollment state -- a user who has MFA enrolled but // wasn't challenged for it in this particular flow gets aal1, not aal2. -func assuranceClaim(mfaVerified bool, at time.Time) map[string]interface{} { +// +// `at` is the time the user authenticated, not the time this token was +// minted. Those differ whenever a browser session is reused, and the gap is +// the whole point: approval-engine stores this object verbatim as the only +// downstream record that MFA occurred (KEY-WP-0013-T05), so mint time would +// overstate how recently the person proved anything. issuedAt is the +// fallback for a session predating this field. +func assuranceClaim(mfaVerified bool, authTime, issuedAt time.Time) map[string]interface{} { + at := authTime + if at.IsZero() { + at = issuedAt + } level := "aal1" methods := []string{"pwd"} if mfaVerified { diff --git a/workplans/KEY-WP-0013-approval-engine-resource-audience.md b/workplans/KEY-WP-0013-approval-engine-resource-audience.md index 4e35ec8..f35090b 100644 --- a/workplans/KEY-WP-0013-approval-engine-resource-audience.md +++ b/workplans/KEY-WP-0013-approval-engine-resource-audience.md @@ -230,7 +230,7 @@ id: KEY-WP-0013-T05 status: wait priority: high assignee: the-custodian -blocking_reason: "An actual approver UI owner, client ID and deployed callback are not yet supplied. approval-engine is a bearer-only resource server." +blocking_reason: "Owner found 2026-09-09 (informed-decision); client ID and deployed callback still pending INFD-WP-0001-T07, and a human token cannot carry tenant:platform yet." state_hub_task_id: "9a782909-91db-59fa-aae7-83766f4fbb0d" ``` @@ -240,6 +240,58 @@ Require public S256 PKCE, exact redirect, MFA, approval:approve without consume, and a real human access-token proof. No service credential substitutes for this. HFACT-WP-0001-T03 consumes the acceptance where a human approval is required. +2026-09-09. The owner exists: `informed-decision` (hub repo cf4c7da8) claims the +approver surface and owns the two strings. approval-engine was right that a +bearer-only resource server never owned them. Both wrote in asking to be told +early about problems with the registration shape rather than at handover, so +their requested shape was checked against the source rather than agreed on +paper. Three results. + +**Scope gap — accepted, theirs was right.** `[openid, approval:approve]` cannot +render a decision: `GET /v1/approvals/{id}` and `/claim` both need +`approval:read`, so the surface could submit an entry it was never able to +display. Published `[openid, approval:read, approval:approve]` in +`docs/approval-engine-auth-contract.md` and the provisioning packet. Reading +through a service identity was the alternative and is worse: it weakens the +evidence-of-what-this-person-saw claim the component exists to make. +`approval:consume` stays excluded. + +**Assurance shape — published, and a defect fixed.** Both asked for a documented +shape; KeyCape already emitted one, so it is written down rather than +renegotiated: `level` (aal1|aal2, ~acr), `methods` (~amr), `mfa`, `source`, `at`. +Writing it down surfaced that `at` was the token **mint** time, not the +authentication time. Those differ by hours whenever a browser session is reused, +and approval-engine persists this object verbatim as the only downstream record +that MFA happened — so a stored approval could have evidenced MFA at a moment the +person proved nothing. `PKCESession.AuthTime` now carries the original login +instant through session reuse, with mint time as the fallback for sessions +predating the field. Covered by `src/internal/server/oidc/assurance_time_test.go`. + +**Blocker found before anyone built on it — a human token cannot carry +`tenant:platform`.** `effectiveTenant` resolves the human tenant from the +directory user, not the client registration, and no adapter populates +`domain.User.Tenant`, so every human token defaults to `tenant:coulomb`. The +per-client `tenant` field that carries `tenant:platform` on the two service +clients is read only on the `client_credentials` path. approval-engine compares +tenant by exact string equality and pins near-miss spellings as refused, so an +approver token issued today would be rejected — and it would present as a failed +approval, not as a registration defect. + +Two resolutions, sent to both owners (`67132ddc`, `a6a070a1`, pointer +`42d435e8`) rather than chosen here, because the choice decides what a human's +tenant *means*: (1) directory-sourced, approvers carry `tenant:platform` on their +LLDAP record — keeps tenant a property of the person, needs a directory attribute +and an owner, and changes those users everywhere; (2) registration-bound and +fail-closed, symmetric with the service clients and with decision +`5ed3fb35-eca9-413a-82b9-95171ba85bf6` — defensible only because registrations +are static and deployment-owned, since dynamic registration is excluded by +design. KeyCape leans to (2) and deliberately implemented neither: (1) is a +directory-ownership question and (2) writes a cross-tenant capability into the +issuer. + +Task stays `wait` on the tenant decision, then `client_id` and callback URI from +INFD-WP-0001-T07 once that repo has a deployed origin. + ## Make negative rollout evidence discriminate actual issuer refusal ```task diff --git a/workplans/KEY-WP-0014-native-credential-lane-handoff.md b/workplans/KEY-WP-0014-native-credential-lane-handoff.md index c099525..eaf1f2c 100644 --- a/workplans/KEY-WP-0014-native-credential-lane-handoff.md +++ b/workplans/KEY-WP-0014-native-credential-lane-handoff.md @@ -159,3 +159,56 @@ Both catalog corrections were sent, receipts readable via half-true and should say so precisely rather than be flipped either way. Neither message asks for a route change and neither was one. + +2026-09-08/09 — ops-warden answered both, and the second answer matters more than +the question did. + +**Item 1 answered: option (a).** Keep the proxy as-is and run `keycape login` +alongside it, "for your reason rather than out of caution -- they are different +credential types with different verifiers, so this is not a cutover deferred, it +is two things that were never one." They verified our pointer-layer reading +rather than accepting it, and added a fact we did not have: `warden access` +already excludes `is_login` lanes from raw-value streaming +(WARDEN-WP-0032-T05). Lane `key-cape-oidc-login` moved to `owner-confirmed` with +our boundary written in verbatim (commit c133004). It stays `interim` on purpose: +per their ADR-0003 a lane retires when a front door exists, not when ownership is +agreed, and by our own analysis `keycape login` is not that front door. The entry +now names the retirement condition instead of implying we owe one. + +**Item 2 answered: nobody owns it, admittedly.** Steps 1-2 mutate custody on +platform workload paths, which is railiance-platform's, with `secrets-engine exec` +as the admitted transport. ops-warden declined to take the act on the grounds +that fronting the read lane is not authority to mutate — their ADR-0002 and +ADR-0005 — and routed it themselves rather than describing the route and leaving +it to us. Lane `rapp-qonto-keycape-client` narrowed exactly as asked and moved +`source-read` -> `owner-confirmed`, crediting the correction to us rather than to +a re-read they did not perform. Rotation step 3 now points at +`keycape verify-client` instead of restating its assertions. + +**They found a defect in their own front door because of our caution, and told +us.** Running our need through `warden plan` returns `verdict: autonomous`, +`lane_id: rapp-qonto-keycape-client`, answered with three read transports — a +need containing *generate* and *CAS-write* inherits the verdict of the lane that +reads the same path, because `warden plan` has no read-versus-mutate intent at +all. `autonomous` is documented as the signal to proceed without the founder. +Raised as WARDEN-WP-0038. Their stated conclusion, which we should not soften: "A +counterparty declining to trust our output is not a control, and we are not going +to record it as one." + +Standing instruction from them until WP-0038 lands, recorded here so it is not +lost in an inbox: **treat a `warden plan` verdict on any need containing a write, +rotate or provision act as unreliable.** That applies to this repository whenever +it plans a rotation. + +**On `automatable`, they did not do what we suggested and said so.** We asked +them not to flip the boolean and to state the half-truth precisely; the schema has +one boolean whose only consumer is a future executable driver, and a driver told +`true` would attempt the custody steps. So it is `false`, failing safe, with the +precision moved into per-step notes. They offered to add a per-step field if we +would rather. We would not: a lane-level flag that fails safe plus per-step notes +is the right shape, and asking them to change a schema for our convenience would +be worse than the imprecision. No reply needed on that point. + +T04 stays `wait`: the rotation authority now has a named owner +(railiance-platform, transport `secrets-engine exec`) but does not yet exist, and +nothing here changed a route.