[krishsharma-code]

Summary

Hardens bash_scripts/backup_critical_configs.sh and adds the first pytest coverage for a shell script in this repo. The old script tarred /etc/shadow into /tmp/security_backups with whatever mode mkdir -p happened to produce, discarded tar's stderr, and never checked what it had actually written. The rewrite makes the archive owner-only, verifies it before publishing, fails loudly when a source is unreadable, and adds --dry-run, --keep, --files and --backup-dir.

Linked Issue

N/A - raised directly by the recurring code-quality audit (no tracking issue opened).

Type of Change

• Bug fix
• Documentation update
• Refactor (no behaviour change)
• Chore / tooling / CI

What Changed

• Owner-only output. umask 077, mkdir -p followed by an explicit chmod 0700 on the backup directory (the chmod is not redundant - a pre-existing 0755 directory is tightened), archive written via mktemp at mode 0600, and a .sha256 sidecar at 0600. A backup of /etc/shadow in a world-readable directory is a local privilege-escalation gift.
• Verified before published. The archive is built under a temporary name inside the destination directory, checked with gzip -t, and its entry count compared to the source count. Only then is it renamed into place, so a truncated file is never mistaken for a good backup. A trap ... EXIT removes the temporary file on any early exit.
• Unreadable sources are fatal. Missing files (e.g. /etc/network/interfaces on a systemd-networkd host) are skipped with a warning; files that exist but cannot be read fail with exit 2 and a "re-run with sudo" hint, because a backup silently missing the password database is worse than no backup. --allow-partial opts into skipping.
• No more swallowed errors. 2>/dev/null on the tar call is gone; tar's own diagnosis is the only explanation a failure gets. set -euo pipefail throughout, and -- before paths so a filename cannot be read as an option.
• Correct failure detection. The old if [ $? -eq 0 ] after a command that always ran in a subshell-free context still left a partial archive on disk. Now nothing is published unless verification passes.
• Retention and collision safety. --keep N (default 5) prunes oldest-first using the fixed-width timestamp in the filename; two runs in the same second no longer overwrite each other.
• Interface. -h/--help, --dry-run, -d/--backup-dir, -f/--files, -k/--keep, -p/--allow-partial, documented exit codes (0/1/2/3/4). Dry run prints the plan and writes nothing, including no pruning.
• Docs. Header rewritten with the what/why, security notes, restore procedure and exit-code contract; README index entry updated and a "Running the tests" section added.
• Tests and CI. tests/test_backup_critical_configs.py (16 cases) drives the script through its real CLI with throwaway directories - no root, no /etc. .github/workflows/ci.yml gains a pytest job so the suite actually gates PRs.

New or Modified Scripts

Script	Language	Change
bash_scripts/backup_critical_configs.sh	Bash	Hardening rewrite: owner-only output, verification, retention, flags
tests/test_backup_critical_configs.py	Python	New 16-case pytest suite
.github/workflows/ci.yml	YAML	New pytest job
README.md	Markdown	Index entry + test instructions

Safety Review

• This script only performs actions the user explicitly requests
• Any destructive operation requires an explicit flag and supports --dry-run (pruning is bounded by --keep; --dry-run suppresses it)
• Any state-changing action is printed before it is carried out
• No credentials, API keys or tokens are committed
• No hardcoded personal or machine-specific paths

Testing Checklist

• python3 -m py_compile python_scripts/*.py passes
• flake8 python_scripts --select=E9,F63,F7,F82,F821,F823 --max-line-length=120 passes
• for f in bash_scripts/*.sh; do bash -n "$f"; done passes (83 scripts)
• shellcheck bash_scripts/*.sh reports no errors (advisory) - shellcheck unavailable in the audit sandbox
• I ran the new or changed script end to end on a machine I am permitted to test
• I tested the --help output
• I tested the failure path (missing argument, bad path, no permissions)
• python3 -m pytest tests -q -> 91 passed
• No new console warnings or unhandled errors

Documentation

• I added the script to the index in README.md in the correct numeric position
• The script has a module docstring / header comment explaining what it does
• Non-obvious logic is commented

Code Quality Checklist

• My code follows the standards in CONTRIBUTING.md
• Imports are at the top of the file, not inside functions
• I have performed a self-review of my own code
• I have removed debugging code and unused imports

Additional Notes

Known limitation, unchanged by this PR: the archive is still written under /tmp by default. /tmp is often tmpfs, so backups vanish on reboot and are not a durable recovery point - --backup-dir /var/backups/configs is the sensible production choice, and a follow-up could change the default. Members are stored with the leading slash stripped, so restoring is sudo tar -xzf <archive> -C / after checking the sidecar.

[krishsharma-code]

Review: hardening of backup_critical_configs.sh

Reviewed the full diff of #4 (4 files, +651/−21) against the four axes of the audit: security boundaries, memory efficiency, edge cases and failure handling, and documentation clarity. Self-review only — GitHub does not let an author approve their own PR, so this is submitted as COMMENT.

What was found in the original script

1. Secret exposure (high). BACKUP_DIR="/tmp/security_backups" was created with bare mkdir -p, so the directory mode followed the caller's umask — commonly 0755. The archive contains /etc/shadow. Any local user could read the password hashes of every account on the box. This is the defect that motivated the PR.
2. Unverified output (medium). The tar exit status was checked but nothing else: no gzip integrity test, no check that the expected members were present. A tar that exits 0 with a truncated stream would still be reported as SUCCESS.
3. Swallowed diagnostics (medium). tar ... 2>/dev/null discarded the only explanation of a failure, and the script then printed "Check permissions." unconditionally.
4. Brittle file list (low-medium). tar -czf ... $FILES_TO_BACKUP relied on unquoted word splitting, and a single missing path (e.g. /etc/network/interfaces on a systemd-networkd host) made tar exit non-zero, so a healthy backup reported failure and left a partial archive on disk.
5. No interface (low). No --help, no destination override, no retention — /tmp filled up over time. TIMESTAMP also collided for two runs in the same second, silently overwriting.

What changed and why

• umask 077 at the top plus an explicit chmod 0700 on the directory after mkdir -p, because mkdir -p leaves an existing directory's mode untouched — the chmod is the part that actually fixes a pre-existing 0755 dir.
• Archive written with mktemp inside the destination directory (mode 0600 by umask, and same filesystem so the publish is a single atomic rename), verified with gzip -t and an entry-count comparison, then mv -n into place. trap cleanup EXIT removes the temporary file on any early exit.
• Sources are collected into a bash array: missing files are warned about and skipped, present-but-unreadable files are fatal (exit 2) with a sudo hint, because a backup missing /etc/shadow that reports success is the failure mode that matters. --allow-partial opts into skipping.
• set -euo pipefail, -- before paths so a filename cannot be parsed as an option, and tar's stderr left intact.
• --keep N retention (default 5) using the fixed-width timestamp in the filename; a _1, _2 suffix prevents same-second overwrites.
• 16 pytest cases driving the real CLI with throwaway directories, plus a pytest job in CI so the suite gates future PRs.

Verification

bash -n on all 83 shell scripts, py_compile on all Python scripts, flake8 --select=E9,F63,F7,F82,F821,F823, and pytest tests -q → 91 passed. Manual end-to-end runs covered success, missing source, unreadable source, --dry-run and default-file-list preflight.

Not fixed here

The default destination is still /tmp, which is usually tmpfs — those backups do not survive a reboot and are not a real recovery point. Changing the default is a behaviour change for anyone with a cron entry, so it is left as a documented limitation and a follow-up.

[krishsharma-code]

The chmod after mkdir -p is deliberate, not redundant: mkdir -p on an existing directory leaves its mode alone, so a backup directory that some earlier version created at 0755 would stay world-readable and keep exposing the /etc/shadow copy inside it. Worth keeping this comment so nobody "simplifies" it away later.

[krishsharma-code]

Good: the temp file is created inside $BACKUP_DIR rather than in /tmp. That keeps the later mv on the same filesystem, which is what makes the publish atomic — a cross-device mv would degrade to copy-then-delete and reintroduce the partial-file window.

[krishsharma-code]

This is the check that was missing before. tar exiting 0 only means it did not report an error; it does not prove the gzip stream is intact or that every intended member is present. Pairing gzip -t with the entry-count comparison below is what makes the SUCCESS message trustworthy.

[krishsharma-code]

Making an unreadable source fatal is the right call. The alternative — warn and continue — produces an archive that looks complete, carries a valid checksum and is missing /etc/shadow, and nobody discovers that until a restore is needed. --allow-partial keeps the old lenient behaviour available for people who genuinely want it.

[krishsharma-code]

mapfile rather than archives=($(...)) so a backup directory path containing a space survives the sort. Since the timestamp is fixed width, plain lexical sort is chronological and no stat/date parsing is needed.

[krishsharma-code]

Two notes for reviewers: the sidecar is written with a redirect, so its mode comes from the surrounding umask 077 plus the explicit chmod on the next line — please keep both if this is refactored. Also, the SHA-256 is computed from the archive after it has been published; since the archive was verified and renamed atomically, that ordering is safe here.