fix: stop DEBUG from printing the GitHub token into the build log - #20
fix: stop DEBUG from printing the GitHub token into the build log#20bradAGI wants to merge 2 commits into
Conversation
The scanner is the whole product here — it resolves a release, verifies it, runs the engine, scales the score, and decides whether the build fails — and until now nothing checked any of it. A regression in the gate logic or in the jq that reads the engine's ScanResult would ship silently. The suite drives the real scan/trustabl-scan.sh. Each test builds an actual gzipped release tarball containing a stub engine and a real checksums.txt over it, then puts a stub curl on PATH that serves that directory by URL basename. The download, sha256 verification, extraction and invocation therefore all run unmodified; only the network and the engine binary are substituted. The fixtures are unmodified output from a real `trustabl scan` (engine v0.1.7), so the assertions pin the scanner against the ScanResult shape the engine actually emits rather than a hand-written approximation of it.
DEBUG=true runs the whole script under set -x, and bash echoes each command
after expansion. Three of those commands carried GITHUB_TOKEN:
+ AUTH=(-H "Authorization: Bearer ${GITHUB_TOKEN}")
+ curl -fSL -H 'Accept: application/octet-stream' \
-H 'Authorization: Bearer ghp_TESTSECRETVALUE' ...
+ curl -fsSL -H 'Authorization: Bearer ghp_TESTSECRETVALUE' ...
CodeBuild and CodeCatalyst logs are readable by anyone with access to the
project, so a token the README tells people to store as a secret ends up in
plaintext next to the scan output — and stays there for the log's retention.
The token is now expanded in exactly one place: auth_curl, which saves xtrace,
turns it off for the duration of the call, and restores it. Holding the header
in an AUTH array could not be made safe, because the assignment is traced too,
so the array is gone and the header is built inside the wrapper from
GITHUB_TOKEN directly.
xtrace is restored rather than left off, so DEBUG still traces everything else:
287 trace lines before, 289 after, with token occurrences going 3 -> 0.
There was a problem hiding this comment.
Confirmed the bug on main: set -x at scan/trustabl-scan.sh:30, the token expanded at :39, and three curl calls at :60, :85, :90. Under xtrace bash echoes commands after expansion, so DEBUG=true writes the credential in plaintext to CloudWatch Logs on CodeBuild and to the run output on CodeCatalyst. Both persist past the build. Your description of the four copies matches what the script actually does.
The auth_curl approach is the right one. The token is expanded in exactly one place, and because nothing is written to disk there is no cleanup path that can be missed on an abnormal exit. case "$-" in *x*) is also the correct way to check xtrace state rather than inferring it from DEBUG, which keeps working if something else turns tracing on later. The call site stays traced, so DEBUG loses almost nothing.
The regression test is what makes this mergeable rather than just correct:
assert_not_contains "trace" "$SCAN_OUT" "ghp_TESTSECRETVALUE"That is the test that stops this coming back.
One nit, non-blocking: set -e is active at that point, so a failing curl exits before rc=$? runs and xtrace is never restored. The shell is on its way out so it does not matter in practice, but the rc capture is dead code on that path.
One process note rather than a code comment: this carries #1 and roughly 1,100 lines of test harness with it. I would rather #1 were reviewed on its own merits than merged as a side effect of a security fix. Happy for both to land, just not for #1 to arrive unexamined.
Before this merges, please pull in the README paragraph from #40 about storing GITHUB_TOKEN via Secrets Manager or Parameter Store rather than as a plaintext environment variable. It covers an exposure this code change does not.
This repo had no CI while taking external pull requests, so every incoming change was reviewed by eye with nothing verifying it. That is also why branch protection here could not require a status check. Three gates: bash -n across every script, shellcheck at -S error, and a YAML parse of buildspec.yml and the CodeCatalyst workflow - the two files AWS actually consumes, where a malformed one fails in a customer's pipeline rather than here. shellcheck runs at error severity only. A required check that argues about quoting preferences gets disabled within a week. Named lint.yml rather than ci.yml so it coexists with the test harness in #20 instead of colliding with it.
|
Thanks @bradAGI. Approving. Confirmed on
One nit, non-blocking: |
sairenchristianbuerano
left a comment
sairenchristianbuerano
left a comment
There was a problem hiding this comment.
Thanks @bradAGI — approving the substance, but it needs a rebase.
Two conflicts now that #7 and #11 are on main:
scan/trustabl-scan.sh— #7 rewrote the checksum block yourauth_curl
wrapper wraps, aroundif auth_curl -fsSL -o "$DEST/checksums.txt"test/run-tests.sh— #11 added tests at the same spot as
test_debug_does_not_echo_the_github_token. That one is additive; both sets
of tests should survive
Worth flagging on the test file: git splits the conflict mid-function there, so a
naive "keep both sides" silently drops a closing brace and bash -n only
complains 30 lines later. I hit exactly that resolving #21. Run
bash test/run-tests.sh after — all 19 pass on main today, and you should see
20 with yours.
The fix itself is unchanged and still correct: DEBUG=true puts the token in
CloudWatch four times over, and the regression test is what makes it stick.
2 participants
The leak
DEBUG=trueruns the whole script underset -x, and bash echoes each command after expansion. Three of those commands carryGITHUB_TOKEN:CodeBuild and CodeCatalyst logs are readable by anyone with access to the project, and they persist for the log group's retention. So a credential the README explicitly tells people to store as a secret ends up in plaintext beside the scan output — and the person who turned on
DEBUGwas debugging a scan, not thinking about credentials.The fix
The token is now expanded in exactly one place —
auth_curl— which savesxtrace, disables it for the call, and restores it.The
AUTHarray could not be made safe and is gone. That is the part worth noting: holding the header in a variable does not help, because the assignment is traced too. The header is now built inside the wrapper fromGITHUB_TOKENdirectly, so there is no traced line anywhere that expands it.xtraceis saved and restored rather than switched off wholesale, soDEBUGstill does its job.Measured
Same scan,
DEBUG=true GITHUB_TOKEN=ghp_TESTSECRETVALUE, before and after:mainTracing is intact — the calls still appear, just without the header:
Test
The new test asserts both halves, so it cannot pass vacuously:
Before:
not ok — trace: output unexpectedly contained 'ghp_TESTSECRETVALUE'. After:all 16 test(s) passed.Related: #19 documents
DEBUGand, until this lands, warns about exactly this. If both merge, that note can be softened.