Repository navigation
Fix the HMAC key derivation, define key_id, and record an unresolved request_id_ref - #272
rocklambros wants to merge 9 commits into
Conversation
…f resolves Section 10 now fixes the HKDF parameters every implementation already uses: SHA-256, no salt, the UTF-8 session_id as info, and a 32-byte key, with a worked example that a test recomputes. key_id for HMAC is an opaque label the deployment chooses for one configured secret; a verifier rejects an unknown label and never falls back. A Guardian SHOULD resolve request_id_ref and MUST record one it cannot resolve. Signed-off-by: rocklambros <rock@rockcyber.com>
A toolCallResult whose request_id_ref names no toolCallRequest in the session's chain now gets a finding line in the envelope log, which is the Guardian's audit record. The decision is unchanged, because hooks.md leaves denial to policy. The Inspector accepts finding lines and labels them, where before it would have warned and dropped them. Signed-off-by: rocklambros <rock@rockcyber.com>
|
This pull request's description says it closes #118, which is on the project roadmap. When it merges, GitHub closes that issue and the roadmap counts it as delivered. Please confirm the change delivers the whole issue before merging. |
|
@afogel @bar-capsule, I found that the signature section of the spec (Section 10, "Cryptographic Signatures") never says exactly how to derive the HMAC key or what goes in the Two quick asks. Ariel, please check the one sentence I added next to your rule: a Guardian signs its response with the No rush on this one, since it isn't blocking Friday. PR #258 and PR #261 come first. |
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>
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>
|
With |
Signed-off-by: rocklambros <rock@rockcyber.com>
…tion gaps Under HMAC the Observed Agent derives the same per-session key as the Guardian, so it can sign a response the Guardian never sent. Section 8.6 named only the Guardian as a key holder who could forge. It now says a Guardian decision is not attributable to the Guardian against the agent, and the 10.1 HMAC row and the chain_hash description say the same. Section 10 now also says that info is the session_id exactly as sent, that a response uses the key of the request it answers, that the secret is at least 32 random bytes, that the unknown key_id error is unsigned, and that the channel-binding input has no interoperable parameters yet. The schema descriptions point at the spec instead of restating it, and the toolCallResult payload line names request_id_ref, the field the schema has, in place of execution_id. The reference Guardian resolves request_id_ref from a per-session set instead of copying the whole chain on every result, and a store that throws on the lookup costs the record, not the decision. The README lists what the finding still does not cover. Signed-off-by: rocklambros <rock@rockcyber.com>
|
@imran-siddique, you're right, and thanks for catching it. Both sides derive the same key, so the Observed Agent can sign an @bar-capsule, thanks for running the example against PR #22. Your LGTM was a comment, not a GitHub review, so the ruleset doesn't count it. Could you submit an Approve review on 97334be? While I was in there I closed a few more gaps in the same text. All of it is in 97334be:
Some things came up that need a decision from the leads, so they're not in this PR:
pytest 921 passed, @afogel, could you take a look at the |
…e a finding Both shipped hookmaps now name the host's per-call id as call_id (tool_use_id for Claude Code, callID for OpenCode). The shim derives the request's request_id from that id and the session, and puts the same value on the result as request_id_ref, so the Guardian can pair them with no state in the shim. A host that sends no call id gets a random request_id and no reference, as before. A derived id repeats if a host asks twice about one call. A Guardian enforcing Specification 10.3 answers that with REPLAY_DETECTED, which carries no decision and would go to the posture. The shim follows the 10.3 recovery instead: it retries once with a random id and drops the pairing for that call. loadHookmap rejects a malformed call_id, one on a non-tool gate, or one declared on only one of the two gates, so a hookmap typo fails at startup rather than running steps ungoverned. The Guardian writes a finding to stderr when the envelope log is off or has disabled itself, with the same fields plus recorded_at, method and rpc_id, and that write cannot throw. It also records a toolCallRequest whose request_id the session already holds, since it does not enforce 10.3 yet. The READMEs now say what a finding does and does not cover, and drop the claim that the session store never evicts. Signed-off-by: rocklambros <rock@rockcyber.com>
CodeQL flagged SHA-1 over data derived from the session id. Nothing secret depends on it, but RFC 9562 gives a name-based UUIDv8 over SHA-256 for exactly this, so new code carries no SHA-1. The pinned derivation vector changes with it. Signed-off-by: rocklambros <rock@rockcyber.com>
|
@bar-capsule @afogel, this is ready for approval on ff09e7e. Either of you can sign off. Since 97334be I closed the gaps I'd listed as still open. Two of the changes go beyond the spec text, so here's where to look:
I ran a second premortem on these changes before pushing, and it caught the The questions for the leads are issue #304 through issue #308. I also left comments on PR #169 and PR #229 about the All checks are green. |
The code review on ff09e7e found ten issues. All ten are fixed here. A finding now goes onto the step's own hash-chain entry as findings plus the unresolved request_id_ref, and the entry's hash covers both. That chain is the Guardian's audit record, so the hooks.md MUST is met there and not only in a log that can be off. Entries with no findings hash as before. The envelope log and stderr keep a readable copy, and the Inspector shows findings on the chain row. The duplicate check now covers every request id the session carried, not only toolCallRequest ids. loadHookmap refuses a call_id with a reserved segment, and a call_id declared only on result gates or only on request gates. The JSON-RPC id stays random when request_id is derived, so the envelope log never pairs two requests under one id, and the host audit log carries request_id on its own for the OTLP converter. The REPLAY_DETECTED comment and README now say what the retry does: the result still resolves against the first request the host sent for that call. The two name-based UUIDs share one formatter, a helper no longer splits governsTool from its doc comment, the README sentence about the sample names both attributes, and new comments carry no semicolons. Signed-off-by: rocklambros <rock@rockcyber.com>
Folding its formatting into the shared helper put the existing SHA-1 UUIDv5 inside a changed hunk, and CodeQL then reported it as new. The session derivation cannot change, since every derived session id depends on it, so it keeps its own inline formatting and the shared helper serves the SHA-256 UUIDv8 alone. Signed-off-by: rocklambros <rock@rockcyber.com>
|
A code review on ff09e7e found ten issues. They're all fixed in df1ab74 and 825b245, and every check is green.
|
Both name-based derivations now set the version and variant bits in one place. The session id's UUIDv5 still hashes with SHA-1, which is how RFC 9562 defines v5, and the pinned toSessionUuid vector shows the output is unchanged. Signed-off-by: rocklambros <rock@rockcyber.com>
What changed
§10 said the HMAC key is "HKDF-derived" and stopped there, so two implementations could hold the same secret and still fail to verify each other. This PR writes down the parameters all four implementations already use, defines what
key_idmeans for HMAC (Ariel's rule from issue #118), and adds therequest_id_refrule settled on issue #118 in September. The reference Guardian now follows that rule.Which issue does this implement
Closes #118
meaning, so it needs no issue
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
Specification changes
Breaking for implementers? Not for anything merged today. The reference shims'
request_idis now derived from the host's call id when the host sends one, instead of random. The HKDF parameters are the ones the TypeScript, Go and Python implementations already use. Thekey_idrule changes the open ports, though. PR #229 tieskey_idtosession_id, PR #22 hardcodes"default", and PR #169 and PR #183 ship default labels. Each was asked on issue #118 to read the label from config and refuse one it doesn't know. The schema changes are descriptions only.Here's what's in it:
infoset to the UTF-8session_id, and a 32-byte key.hkdfSyncagree on its key, andtests/test_hmac_key_derivation.pyrecomputes it from the stated parameters, so a typo in the example fails the build.key_idfor HMAC:key_idand never falls back to another secret, andkey_idis never derived from the secret."2026-10", which gives rotation something to hang on. One sentence is my addition, so please check it: "A Guardian signs its response with the secret, and thekey_id, that the request named." Rotation doesn't work without it.hooks.md,toolCallResult:request_id_refagainst the session'stoolCallRequestrecords.infois thesession_idexactly as sent, a response uses the key of the request it answers, the secret is at least 32 random bytes, and the unknown-key_iderror is unsigned.request_id_refis recorded on the result's own hash-chain entry, asfindingsplus the reference, and the entry's hash covers both. The decision doesn't change. Entries with no findings hash as before. A readable copy goes to the envelope log, or to stderr as anacs-findingline when there is no envelope log.request_idis unique only there, using a per-session set rather than a scan of the chain.request_idany earlier step in the session carried gets arequest_id_duplicatefinding. This Guardian doesn't enforce §10.3 yet.! FINDING. Before this, it would have warned about each one and dropped it.call_idkey (tool_use_idfor Claude Code,callIDfor OpenCode). The shim derives the request'srequest_idfrom it and sends the same value as the result'srequest_id_ref, with no state.REPLAY_DETECTEDthe shim retries that request once with a random id, which is §17.1's recovery. The result still resolves against the first request the host sent for the call.idstays random, and the host audit log recordsrequest_idon its own.loadHookmaprejects a malformedcall_id, a reserved path segment, or acall_idthat isn't declared on both the request gate and the result gate.I tested this
uv run pytest -vpasses on my machineuv run mkdocs build --strictpasses on my machineFrom
reference-implementations/agt/,bun run typecheckis clean andbun testpasses (1275).uv run pytestpasses 921. The new Guardian tests drive a running Guardian, and I watched each one fail before the change. They cover:Checklist
git commit -s(required by the DCO)Security
It closes two gaps. Without fixed HKDF parameters, a second implementation had to match our code, not our spec, to verify ACS-Core signatures. Without a
key_idrule, nothing stopped a verifier from falling back to some other secret, and that's exactly the habit a"default"label invites.