Repository navigation
feat(proxy): record advertised but unadmitted tools as audit observat… - #723
Conversation
…ions Signed-off-by: Loek <solloek369@gmail.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
imran-siddique
left a comment
There was a problem hiding this comment.
@solloek369-arch this records what the #566 ruling asked for, and a separate entry at comparison time is the right carrier: it keeps the observation off the triggering call's result.
One change before merge. Every unadmitted name becomes its own synchronous audit append, and discovery allows MAX_DISCOVERY_PAGES = 1000 pages with no cap on names per page or on name length. Before this PR those names cost nothing. Now a hostile upstream decides how many entries we write at first contact.
Bound it: record at most a fixed number of names per comparison, sorted as now, carry the remainder as a count, and cap tool_name length. Add a test that a listing over the bound writes exactly the cap plus the count. Can you have it up by 9 October?
Agreed that aligning audit-entry.schema.json is a separate change.
Record at most 64 sorted unadmitted names per completed comparison and retain the omitted count in one existing observation entry. Limit recorded name prefixes to 256 characters with explicit truncation and digest evidence while preserving full-name admission and drift comparisons. Signed-off-by: Loek <solloek369@gmail.com>
|
Independent review at head A focused harness using the exact drift method and real AuditChain independently confirms the existing bounds concern: 1,024 extras produce 1,024 observations, and an 8,192-character name is retained intact. The harness stubs catalog/digest/transport dependencies; this is focused reproduction, not a full integration test. The real discovery function also accepts that long name. For the bounded revision, I suggest checking:
This supports Imran’s requested change; it is not merge approval. I can review the revised pinned head once available. |
|
@imran-siddique Yeah, you're right — sorry. But it gave some extra fuel for a proper check :D. Pushed dc82afe, addressing the requested bound: each completed comparison now records at most 64 individually sorted unadmitted-name observations, with recorded tool_name capped at 256 Python characters. Any remainder is represented by one summary observation with tool_name=null, status=observed_unadmitted_summary, and integer detail.omitted_name_count. The persisted 65-name test records exactly 64 individual observations plus one summary with 1 omitted. The 1,041-name test records 64 plus one summary with 977 omitted. No audit-schema cleanup is included. I also ran a broader local boundary and regression pass around this bounded-audit change. Nothing in that pass changed the design above, so I've kept those checks as supporting evidence rather than turning additional observations into new acceptance criteria. |
|
@rajnisht7 this merged with one maintainer approval where |
What
At successful first-contact comparison,
_check_upstream_driftrecords at most 64 sorted advertised names outside the same server's active catalog entries as individualtool_observed_unadmittedAuditChain entries. Recordedtool_nameis capped at 256 Python characters. Any remainder is one existingtool_observed_unadmittedentry withtool_namenull,detail.statusobserved_unadmitted_summary, and integerdetail.omitted_name_count. Extra names stay outside the drift comparison and do not themselves deny approved calls; an unadmitted call remains subject tocatalog_miss, anddefinition_changed/withdrawnbehavior is preserved. Part of #566 (Obligation 3), per the ruling.Carrier. A separate observation at comparison time, alongside the existing drift audit path, so the evidence describes the advertisement versus the active admission basis rather than the triggering approved call's terminal result. Attaching evidence to a genuine call terminal remains a plausible alternative; the ruling establishes the semantics, not this carrier choice.
Fields.
server_identityfollows the existing server-URL convention and does not encode the full provenance key; for a stdio server it is empty. Scalar detail recordsstatus: observed_unadmitted,source: upstream,measured_catalog_hash,admission_basis: active_catalog_entries,active_admitted_countandactive_exception_count. A truncated recorded name also storesrecorded_name_truncated,tool_name_original_length, andtool_name_sha256. Names past the cap are not stored individually: the summary entry usestool_name: null,status: observed_unadmitted_summary, and integeromitted_name_count, with the same context fields. Active admission includes runtime break-glass entries, so an exception-admitted tool is not recorded as unadmitted; the measured catalog hash stays unchanged.Boundaries. Appended after complete discovery/classification and before the no-drift return, including when approved tools also drift. Incomplete discovery produces no observation. A successfully appended observation survives a later denial, fault or cancellation. Completed comparisons use the existing provenance-key cache and post-await guard, including concurrent first contact; this is not continuous monitoring or a global exactly-once guarantee after resets or partial write failures. The append is synchronous and uses the same persistence and exception-propagation mechanism as the sibling
catalog_driftappend. TRACE schema and tool-call transcript entries are unchanged; the original audit root is preserved, while audit length/tip andtool_transcript.hashchange as expected.Schema.
schemas/audit-entry.schema.jsonis already stale on main: it omits five runtimeEntryTypevalues (egress_denied,suspicious_call_sequence,attestation_stale,catalog_drift,break_glass_used), disallowsdetail, and requires a stringcall_id. An existingcatalog_driftentry already fails strict validation with the same errors. Aligning the schema seems like a separate change. This commit does not include that cleanup.Why
Advertised names outside the approved catalog previously left no evidence that a server offered them. The ruling asks for them to be recorded as observed-and-unadmitted, without turning them into drift or a denial.
Security impact
No change to admission or denial behavior. The new entry is evidence only; it never admits a tool or suppresses real drift.
Test plan
Local only, not GitHub CI.
tests/unit/test_unadmitted_observation_bounds.pytogether withtests/unit/test_upstream_catalog_drift.py: 107 focused passed. Local prescribed population (unit/conformance/integration/confinement): 2421 passed, 29 skipped. The persisted 65-name case records 64 individual observations plus one summary with 1 omitted; the 1,041-name case records 64 plus one summary with 977 omitted. Mutants that drop the recording, record only without drift or only on a successful terminal, count extras as drift, write the entry as catalog_drift or tool_call, ignore exceptions, or remove the post-await dedup guard each fail the tests. Local Python 3.12 only.pytestpassesruff checkpassesmypypassesDCO sign-off
Developer Certificate of Origin (https://developercertificate.org).