Repository navigation
Conversation
…ations The directory had no top-level document. The new README says what the directory holds, then states what we expect from each maintained Guardian reference implementation. Four open pull requests add or rework a Guardian, and each read the one-sentence porting issues differently. The expectations let every implementation be reviewed against the same list. The expectations are a working aid for review. They are not part of the specification and they are not normative. Part of GenAI-Security-Project#252 Signed-off-by: Evgeny Kokuykin <artmaro@gmail.com>
github-actions
Bot
added
the
status:needs-triage
This comment has been minimized.
This comment has been minimized.
…n bytes The introduction now says only what the directory is. The table of contents and the status line are gone, because both would go stale as implementations land. Key material is at least 32 bytes, not 32 characters. The specification and every implementation count bytes. Part of GenAI-Security-Project#252 Signed-off-by: Evgeny Kokuykin <artmaro@gmail.com>
acs-board-reconciler
Bot
moved this from Needs triage
to In progress
in ACS Project Tracker
rocklambros
removed
the
status:needs-triage
rocklambros
left a comment
rocklambros
left a comment
There was a problem hiding this comment.
@artmaro, a few fixes before we merge:
- Lines 49 and 51 don't match the TypeScript Guardian (PR #229). It sets
key_idto the session id, notdefault, and it base64-decodes the secret instead of using the raw string. A Go or Rust Guardian built to this README won't verify against the TypeScript one. Please change both lines to match what PR #229 does. - Line 23 says we maintain a contract test suite, but we don't have one yet. Please change it to "will maintain". Line 79 depends on the same suite, so please reword it too.
- Line 35 should name the runtime:
agent_control_specification, at the version inagt.lock. AGT's Go module is a different policy engine, so "the AGT runtime" is ambiguous for Go. - Lines 69-75 read as rules the TypeScript Guardian already meets, but its logs aren't bounded yet. Please mark them as targets.
On your questions: I'd list only TypeScript as maintained for now, and add the others as they merge. Yes to the AGT rule and to moving the shims.
- Name the policy runtime, agent_control_specification, at the version in agt.lock, and say that AGT's Go module does not qualify. - Set key_id to the session_id and base64-decode the key material, as the TypeScript Guardian in GenAI-Security-Project#229 does. - List only TypeScript as maintained. Other languages are added as they merge. - Say that the contract test suite will exist, and make verification depend on it. - Mark bounded processing and bounded logging as targets the TypeScript Guardian does not meet yet. - Add message signing to the architectural boundaries. Part of GenAI-Security-Project#252 Signed-off-by: Evgeny Kokuykin <artmaro@gmail.com>
|
Thanks for reviewing, @rocklambros. All points are addressed in the latest commit:
|
|
@artmaro, thanks for turning these around. One correction before this goes further, and it's on me for not flagging it sooner.
So could you make that line say |
The embedded schemas follow integration after GenAI-Security-Project#63, GenAI-Security-Project#258 and GenAI-Security-Project#261. The parity recording stores the git object IDs of the deciding TypeScript paths instead of a commit, so a squash merge no longer makes TestRecordingIsCurrent fail. The paths include agt.lock and the agt-bridge package.json, which pin the AGT commit and SDK. The recorder and the test read them from one list; the test fails when the recording names other paths or was made from another corpus, and the recorder refuses uncommitted changes to them. A test checks that the Go workflow runs on every deciding path. agt/README.md tells a TypeScript contributor when to re-record. A test requires agt-native.lock and agt.lock to pin the same AGT repo and commit. The native build fails when AGT at that commit changes ffi.rs, which agteval/acs.h is written from, or fetches another opa than agt-native.lock pins. Under the agt evaluator, policy.opa_timeout must be shorter than protocol.decision_timeout; guardian.yaml ships 4s. Otherwise a slow opa lets the agent stop waiting before AGT's deny arrives (GenAI-Security-Project#269). make load measures either evaluator under concurrent clients, selected by EVALUATOR like the other targets. Under agt each decision runs one opa process; on a 20-core host decisions stop rising near 16 concurrent ones, and at 256 they reach the 5s timeout. guardian.yaml now ships limits.max_engine_calls 32, and docs/load.md records the results. The load run found that an engine call gave its slot back only after its answer was sent, so a client's next step could be refused engine_saturated with no call running; callEngine now releases the slot first, also after a panic. When the host adapter's audit log cannot be written, the record goes to stderr as one JSON line with the final disposition, and the refusal's reasoning says whether the record went to stderr or nowhere. The audit file's close error now counts. The Codex and OpenCode hooks pass their own stderr. The §6.3 row says that ALLOW at a hook that permits no DENY is an interpretation (GenAI-Security-Project#270), and the docs say only the adapter library reaches those hooks. Dependabot proposes Go updates, one pull request per dependency across both modules. The Go workflow runs weekly, so govulncheck sees advisories for the toolchain that Dependabot does not move, triggers on the agt-bridge package.json, and no longer fetches full history. A test checks that the method table permits DENY exactly where hooks.md does, and fails on a Decision line it cannot classify. The coverage floor counts every package except commands, examples and test helpers. The Guardian meets the expectations GenAI-Security-Project#256 sets for reference implementations, mapped to tests in docs/conformance.md. audit.max_file_bytes and audit.max_files rotate each audit file; guardian.yaml keeps at most 1 GiB per log; max_file_bytes needs max_files of at least 1, so a rotation never deletes the live log. make load can set the rotation and fails if the log drops, splits or reorders a record or exceeds its bound; docs/load.md records a run and the cost of long sessions. storage.provider names the session store. The HMAC secret file holds base64 of at least 32 bytes. key_id is a configured label with no default, an unknown key_id is refused (GenAI-Security-Project#118, GenAI-Security-Project#272), and the hooks and the conformance harness take it as a flag. A toolCallResult whose request_id_ref matches no request is recorded (GenAI-Security-Project#272). Decisions from other pull requests that the Go code contradicted: - A shell command's URL ends only at ASCII whitespace; U+FEFF or U+00A0 in the userinfo no longer hides the host curl reaches (GenAI-Security-Project#268). - Versions are compatible when they share X.Y under 0.x; the handshake negotiates only such a version, and a step must match it (GenAI-Security-Project#196, GenAI-Security-Project#255). - A policy target that is not a string is denied evaluation_failed; the TypeScript fix is in GenAI-Security-Project#229, so two recorded cases are listed as ahead of the recording until it is re-recorded. - The host adapter records every decision failure under either posture, with its class, handshake failures included (GenAI-Security-Project#37). - The host adapter denies under any posture a signed error other than -32603, an unverified answer and a malformed answer; the docs mark this as an interpretation of §6.4 ahead of GenAI-Security-Project#32, which is open. The version rule is marked the same way against §3. - The audit directory is 0700; a symlinked audit path or an audit file open to group or others is refused (GenAI-Security-Project#71). Claude-Session: https://claude.ai/code/session_01DFdYnoBitHhPcUSq7wMWs1 Signed-off-by: Konstantin Trunin <konstantin@trunin.com>
|
@artmaro, one more thing on the signing section, and it folds into the Here's the exact wording from §10 so you can match it in one pass ( The bit I don't think anyone's caught: line 49 describes the HKDF derivation but never says what Honestly, I'd just link §10 for this whole block and list only what a reviewer checks on the PR, rather than restate the crypto here. We watched the restatement drift once already. If it says "sign and verify per §10" it can't fall out of sync the next time §10 moves. The HKDF params and the base64 decode are both fine, so nothing to change there. |
Review on this pull request asked to link section 10 rather than restate the HMAC key derivation and the meaning of key_id, so the README cannot drift when section 10 changes. GenAI-Security-Project#272 adds those rules to section 10. The section keeps only what section 10 does not define: how key material is encoded and read, that it stays out of code and logs, and that each implementation documents how to configure and replace it. Part of GenAI-Security-Project#252 Signed-off-by: Evgeny Kokuykin <artmaro@gmail.com>
…rchitecture-concept Signed-off-by: Evgeny Kokuykin <artmaro@gmail.com>
|
Thank you, @rocklambros, agreed that's a better shape for this section. The signing section now links §10 instead of restating it, so the HKDF clause and the We can merge this PR after #272. Please take a final look. Thanks |
This branch has not been deployed
2 participants
What changed
The README says what we expect from each maintained Guardian reference implementation. The expectations cover architecture, message signing, logging, concurrency, use of the AGT runtime, and verification.
Which issue does this implement
Closes #252
Base branch
integration, because this touches the specification, schemas, a referenceimplementation, an adapter, tests, or CI
main, because every changed path is on the documentation lane allowlist inCONTRIBUTING.md
Type of change
What I would like reviewers to weigh in on
key_id. The specification does not say whatkey_idholds for HMAC. The document proposes a key name, withdefaultas the default.I tested this
uv run pytest -vpasses on my machineuv run mkdocs build --strictpasses on my machineChecklist
git commit -s(required by the DCO)Security