Repository navigation
fix(trace): align OCSF provenance to unmapped.acs.provenance and add acs.provenance.ids - #335
Conversation
ba57a6d to
821c0b6
Compare
…acs.provenance.ids
Update ocsf-mapping.json provenance_metadata to place Provenance objects
under unmapped.acs.provenance as {provenance_id, origin, source_id when
present}, add acs.provenance.ids to otel-mapping.json provenance_attributes,
and define deterministic ordering (argument keys in code-point order,
outputs by index, deduplicated by provenance_id) so cited_provenance_ids
on a decision event joins to its provenance facts on the step event.
Fixes GenAI-Security-Project#260
Fixes GenAI-Security-Project#262
Signed-off-by: Raghuvar Karthik Durga <177662415+drk-ram@users.noreply.github.com>
821c0b6 to
fabc52b
Compare
rocklambros
left a comment
There was a problem hiding this comment.
Raghuvar, your mapping change fixes the contradiction in issue #260. The old enrichments shape still fails the OCSF 1.9.0 validator with the two errors the issue lists, and your shape matches provenance.json. You regenerated the sample trace, which is the right way, and it matches what the generator writes today. Every suite passes on integration 19b1197: pytest 924 passed and 1 skipped, and 1287 Bun tests on 1.3.11 and 1.3.14.
This PR can't merge yet. It needs answers from the leads, and it has to land after PR #229.
@bar-capsule @afogel, I need your answers on issue #260 (where Provenance lives) and issue #262 (the three yes or no calls). I'm not repeating them, and this PR doesn't decide them. Ariel's issue #228 answers settle the padding and the minor bump. Running the PR showed the points below, which your answers need to cover. The last one is a new question.
-
If
acs.provenance.idsis required, doesprovenance_idgetminLength: 1? Today""passesprovenance.json, and""is also the padding value. On issue #228, Ariel asked forminLength: 1onsource_id, andprovenance_iddidn't get it. -
JavaScript's
.sort()compares UTF-16 code units, so Bun puts aU+1F600key before aU+FF5Ekey, and Python and Go do the opposite. RFC 8785, which ACS uses for hashing, also sorts by UTF-16 code units. If the rule stays code-point order, ask 1 fixes the code. If you pick UTF-16 order, the text changes. I expect this to be rare, because tool argument names are usually ASCII.Also, 14 hook schemas carry Provenance, and the rule names only
argumentsandoutputs. Issue #262 said "list fields by index".knowledgeRetrievalandmemoryContextRetrievalcarry aqueryandresultson one payload, so those need an order too. PR #359 from Eli Sherman already addscontentandparser_provenanceto this function. -
The PR compares
provenance_idonly, so if two different objects share an id, the second is dropped. I sent argumentaasp1withuser_inputand argumentbasp1withexternal. The span showed["p1"]and["user_input"]. Integration shows both origins today, and the argument names decide which one stays.§7 says the id is unique in a session, and nothing enforces that. A sender that lies can already write any origin, so this is no new attack. It matters when a framework reuses an id by mistake.
Does "reached twice" mean the same object or the same id? For the same object, comparing
provenance_id,originandsource_idkeeps both, and the reader sees["p1", "p1"]with two origins. I ran that, and the exporter and sample tests pass. -
Which span does a cited id join to? The PR says "directly" to the step span (
events.md:63,events.md:120), and it removes the sentence in requirement 3 (events.md:130) saying this join needs Guardian state. A decision can cite ids from earlier steps, though.I ran the FIDES deny from
ACS_in_action_example.mdthrough the exporter, with the retrieval as an earlier tool call. It citesp2,p4andp5, and none of them is on the gated step's span.p2is on the earlier span.p4andp5are on no span, becausederived_fromisn't on the trace. Onextend_ocsf.md, the deny citesp1, andp1is on a different request. The reference Guardian also stops at 64 objects with no marker.Should the join look across the session's step spans, and should requirement 3 keep a sentence for ids no span carries? If yes to both, these words fit: "A reader looks up each id in
acs.cited_provenance_idsinacs.provenance.idson the step spans of the same session." and "A cited id that no span carries, such as one that appears only inderived_from, still needs the Guardian's own records." (context-entry.jsonholds no provenance ids, so I wrote "own records".)
The tool-call example on extend_ocsf.md has one validator error, the missing device, which is issue #235. The deny example has none.
This PR closes issue #260 if your answer keeps unmapped.acs.provenance with no native copy. It closes issue #262 once points 2 and 4 are settled, because today the order rule and the join fall short of what that issue asks. Once you answer, Raghuvar can push once, and one of you can approve and merge after PR #229 lands.
Raghuvar, please wait for those answers and for PR #229, so you only push once. Asks 1 to 3 may change with the answers. Paths start at reference-implementations/agt/ unless they start with docs/, specification/ or tests/, and line numbers are from your current head. I ran all the asks in a scratch copy, on a real rebase over PR #229. The suites pass, apart from the PR #229 test on Bun 1.3.14 that Jason Crittenden already has to fix.
-
packages/guardian/src/otel-mapping.ts:376: a bare.sort()is UTF-16 order, and your text says code-point order. Add this aboveprovenanceObjects, and change.sort()to.sort(byCodePoint):/** Code-point order, the rule in Trace Events. A bare `.sort()` compares UTF-16 * code units, which puts U+1F600 ahead of U+FF5E. */ function byCodePoint(a: string, b: string): number { const shared = Math.min(a.length, b.length); for (let i = 0; i < shared; i += 1) { if (a.charCodeAt(i) !== b.charCodeAt(i)) { return a.codePointAt(i)! - b.codePointAt(i)!; } } return a.length - b.length; }
This sorts every key of a request, so I timed it at the 1 MiB body limit. The worst case was about 20 ms, against 13 ms for
.sort().Test: the first test in ask 2 fails without this change.
-
packages/guardian/test/otel-trace-exporter.test.ts:444: nothing tests your sort or your duplicate check. I removed each one, and every test still passed. Paste this above the test named "carries no provenance attribute when nothing in the payload has one":it("lists provenance by argument key in code-point order, whatever order the wire used", async () => { const { exporter, spans } = memoryExporter(); const argument = (id: string) => ({ value: "v", provenance: { provenance_id: id, origin: "system" } }); const request = makeEnvelope( "steps/toolCallRequest", { tool: { name: "run_shell" }, arguments: { "\u{1F600}": argument("p-U+1F600"), "\uFF5E": argument("p-U+FF5E"), ab: argument("p-ab"), b: argument("p-b"), a: argument("p-a"), }, }, { id: 1 }, ); exporter.end(exporter.start(request, "steps/toolCallRequest", TRUSTED), decisionResponse(1, { decision: "allow" })); await exporter.flush(); expect(spans()[0]?.attributes["acs.provenance.ids"]).toEqual(["p-a", "p-ab", "p-b", "p-U+FF5E", "p-U+1F600"]); }); it("lists outputs by index, and a repeated provenance_id once, at its first occurrence", async () => { const { exporter, spans } = memoryExporter(); const first = { provenance_id: "p-1", origin: "tool_output", source_id: "Bash" }; const second = { provenance_id: "p-2", origin: "retrieved" }; const request = makeEnvelope( "steps/toolCallResult", { tool: { name: "Bash" }, request_id_ref: crypto.randomUUID(), status: "success", outputs: [ { type: "text", value: "a", provenance: first }, { type: "text", value: "b", provenance: second }, { type: "text", value: "c", provenance: first }, ], }, { id: 1 }, ); exporter.end(exporter.start(request, "steps/toolCallResult", TRUSTED), decisionResponse(1, { decision: "allow" })); await exporter.flush(); const attributes = spans()[0]?.attributes; expect(attributes?.["acs.provenance.ids"]).toEqual(["p-1", "p-2"]); expect(attributes?.["acs.provenance.origin"]).toEqual(["tool_output", "retrieved"]); expect(attributes?.["acs.provenance.source_id"]).toEqual(["Bash", ""]); });
At line 399, change "in argument order" to "in argument-key order". In
test/sample-trace.test.ts:185, add this line above thesource_idcheck:expect((denied["acs.provenance.ids"] as string[]).length).toBe((denied["acs.provenance.origin"] as string[]).length);
If the leads answer that the order or the duplicate rule changes, change the expected values to match.
-
These places still describe the old behavior, and an implementer who reads them builds the wrong exporter.
docs/spec/trace/extend_opentelemetry.md:23: change "the two Provenance columns" to "the three Provenance columns".docs/spec/trace/extend_opentelemetry.md:266: change "MUST carryacs.provenance.originand SHOULD carry" to "MUST carryacs.provenance.idsandacs.provenance.originand SHOULD carry". Today this line contradicts yourevents.md:63.packages/guardian/README.md:63: change "the span carriesacs.provenance.originas an array, one element per Provenance object in wire order." to "the span carriesacs.provenance.idsandacs.provenance.originas arrays, one element per Provenance object: arguments by key in code-point order, then outputs by index, with a repeatedprovenance_idlisted once." Then change "Both stop at 64 objects." to "All three stop at 64 objects."packages/guardian/src/otel-mapping.ts:352: change "in wire order" to "in the order provenanceObjects gives". At line 355, say thatacs.provenance.idsis always complete too, becauseprovenance.jsonrequiresprovenance_id.specification/v0.1.0/trace/ocsf-mapping.json:177: change "the OCSF event MUST carry" to "the OCSF step event MUST carry", so it matches yourevents.md:120. The decision example doesn't carry Provenance, andacs_fieldsdoesn't list it.
Test:
uv run mkdocs build --strict, then the tests in ask 2 and ask 4. -
tests/test_trace_mapping_defaults.py: your test pins the mapping lists and nothing checks the example against them. OCSF doesn't validateunmapped, so I put junk there and the validator returned 0 errors. Addimport reat the top, and add this test at the end:def test_ocsf_example_provenance_has_the_fields_the_mapping_names(): """OCSF does not validate `unmapped`, so nothing else checks this shape.""" page = (TRACE.parents[2] / "docs" / "spec" / "trace" / "extend_ocsf.md").read_text(encoding="utf-8") heading = "## Example: tool call as Process Activity (1007)" block = re.search(r"```json\n(.*?)```", page[page.index(heading):], re.S) entries = json.loads(block.group(1))["unmapped"]["acs"]["provenance"] fields = json.loads((TRACE / "ocsf-mapping.json").read_text(encoding="utf-8"))["properties"]["provenance_metadata"]["properties"] required = set(fields["required_fields"]["default"]) allowed = required | set(fields["optional_fields"]["default"]) assert entries for entry in entries: assert required <= set(entry) <= allowed
Test: delete
"provenance_id": "p1",from the example inextend_ocsf.md, and this test fails. -
In the PR description, "Breaking for implementers?" says no. Please change it to yes, because an exporter that claims ACS-Trace and doesn't emit
acs.provenance.idsstops conforming. Say that the order of the columns changes too, and thatrequired_enrichmentsandoptional_enrichmentsare renamed. The next release notes need that. -
When PR #229 has merged, do this rebase first, and then make the asks above, so the rebase can't lose them:
git fetch upstream git rebase upstream/integrationI wrote
upstreamfor this repository, so use your own name for that remote if it differs. Git stops ondocs/spec/trace/extend_opentelemetry.mdandsamples/trace/acs-trace-sample.jsonl. Both conflicts are generated text, so please don't merge them by hand. In a rebase,--oursmeans theintegrationside:git checkout --ours -- docs/spec/trace/extend_opentelemetry.md reference-implementations/agt/samples/trace/acs-trace-sample.jsonl git add docs/spec/trace/extend_opentelemetry.md reference-implementations/agt/samples/trace/acs-trace-sample.jsonl git rebase --continue cd reference-implementations/agt bun run sample:traceThen copy the span with
acs.request_id1b6f0c2a-3d4e-4f5a-8b6c-7d8e9f0a1b03from the new sample into the JSON block under## Example: a denied tool call, with two-space indents. Make the asks, commit, and push withgit push --force-with-lease.Test:
uv run pytest tests/test_trace_docs_example.pyandbun test test/sample-trace.test.ts. Please run both. The first one still passes if you skipbun run sample:trace, and only the second one catches that.PR #359 changes
provenanceObjectsand the sample too. If it lands first, keep both changes and runbun run sample:traceagain.
GitHub still won't let me assign you issue #260 or issue #262. Please accept the repository invite and leave a comment on each issue, as I asked on PR #334, because I can't merge until you're assigned. After you push, I'll run all of this again.
|
@drk-ram, answered on #260 and #262. Go ahead after #229 lands, following Rock's rebase steps, with three changes to his asks: sort in RFC 8785's order (a bare .sort() already does that in JavaScript, so ask 1 goes away), list two different objects that share an id separately, and add the native enrichments copy from #260. |
What changed
Aligns
specification/v0.1.0/trace/ocsf-mapping.jsonwithdocs/spec/trace/extend_ocsf.mdby placing Provenance objects underunmapped.acs.provenanceas{provenance_id, origin, source_id when present}(required_fields: ["provenance_id", "origin"],optional_fields: ["source_id"]), addsacs.provenance.idstospecification/v0.1.0/trace/otel-mapping.json(provenance_attributes.required), and defines deterministic ordering (argument keys in code-point order, outputs by index, deduplicated at first occurrence byprovenance_id) across the Trace docs, exporter, conformance checker, and sample trace socited_provenance_idson a decision event resolves to its provenance facts on the step event.Which issue does this implement
Fixes #260
Fixes #262
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
Specification changes
Breaking for implementers? Yes — an exporter claiming ACS-Trace conformance must now emit
acs.provenance.idsalongsideacs.provenance.origin, the provenance arrays are emitted in deterministic order (arguments by key in code-point order, then outputs by index, with a repeatedprovenance_idlisted once), andrequired_enrichments/optional_enrichmentsinocsf-mapping.jsonare renamed torequired_fields/optional_fieldsunderunmapped.acs.provenance.I tested this
uv run pytest -vpasses on my machineuv run mkdocs build --strictpasses on my machineChecklist
git commit -s(required by the DCO)Security