Answer the approver-client questions, and fix what checking them turned up
All checks were successful
Build and Publish Container Image / build-and-push (push) Successful in 41s
All checks were successful
Build and Publish Container Image / build-and-push (push) Successful in 41s
informed-decision and approval-engine both asked to hear problems with the human
approver registration now rather than at handover. Checking their requested shape
against the source rather than agreeing it on paper turned up three things.
Scope gap, accepted: [openid, approval:approve] cannot render a decision, since
GET /v1/approvals/{id} and /claim both need approval:read -- the surface could
submit an entry it was never able to display. Published
[openid, approval:read, approval:approve]. 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
and KeyCape already emitted one, so it is written down rather than renegotiated.
Writing it down surfaced that `at` was the token mint time rather than 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.
Blocker found before anyone built on it: a human token cannot carry
tenant:platform. effectiveTenant resolves the human tenant from the directory
user, no adapter populates User.Tenant, and the per-client tenant field is read
only on the client_credentials path -- so every human token defaults to
tenant:coulomb, which approval-engine refuses by exact string equality. It would
have presented as a failed approval rather than a registration defect. Two
resolutions sent to the owners and neither implemented here: the choice decides
whether a human's tenant is a property of the person or of the registration, and
that is not KeyCape's alone to make.
Also recorded ops-warden's answers to KEY-WP-0014-T04, including their finding
that `warden plan` returns `autonomous` for a need containing generate and
CAS-write, because it has no read-versus-mutate intent. Their standing
instruction -- treat a warden plan verdict on any write, rotate or provision need
as unreliable until WARDEN-WP-0038 lands -- is recorded in the workplan rather
than left in an inbox.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016uV8zoCKpA1WRAxsKRYbdH
Assistant: claude-code
Assistant-Model: opus
Assistant-Process: 1182213@bnt-lap001
Assistant-Session: 966597b9-ae61-46a4-8b9e-1594ab3ec4ad
This commit is contained in:
parent
0f5535eed9
commit
a73da29093
8 changed files with 248 additions and 9 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
54
src/internal/server/oidc/assurance_time_test.go
Normal file
54
src/internal/server/oidc/assurance_time_test.go
Normal file
|
|
@ -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"])
|
||||
}
|
||||
}
|
||||
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue