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