Repository navigation
Conversation
|
Same check I ran on #169, and not a review of the Python implementation beyond it. Each of the reference-Guardian bugs in the #160 to #167 range has a named mechanism here, and the dispatch ladder at the top of The broader thing is the relationship with #169. This tree does not just borrow its HKDF table: the README says its documented choices for the wire details ACS leaves undefined "are the ones this tree follows, so the two are intended to interoperate", and that shows up in the canonicalization locations, the wrapped MCP checks, the routing order and the error rules as well as in The fragile part is that the dependency only points one way. This tree records it carefully, in the README and in five module docstrings. The Go tree records nothing in return, so if its table or its routing order moves before it merges, nothing on that side signals that another implementation is pinned to it, and these vectors are where it would surface. Naming both in #118 would make the convergence something the spec owns rather than something two open PRs maintain by hand. |
rocklambros
left a comment
There was a problem hiding this comment.
@dtduc-git, I know this is still a draft while placement gets decided, but I ran it against current integration (185ef4e) and found a few things you'll want to fix before you take it out of draft. The first two are security bugs, and I reproduced both over HTTP.
-
An unsigned
system/pinggets back a Guardian-signed ALLOW. Inserver.py:506-510the ping answer is signed whenevermetadata.session_idis present, becausekeycomes from the session id alone (server.py:143-144) and the ping's own signature is never checked. So anyone who can reach/acscan send an unsigned ping with a victim'ssession_id,idandrequest_id, and get back a signeddecision: "allow"that verifies under that session's key. Someone on the network path can then swap it for a real signed DENY. The comment there says "if one authenticated", so I think the intent was right and the check just got lost. Please sign the answer only when the ping's own signature verified (self._signature_ok(raw, params, key)), and add tests showing that an unsigned ping and a bad-signature ping both get answers with noresult.signature. The Go port in PR #169 already works this way. -
Deep nesting turns a deny into an error, and the error lets the tool run. I sent a signed
toolCallRequestforrm -rf /with one argument nested N levels deep. At 450 levels I got a signed deny. At 500 levels (about 4 KB) I got an unsigned-32603, and at 985 the connection dropped with no answer. The ServerHello saidon_decision_failure: proceed, so the agent runs the command. The cause iscopy.deepcopyincanonical.py:34, andserver.py:124-125only catchesValueError. Please use a shallow copy in_without_signature, catchRecursionErrorinhandle(), and once the signature has verified in_step, turn any exception into a signed deny withevaluation_failed, the same way engine failures already work. A regression test at 600 levels would pin it. The existing test only tries 10,000 levels, which the parser rejects first. -
Version negotiation accepts any
0.x.handshake.py:99-103returns0.1.0if the client offers anything with major0, even0.9.0alone, and a step withacs_version: "7.3.0"in an open session was allowed. §4 says a mismatch ends with-32001. Please pick only a version the client actually offered, and refuse a step whoseacs_versiondiffers from the session's negotiated version. -
The
acs-coreclaim is from before PR #21. The branch is 13 commits behind, and PR #21 changed the floor indocs/spec/conformance.md. MODIFY is now SHOULD, §6.5 needs amodify_unsupportedsubstitution (ALLOW atpostCompact), and §9.2 needs the substitution recorded as an audit event. There's nomodify_unsupportedanywhere insrc/, and_substitute_permittedonly prints to stderr. Please rebase, update the README table to the current bullets, and either add the §6.5 path or dropacs-corefromSUPPORTED_PROFILESuntil it's there. -
The narrowed path filter switches off the Rust CI in PR #187. This PR changes
.github/workflows/reference-implementation.ymlfromreference-implementations/**toreference-implementations/agt/**. PR #187 adds itsagt-rustjob to that same file, andagt/**doesn't matchagt-rust/. Git won't flag it, because the hunks don't overlap. I merged both onto integration and confirmed that a Rust-only change would trigger nothing. Keep"reference-implementations/**"and add"!reference-implementations/agt-python/**"right after it in bothpathslists. That still keeps Python-only changes off the Bun install. -
Two claims need fixing. First, the PR says
Closes #86, but issue #86 is "Port the AGT reference implementation to Python", and this tree has no AGT. The engine isBuiltinEngineinengine.py. Please change it toContributes to #86so the roadmap doesn't mark the AGT port as delivered. Theagt-pythonname has the same problem, so worth settling when placement is decided. Second, the README says this tree follows the Go port's chain choices, butchain.py:91usesuuid.uuid4()forentry_idwhere Go usesrequest_id, and the entry has noturn_id. When I fed the same steps to both Guardians, I got different chain heads. Either align with Go or say where they differ.
These aren't blockers:
json.loadsatserver.py:759takes the last value of a duplicate key, but Go refuses the request. Anobject_pairs_hookthat raises fixes that.schemas.py:114builds validators with noformat_checker, souuidanddate-timeare never checked.- The 10-second timeout at
server.py:697-700limits idle time per read, not the total request. I held 200 slow connections open for 30s with no auth. - The Python workflow doesn't pin uv or Python. CI ran 3.12.3, and
requires-pythonsays 3.11.
The vendored vectors in tests/testdata/envelope-vectors.json are still byte-identical to the ones in PR #169, so that part is fine.
An independent implementation of the ACS v0.1.0 wire contract under reference-implementations/agt-python/: JSON-RPC envelopes, handshake negotiation, the hook taxonomy, the five dispositions, the SessionContext chain with a published chain_hash, replay and skew protection, and the HMAC-SHA256 baseline signature. Wrapped MCP is not implemented yet, so the ServerHello answers profiles_accepted: [] rather than claiming acs-core; the README's status table says exactly which parts are implemented. The suite validates envelopes against this repository's schemas by relative path, pins the cross-implementation signature vectors from the open Go port (GenAI-Security-Project#169), ports the conformance harness's ten external probes, and pins every hook's schema to its method name. A path-scoped workflow runs it in CI. Signed-off-by: Duke - Duc Dinh <dtduc.contact@gmail.com>
protocols/MCP/* and the explicit-version wrapped:mcp-<version>/* form now route to an MCP handler that reads the wrapped JSON-RPC 2.0 message (a request or the response it answers), checks it, and hands it to the policy engine intact; a malformed message is INVALID_PARAMS. A wrapped method is negotiated only when the ClientHello declares the protocol in wrapped_protocols, and the explicit-version form only at a declared version. The method table now carries hooks.md's per-hook permitted dispositions: a disposition a hook cannot carry is substituted (DENY where the hook permits it, ALLOW with a reason at audit-only hooks and postCompact), including on every Guardian-authored denial, so no DENY is ever sent at a hook that permits none. Decisions are validated against modifications.json, ask-details.json, and defer-details.json, plus §6.3's disjoint-target rule with the wrapped call's own parameter-override pointer. steps/sessionEnd closes the session (a later step is answered with a session_closed denial, replay history survives), a key_id or agent_id other than the session's is answered with a signed denial rather than an error, and an undefined method is METHOD_NOT_FOUND while agbom/* stays CAPABILITY_NOT_NEGOTIATED. The handshake now enforces the timestamp skew like any request, responses are signed under the deployment's key_id throughout, the transport refuses a missing or malformed Content-Length and closes cleanly, and the TypeScript workflow's path filter is narrowed to its own tree so a Python-only change does not pay for a Bun install. Signed-off-by: Duke - Duc Dinh <dtduc.contact@gmail.com>
One recv() is not the whole response: http.server flushes headers and body as two writes, and Linux split them across segments, so the tests failed on CI while passing on macOS. The loop ending is itself the proof the server closed the connection. Signed-off-by: Duke - Duc Dinh <dtduc.contact@gmail.com>
…en, align key_id and the chain with the Go port - ping: an answer is signed only when the ping itself verified; a bad signature or an unknown key_id never resolves a key - over-deep bodies fail closed at every depth the JSON parser accepts: the dispatch ladder runs with signed refusals and denials, verification and hashing run on one serialized enlarged thread, and raw text past the parse boundary is refused before it is parsed - key_id comes from configuration with no default (GenAI-Security-Project#118); a step naming another label is refused -32004, unsigned - section 6.5: a MODIFY the Observed Agent cannot apply is substituted (DENY modify_unsupported; ALLOW at postCompact) and recorded with the intended modification in the audit event - the chain follows the Go port's choices: entry_id = request_id, metadata.turn_id carried, write-clock timestamp; README interop updated - a step's acs_version must share the session's negotiated major version (-32001); duplicate JSON members are a parse error; the TypeScript workflow's path filter keeps future agt-rust changes covered; CI pins uv 0.9.9 and Python 3.12 153 tests pass. Signed-off-by: Duke - Duc Dinh <dtduc.contact@gmail.com>
a606405 to
5e7f4bb
Compare
|
Thanks Rock — all addressed except the handler timeout you flagged as a non-blocker (see the end), rebased onto integration (153 tests green). Point by point:
Non-blockers: duplicate members are now a parse error (-32700), and uv and Python are pinned in the Python workflow. The handler timeout is unchanged (a per-read idle bound, not a total request budget); happy to open a follow-up issue for a total deadline if you want it tracked. |
What changed
Adds a Python reference Guardian implementation under
reference-implementations/agt-python/: the ACS-Core wire contract(JSON-RPC envelopes, handshake negotiation, the 19 hooks, wrapped MCP, the
five dispositions, the SessionContext chain with a published
chain_hash,replay and skew protection, the HMAC-SHA256 baseline signature), a policy
engine interface with a small built-in engine, and a test suite that validates
envelopes against this repository's schemas by relative path.
Which issue does this implement
Contributes to #86
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
I tested this
uv run pytest -vpasses on my machine (149 tests, fromreference-implementations/agt-python/)uv run mkdocs build --strictpasses on my machineChecklist
git commit -s(required by the DCO)Security
Coordination
I posted on #78 on 2026-09-22 asking whether that sample is still active, and
flagged the structural question (
samples/vsreference-implementations/)to @rocklambros; no reply yet at the time of opening. This is a draft until
the workstream decides where Python lives. It currently follows the
agt-goprecedent underreference-implementations/agt-python/. It builds on#78's FastMCP instrumentation idea rather than duplicating it — a host adapter
is not in this PR.
What it implements, and what it does not
Every ACS-Core item in
docs/spec/conformance.mdis implemented, so theServerHello accepts
acs-core. Not claimed:acs-trace,acs-inspect,acs-provenance,acs-crypto,acs-audit;agbom/*is refused with-32003; Intent/IBAC is not implemented (Intent is optional andIBAC-conditional, and
ask_detailspasses through).README.mdcarries theper-item status table naming the tests that cover each row.
Evidence
ported (
tests/test_external_probes.py), nine of them signature-verified;the external mode itself is contributed in Add a Go Guardian reference implementation #169, which is not merged, so
this port is the local gate until it lands.
in
tests/testdata/README.md, and pinned byte for byte.specification/v0.1.0/by relative path, soa schema change fails the suite.
.github/workflows/reference-implementation-python.ymlis path-scopedto this tree; the existing TypeScript workflow's path filter is narrowed to
its own tree so a Python-only change does not pay for a Bun install.
One spec inconsistency this PR works around
The examples in
docs/spec/instrument/extend_mcp.mdplace the wrapped MCPmessage directly in the ACS envelope's
params— a shaperequest-envelope.jsonrejects (paramsrequiresacs_version,request_id,timestamp,metadata, andpayload). This tree follows theschema (the MCP message is carried intact in
params.payload), as the Go portin #169 does, and says so in its README. Worth an upstream issue if you agree.