From 63d646b594b1b0e5fcbb0bb40469f0ef908ecd3b Mon Sep 17 00:00:00 2001 From: tegwick Date: Thu, 10 Sep 2026 07:58:29 +0200 Subject: [PATCH] Carry the tenant claim's provenance, and correct a guard the ruling voided MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GH-DEC-2026-013 §5 is a finding nobody asked for and ours to implement: tenant is a bare string, so a consumer cannot tell a zone the directory asserted about the person from one a registration supplied about the client they came through. approval-engine exact-matches that string while its contract reads as though it relies on the first -- the check is sound and the property a reader infers from it is absent. gate-house named the property and left the mechanism to us. Every token now carries tenant_source beside tenant: directory, registration or default. Advertised in claims_supported, and asserted at the token level on both grants rather than only in the resolution function, since the claim a consumer reads is the thing under obligation. Three values where the ruling names two, which is the judgement here. Labelling an unasserted profile default as directory would reproduce the same defect one level down -- a consumer reading an assertion the identity layer never made. The ruling cites GH-DEC-2026-011 §3 on unknown versus absent for the case it examined; the same rule applies to our own fallback. The agreement case resolves to directory deliberately: if a registration declares the zone the directory also assigned, the directory did assert it, and reporting the weaker source would understate what is known. Also corrects the guard shipped in 5f516a0. Its failure message offered two ways out of adding dynamic registration, and condition (b) voids the second: admitting dynamic registration voids the registration-bound shape that day, whatever state the adapter is in. The message named an inadmissible resolution in the exact place someone would read it while making that change. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NV9oijZukGyGbRQGGKnK4P Assistant: claude-code Assistant-Model: opus Assistant-Process: 713576@bnt-lap001 Assistant-Session: 384c511d-9bce-4cb8-a676-2aef6c0c8df6 --- docs/tenant-claim-contract.md | 36 ++++++++ src/internal/server/oidc/discovery.go | 2 +- src/internal/server/oidc/discovery_test.go | 7 +- src/internal/server/oidc/human_tenant_test.go | 33 +++++-- .../server/oidc/tenant_precondition_test.go | 87 ++++++++++++++++++- src/internal/server/oidc/token.go | 69 ++++++++++++--- .../KEY-WP-0030-tenant-precondition-guard.md | 55 ++++++++++++ 7 files changed, 262 insertions(+), 27 deletions(-) diff --git a/docs/tenant-claim-contract.md b/docs/tenant-claim-contract.md index 9f0926c..0c917c7 100644 --- a/docs/tenant-claim-contract.md +++ b/docs/tenant-claim-contract.md @@ -170,6 +170,42 @@ two cannot drift apart silently. Covered by `src/internal/server/oidc/human_tenant_test.go`, including the relabel refusal. +### The claim carries its provenance + +GH-DEC-2026-013 §5 found what the rules above leave unsaid: `tenant` is a bare +string, so a consumer cannot tell a zone **the directory asserted about the +person** from one **a registration supplied about the client they came through**. +approval-engine admits an approver by exact-matching that string while its +contract reads as though it relies on the first. The check is sound; the property +a reader infers from it is absent. gate-house named the property and left the +mechanism to us. + +Every token therefore carries `tenant_source` alongside `tenant`: + +| `tenant_source` | Meaning | +| --- | --- | +| `directory` | The identity layer asserted this zone about this person. Includes the agreement case — if a registration declared the same zone the directory did, the directory still asserted it, so the stronger provenance is the true one. | +| `registration` | Supplied by the client registration for a person the directory has placed nowhere. A fact about the client, not about the person. Always the value on service tokens: there is no directory principal behind one. | +| `default` | Nobody asserted a zone. This is the profile's non-empty default. | + +**The third value is not padding.** The ruling names two sources, but the code +has three states, and labelling an unasserted default as `directory` would +reproduce the same defect one level down — a consumer reading an assertion the +identity layer never made. That is the unknown-versus-absent distinction +GH-DEC-2026-011 §3 requires, applied to our own fallback rather than only to the +case we were asked about. + +**For consumers.** Do not treat the three as equivalent for any decision that +turns on a fact about the *person*. `registration` and `default` are not weaker +evidence of the same thing; they are evidence of a different thing. What follows +from that is the consumer's call — gate-house explicitly did **not** rule on +whether approval-engine's exact-match admission is the right gate. Carrying +provenance makes that question answerable; it does not answer it. + +`tenant_source` is advertised in `claims_supported`, and +`TestIssuedTokensCarryTenantProvenance` asserts it reaches issued tokens on both +grants, including the unasserted-default case. + These are local issuance proofs. They are not live-rollout evidence; see `docs/approval-engine-auth-contract.md` and KEY-WP-0013-T02 for the deployment boundary. No token or secret values appear in this document or in test output. diff --git a/src/internal/server/oidc/discovery.go b/src/internal/server/oidc/discovery.go index 596bace..4b0074b 100644 --- a/src/internal/server/oidc/discovery.go +++ b/src/internal/server/oidc/discovery.go @@ -78,7 +78,7 @@ func NewDiscoveryHandler(cfg DiscoveryConfig) http.Handler { // follow them. ClaimsSupported: []string{ "sub", "iss", "aud", "exp", "iat", - "tenant", "principal_type", "roles", "groups", "assurance", "scope", + "tenant", "tenant_source", "principal_type", "roles", "groups", "assurance", "scope", "nonce", "preferred_username", "email", "name", "tenant_roles", }, SubjectTypesSupported: []string{"public"}, diff --git a/src/internal/server/oidc/discovery_test.go b/src/internal/server/oidc/discovery_test.go index 5b7e3eb..f2d069d 100644 --- a/src/internal/server/oidc/discovery_test.go +++ b/src/internal/server/oidc/discovery_test.go @@ -291,7 +291,7 @@ func TestDiscoveryHandler_Claims(t *testing.T) { } doc := discoveryDoc(t, cfg) assertStringSlice(t, doc, "claims_supported", - []string{"sub", "iss", "aud", "exp", "iat", "tenant", "principal_type", "roles", "groups", + []string{"sub", "iss", "aud", "exp", "iat", "tenant", "tenant_source", "principal_type", "roles", "groups", "assurance", "scope", "nonce", "preferred_username", "email", "name", "tenant_roles"}) } @@ -310,7 +310,10 @@ func TestDiscoveryAdvertisesEveryCoreProfileClaim(t *testing.T) { for _, claim := range doc["claims_supported"].([]interface{}) { advertised[claim.(string)] = true } - for _, claim := range []string{"iss", "sub", "aud", "exp", "iat", "tenant", "principal_type", "roles", "groups", "assurance"} { + // tenant_source is core: it is emitted on every token, and a consumer that + // cannot see it cannot tell an asserted zone from a supplied one + // (GH-DEC-2026-013 §5). + for _, claim := range []string{"iss", "sub", "aud", "exp", "iat", "tenant", "tenant_source", "principal_type", "roles", "groups", "assurance"} { if !advertised[claim] { t.Errorf("core profile claim %q is emitted on every token but not advertised", claim) } diff --git a/src/internal/server/oidc/human_tenant_test.go b/src/internal/server/oidc/human_tenant_test.go index 65363e1..187bd99 100644 --- a/src/internal/server/oidc/human_tenant_test.go +++ b/src/internal/server/oidc/human_tenant_test.go @@ -14,13 +14,18 @@ func TestHumanTenantBindsTheZoneAClientDeclares(t *testing.T) { approver := &domain.Client{ClientID: "informed-decision", Tenant: "tenant:platform"} unplaced := &domain.User{ID: "user:alice"} - got, err := humanTenant(approver, unplaced) + got, source, err := humanTenant(approver, unplaced) if err != nil { t.Fatalf("declared zone refused for an unassigned user: %v", err) } if got != "tenant:platform" { t.Errorf("tenant = %q, want tenant:platform", got) } + // This is the case the ruling cares about: a fact about the client, not one + // the identity layer asserted about the person (GH-DEC-2026-013 §5). + if source != TenantSourceRegistration { + t.Errorf("provenance = %q, want %q", source, TenantSourceRegistration) + } } func TestHumanTenantKeepsDirectoryAnswerWhenNoClientTenantIsDeclared(t *testing.T) { @@ -29,16 +34,21 @@ func TestHumanTenantKeepsDirectoryAnswerWhenNoClientTenantIsDeclared(t *testing. client *domain.Client user *domain.User want string + // source is the provenance the claim must carry (GH-DEC-2026-013 §5). + source string }{ - {"no client at all", nil, &domain.User{ID: "u"}, defaultTenant}, - {"client declares nothing", &domain.Client{ClientID: "demo-app"}, &domain.User{ID: "u"}, defaultTenant}, - {"directory answer wins", &domain.Client{ClientID: "demo-app"}, &domain.User{ID: "u", Tenant: "tenant:friendly:binky"}, "tenant:friendly:binky"}, + {"no client at all", nil, &domain.User{ID: "u"}, defaultTenant, TenantSourceDefault}, + {"client declares nothing", &domain.Client{ClientID: "demo-app"}, &domain.User{ID: "u"}, defaultTenant, TenantSourceDefault}, + {"directory answer wins", &domain.Client{ClientID: "demo-app"}, &domain.User{ID: "u", Tenant: "tenant:friendly:binky"}, "tenant:friendly:binky", TenantSourceDirectory}, } { t.Run(tc.name, func(t *testing.T) { - got, err := humanTenant(tc.client, tc.user) + got, source, err := humanTenant(tc.client, tc.user) if err != nil || got != tc.want { t.Errorf("humanTenant = %q, %v; want %q, nil", got, err, tc.want) } + if source != tc.source { + t.Errorf("provenance = %q, want %q", source, tc.source) + } }) } } @@ -50,12 +60,12 @@ func TestHumanTenantRefusesToRelabelAPlacedUser(t *testing.T) { approver := &domain.Client{ClientID: "informed-decision", Tenant: "tenant:platform"} placed := &domain.User{ID: "user:bob", Tenant: "tenant:friendly:binky"} - got, err := humanTenant(approver, placed) + got, source, err := humanTenant(approver, placed) if err == nil { t.Fatalf("client relabelled a placed user into %q", got) } - if got != "" { - t.Errorf("a refused binding still returned a tenant: %q", got) + if got != "" || source != "" { + t.Errorf("a refused binding still returned tenant %q source %q", got, source) } } @@ -65,10 +75,15 @@ func TestHumanTenantAcceptsAgreementBetweenClientAndDirectory(t *testing.T) { approver := &domain.Client{ClientID: "informed-decision", Tenant: "tenant:platform"} placed := &domain.User{ID: "user:carol", Tenant: "tenant:platform"} - got, err := humanTenant(approver, placed) + got, source, err := humanTenant(approver, placed) if err != nil { t.Fatalf("agreeing client and directory refused: %v", err) } + // The directory did assert this zone about this person, so agreement carries + // the stronger provenance, not the registration's. + if source != TenantSourceDirectory { + t.Errorf("agreement provenance = %q, want %q", source, TenantSourceDirectory) + } if got != "tenant:platform" { t.Errorf("tenant = %q, want tenant:platform", got) } diff --git a/src/internal/server/oidc/tenant_precondition_test.go b/src/internal/server/oidc/tenant_precondition_test.go index f5bb870..a955146 100644 --- a/src/internal/server/oidc/tenant_precondition_test.go +++ b/src/internal/server/oidc/tenant_precondition_test.go @@ -22,6 +22,14 @@ import ( // lifted while the capability is present, and its failure message says what to do // about it rather than merely reporting the endpoint. // +// gate-house GH-DEC-2026-013 condition (b) then strengthened the rule against us. +// We had written that it "must be revisited" if the exclusion is lifted; the +// ruling holds that revisited implies the answer might survive review, and it +// would not. Admitting dynamic registration voids the registration-bound shape +// outright. The failure message says that, because the earlier wording offered a +// way out the ruling forbids, in the exact place someone would read it while +// making the change. +// // The two halves are asserted together on purpose. Whichever is removed first, // the test points at the other. @@ -59,8 +67,81 @@ func TestRegistrationBoundTenantRequiresStaticRegistration(t *testing.T) { t.Fatal("dynamic client registration is advertised while a client registration " + "may declare its users' tenant. That combination lets anyone who can register " + "a client relabel the users who log in through it into a zone of their choosing. " + - "Resolve it deliberately: either drop the registration-bound tenant, or gate it " + - "so only statically configured registrations may declare one. See " + - "docs/tenant-claim-contract.md, 'How a human token's tenant is resolved'.") + "Per gate-house GH-DEC-2026-013 condition (b) this is not a trade-off to " + + "rebalance: admitting dynamic registration VOIDS the registration-bound tenant " + + "that day, and the directory becomes the only source, whatever state the " + + "directory adapter is in. Gating the capability to statically configured " + + "registrations is not an available answer. Remove the client-declared tenant, " + + "or do not admit dynamic registration. See docs/tenant-claim-contract.md, " + + "'How a human token's tenant is resolved'.") } } + +// GH-DEC-2026-013 §5: the claim must carry its provenance, so a consumer can +// tell a zone the identity layer asserted about the person from one a +// registration supplied about the client they came through. The unit rule is +// covered in human_tenant_test.go; this asserts the claim actually reaches +// issued tokens, on both grants, which is what a consumer reads. +func TestIssuedTokensCarryTenantProvenance(t *testing.T) { + t.Run("human, registration-supplied", func(t *testing.T) { + sessions := oidc.NewSessionStore() + h, _ := newTokenHandler(t, sessions, &mockUserRepo{users: map[string]*domain.User{"alice": aliceUser()}}) + h.ClientConfig["test-client"].Tenant = "tenant:platform" + claims := issueHumanToken(t, h, sessions) + if claims["tenant"] != "tenant:platform" || claims["tenant_source"] != oidc.TenantSourceRegistration { + t.Fatalf("tenant %v / source %v", claims["tenant"], claims["tenant_source"]) + } + }) + + t.Run("human, directory-asserted", func(t *testing.T) { + user := aliceUser() + user.Tenant = "tenant:friendly:binky" + sessions := oidc.NewSessionStore() + h, _ := newTokenHandler(t, sessions, &mockUserRepo{users: map[string]*domain.User{"alice": user}}) + claims := issueHumanToken(t, h, sessions) + if claims["tenant"] != "tenant:friendly:binky" || claims["tenant_source"] != oidc.TenantSourceDirectory { + t.Fatalf("tenant %v / source %v", claims["tenant"], claims["tenant_source"]) + } + }) + + // Nobody asserted a zone. Reporting "directory" here would be the same defect + // one level down: a consumer reading an assertion the directory never made. + t.Run("human, nobody asserted", func(t *testing.T) { + sessions := oidc.NewSessionStore() + h, _ := newTokenHandler(t, sessions, &mockUserRepo{users: map[string]*domain.User{"alice": aliceUser()}}) + claims := issueHumanToken(t, h, sessions) + if claims["tenant_source"] != oidc.TenantSourceDefault { + t.Fatalf("source %v, want %v", claims["tenant_source"], oidc.TenantSourceDefault) + } + }) + + t.Run("service is always registration-supplied", func(t *testing.T) { + h := serviceTokenHandler(t) + req := tokenRequest(url.Values{"grant_type": {"client_credentials"}, "scope": {"finance.qonto.read"}}) + req.SetBasicAuth("rapp-qonto", "test-service-secret") + w := httptest.NewRecorder() + h.ServeHTTP(w, req) + if w.Code != http.StatusOK { + t.Fatalf("status %d", w.Code) + } + claims := parseJWTPayload(t, decodeTokenResponse(t, w.Body.String())["access_token"].(string)) + if claims["tenant_source"] != oidc.TenantSourceRegistration { + t.Fatalf("source %v, want %v", claims["tenant_source"], oidc.TenantSourceRegistration) + } + }) +} + +func issueHumanToken(t *testing.T, h *oidc.TokenHandler, sessions *oidc.SessionStore) map[string]interface{} { + t.Helper() + verifier := "test-verifier" + code := seededSession(sessions, verifier) + w := httptest.NewRecorder() + h.ServeHTTP(w, codeExchange(t, url.Values{ + "grant_type": {"authorization_code"}, "client_id": {"test-client"}, + "code": {code}, "code_verifier": {verifier}, "redirect_uri": {seededRedirectURI}, + })) + if w.Code != http.StatusOK { + t.Fatalf("status %d: %s", w.Code, w.Body.String()) + } + return parseJWTPayload(t, decodeTokenResponse(t, w.Body.String())["access_token"].(string)) +} diff --git a/src/internal/server/oidc/token.go b/src/internal/server/oidc/token.go index 93ddbfd..b1d7f57 100644 --- a/src/internal/server/oidc/token.go +++ b/src/internal/server/oidc/token.go @@ -197,7 +197,7 @@ func (h *TokenHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { // Core claims required by net-kingdom/canon/standards/iam-profile_v0.3.md // for every production token -- not scope-gated, unlike the recommended // human claims above (KEY-WP-0005-T01). - tenant, err := humanTenant(h.ClientConfig[clientID], user) + tenant, tenantSource, err := humanTenant(h.ClientConfig[clientID], user) if err != nil { profileerrors.RejectedForSafety( "tenant binding conflict", @@ -206,6 +206,7 @@ func (h *TokenHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } claims["tenant"] = tenant + claims["tenant_source"] = tenantSource claims["principal_type"] = "human" claims["groups"] = nonNilStrings(user.Groups) claims["roles"] = nonNilStrings(user.Roles) @@ -335,12 +336,15 @@ func (h *TokenHandler) serveClientCredentials(w http.ResponseWriter, r *http.Req tokenLifetime = client.TokenLifetime } claims := map[string]interface{}{ - "iss": h.Issuer, - "sub": client.ServiceSubject, - "aud": accessAudience(client), - "exp": now.Add(tokenLifetime).Unix(), - "iat": now.Unix(), - "tenant": client.Tenant, + "iss": h.Issuer, + "sub": client.ServiceSubject, + "aud": accessAudience(client), + "exp": now.Add(tokenLifetime).Unix(), + "iat": now.Unix(), + "tenant": client.Tenant, + // A service client's tenant is always registration-supplied: there is no + // directory principal behind it to assert one (GH-DEC-2026-013 §5). + "tenant_source": TenantSourceRegistration, "principal_type": "service", "groups": []string{}, "roles": nonNilStrings(client.Roles), @@ -433,16 +437,57 @@ func effectiveTenant(user *domain.User) string { // This is only safe because client registrations are static and // deployment-owned; KeyCape excludes dynamic client registration by design. A // self-service client that could name its users' tenant would be an escalation. -func humanTenant(client *domain.Client, user *domain.User) (string, error) { +func humanTenant(client *domain.Client, user *domain.User) (string, string, error) { if client == nil || client.Tenant == "" { - return effectiveTenant(user), nil + if user.Tenant != "" { + return user.Tenant, TenantSourceDirectory, nil + } + return defaultTenant, TenantSourceDefault, nil } - if user.Tenant != "" && user.Tenant != client.Tenant { - return "", fmt.Errorf("client %q binds tenant %q but the directory assigns this user a different tenant", client.ClientID, client.Tenant) + if user.Tenant != "" { + if user.Tenant != client.Tenant { + return "", "", fmt.Errorf("client %q binds tenant %q but the directory assigns this user a different tenant", client.ClientID, client.Tenant) + } + // Agreement: the directory did assert this about the person, so the + // stronger provenance is the true one. + return user.Tenant, TenantSourceDirectory, nil } - return client.Tenant, nil + return client.Tenant, TenantSourceRegistration, nil } +// Tenant provenance values for the tenant_source claim (GH-DEC-2026-013 §5). +// +// A bare tenant string cannot tell a consumer whether the identity layer +// asserted the zone about this PERSON or a registration supplied it about the +// CLIENT they came through. approval-engine admits an approver by exact-matching +// that string while its contract reads as though it relies on the first, so the +// check is sound and the property a reader infers from it is absent. The ruling +// requires the claim to carry its provenance and forbids a consumer treating the +// two as equivalent for any decision turning on a fact about the person. It names +// the property; the field is ours. +// +// Three values, not the two the ruling names, and the third is the point. A +// tenant nobody asserted -- neither directory nor registration, just the +// profile's non-empty default -- is not directory-asserted, and labelling it so +// would reintroduce the same defect one level down: a consumer would read +// "directory" for a fact the directory never stated. That is the unknown-versus- +// absent distinction the ruling cites from GH-DEC-2026-011 §3, applied to our own +// fallback rather than only to the case we were asked about. +const ( + // TenantSourceDirectory: the identity layer asserted this zone about this + // person. Includes the agreement case, where a registration declared the + // same zone the directory did -- the directory still asserted it. + TenantSourceDirectory = "directory" + + // TenantSourceRegistration: supplied by the client registration for a person + // the directory has placed nowhere. A fact about the client, not the person. + TenantSourceRegistration = "registration" + + // TenantSourceDefault: nobody asserted a zone; this is the profile default, + // emitted because the profile requires a non-empty tenant. Weaker than both. + TenantSourceDefault = "default" +) + // nonNilStrings returns s, or an empty (non-nil) slice if s is nil, so the // claim always serializes as `[]`, never `null` -- the profile requires // groups/roles to be present, "possibly empty", not absent. diff --git a/workplans/KEY-WP-0030-tenant-precondition-guard.md b/workplans/KEY-WP-0030-tenant-precondition-guard.md index 583961a..798c606 100644 --- a/workplans/KEY-WP-0030-tenant-precondition-guard.md +++ b/workplans/KEY-WP-0030-tenant-precondition-guard.md @@ -9,6 +9,7 @@ owner: claude topic_slug: tenant-precondition-guard created: "2026-09-09" updated: "2026-09-09" +state_hub_workstream_id: "9ed8f851-080a-5d15-8642-f799c2e3295c" --- `informed-decision` replied on KEY-WP-0013-T05 and asked for one thing that is @@ -27,6 +28,7 @@ test about discovery metadata, not a warning about relabelling users. id: KEY-WP-0030-T01 status: done priority: medium +state_hub_task_id: "4729d591-39d4-5f33-bef8-db8057090913" ``` `TestRegistrationBoundTenantRequiresStaticRegistration` asserts both halves @@ -49,12 +51,65 @@ This is deliberately not a vote on the tenant question. It makes the preconditio of one option checkable; it does not choose between them, and if option 1 lands the capability and this guard are removed together. +## Carry the tenant claim's provenance + +```task +id: KEY-WP-0030-T03 +status: done +priority: high +``` + +gate-house ruled on GH-DEC-2026-013 while this was in flight. Section 5 is a +finding nobody asked for and is ours to implement: `tenant` is a bare string, so +a consumer cannot tell a zone the directory asserted about the person from one a +registration supplied about the client they came through. approval-engine +exact-matches that string while its contract reads as though it relies on the +first — the check is sound and the property a reader infers from it is absent. + +Every token now carries `tenant_source` beside `tenant`: `directory`, +`registration`, or `default`. It is advertised in `claims_supported` and asserted +on both grants at the token level, not only in the resolution function, because +the claim a consumer reads is the thing under obligation. + +Three values where the ruling names two, and that is the substantive judgement +here. Labelling an unasserted profile default as `directory` would reproduce the +same defect one level down — a consumer reading an assertion the identity layer +never made. The ruling cites GH-DEC-2026-011 §3 on unknown versus absent for the +case it examined; the same rule applies to our own fallback, so the fallback is +named rather than folded into the strongest neighbouring value. + +The agreement case resolves to `directory` deliberately: if a registration +declares the zone the directory also assigned, the directory did assert it, and +reporting the weaker source would understate what is known. + +## Condition (b) corrected a message I had just shipped + +```task +id: KEY-WP-0030-T04 +status: done +priority: high +``` + +The guard committed in `5f516a0` told a future reader that adding dynamic +registration could be resolved by dropping the capability **or gating it to +statically configured registrations**. Condition (b) voids the second: admitting +dynamic registration voids the registration-bound shape that day, and the +directory becomes the only source whatever state the adapter is in. gate-house +strengthened our own "must be revisited" on the grounds that *revisited* implies +the answer might survive review, and it would not. + +So the message offered a way out the ruling forbids, in the exact place someone +would read it while making that change. Corrected the same day. Worth recording +rather than quietly editing: the guard was written to force a confrontation, and +a guard that suggests an inadmissible resolution is worse than none. + ## What stays with the owners ```task id: KEY-WP-0030-T02 status: done priority: medium +state_hub_task_id: "32b10327-f55d-5a0e-a4bc-be92929469dc" ``` `informed-decision` prefers option 2 and explicitly declines to treat that as the