fix(kernel): name Cedar policies in denials, close policy/actor-directory races - #418
Open
rita-aga wants to merge 1 commit into
Open
fix(kernel): name Cedar policies in denials, close policy/actor-directory races#418rita-aga wants to merge 1 commit into
rita-aga wants to merge 1 commit into
Conversation
…tory races
Found while diagnosing why a contributor agent could not learn why it was
denied, and burned ~76 minutes and ~$8 hunting introspection endpoints that do
not exist (ARN-286).
Denials name their source file
A denial read `denied by policy: policy1874, policy1875` — positional ids that
map to nothing, so neither an agent nor a human can tell WHICH rule fired.
Policies loaded from an app bundle now carry the label
`{app}/{path under policies/}#{n}`, so the same denial reads
`katagami-commons/art_style.cedar#2`. The durable `policies` row id is the same
string, so a denial recovered after a restart still names the file rather than
reverting to a position. The pre-ARN-286 row id is deleted once the policy has
been re-saved under its label — fatally, not warn-only: a surviving legacy row
is loaded again on the next recovery, bringing the OLD generation of those
statements back alongside the new one, and with forbid-overrides-permit that
silently reactivates a forbid the operator believes they deleted.
Policy reload was read-then-write
Reading a tenant's sources, compiling, and swapping them in were three separate
steps, so two concurrent rebuilds both read the same pre-state, both wrote, and
the first caller's policies were gone — with both calls returning Ok.
`update_tenant_policy_sources` now does the whole read-modify-write under the
tenant write lock, and the os-app installer mirrors what the engine actually
installed rather than what it computed before the merge.
Policy load failures no longer fail open
A failed reload after an os-app install was logged and swallowed, leaving the
tenant running on whatever policy set happened to be live. It is now fatal to
the install.
Three maps had to agree about every live actor
The actor registry, the entity index and the last-accessed stamps were mutated
independently, so a failure on a cleanup path could remove an actor from one
and leave it in the others. Rather than patch the four sites that leaked, the
invariant moved into one guarded directory that owns all three and takes the
registry write lock first — the lock everything else already takes first.
Verification: 279 tests pass; build and clippy clean. The only failures in the
workspace are testcontainers tests that need a Docker daemon (not running
here), in temper-actor-runtime, which this branch does not touch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@greptile review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+80
to
+83
| let legacy_id = legacy_os_app_policy_row_id(app_name, &source.relative_path); | ||
| if legacy_id != policy_id { | ||
| policy_store | ||
| .delete_policy(tenant, &legacy_id) |
There was a problem hiding this comment.
When two policy sources share the same lossy legacy slug, this cleanup deletes the row by tenant and derived ID without validating its source or owner, causing an unrelated durable permit or forbid to disappear after recovery.
How this was verified: Both dots and path separators collapse to -, while deletion uses only the resulting tenant-scoped policy ID.
Knowledge Base Used: temper-authz: Cedar-Based Authorization for Agents
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/temper-platform/src/os_apps/policy_rows.rs
Line: 80-83
Comment:
**Legacy cleanup deletes colliding policies**
When two policy sources share the same lossy legacy slug, this cleanup deletes the row by tenant and derived ID without validating its source or owner, causing an unrelated durable permit or forbid to disappear after recovery.
**How this was verified:** Both dots and path separators collapse to `-`, while deletion uses only the resulting tenant-scoped policy ID.
**Knowledge Base Used:** [temper-authz: Cedar-Based Authorization for Agents](https://app.greptile.com/arni-labs/-/custom-context/knowledge-base/nerdsane/temper/-/docs/temper-authz.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
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
1 participant
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.
Found while diagnosing why a contributor agent could not learn why it had been denied — it burned ~76 minutes and ~$8 hunting introspection endpoints that do not exist (ARN-286).
Denials name their source file
A denial read
denied by policy: policy1874, policy1875— positional ids that map to nothing, so neither an agent nor a human can tell which rule fired.Policies loaded from an app bundle now carry the label
{app}/{path under policies/}, so the same denial readskatagami-commons/art_style.cedar#2. The durablepoliciesrow id is that same string, so a denial recovered after a restart still names the file rather than reverting to a position.The pre-ARN-286 row id is deleted once the policy has been re-saved under its label — fatally, not warn-only. A surviving legacy row is loaded again on the next recovery, bringing the OLD generation of those statements back alongside the new one; with Cedar's forbid-overrides-permit that silently reactivates a forbid the operator believes they deleted. Stale authorization restored by a restart, with no signal at install time, is not something to log and continue past.
Policy reload was read-then-write
Reading a tenant's sources, compiling them, and swapping them in were three separate steps. Two concurrent rebuilds both read the same pre-state, both wrote, and the first caller's policies were gone — with both calls returning
Ok.update_tenant_policy_sourcesnow performs the whole read-modify-write while holding the tenant write lock, and the os-app installer mirrors what the engine actually installed rather than what it computed before the merge.Policy load failures no longer fail open
A failed reload after an os-app install was logged and swallowed, leaving the tenant running on whatever policy set happened to be live. It is now fatal to the install.
Three maps had to agree about every live actor
The actor registry, the entity index, and the last-accessed stamps were mutated independently, so a failure on a cleanup path could remove an actor from one and leave it in the others. Rather than patch the four sites that leaked, the invariant moved into one guarded directory that owns all three and takes the registry write lock first — the lock everything else already takes first.
Also in here
// TEMP probe: lock acquisition removedthat had disabled per-tenant install serialization on this branch.#[cfg(test)]modules fromcell.rs,recovery.rs, anddispatch/mod.rsinto*_test.rssiblings, and renamedengine/tests.rstoengine_test.rs. This is what the readability ratchet is asking for — production files shrink, tests are excluded by design. The ratchet is now better than baseline:PROD_FILES_GT50083 vs 84.Verification
cargo build --workspaceclean.cargo clippy --workspace --all-targets -- -D warningsclean (the CI gate — an earlier run without-D warningshid atype_complexityerror).temper serveagainstkatagami-commons/specs, created anArtStyleas anagent_type=contributorprincipal, attemptedSubmitForReview— correctly denied.One honest gap. That live denial read
primary#5, primary#4, notkatagami-commons/art_style.cedar#2. The labelling is on the os-app install path, which is what production uses and which is covered by tests asserting the exact string (including after a reload).temper serve --apptakes a different path that flattens the policies into one blob before they reach the engine. So this is fixed and tested for the production path, but not demonstrated end-to-end against a running server — that needs a Genesis-backed install. Filed as ARN-314 with the code path and a verification recipe. I am not claiming the live run proves the headline fix; it does not.Unrelated flakes that gate this repo's pre-push hook are filed as ARN-323.
🤖 Generated with Claude Code
Greptile Summary
The PR gives Cedar statements stable source-file labels, serializes tenant policy rebuilds, makes policy activation failures fatal, and centralizes actor registry/index/access bookkeeping. It also improves authorization error classification, entity-initialization failure propagation, OData key parsing, metrics, recovery, and associated tests.
Confidence Score: 3/5
The PR should not merge until legacy policy cleanup verifies ownership or otherwise avoids deleting colliding policy IDs.
A valid app installation can derive the same lossy legacy key as another durable policy and delete that row, potentially removing an authorization forbid or permit during recovery.
Files Needing Attention: crates/temper-platform/src/os_apps/policy_rows.rs
Security Review
App-policy migration cleanup derives a lossy legacy ID and deletes that row without validating ownership, allowing an installation to remove an unrelated durable Cedar policy after an ID collision.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Installer participant PolicyStore participant AuthzEngine participant Recovery Installer->>PolicyStore: Save app/path.cedar Installer->>PolicyStore: Delete derived legacy slug Note over PolicyStore: Lossy slug may identify another policy Installer->>AuthzEngine: Atomically merge labelled sources Recovery->>PolicyStore: Load durable policy rows PolicyStore-->>Recovery: Remaining rows Recovery->>AuthzEngine: Activate recovered labelled policiesPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(kernel): name Cedar policies in deni..." | Re-trigger Greptile
Context used (4)