Repository navigation
fix(agt): create envelope, session-context, audit, and session-config files with 0600/0700 modes - #343
Conversation
|
This pull request's description says it closes #71, which is on the project roadmap. When it merges, GitHub closes that issue and the roadmap counts it as delivered. Please confirm the change delivers the whole issue before merging. |
rocklambros
left a comment
There was a problem hiding this comment.
Raghuvar, your modes work on a new install. I ran the real Guardian and the real host hook under umask 000, then listed .acs/. On integration 19b1197 the envelope, session and audit files were 0666 and the directories 0777. With your PR every file is 0600 and every directory is 0700, on Bun 1.3.11 and 1.3.14.
The third file from my note on issue #71 is also fine: ACS_OTEL_FILE lands 0600 and holds no copy of the key. Every suite passes on the merged tree, with 1289 Bun tests (the old 1287 plus your 2).
I need the changes below. None of them depends on the leads, so please push them now, all in one push. All paths are under reference-implementations/agt/. I ran all four in a scratch copy, and the suites pass.
-
An old log stays open. The
modeoption only works when the call creates the file, sopackages/guardian/src/envelope-log-sink.ts:145,packages/guardian/src/server.ts:398andpackages/host-adapter/src/audit-sink.ts:326leave an existing file as it was. Marco Bellingeri measured his files at 644 on issue #71. I ran that case with your PR: the log stayed 0644, and the key count in it went from 1 to 4. Issue #283 already sets the rule for the OTLP file, which is to tighten and not refuse. Please use the same rule here.Add this above
createEnvelopeLogSinkinenvelope-log-sink.ts, and importcloseSync,fchmodSync,fstatSyncandopenSync:/** * Appends to a file only its owner can read. The mode on `openSync` applies * only when this call creates the file, so a log left by an older version * keeps its 0644. `fchmodSync` tightens that log too. Windows has no such * bits, and a device such as /dev/null is not ours to change. */ export function appendPrivate(path: string, text: string): void { const fd = openSync(path, "a", 0o600); try { if (process.platform !== "win32" && fstatSync(fd).isFile()) { fchmodSync(fd, 0o600); } appendFileSync(fd, text); } finally { closeSync(fd); } }
Call
appendPrivateat line 145. Inserver.ts, import it from./envelope-log-sink.ts, call it at line 398, and removeappendFileSyncfrom line 66. Inaudit-sink.ts, add the same function and imports withoutexport, and call it at line 326, because that file imports nothing from the Guardian.Keep your
mkdirSynclines as they are, and do not chmod a directory that already exists.ACS_ENVELOPE_LOG=/tmp/x.jsonlwould then try to change/tmp. Leaveotlp-file-exporter.tsalone, because issue #283 covers it.Test: the tests in ask 2 fail without this change.
-
server.ts:388,server.ts:398,session-config.ts:166andsession-config.ts:169have no test. I removed the mode from each one, and all 1289 tests still passed. Your two tests also read whatever umask the shell has, so underumask 077they cannot see a missing mode.In your two tests (
packages/guardian/test/envelope-log-sink.test.ts:173,packages/host-adapter/test/audit-sink.test.ts:108), addconst previous = process.umask(0);beforetry, and addprocess.umask(previous);as the first line offinally. Then add these lines after the lastexpect, and importchmodSync. In the audit test, usecreateAuditSink({ path }).write(EVENT)on the third line.// A log an older version left behind, opened by a new sink. chmodSync(path, 0o644); expect(createEnvelopeLogSink({ path }).write("request", REQUEST, "steps/toolCallRequest")).toBe(true); expect((statSync(path).mode & 0o777).toString(8)).toBe("600"); expect(readFileSync(path, "utf8").trim().split("\n")).toHaveLength(2);
After each of those tests, add this one. The audit version uses
createAuditSink({ path: "/dev/null" }).write(EVENT).it.skipIf(process.platform === "win32")("writes to a device without changing its mode (#71)", () => { expect(createEnvelopeLogSink({ path: "/dev/null" }).write("request", REQUEST, "steps/toolCallRequest")).toBe(true); });
Add this after the test at
packages/guardian/test/server.test.ts:1364, and importchmodSyncandstatSync:it.skipIf(process.platform === "win32")("keeps the session context log private, new or old (#71)", async () => { const dir = join(tmpdir(), `acs-session-dir-${crypto.randomUUID()}`); const path = join(dir, "session-context.jsonl"); const sessionId = crypto.randomUUID(); const previous = process.umask(0); const guardian = await startGuardian({ ...baseOptions, sessionContextLog: path }); const step = () => postStep(guardian, toolCallRequest({ session_id: sessionId, request_id: crypto.randomUUID() })); try { await step(); expect((statSync(dir).mode & 0o777).toString(8)).toBe("700"); expect((statSync(path).mode & 0o777).toString(8)).toBe("600"); chmodSync(path, 0o644); await step(); expect((statSync(path).mode & 0o777).toString(8)).toBe("600"); } finally { process.umask(previous); await guardian.close(); rmSync(dir, { recursive: true, force: true }); } });
Add this as the first test in the
describeatpackages/host-adapter/test/session-config.test.ts:40, and importstatSync:it.skipIf(process.platform === "win32")("writes the file 0600 and creates its directory 0700 (#71)", () => { const dir = join(scratch(), "sessions"); const path = sessionConfigPath(dir, "sess-1"); const previous = process.umask(0); try { createFileSessionConfigStore({ dir, sessionId: "sess-1" }).set(HELLO); expect((statSync(dir).mode & 0o777).toString(8)).toBe("700"); expect((statSync(path).mode & 0o777).toString(8)).toBe("600"); } finally { process.umask(previous); unlinkSync(path); rmdirSync(dir); } });
Test:
bun testfromreference-implementations/agtshows 1293 tests. If you undo ask 1, or remove the mode from amkdirSynccall or fromsession-config.ts:169, a test fails. -
README.md:370promises 0600 for log files, and today that holds only for a new file. Replace everything after "Nothing is stripped or redacted," with:so the file holds raw tool arguments and tool outputs. The Guardian creates a new `.acs/` directory with mode `0700`, and sets the log to mode `0600` on every write. An `.acs/` directory that already exists keeps its mode. The file is gitignored. Do not commit it.Test:
git grep -n 'log files with mode' reference-implementations/agt/README.mdprints nothing. -
Please change
Fixes #71toPart of #71in the issue section, and in your commit message too. The issue's title names two problems, and this PR answers the second one.I ran Marco's flow on the merged tree: the model got
AWS_ACCESS_KEY_ID=[REDACTED], andparams.payload.outputs[0].valuein.acs/envelopes.jsonlstill held the key. Your README line says the same thing, and that line is right.packages/guardian/README.mdlinks issue #71 for the raw content, andREADME.md:305links it for the collector's 0644 file, which this PR doesn't touch. Closing the issue would point both at a closed issue. If the leads say below that file modes are all this issue needs, you can change it back.Test:
closing-choicestays green. I ran it on the edited description and it passes.
PR #112 rewrites the same two Guardian writes, so whichever PR lands second has to rebase. Its file-log-permissions.test.ts keeps an existing file's mode, which is the opposite of ask 1 and issue #283, so that test has to change too.
@bar-capsule @afogel, one scope question on issue #71. It's a P1, it has no assignee, and it isn't on the issue #32 list.
Can raw content stay in .acs/envelopes.jsonl at mode 0600, or must it stop reaching the disk?
These facts come from running the merged tree:
- The key lands in three fields:
arguments.command.valueandraw_commandon the request, andoutputs[0].valueon the result. The Guardian answersmodifyat both gates, after the raw line is already written (on integration,server.ts:577runs beforedispatchat line 598). - That order is deliberate, so an envelope that fails the schema stays visible.
envelope-log-sink.ts:10-12says the log does no redaction, and the Inspector reads this file. - The hash chain doesn't depend on the raw content, because
session-contextentries hold no payload. - File modes stop a second account. They don't stop the agent being governed, which runs as the same account. With the log at 0600,
base64 -i .acs/envelopes.jsonlpassed both gates and the key decoded from the output. The same command on the source filecreds.envalso passed, so the log isn't the only path. What the log adds is every value from every earlier session in one file.
If raw content can stay, Raghuvar can go back to Fixes #71 after these asks, and the issue's title should change. If it must stop, this PR stays Part of #71 and the rest of issue #71 needs an owner.
… files with 0600/0700 modes Pass mode 0o700 to mkdirSync and tighten existing log files to 0o600 via appendPrivate (openSync + fchmodSync) in envelope-log-sink.ts, server.ts (createSessionContextLogAppender), and audit-sink.ts, plus mode 0o600 on session-config.ts writeFileSync, and update reference-implementations/agt/README.md. Part of GenAI-Security-Project#71 Signed-off-by: Raghuvar Karthik Durga <177662415+drk-ram@users.noreply.github.com>
2af86e4 to
fea3ad1
Compare
|
@rocklambros Addressed all four asks in
|
There was a problem hiding this comment.
Raghuvar, all four asks are done, and the code works. I ran your head fea3ad1 merged on integration 1b8f970.
Ask 1 works. An old 0644 log becomes 0600 on the next write, for the envelope log, the session context log and the audit log. A new install is still 0600 and 0700 under umask 000 and 022, on Bun 1.3.11 and 1.3.14.
Ask 2 works too. Your tests no longer depend on the shell's umask. I broke the code on purpose in 13 ways, and a test failed for 12 of them. The other one is the mode on openSync, which fchmodSync covers, as I expected.
Asks 3 and 4 are done, and closing-choice passes with Part of #71.
Every suite passes: pytest 924 passed and 1 skipped, and 1293 Bun tests with 0 failures on both Bun versions.
I found one limit in the code I asked you to write, and I accept it. Some file systems refuse a mode change. When that happens, the sink stops writing and prints an error, the same way it does when the disk is full.
On the host, a step under proceed is then blocked during a Guardian outage, because the host proceeds only with an audit line. I forced this failure to check it. It fails closed, so you don't need to change anything.
I'm approving this PR.
@bar-capsule @afogel, the scope question from my last review is still open: can raw content stay in .acs/envelopes.jsonl at mode 0600, or must it stop reaching the disk?
Your answer decides issue #71, and it doesn't change this code. Please answer first. This PR waits for your answer. If raw content can stay, Raghuvar changes that line to Fixes #71 first, and the issue closes with the merge. If it must stop, this PR stays Part of #71 and the rest of the issue needs an owner.
What changed
Sets
mode: 0o700onmkdirSyncandmode: 0o600onappendFileSync/writeFileSyncacrossenvelope-log-sink.ts,server.ts(createSessionContextLogAppender),audit-sink.ts, andsession-config.ts(matchingotlp-file-exporter.ts), notes inreference-implementations/agt/README.mdthat.acs/envelopes.jsonlholds raw tool outputs as well as raw tool arguments, and adds mode tests inenvelope-log-sink.test.tsandaudit-sink.test.ts.Which issue does this implement
Part of #71
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
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
Restricts newly created
.acs/directories (0700) and log/session files (0600) to the owning user so raw tool arguments and tool outputs written before redaction are not world-readable under default process umasks.