Repository navigation
policy: Holds, for an allowance on what a call says - #191
Closed
kam-validmind wants to merge 2 commits into
Closed
kam-validmind wants to merge 2 commits into
kam-validmind wants to merge 2 commits into
Conversation
Evaluate decides a call as sent and twice cleaned and keeps the strictest answer, which makes a deny stricter: /tmp/../etc/passwd matches a deny on prefix /etc/ once cleaned. An embedder that lets a person allow calls by what they say needs the mirror image, and had to copy the cleaning to get it: Agent Authority's session approvals covered /tmp/../etc/passwd under "args.path prefix /tmp/" because the as-sent spelling matched. Holds(when, call) reports whether every condition holds in every pass, so such an allowance covers /tmp/a/../b.log and not /tmp/../etc/passwd nor /tmp/%2E%2E/etc/passwd; a spelling that fails as sent (/TMP/x) is never allowed. No new cleaning: the engine's three cleaners, required together.
Contributor
Author
|
Closing: Agent Authority is dropping conditions from session approvals (see validmind/agent-authority#161), so no embedder needs the allow-side check. The unit-tests workflow fix in this PR (run on pkg/** too) is worth landing on its own; separate PR to follow. |
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.
What this adds, in one paragraph
A new exported function,
policy.Holds(when, call), that reports whether a set of conditions holds for a call in every pass the engine takes: as the agent sent it, with the text cleaned up, and with paths cleaned too.Evaluatealready does all that cleaning, but uses it to make a deny stricter. An embedder that lets a person allow calls by what they say needs the opposite, and until now had to copy the engine's cleaning to get it. Agent Authority's session approvals are that embedder (validmind/agent-authority#161 carries the copy this replaces).Why
Evaluatealone isn't enough for an allowEvaluatedecides a call three times, as sent and twice cleaned, and keeps the strictest answer. That is exactly right for a deny: if/tmp/../etc/passwdmatchesprefix /etc/once cleaned, the call is denied, however it was spelled.An allow is the mirror image. If a person says "allow writes under
/tmp/", then/tmp/../etc/passwdmatches as sent but names/etc/passwdonce cleaned. The right answer is "not allowed", and no singleEvaluatecall gives it: an allow rule's match in the cleaned pass is never stricter than the as-sent result, so the engine simply reports the as-sent match.flowchart LR subgraph deny["Evaluate (a deny): one pass is enough"] A1["as sent: no match"] --> R1["deny"] A2["cleaned: matches"] --> R1 end subgraph allow["Holds (an allowance): every pass must hold"] B1["as sent: holds"] --> R2["not allowed"] B2["cleaned: fails"] --> R2 endWhat
HoldssaysCondition:
args.path prefix /tmp/.Holds/tmp/build/out.log/tmp/a/../b.log/tmp/b.logholds/tmp/100%2Efile/tmp/100.fileholds/tmp/../etc/passwd/etc/passwdfails/tmp/%2E%2E/etc/passwd/etc/passwdfails/tmp/../etc/passwd/etc/passwdfails/TMP/xThe same holds for
eq,containsandin, for nested arguments, and for a command that contains a path (cat /tmp/../etc/passwddoes not satisfycontains /tmp/). Facts are compared as they are, as inEvaluate.What changed
pkg/policy/holds.goHoldsrunscondMatcheswith the engine's three cleaners (nil,plainText,canonicalText) and requires every condition to hold in each. Eleven lines; no new cleaning logic.pkg/policy/holds_test.goHoldsnever allows a spelling that a deny on the same condition would not catch.CHANGELOG.md.github/workflows/unit-tests.ymlpkg/**too. It listed onlycmd/**andinternal/**, so a change to the policy engine did not run the unit tests in CI.No behaviour of
Evaluatechanges. Nothing in the CLI or UI changes.How I validated it
go test ./pkg/policyand the fullgo test ./....gofmt,go vet.Coversimplemented aspolicy.Holds(a local Go workspace), the session-approval property test there ("an allowance on a prefix never covers a call whose path resolves outside it") passes. Two of its example rows read differently: AA's interim resolver treats/tmp/..<zero-width space>/etc/passwdand full-width dots as literal file names and covers them;Holdsdrops the invisible character and folds the full-width dots, reads both as/etc/passwd, and holds the call. The stricter reading is the one AA will adopt, so those two rows were dropped from its table.Follow-ups
review.SessionApproval.Coversbecomes a call topolicy.Holdsandinternal/review/resolve.gois deleted (markedponytailthere).cleanPathreads//./at the start of a POSIX path as a Windows device prefix and turns the path relative, so//./etc/passwdescapes aprefix /etc/deny. Found by the same property tests; I'll open it on its own.For the reviewer
pkg/policy/holds.gois the whole change. The doc comment is the contract.holds_test.gohas the table as executable examples;TestHoldsAgreesWithEvaluateis the safety argument.