docs(sandbox): correct "can't fake hardware" claim in TraceSandboxAdapter - #315
Conversation
748a21b to
08ce15a
Compare
fcd55b8 to
6e08d43
Compare
lywinged
left a comment
There was a problem hiding this comment.
Two things to change, both small, and then this is an approval. Reviewed at 6e08d43 on a247244.
The correction is right. The original sentence claimed an appraisal the adapter never performed, and the two new docstrings say what is enforced and what is not. The second test is the more valuable half: it pins a property the docs already state and the adapter cannot enforce, that a genuinely verified quote signed with an unrelated key is indistinguishable here from a fabricated one.
What I checked rather than read:
sandbox.pywith every docstring stripped parses to the same tree on this head and onmain, so "No runtime behavior changes" is measured._runtime()copiesplatform,measurementandnoncefrom the attestation verbatim, andbuild_trust_recordreturns a placeholdercnf, which is what the new text says.- The three sentences attributed to
docs/trust-levels.mdand the one attributed todocs/verification.md, "The producing profile defines that binding", exist as quoted. Both new anchors resolve in an mkdocs build assembled the waydocs.ymlassembles it. examples/sandbox-runtime.json'snoncedecodes tosandbox-runtime-nonce.- The four CI steps with the pinned install, on 3.11 and on 3.12: green, 1404 passed and 1 skipped, the sandbox file 45 passed.
The first. examples/README.md line 24 still reads "sandbox-runtime.json: a sandboxed agent runtime, TPM 2.0 rooted." The guide now says of the same file "It is not a TPM-rooted record", so as merged the repository would say both about one file. Something like:
- `sandbox-runtime.json`: a sandboxed agent runtime in the shape a `tpm2` attestation
produces. Its measurement, nonce and key are placeholders; see
`docs/integration/sandbox-runtime.md`. Produced by `TraceSandboxAdapter`; the decision
log is a kernel-sandbox policy trace rather than MCP tool calls.
The second. Two places say the nonce is checked against nothing: the field docstring, "nothing here or downstream checks that binding, so an unrelated string is accepted just as readily", and the new test's docstring, "or against anything else". The module docstring's "nothing in this codebase does" reads the same way to anyone who does not already know the parameter. sign.verify_record takes expected_nonce and compares it with runtime.nonce in constant time, and that parameter is the hook the paragraph's own advice needs, since a verifier that knows the challenge can already check the nonce. What nothing checks is the nonce's binding to the key, which is the true claim underneath all three. Naming the parameter is worth a clause, because no page under docs/ mentions it and a reader who takes the advice will otherwise not find it. For the field docstring, something like:
where that challenge goes; nothing here binds it to the key, and downstream
``verify_record(expected_nonce=...)`` compares it only with a value the verifier
already knows.
Three things I would not hold the merge for:
- The CHANGELOG entry says the correction is "consistent with how every other Level 1 producer in this codebase is documented". The only other adapter,
agt.py, emits Level 0, so that sentence describes a set with no other member.docs/trust-levels.md, which the entry already cites, is the claim that can be checked. - Twenty-two
--in the added text. They passcheck_dashes.py, whose own hint for an em dash is "use a colon, a comma, or two sentences".mainalready carries fifteen, in one crosswalk page and three modules, so this is a preference rather than a rule. sandbox.pyline 307 has one extra leading space at the start of a docstring line.
Signed-off-by: rajnisht7 <rajnishtiwari9787@gmail.com>
lywinged
left a comment
There was a problem hiding this comment.
Approved on 4382e66.
Both changes landed as asked: examples/README.md no longer calls the example TPM-rooted, and the nonce field docstring now names verify_record(expected_nonce=...). The stray space is gone too. Re-run on this head with the pinned install: the four CI steps green, 1404 passed and 1 skipped, and a sweep of the whole tree for "TPM 2.0 rooted" finds nothing.
One clause from the same request is still standing, in the new test's docstring at lines 247 and 248: "or against anything else". verify_record(expected_nonce=...) is the thing it says does not exist. Not holding the merge for a test docstring, but since the field docstring beside it now says the true thing, this is the one line left saying the other:
``nonce`` is carried through verbatim and is never checked against ``cnf.jwk`` or
against the signing key passed to ``sign_record``; ``verify_record(expected_nonce=...)``
compares it only with a value the verifier already knows.
The other two notes stand as preferences and need nothing from you.
`main` moved from a247244 to 760cc3f while this branch was open, taking df0120b (agentrust-io#315) and 760cc3f (agentrust-io#327). The only conflict is CHANGELOG.md, where agentrust-io#315 added an entry under `### Fixed` at the same anchor this branch does. Both entries are kept, this branch's first, which is the order the file already uses. Nothing else overlaps: of the eight files this branch touches, CHANGELOG.md is the only one main also touched. The check worth stating is not the conflict. This branch's sweep accounts for every parameter of every public function by name and fails on one it has not swept or declared, and agentrust-io#315 changed src/agentrust_trace/adapters/sandbox.py. It reports nothing new: on the merged tree the suite gives 1494 passed, 1 skipped and 2 xfailed, against 1491 passed and 2 xfailed on this branch alone and 1402 passed on main alone, so the three tests main adds are the whole difference and none is lost. The two xfailed are still xfailed rather than xpassed, so agentrust-io#320 is still open and the strict marker still holds. ruff, mypy on src/agentrust_trace, tools/check_dashes.py and the suite all pass on the merged tree. Signed-off-by: Louie Lu <48041247+lywinged@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
What this changes
The module docstring in
sandbox.pyanddocs/integration/sandbox-runtime.mdboth said: "It will not let a caller claim hardware it does not have" and that a record sayingtpm2"carries a measurement that something other thanthis process produced."
That's doesnt seem true. As
SandboxAttestationonly checks shape platform is in the allowed enum, measurement looks like asha256:/sha384:digest. There's no quote, signature, or nonce check anywhere. So this is accepted as-is, signed,and passes schema validation:
Type of change
Spec section
None
Checklist
git commit -s)CHANGELOG.mdupdated (for any normative change)<!-- CHANGED: #NNN: description -->in spec text