Repository navigation
Conversation
Part of GenAI-Security-Project#267. The Instrumentable tab used undefined ACS methods and an invalid MODIFY response. Replace it with the database_query toolCallRequest/modify scenario using the v0.1 envelopes and payload, and remove the v0.2 A2A tab. Leave the SDK tabs pending the removal or illustrative-label choice on GenAI-Security-Project#267. Document the existing ACS-Core signing input: UTF-8 JCS of the whole envelope after removing params.signature or result.signature (§10). Keep explicit placeholders for signatures and a sample chain head; no cryptographic authenticity or authenticated interoperability is claimed. Guard every displayed request before indexing by ID and reject ambiguous duplicate IDs. Check defined methods, payloads, response pairing, argument overrides, Core signature presence, and the toolCallRequest chain head (§8.6). The schema checks alone leave those required Core fields optional. Regression cases reject omitted signatures and chain heads, invalid requests sharing an ID with valid ones, and duplicate valid request IDs. Signed-off-by: Lrd0036 <lrd0036@auburn.edu>
|
@Lrd0036, since you already have this moving, can you run with the rest of issue #267 in this PR? I've taken myself off the issue, so it's yours. That covers everything on the checklist at the top of the issue that lives in this repo: every SDK tab in One thing sits outside this PR, so please keep If the PR gets too big to review in one pass, tell me and we'll split it. |
drk-ram
left a comment
There was a problem hiding this comment.
Reviewed at head 92c3128105e323febd68e50f40ab8606b6f2e16b. Approving this slice of #267. The new example is faithful to the normative text, and the guard makes the failure that produced the old example much harder to repeat.
Checked against the specification
- Every wire element in the new "ACS wire (JSON-RPC)" tab matches the v0.1.0 schemas.
request-envelope.jsonandresponse-envelope.jsonaccept both messages, and the payload satisfieshooks/tool-call-request.json(tool.name, with each argument wrapped as{ "value": ... }). - The signing note matches §10. The signed input is the RFC 8785 (JCS) canonicalization of the envelope with
signatureremoved, encoded as UTF-8. Namingparams.signaturefor requests andresult.signaturefor responses is the right JSON-RPC reading of that rule. - The
chain_hashon themodifyresponse matches §8.6, which requires it on content-bearing steps and under the response signature. - The MODIFY-incapable client note matches §6.5. The substituted
denywithreason_codes: ["modify_unsupported"]is the specified behaviour. - Deferring A2A to v0.2 is the honest choice, and removing the old A2A tab is correct. It documented a hook table, including a
messagehook, that no part of the specification defines.
Validation I ran on a clean checkout of this head (Windows host)
uv run --locked pytest -q tests/test_acs_examples.py: 17 passed.uv run --locked mkdocs build --strict: passed.- Full suite, excluding
tests/test_closing_choice.py, which errors on this host: 866 passed, 10 failed. The failures are intests/test_fetch_roadmap.py(3),tests/test_pr_intake_workflow.py(5) andtests/test_publish_schemas.py(2). None of those files are touched by this PR. They depend on symlinks and shell workflows that my host handles differently, so I am treating them as environmental, not as regressions. - Merge simulation: this branch merges without conflicts with #247 (
cffa5a870dd8) and with #253 (f033a6365fc9), which also editdocs/acs.mdanddocs/spec/instrument/hooks.md.
On the guard. Validating only the envelope would accept steps/anything with any payload. Resolving each method to its payload schema, requiring request/response pairing, rejecting duplicate request IDs and requiring the Core signature and chain_hash fields all close real holes. The mutation cases (old method names, bare argument values, unsigned messages, shadowed and duplicate IDs) are the right set.
Non-blocking observations
parameter_overridescarries bare replacement values, while the request'sargumentsare wrapped objects withvalueand optional provenance. The bare form is what the schema and the reference adapter use, and your note says so. A reader still sees an asymmetry. One sentence saying that an overridden argument is a replacement value, and what provenance, if any, it carries underprovenance_producer: deterministic, would pre-empt the question.- The sample
chain_hashis a 64-hex string and the note says it is a sample. A visibly synthetic value would stop anyone copying it into a verifier test. This also applies to the placeholder signature text. - In the guard,
section()ends a section at the next\n#{1,3}line, andmessages()matches only fences tagged exactlyjson. A futurejsoncfence or an unindented#line inside a code block would be skipped or truncate the section without failing. A short comment on that assumption, or an assertion that every fenced block in a guarded section is parsed, would make it robust as more sections joinSECTIONS. - Since #272 pinned the HKDF derivation, computing real signature values from a published sample secret and having the guard verify them would turn the guard into an interoperability fixture. A later slice is fine.
@rocklambros asked for the rest of #267 to land in this PR. My approval covers the content at 92c3128105e3; any later commits that expand scope will need their own look from me.
Signed-off-by: Lrd0036 <lrd0036@auburn.edu>
The November 1 ruling on GenAI-Security-Project#267 removes SDK code from the docs, because ACS publishes no SDK or package. The LangChain and Vercel tabs imported acs and @acs/guardian, so they come out with no "illustrative" label. The section now shows only the ACS wire exchange the guard checks. Signed-off-by: Lrd0036 <lrd0036@auburn.edu>
|
@rocklambros the two SDK tabs are out of the Instrumentable section ( |
drk-ram
left a comment
There was a problem hiding this comment.
Reviewed at head de524c6296be364d88f70350cbcdc0b3cc3ffc2c. Approving. My earlier approval at 92c3128 covered the content then; this review covers what has changed since.
What changed since 92c3128. A merge of integration at bba1b84, and de524c6, which is 71 deletions in docs/acs.md and nothing else: the "LangChain (Python)" and "Vercel AI (TypeScript)" tabs come out of the Instrumentable section. I read the commit on its own. It does not touch the guard, the wire tab, or any other section, so the 17 tests and the exchange I approved before are byte-identical.
What I checked
- The two removed tabs imported
acsand@acs/guardian, packages ACS does not publish. They go with no "illustrative" label, which is what the 1 November ruling on #267 asks for, and the description no longer says they stay. The section now shows only the exchange the guard checks. - The tab group in that section now has a single tab, "ACS wire (JSON-RPC)". It renders under
mkdocs build --strict, and no prose in the section refers to the removed tabs. - The remaining SDK imports on
docs/acs.md(the quick-start tabs near the top, and the Traceable and Inspectable sections) are outside this slice. The description keepsPart of #267and says so under "Follow-up", so I am not asking for them here.
Validation at this head (Windows host)
uv run pytest -q tests/test_acs_examples.py: 17 passed.- Full
uv run pytest -q: 1202 passed, 1 skipped, 10 failed, 20 errors. The 30 are intest_closing_choice.py,test_fetch_roadmap.py,test_overrides.py,test_pr_intake_workflow.pyandtest_publish_schemas.py; the same five files fail identically onintegrationatbba1b84, and none is touched here. uv run --no-dev mkdocs build --strict -d _site/docs: exit 0.- Merge simulation against #247 (
147f7d4), #498 (d3966f3) and #496 (034f52a): no conflicts.
Non-blocking, and new. I added an unknown key ("bogus": 1) beside capability in the request payload and all 17 tests still passed. That is how the payload schemas are written (they are extensible), but it means a misspelled field in the page example, such as capabilty, would not be caught. #247's guard has a declared_objects_only helper that closes this for its example by setting additionalProperties: false on declared objects. Reusing it for this payload and the response result would make the guard catch a typo, not only a wrong method or a missing field. It can wait for the slice that adds the next section to SECTIONS.
My four non-blocking notes from the earlier review are unchanged and still optional. Approving.
What changed
The first slice of #267. In
acs.md's Instrumentable section, the MCP tab is replaced with asteps/toolCallRequest/MODIFY exchange, and the A2A and SDK tabs are removed. A new guard validates every message in the section against the v0.1.0 schemas and checks the required Core fields those schemas leave optional.Which issue does this implement
Part of #267
Changes
New "ACS wire (JSON-RPC)" tab. It keeps the old tab's scenario: an agent asks to run
SELECT * FROM customers, and the Guardian narrows it. The request and response now use the defined shapes.steps/toolCallRequestwith the requiredacs_version,request_id,timestamp, andmetadata; a payload withtool.nameand a wrappedqueryargument; and the HMAC-SHA256 signature ACS-Core requires (§10).finalresult withdecision: "modify", the requiredreasoning,modifications.parameter_overrides, and thechain_hash§8.6 requires on a content-bearing step.params.signatureorresult.signaturebefore JCS canonicalization and UTF-8 encoding (§10). Signature values are placeholders, MODIFY is SHOULD-support with adenysubstitution (§6.5), an MCPtools/callcan use the same hook, and A2A is reserved for v0.2.A2A tab removed. It listed a hook table, including a
messagehook, that no specification defines. v0.1 has no A2A wire to show in its place.SDK tabs removed. The LangChain and Vercel tabs imported
acsand@acs/guardian, which ACS does not publish. Following the November 1 ruling on #267, they come out with no "illustrative" label.tests/test_acs_examples.py. Envelope validation alone would passsteps/anythingwith any payload, so the guard checks more than the envelope:system/pingretains its signature exemption.toolCallRequestchain head, and override only arguments that request sent.steps/registerHook, a bare argument value, and malformed MODIFY results. New regressions reject unsigned Core requests and responses, missing chain heads, and invalid requests sharing an ID with a valid request in either order. A separate case rejects duplicate valid request IDs.Other sections join the guard's
SECTIONSlist as their slices land; the content-bearing response checks must expand with them.Judgment calls for review:
infoset tometadata.session_id) and definedkey_idas an opaque label the deployment chooses, so real values are now computable from a published sample secret. This slice keeps placeholders; switching to real values that the guard verifies can follow if reviewers want it. The guard checks structure and required field presence; it does not verify the illustrative signatures or sample chain head.parameter_overridesuses a bare replacement value, matching the schema's description ("replacement values for tool call arguments, keyed by argument name") and how the reference host adapter applies it.Base branch
integration, because this touches tests.Type of change
tests/)I tested this
integrationcommit,bba1b84(checked 2026-11-04).uv run --locked pytest -qpasses on my machine: 1222 passed, 1 skipped, including all 17 tests in the new guarduv run --locked mkdocs build --strictpasses on my machinegit diff --checkpassesThe strict build doesn't validate anchors, so I checked the four new link fragments against the built HTML (rechecked 2026-11-04). The tab renders both JSON blocks as highlighted code. Separate page-level mutation probes confirmed that removing each required field, inserting a shadowed invalid request, or repeating a valid request ID fails on the intended rule.
An isolated merge with #247 at
147f7d4has no conflicts. The full pytest suite passes on that combined tree (1237 passed, 1 skipped), including all 32 tests across the two example guards. #247 remains open on GitHub; this is local combined validation.Checklist
git commit -s(required by the DCO)Security
No material runtime change identified. The documentation explains existing ACS-Core signing and chain-head requirements; the schemas and signing implementation are unchanged. Placeholder signatures and the sample chain head demonstrate message structure, not authenticated interoperability.
Architecture and knowledge
Only the overview documentation and its guard change. The normative signing contract in §10 and chain-head rule in §8.6 remain the authoritative sources.
Follow-up
The remaining #267 pillars stay open, including the SDK tabs in the other sections. The OCSF tab and Inspect extension mappings are outside this slice. Revisit the ping-response signature interpretation when a ping example is added; this slice contains only a tool-call exchange.