fix: an API key inherits its identity's credential policy, not the tenant default - #363
Merged
Merged
Conversation
…nant default (#362) POST /api-keys with an identity_id and no credential_policy_id gave the key the tenant default policy, then checked that policy against the identity's. An identity whose policy is stricter than default therefore failed the subset check and could never be issued another key — its first one survives only because registration passes the identity policy directly. RotateKey calls CreateKey the same way, so rotation was broken for those identities too, which matters more: that is the path you are on when a key is already compromised. The comment above the subset check already described the intended behaviour — that a caller who does not set CredentialPolicyID inherits the identity policy. The code could not do it: policy resolution ran before the identity was loaded, so the tenant default was the only option it had. Loading the identity first makes the documented behaviour expressible, and resolution becomes explicit: the caller's policy, else the identity's, else the tenant default when the identity has none. EnsureDefaultPolicy is now called only on that last path. The tenant default is still seeded by identity registration and token issuance, so no tenant is left without one. TestAPIKeyCreate_InheritsIdentityPolicyPasses asserted the old behaviour: its "Path 1" expected the 400, under a name claiming inheritance. It now expects the key to be created, and reads it back through GET /api-keys/{id} to confirm the key is bound to the identity's policy rather than merely allowed through the subset check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
saucam
approved these changes
Sep 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #362.
What was wrong
POST /api-keyswith anidentity_idand nocredential_policy_idassigned the key the tenant default policy, then checked that policy against the identity's. An identity whose policy is stricter than default failed the subset check and could never be issued another key. Its first key survives only because registration passes the identity policy directly.RotateKeycallsCreateKeythe same way, so rotation was broken for those identities too. The issue flagged that as untested — it is real, and it is reachable from the product atPOST /v1/admin/agents/:id/rotate-key. That matters more than creation: rotation is the path you are on when a key is already compromised.Why it happened
The comment above the subset check already described the intended behaviour — that a caller who does not set
CredentialPolicyIDinherits the identity policy. The code could not do it: policy resolution ran before the identity was loaded, so the tenant default was the only option available at that point. The bug was structural, not a typo, which is why a reasonable-lookingif policyID == ""branch had nothing better to choose.Loading the identity first makes the documented behaviour expressible. Resolution is now explicit: the caller's policy, else the identity's, else the tenant default when the identity has none.
Two incidental improvements: the old
if req.IdentityID != ""guard was already dead (ErrIdentityLinkRequiredreturns early andEnsureServiceIdentityfills the field), so it is gone; andEnsureDefaultPolicyis now called only on the last path — the tenant default is still seeded by identity registration (identity.go:1398) and token issuance (credential.go:762), so no tenant is left without one.A test asserted the bug
TestAPIKeyCreate_InheritsIdentityPolicyPasseshad a "Path 1" expecting the400, with a comment reasoning its way there, under a test name claiming inheritance. It now expects the key to be created, and reads it back throughGET /api-keys/{id}to confirm the key is bound to the identity's policy rather than merely allowed through the subset check.Worth a reviewer's eye: if that assertion was deliberate design rather than written to match observed behaviour, this is the line to push back on.
Verified
-race.apikey.gowhile keeping the inverted test reproduces the reported failure exactly —expected: 201 … must inherit the identity's policy, not the broader tenant default.Downstream
highflame-admin#1367 is the same incident from the other side — Admin turned this
400into an opaque500and droppedcredential_policy_id. Fixed in highflame-admin#1369, which also works around this bug by forwarding the identity's own policy id (that takes the subset-check skip path), so Admin does not need to wait on this release.🤖 Generated with Claude Code