Repository navigation
prototype(gate_store,cmcp_gate): support binary secrets, enforce canonical base64url, and wrap malformed store responses in GateStoreUnavailable (#606) - #609
Conversation
430224f to
4b1e421
Compare
…and gate inputs (agentrust-io#606) - Require `type(secret) is bytes` and `len(secret) >= 16` in both `GateStoreServer` and `RemoteGateStore`, and authenticate `_b64(secret)` on the wire so non-UTF-8 binary secrets do not raise `UnicodeDecodeError`. - Enforce canonical unpadded base64url in `gate_store._unb64`, reject duplicate JSON keys, UTF-8 BOMs, non-finite JSON constants, and deep recursion in `gate_store._recv`, and wrap malformed store responses in `GateStoreUnavailable`. - Validate `context`, `gateway_key`, `gateway_issuer`, `clock`, `database`, `session_id`, `call_id`, `reason`, `handle`, and `isinstance(action, dict)` in `CMCPTraceGate` with `ProfileError`. Closes agentrust-io#606 Signed-off-by: Raghuvar Durga <177662415+drk-ram@users.noreply.github.com> Co-authored-by: Raghuvar Durga <177662415+drk-ram@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
4b1e421 to
b5b7433
Compare
dinakarjs
left a comment
There was a problem hiding this comment.
Reviewed b5b7433. Linux/Python 3.12.14: the new hardening suite passes 76 cases. I then enabled the otherwise skipped gateway composition tests using cMCP source 1206ad22949c4e31d041fb82cda2ac5a3545c1f5, its hash-locked dev environment, and this TRACE checkout on PYTHONPATH: hardening + test_cmcp_trace_gate.py + test_multihost_replay.py gives 117 passed with no skips. Proxy environment variables were removed for controlled loopback tests. Targeted Ruff passes.
The malformed reply paths preserve GateStoreUnavailable and single-attempt semantics, while the real TCP/CLI controls exercise non-UTF8 secrets. No additional blocker found in the bounded change. One compatibility note: the wire secret changes from raw UTF-8 to base64url even for ASCII keys, so old and new prototype clients/servers must be upgraded together; retaining TRACE_GATE_STORE_SECRET as an environment input does not preserve mixed-version wire compatibility. Please state that in prototype deployment guidance. I did not rerun every author's guard-removal trial or establish a production replicated-store guarantee. Required maintainer approval remains separate. Reviewed with OpenAI Codex.
lywinged
left a comment
There was a problem hiding this comment.
REVIEW on #609, head b5b7433: Approve
Measured on b5b7433 with the hash-pinned requirements, on 3.11 and on 3.12, the steps ci.yml runs: ruff with prototype added, as the description runs it, mypy over src/agentrust_trace and the dash check clean; 4756 passed and 48 skipped here; the experiment 116 passed and 4 skipped; the portability run 4755 passed and 49 skipped. The new test file collects 76 cases in 11 functions, the description's 76; the description says 9 functions, and 26 cases for the response test, which collects 27. Against the merge base 295c36d the review bar sorts all 76 as failing on behaviour, which is the description's count. The change does what #606 asks, all three of its items, and the CHANGELOG entry under Unreleased says what the code does, short of the sentence asked for in item 1 below.
-
The wire secret, which dinakarjs's note names. Measured with
python -m prototype.gate_storeand an ASCII secret: the merge base's server with this head'sRemoteGateStore, and this head's server with the merge base's client, both refuse with "unauthenticated", while each side with its own peer works. So a deployment that upgrades one side first loses the store until the other follows, and "retainingTRACE_GATE_STORE_SECRETcompatibility" in the description is about the environment variable, not the wire. The CHANGELOG already names the new form, "authenticate_b64(secret)across the wire". What it and the description do not say is that the old form is refused, so a server and its clients move together. Two facts for the decision. A server that accepts either form, the base64url string or the raw secret as before, passes all 76 new cases and the whole suite, 4756, so nothing in the matrix pins the incompatibility, and compatibility costs oneorin the handler. And the prototype is unreleased, so the sentence dinakarjs asked for may be all it needs. -
Confirmed, on the whole suite on a copy, one mutation at a time, 4756 passing before any, each guard holding its own cases and nothing else. The
_unb64round-trip check removed fails the four_unb64cases and the paddedadmissionreply, five. The client's secret checks removed fails the five cases oftest_remote_gate_store_rejects_short_or_non_bytes_secret. The duplicate-key hook removed from_recvfails theduplicate-keyframe and the reply withoktwice, two._digestput back to_stron the challenge and receipt-head replies, the description's cold-reader finding reverted, fails exactly its two regression cases. The store-interface check removed fails theobject()case alone, and theboolcheck onallowedfails theallowed=1case alone.canonical_digestisdigest(rfc8785.dumps(value)), so hashing_action_bytes(action)gives the value it gave before.Identifieradmitstrace_refused, the one reason the gate writes itself, andcedar_allowed, the one its callers pass, andReceipt.reasonwas already typedIdentifier, so the new check onreasonchanges the exception class and the timing, not what is admitted. -
Small things. The description cites the wire-protocol comment at
prototype/gate_store.py:250; it is at 277 on this head and 276 on the merge base. "Editorial" is checked under Type of change for a change that makes the store refuse input it accepted; the template has no box for this kind of change, and leaving them all unchecked with a sentence under the list, as #611's description does, reads right.python -m prototype.gate_storewith a non-canonicalTRACE_GATE_STORE_SECRET_B64exits with aValueErrortraceback from_unb64rather than a message, as the plain variable already does for a short value. A prototype command line, so a nit. Andmypy --strict prototype/gate_store.py prototype/cmcp_gate.pydoes not give the description's "Success" here: with the pinned mypy 2.4.0 it follows the import intoprototype/verifier_token.pyand reports line 167, on the merge base as on this head, nothing in the two files. The description's run may have silenced imports, as #610's command does.
Approve. Item 1 is a sentence in the description and the CHANGELOG, or the one-line compatibility if mixed versions should keep working; nothing here waits on either.
What this changes
Closes #606.
Fixes three fail-closed and input-validation gaps in
prototype/gate_store.pyandprototype/cmcp_gate.py:Binary
bytessecrets and length/type enforcement (prototype/gate_store.py):GateStoreServer.__init__andRemoteGateStore.__init__both enforcetype(secret) is bytesandlen(secret) >= 16, raisingValueError("store secret must be at least 16 bytes")before opening any SQLite file or socket.RemoteGateStoreencodessecreton the wire as_b64(secret)(unpadded base64url, matching the wire protocol comment atprototype/gate_store.py:250) andGateStoreServer._Handler.handlecomparesrequest.get("secret")(requiringisinstance(secret, str)) against_b64(self.server.secret)viahmac.compare_digest, so non-UTF-8 binary secrets (b"\x80\xff\x00\xfe\x91\xa2\xb3\xc4" * 4) work without raisingUnicodeDecodeError.prototype.gate_store.main(python -m prototype.gate_store) also acceptsTRACE_GATE_STORE_SECRET_B64(decoded via_unb64) so subprocess deployments can pass arbitrary binary secrets without UTF-8 encoding constraints, while retainingTRACE_GATE_STORE_SECRETcompatibility.Canonical unpadded base64url, JSON wire checks, and
GateStoreUnavailableresponse wrapping (prototype/gate_store.py):_unb64verifies_b64(raw) == textafterbase64.b64decode(..., altchars=b"-_", validate=True), rejecting padded ("AAAA====","AA==","AAA=") and non-canonical ("AB") strings withValueError("bytes field")._recvdecodes frames as UTF-8 (raw.decode("utf-8")), rejects duplicate JSON object keys (object_pairs_hook=_pairs), UTF-8 BOMs, and non-finite JSON constants (parse_constant=_no_constants), and convertsRecursionErrortoValueError("malformed json").RemoteGateStore._call,mint_challenge,consume_challenge,bump_admission,admission,receipted,reserve_call,receipt_head, andreceiptsvalidate response shapes, field types, andsha256:<64 lowercase hex>digests (_digest) and wrap anyTypeErrororValueError(includingbinascii.Error) inGateStoreUnavailable, so malformed store replies do not leakValueErrororTypeErrorinto cMCP's refusal-receipt path.CMCPTraceGateconfiguration and method input validation (prototype/cmcp_gate.py):CMCPTraceGate.__init__validatesdatabase(isinstance(database, (str, Path))and non-empty string when provided) andstore(callableGateStoremethods when provided) withProfileError("gate_store_configuration_invalid"), and validatescontext(isinstance(..., VerificationContext)),gateway_key(isinstance(..., Ed25519PrivateKey)),gateway_issuer(v.Identifier), andclock(callable) withProfileError("gate_configuration_invalid").challenge,_prove,admit,begin,recheck,receipt, andrefusalvalidatesession_id,call_id, andreasonagainstv.Identifier,isinstance(handle, CallAdmission),type(allowed) is bool, andisinstance(action, dict)(plus JCS serializability viarfc8785.dumps), raisingProfileError("input_invalid").SQLiteGateStoredirect method calls with unvalidated types (callers reach it throughCMCPTraceGateorGateStoreServer._dispatch), and livecmcp_runtimeproxy tests intests/test_cmcp_trace_gate.pyandtests/test_multihost_replay.pywhen the siblingcmcp_runtimepackage is not installed in the test environment.Cold-reader review (
2b0d982->b5b7433):A separate agent session given only the repository at
2b0d982,git diff 295c36d..2b0d982, #606, and the draft description found three issues:RemoteGateStore.receipt_headandconsume_challengechecked_str(...)on digest fields rather thansha256:[0-9a-f]{64}, so a wire reply{"ok":[null,"garbage"]}fromreceipt_headwould passRemoteGateStoreand later raisepydantic.ValidationError(ValueError) insideCMCPTraceGate.receipt(). Changed: added_digest(sha256:[0-9a-f]{64}) inprototype/gate_store.pyand applied it in_dispatch,RemoteGateStore.consume_challenge, andRemoteGateStore.receipt_head, plus added regression cases{"ok":[null,"garbage"]}and{"ok":["not-a-digest","s1","not-a-digest",1,2]}.python -m prototype.gate_storeonly readTRACE_GATE_STORE_SECRETvia.encode(), which cannot carry arbitrary non-UTF-8 bytes in a subprocess environment variable. Changed: addedTRACE_GATE_STORE_SECRET_B64support inprototype.gate_store.mainand added subprocess testtest_cli_server_supports_binary_secret_via_b64_env.CMCPTraceGate.__init__did not validate a non-Nonestoreobject for theGateStoremethod interface, and the description called_recv"strict JSON wire framing" without naming the specific checks. Changed: validated_STORE_METHODSonstoreinCMCPTraceGate.__init__(with test case{"database": None, "store": object()}), and narrowed the description to list the exact JSON checks performed.Type of change
Spec section
None (unreleased
prototype/gate_store.pyandprototype/cmcp_gate.py).Tests for a fix
tests/test_gate_store_and_cmcp_gate_hardening.pyadds 76 test cases (10 test functions). Runningpython tools/review_bar.py --base origin/main --head HEAD(295c36d..b5b7433) reports:295c36d, and 76 passed onb5b7433:test_remote_and_server_support_non_utf8_binary_secret(1 case): fails on295c36dwithUnicodeDecodeErrorinRemoteGateStore.__init__.test_cli_server_supports_binary_secret_via_b64_env(1 case): fails on295c36dwithValueError: store secret must be at least 16 bytesbecauseTRACE_GATE_STORE_SECRET_B64is ignored.test_remote_gate_store_rejects_short_or_non_bytes_secret(5 cases:b"",b"short",b"x" * 15,bytearray(b"a" * 16),None, plus"a" * 16): fails on295c36d(DID NOT RAISE ValueError/AttributeErroronNone).test_gate_store_server_rejects_non_bytes_or_short_secret(3 cases:"a" * 16,bytearray(b"a" * 16),[1] * 16, plusb"x" * 15): fails on295c36d(DID NOT RAISE ValueError).test_unb64_rejects_padded_and_noncanonical_base64url(4 cases:"AAAA====","AA==","AAA=","AB"): fails on295c36d(DID NOT RAISE ValueError).test_recv_rejects_duplicate_keys_nonfinite_bom_and_deep_nesting(5 cases:b'{"ok":1,"ok":2}',b'{"ok":NaN}',b'{"ok":Infinity}',b'\xef\xbb\xbf{"ok":1}',b"[" * 200_000 + b"]" * 200_000): fails on295c36d(DID NOT RAISE ValueErrororRecursionError).test_remote_gate_store_wraps_malformed_responses_in_gate_store_unavailable(26 cases across_call,mint_challenge,consume_challenge,bump_admission,admission,receipted,reserve_call,receipt_head, andreceipts): fails on295c36d(DID NOT RAISE GateStoreUnavailable,binascii.Error,TypeError,IndexError,KeyError, orValueError).test_cmcp_trace_gate_init_validates_configuration(12 cases acrosscontext,gateway_key,gateway_issuer,clock,database, andstore): fails on295c36d(DID NOT RAISE ProfileError).test_cmcp_trace_gate_rejects_invalid_session_and_call_identifiers(5 cases:"","bad id","-leading-dash",123,None): fails on295c36d(DID NOT RAISE ProfileErrororsqlite3.IntegrityError).test_cmcp_trace_gate_rejects_non_dict_or_non_jcs_action(5 cases:["list"],"string",123,None,{"nan": float("nan")}): fails on295c36d(DID NOT RAISE ProfileError).test_cmcp_trace_gate_recheck_and_receipt_validate_handle_and_reason(8 cases acrossrecheckandreceipt): fails on295c36d(AttributeError,AssertionError,DID NOT RAISE ProfileError, orpydantic_core.ValidationError).Validation commands run on
b5b7433:pytest tests/test_gate_store_and_cmcp_gate_hardening.py -q->76 passedpython tools/review_bar.py --base origin/main --head HEAD->76 fail on behaviour, 0 fail on a missing name or file, 0 pass or skippython tools/sweep_public_surface.py --strict->89 parameters swept, 140 accepted junk values, 0 unfiled leak(s)python tools/check_dashes.py->no dashes outside examples/, spec/trace-v0.1.mdruff check src tests scripts prototype->All checks passed!mypy src/agentrust_trace->Success: no issues found in 16 source filesmypy --strict prototype/gate_store.py prototype/cmcp_gate.py->Success: no issues found in 2 source filesChecklist
git commit -s)CHANGELOG.mdupdated (for any normative change)<!-- CHANGED: #NNN: description -->in spec text