fix(enroll): sixth audit — umask, TOCTOU, atomic AGE_PUB, stale tmp cleanup, step-12 polish#178
Merged
Merged
Conversation
…leanup, step-12 polish - umask 077 at script top: all file creates default to owner-only permissions, closing the window between creation and explicit chmod 600 for HW_CONFIG.tmp, ENROLL_NIX.tmp, and any future additions - Use `install -m 600` instead of `cp` + `chmod 600` for COMPOSE_ENV: closes TOCTOU window where .env with plaintext passwords is world-readable from cp until the separate chmod call - Atomic AGE_PUB write: `printf > .tmp && mv` consistent with all other file writes in the script; echo > AGE_PUB was the last non-atomic write - Stale .tmp cleanup: add `rm -f` at start of steps 8 and 9 for MINISIGN_CACHE_INFO.tmp and ENROLL_NIX.tmp, matching the pattern established for HW_CONFIG.tmp in step 1 - docker-compose consistency: info message in step 4 used `docker compose` (v2 plugin syntax) while the script requires `docker-compose` (v1); operator copy-paste would fail if only v1 is installed - Step 12 sleep 10 → 30s poll loop: `systemctl is-active` polled every 5s up to 30s; faster on the hot path, reliable on the slow path - _SIG_TMP trap in step 12: mktemp'd file now covered by EXIT trap cleared after rm; prevents leak if set -e fires inside the minisig verify block - nix copy URL: add ?compression=zstd to match katello-sourceos-setup.sh docs hint; all three call sites updated for consistency
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Findings and fixes
umask 077— all file creates expose a world-readable window beforechmod 600umask 077at line 16.envcreation:cpthenchmod 600leaves plaintext passwords world-readable brieflyinstall -m 600replacescp+chmodecho "${AGE_PUBKEY}" > "${AGE_PUB}"was the last non-atomic file write in the scriptprintf > .tmp && mv.tmpcleanup at start of steps 8 and 9 forMINISIGN_CACHE_INFO.tmp/ENROLL_NIX.tmprm -f *.tmpat each step startinfomessage in step 4 useddocker compose(v2) but script requiresdocker-compose(v1)docker-composesleep 10in step 12 is a blind wait — may under-shoot on loaded systemssystemctl is-activeevery 5s)_SIG_TMPmktemp'd file in step 12 not covered by a trap — leaks onset -eexittrap "rm -f ..." EXIT+trap - EXITafter cleanupnix copyURL inconsistent with docs (?compression=zstdmissing)Test plan
bash -n scripts/enroll.sh— no syntax errorsumask 077: addtouch /tmp/enroll-test-$$; stat -c %a /tmp/enroll-test-$$after umask line →600.envfile perms:stat -c %a ${COMPOSE_ENV}immediately after creation →600AGE_PUB.tmpdisappears after step 3 (onlyage.pubremains)nix-cache-info.tmpcleaned on re-runsourceos-syncdactive within 30s on healthy system