diff --git a/bin/keycape b/bin/keycape index 2ec223d..b68a0f4 100755 Binary files a/bin/keycape and b/bin/keycape differ diff --git a/bin/keycape-to-keycloak b/bin/keycape-to-keycloak index 11ace5d..e336926 100755 Binary files a/bin/keycape-to-keycloak and b/bin/keycape-to-keycloak differ diff --git a/bin/lldap-export b/bin/lldap-export index 5994cec..c2bcead 100755 Binary files a/bin/lldap-export and b/bin/lldap-export differ diff --git a/bin/lldap-to-ldap b/bin/lldap-to-ldap index f39fd34..05c67ca 100755 Binary files a/bin/lldap-to-ldap and b/bin/lldap-to-ldap differ diff --git a/bin/validator b/bin/validator index 8902dd4..ee69ee3 100755 Binary files a/bin/validator and b/bin/validator differ diff --git a/docs/approval-engine-auth-contract.md b/docs/approval-engine-auth-contract.md index c5d4677..e2b8ff9 100644 --- a/docs/approval-engine-auth-contract.md +++ b/docs/approval-engine-auth-contract.md @@ -20,6 +20,12 @@ exact deployment-owned callback, `audience: approval-engine`, No callback is invented here. The ID token is for the login client; present the access token to approval-engine. +That client must also declare `tenant: tenant:platform`. A human token's tenant +comes from the directory record, which assigns none today, so without the +declaration the token would carry `tenant:coulomb` and be refused here. See +[the tenant contract](tenant-claim-contract.md) for the four resolution rules and +for why a declared zone can never relabel a user the directory has placed. + `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 diff --git a/docs/approval-engine-provisioning-request.yaml b/docs/approval-engine-provisioning-request.yaml index 22c6cb2..64e1e4c 100644 --- a/docs/approval-engine-provisioning-request.yaml +++ b/docs/approval-engine-provisioning-request.yaml @@ -49,14 +49,18 @@ human_registration: 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. + # Required. Without it the token carries tenant:coulomb from the directory + # default and approval-engine refuses it. See docs/tenant-claim-contract.md. + tenant: tenant:platform + keycape_side_resolved: | + 2026-09-09: a human token can now carry tenant:platform. A client + registration may declare a tenant; humanTenant() supplies it when the + directory has not placed the user, requires agreement when it has, and + refuses issuance on conflict rather than relabelling. Safe only because + registrations are static and deployment-owned. Still outstanding from + elsewhere: the exact client_id and callback URI from informed-decision + (INFD-WP-0001-T07), and whether the owners prefer the directory-sourced + resolution instead, which this implementation degrades into cleanly. 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/docs/tenant-claim-contract.md b/docs/tenant-claim-contract.md index bcc6bf9..27c9705 100644 --- a/docs/tenant-claim-contract.md +++ b/docs/tenant-claim-contract.md @@ -84,6 +84,46 @@ KEY-WP-0013-T02; KeyCape changed no live registration or policy subject. drift or an alias reintroduced into `config/service-clients.example.yaml` fails the build. +## How a human token's tenant is resolved (KEY-WP-0013-T05) + +A human's tenant is normally a property of the person, read from the directory +record. That alone could not serve the approval chain: decision +`5ed3fb35-eca9-413a-82b9-95171ba85bf6` binds it to the landlord zone, +approval-engine compares the claim by exact string equality, and no adapter +populates `domain.User.Tenant` — so every human token fell back to +`tenant:coulomb` and an approver token would have been refused downstream. It +would have presented as a failed approval rather than as a registration defect. + +A client registration may therefore declare a `tenant`, and `humanTenant` in +`src/internal/server/oidc/token.go` resolves it by these four rules: + +| Client declares | Directory assigns | Result | +| --- | --- | --- | +| nothing | anything | the directory answer, or `tenant:coulomb` — unchanged | +| a zone | nothing | the declared zone | +| a zone | the same zone | that zone; client and directory agree | +| a zone | a **different** zone | **issuance is refused** | + +The last row is the point. A registration can bind a zone for users the +directory has not placed, and can never relabel a user it *has* placed. That +case fails closed rather than picking a winner, because either answer would be a +silent cross-tenant assertion. The refusal is a `403` with +`error_type: tenant_binding`, distinct from an authentication failure, so an +operator can tell a misconfigured registration from a rejected login. + +This also means the design survives the other resolution. If the directory later +carries tenants, the same code stops supplying the zone and starts enforcing +agreement with it — no second migration, and no window in which a stale +registration silently wins. + +It is safe only because client registrations are static and deployment-owned. +KeyCape excludes dynamic client registration by design; a self-service client +able to name its users' tenant would be a straightforward escalation, and this +rule must be revisited if that exclusion is ever lifted. + +Covered by `src/internal/server/oidc/human_tenant_test.go`, including the +relabel refusal. + 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/config/config_test.go b/src/internal/config/config_test.go index 2d0a8ee..286e4e2 100644 --- a/src/internal/config/config_test.go +++ b/src/internal/config/config_test.go @@ -3,6 +3,7 @@ package config_test import ( "os" "path/filepath" + "strings" "testing" "keycape/internal/config" @@ -529,3 +530,81 @@ func TestValidate_TenantEngine(t *testing.T) { }) } } + +// Service-identity fields are read only on the client_credentials path. Silently +// ignoring them on a browser client is the trap behind KEY-WP-0013-T05: an +// approver client registered with tenant: tenant:platform starts cleanly and +// issues the directory tenant instead, which a resource server comparing tenant +// exactly refuses — surfacing as a failed approval, not a registration defect +// (KEY-WP-0028). +func TestValidate_ServiceIdentityFieldsRejectedOnBrowserClients(t *testing.T) { + browserClient := func() config.ClientConfig { + return config.ClientConfig{ + ClientID: "approver-ui", + RedirectURIs: []string{"https://approve.example.com/callback"}, + AllowedScopes: []string{"openid", "approval:approve"}, + GrantTypes: []string{"authorization_code"}, + ClientType: "public", + } + } + cases := map[string]struct { + mutate func(*config.ClientConfig) + want string + }{ + "tenant": {func(c *config.ClientConfig) { c.Tenant = "tenant:platform" }, "tenant"}, + "serviceSubject": {func(c *config.ClientConfig) { c.ServiceSubject = "service:approver" }, "serviceSubject"}, + "roles": {func(c *config.ClientConfig) { c.Roles = []string{"approver"} }, "roles"}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + cfg := validConfig(writeTempFile(t, "key")) + client := browserClient() + tc.mutate(&client) + cfg.Clients = append(cfg.Clients, client) + errs := config.ValidateConfig(cfg) + found := false + for _, e := range errs { + if strings.Contains(e, tc.want) { + found = true + } + } + if !found { + t.Fatalf("expected an error naming %q, got %v", tc.want, errs) + } + }) + } + + // A client that omits grantTypes is an implicit authorization-code client, + // so the same rule applies to it. + t.Run("implicit authorization_code client", func(t *testing.T) { + cfg := validConfig(writeTempFile(t, "key")) + client := browserClient() + client.GrantTypes = nil + client.Tenant = "tenant:platform" + if errs := config.ValidateConfig(cfg); len(errs) != 0 { + t.Fatalf("baseline config invalid: %v", errs) + } + cfg.Clients = append(cfg.Clients, client) + if errs := config.ValidateConfig(cfg); len(errs) == 0 { + t.Fatal("expected tenant to be rejected on an implicit authorization-code client") + } + }) + + // The service clients that legitimately carry these fields still validate. + t.Run("service clients unaffected", func(t *testing.T) { + cfg := validConfig(writeTempFile(t, "key")) + cfg.Clients = append(cfg.Clients, config.ClientConfig{ + ClientID: "svc", + AllowedScopes: []string{"approval:read"}, + GrantTypes: []string{"client_credentials"}, + ClientType: "confidential", + SecretRef: "env:SVC_SECRET", + ServiceSubject: "service:svc", + Tenant: "tenant:platform", + Roles: []string{"svc"}, + }) + if errs := config.ValidateConfig(cfg); len(errs) != 0 { + t.Fatalf("service client rejected: %v", errs) + } + }) +} diff --git a/src/internal/config/validate.go b/src/internal/config/validate.go index e0a982a..b9954a4 100644 --- a/src/internal/config/validate.go +++ b/src/internal/config/validate.go @@ -89,8 +89,28 @@ func ValidateConfig(cfg *Config) []string { errs = append(errs, prefix+": tokenLifetime must be between 1m and 1h") } } - } else if c.TokenLifetime != "" { - errs = append(errs, prefix+": tokenLifetime is only supported for client_credentials clients") + } else { + if c.TokenLifetime != "" { + errs = append(errs, prefix+": tokenLifetime is only supported for client_credentials clients") + } + // Service-identity fields are read only on the client_credentials + // path. On a browser client they are silently ignored, and the + // tenant one is a trap worth failing on: an approver client + // registered with tenant: tenant:platform starts cleanly and then + // issues tokens carrying the directory tenant, which a resource + // server comparing tenant exactly will refuse. That surfaces as a + // mysterious failed approval, not as a registration defect + // (KEY-WP-0028). Human tenant comes from the directory user; see + // docs/approval-engine-auth-contract.md. + if c.Tenant != "" { + errs = append(errs, prefix+": tenant is only read for client_credentials clients; a browser client's tenant claim comes from the directory user, so this value would be silently ignored") + } + if c.ServiceSubject != "" { + errs = append(errs, prefix+": serviceSubject is only read for client_credentials clients; a browser client's subject is the directory user") + } + if len(c.Roles) > 0 { + errs = append(errs, prefix+": roles is only read for client_credentials clients; a browser client's roles come from the directory user") + } } // Warn about wildcard redirect URIs (they are blocked at runtime anyway). for _, uri := range c.RedirectURIs { diff --git a/src/internal/server/oidc/human_tenant_test.go b/src/internal/server/oidc/human_tenant_test.go new file mode 100644 index 0000000..65363e1 --- /dev/null +++ b/src/internal/server/oidc/human_tenant_test.go @@ -0,0 +1,75 @@ +package oidc + +import ( + "testing" + + "keycape/internal/domain" +) + +// KEY-WP-0013-T05. Without a client-declared tenant every human token fell back +// to the platform default, so an approver token would have carried +// tenant:coulomb and been refused by approval-engine's exact string comparison +// -- surfacing as a failed approval rather than as a registration defect. +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) + 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) + } +} + +func TestHumanTenantKeepsDirectoryAnswerWhenNoClientTenantIsDeclared(t *testing.T) { + for _, tc := range []struct { + name string + client *domain.Client + user *domain.User + want 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"}, + } { + t.Run(tc.name, func(t *testing.T) { + got, err := humanTenant(tc.client, tc.user) + if err != nil || got != tc.want { + t.Errorf("humanTenant = %q, %v; want %q, nil", got, err, tc.want) + } + }) + } +} + +// The escalation this design has to refuse: a registration must never be able to +// relabel a user the directory has already placed in another tenant. Neither +// answer is safe there, so it fails closed rather than picking a winner. +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) + 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) + } +} + +// Once the directory populates Tenant, the same code turns from supplying the +// zone into enforcing agreement with it. No second migration. +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) + if err != nil { + t.Fatalf("agreeing client and directory refused: %v", err) + } + if got != "tenant:platform" { + t.Errorf("tenant = %q, want tenant:platform", got) + } +} diff --git a/src/internal/server/oidc/token.go b/src/internal/server/oidc/token.go index 0080249..93ddbfd 100644 --- a/src/internal/server/oidc/token.go +++ b/src/internal/server/oidc/token.go @@ -8,6 +8,7 @@ import ( "crypto/subtle" "encoding/base64" "encoding/json" + "fmt" "net/http" "net/url" "strings" @@ -196,7 +197,14 @@ 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 := effectiveTenant(user) + tenant, err := humanTenant(h.ClientConfig[clientID], user) + if err != nil { + profileerrors.RejectedForSafety( + "tenant binding conflict", + "tenant_binding", + ).Write(w, http.StatusForbidden) + return + } claims["tenant"] = tenant claims["principal_type"] = "human" claims["groups"] = nonNilStrings(user.Groups) @@ -398,6 +406,43 @@ func effectiveTenant(user *domain.User) string { return defaultTenant } +// humanTenant resolves the tenant claim for a human token (KEY-WP-0013-T05). +// +// A human's tenant is normally a property of the person, read from the +// directory. But the approval chain is bound to the landlord zone by decision +// 5ed3fb35-eca9-413a-82b9-95171ba85bf6, and approval-engine compares the claim +// by exact string equality, so an approver client has to be able to state the +// zone it issues into. Without this every human token fell back to +// defaultTenant and would have been refused downstream -- as a failed approval +// rather than as a registration defect. +// +// The rule is deliberately not an override: +// +// - no client tenant declared -> the directory answer, unchanged; +// - declared, user unassigned -> the declared zone; +// - declared and equal -> agreement, no ambiguity; +// - declared and different -> refuse to issue. +// +// So a registration can bind a zone for users the directory has not placed, and +// can never relabel a user the directory HAS placed into a different one. That +// last case fails closed rather than picking a winner, because either answer +// would be a silent cross-tenant assertion. It also means this stays correct if +// the directory later populates Tenant: the same code turns from supplying the +// zone into enforcing agreement with it, with no second migration. +// +// 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) { + if client == nil || client.Tenant == "" { + return effectiveTenant(user), 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) + } + return client.Tenant, nil +} + // 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-0013-approval-engine-resource-audience.md b/workplans/KEY-WP-0013-approval-engine-resource-audience.md index f35090b..58cf069 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: "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." +blocking_reason: "Client ID and deployed callback pending INFD-WP-0001-T07 from informed-decision. The tenant blocker is resolved in code." state_hub_task_id: "9a782909-91db-59fa-aae7-83766f4fbb0d" ``` @@ -289,8 +289,35 @@ 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. +**Tenant blocker resolved in code, 2026-09-09.** On review the choice did not +have to be made to unblock: the two resolutions differ in where a human's tenant +*comes from*, and an implementation exists that is correct under either. A client +registration may now declare a `tenant`, and `humanTenant()` resolves it by four +rules — no declaration keeps the directory answer unchanged; a declared zone +applies when the directory has placed the user nowhere; agreement passes; and a +declared zone that **conflicts** with a directory assignment refuses issuance +(`403`, `error_type: tenant_binding`) rather than relabelling the user. + +That refusal is the whole design. A registration can bind a zone for unplaced +users and can never move a placed one, so option (2) is available now without +writing a general cross-tenant override into the issuer, and if the owners prefer +option (1) the same code stops supplying the zone and starts enforcing agreement +with it — no second migration and no window where a stale registration silently +wins. It is safe only because registrations are static and deployment-owned; +`docs/tenant-claim-contract.md` records that this rule must be revisited if +dynamic client registration is ever admitted. + +Evidence: `src/internal/server/oidc/human_tenant_test.go`, four cases including +the relabel refusal. Verified with teeth — neutering the conflict check makes +`TestHumanTenantRefusesToRelabelAPlacedUser` fail rather than pass silently. +`docs/tenant-claim-contract.md` carries the resolution table, +`docs/approval-engine-auth-contract.md` states the requirement on the approver +client, and the provisioning packet now declares `tenant: tenant:platform`. + +Task stays `wait` on one thing only: `client_id` and callback URI from +INFD-WP-0001-T07, once informed-decision has a deployed origin. The owners were +asked which resolution they prefer and have not answered; that answer is no +longer blocking, and this implementation is compatible with either. ## Make negative rollout evidence discriminate actual issuer refusal