Skip to content

fix(ci): write Cloud config to 0600 temp file with cleanup trap - #536

Merged
margaretjgu merged 2 commits into
mainfrom
fix/buildkite-config-file-perms
Aug 12, 2026
Merged

fix(ci): write Cloud config to 0600 temp file with cleanup trap#536
margaretjgu merged 2 commits into
mainfrom
fix/buildkite-config-file-perms

Conversation

@margaretjgu

Copy link
Copy Markdown
Member

Closes #519.

@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

MegaLinter analysis: Success

Descriptor Linter Files Fixed Errors Warnings Elapsed time
✅ BASH shellcheck 1 0 0 0.29s
✅ COPYPASTE jscpd yes no no 0.54s
✅ REPOSITORY gitleaks yes no no 57.11s
✅ REPOSITORY git_diff yes no no 0.04s
✅ REPOSITORY secretlint yes no no 1.7s
✅ REPOSITORY trivy yes no no 21.94s

Notices

📣 MegaLinter 9.5.0 is out! Discover the new features and security recommendations in the release announcement. (Skip this info by defining SECURITY_SUGGESTIONS: false)

See detailed reports in MegaLinter artifacts
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security
Show us your support by starring ⭐ the repository

@github-actions

Copy link
Copy Markdown
Contributor

The fix looks correct and is a genuine security improvement. A few real issues worth flagging:

Bug: trap won't fire in a sourced script

The script is sourced (uses return not exit). In bash, trap EXIT fires when the shell process exits, not when a sourced script returns. The temp directory will persist for the entire lifetime of the CI job's shell session, not just the script's execution. The comment says "removing the temp config for the lifetime of the CI job" which is actually the correct description of what happens — but if the intent is to clean up after the job finishes, this is fine. If the intent were to clean up when the sourced script returns, this wouldn't work.

Minor but real: node -e process substitution is fragile

If node isn't in PATH or fails, $CI_CONFIG_FILE will be empty/incomplete with no error surfaced (exit code from the pipeline is the printf exit code, not node's). Consider adding set -o pipefail before this block or checking the file was written:

set -o pipefail
printf '%s' "$EC_API_KEY" | node -e '...' > "$CI_CONFIG_FILE"

Mild concern: node for JSON construction is heavyweight

Using node just to JSON-serialize a string is over-engineering. The API key from Vault is almost certainly a base64 string (no characters needing JSON escaping). A heredoc with jq (if available) or even careful quoting would be simpler. But if node is guaranteed available in CI context and the goal is correctness against arbitrary key formats, it's defensible.

The core fix is sound: temp dir at 0700, file at 0600, unset EC_API_KEY, printf instead of interpolation in heredoc — all correct.

@margaretjgu
margaretjgu merged commit e4f407c into main Aug 12, 2026
30 checks passed
@margaretjgu
margaretjgu deleted the fix/buildkite-config-file-perms branch August 12, 2026 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ECLI-013: Buildkite writes Cloud admin key to config file without 0600 permission or cleanup

2 participants