feat(groq): A whimsical Bash utility that safely rotates SSH host keys, backing up old keys and creating fresh placeholders.

[polsala]

Implementation Summary

• Utility: nightly-ssh-key-rotator
• Provider: groq
• Location: bash-utils/nightly-nightly-ssh-key-rotator-5
• Files Created: 3
• Description: A whimsical Bash utility that safely rotates SSH host keys, backing up old keys and creating fresh placeholders.

Rationale

• Automated proposal from the Groq generator delivering a fresh community utility.
• This utility was generated using the groq AI provider.

Why safe to merge

• Utility is isolated to bash-utils/nightly-nightly-ssh-key-rotator-5.
• README + tests ship together (see folder contents).
• No secrets or credentials touched.
• All changes are additive and self-contained.

Test Plan

• Follow the instructions in the generated README at bash-utils/nightly-nightly-ssh-key-rotator-5/README.md
• Run tests located in bash-utils/nightly-nightly-ssh-key-rotator-5/tests/

Links

• Generated docs and examples committed alongside this change.

Mock Justification

• Not applicable; generator did not introduce new mocks.

[polsala]

🤖 Review by GROQ Agent

✅ What's solid

• Self‑contained utility – All code lives under bash-utils/nightly‑nightly‑ssh‑key‑rotator‑5; no cross‑module side‑effects.
• Robust Bash options – set -euo pipefail protects against unset variables and silent failures.
• Clear CLI – --key-dir, --backup-dir, --dry‑run, and -h/--help are parsed with a straightforward while loop.
• Idempotent backup logic – Existing keys are moved with a timestamped suffix, preventing accidental overwrites.
• Graceful dry‑run – The run helper prints the command instead of executing it, making the script safe for previewing actions.
• Test harness – The test script creates an isolated temporary directory, runs the rotator, and asserts the expected state, keeping CI clean.

---

🧪 Tests

Positive points

• Uses mktemp -d and a trap to guarantee cleanup, avoiding litter on the runner.
• Verifies three core behaviours: backup creation, placeholder generation, and non‑creation of keys that didn’t exist.

Actionable improvements

• Add a dry‑run test – Ensure that with --dry-run no files are moved or created, and that the output contains the expected [dry‑run] prefix. Example snippet:

  bash "$SCRIPT_PATH" --key-dir "$KEY_DIR" --backup-dir "$BACKUP_DIR" --dry-run 2>&1 |
    grep -q "\[dry‑run\] mv"

• Validate backup naming – Check that each backup file matches the pattern ssh_host_*.<timestamp>.bak. This guards against future changes that might drop the timestamp.

  for f in "$BACKUP_DIR"/ssh_host_*; do
    [[ "$f" =~ \.[0-9]{14}\.bak$ ]] || { echo "FAIL: $f has wrong name"; exit 1; }
  done

• Negative test for missing key directory – Invoke the script with a non‑existent --key-dir and assert that it exits with a non‑zero status and prints the appropriate error message.

• Mock systemctl/service – The current test runs the script with root privileges on the CI host, which could unintentionally restart the real sshd. Override the command lookup path:

  export PATH="$(mktemp -d):$PATH"
  touch "$(dirname "$PATH")/systemctl" "$(dirname "$PATH")/service"
  chmod +x "$(dirname "$PATH")/systemctl" "$(dirname "$PATH")/service"

  This ensures the restart block is exercised without affecting the host.

• Permission checks – After rotation, verify that the newly created placeholder files have restrictive permissions (600) and that the backup files preserve the original mode (if you decide to copy it).

---

🔒 Security

• Avoid eval – The run helper uses eval "$@", which can be unsafe if any argument contains spaces or shell metacharacters. Replace it with direct command execution:

  run() {
    if $DRY_RUN; then
      echo "[dry‑run] $*"
    else
      "$@"
    fi
  }

• Preserve file permissions – When moving the original keys, the script should retain their mode (typically 600). mv does this automatically, but the newly created placeholders are created with default umask. Explicitly set permissions:

  run "touch \"$src\""
  run "chmod 600 \"$src\""

• Restart side‑effect – Restarting sshd after creating empty placeholder keys will cause the daemon to fail to start (it expects valid private keys). Consider adding a flag such as --no-restart or, at minimum, a warning that the placeholders are not functional keys.

• Root checks – The script already checks $EUID -eq 0 before attempting a restart, which is good. Document that rotating real host keys must be run as root.

• Input validation – The script trusts the paths supplied via --key-dir and --backup-dir. Adding a sanity check that they are absolute paths (or at least not .. traversals) would prevent accidental directory traversal when the utility is used in CI pipelines.

---

🧩 Docs / Developer Experience

Strengths

• README provides a concise purpose, installation steps, usage examples, and a test command.
• Options are listed clearly, and the help output mirrors the README.

Suggested enhancements

• Clarify placeholder nature – Explicitly state that the script creates empty files, not real SSH keys, and that users should replace the touch step with ssh-keygen for production use.
• Explain restart behaviour – Mention that the script attempts to restart sshd and that this will fail unless real keys are generated, or advise using --no-restart.
• Add exit‑code table – Document the meaning of 0 (success), 1 (argument error), 2 (missing key directory), etc., to aid automation.
• Provide a “dry‑run” example – Show sample output of a dry‑run to illustrate what the user will see.
• Versioning / attribution – Include a short “Generated by Groq” banner with a link to the generator, for traceability.

---

🧱 Mocks / Fakes

• The PR notes “Not applicable; generator did not introduce new mocks.”
• While the utility itself does not need mocks, the test suite could benefit from faking external commands (systemctl, service) to avoid side‑effects on the host. Implementing a lightweight mock (as shown in the Security section) would make the test suite safer and more deterministic.

---

Overall, the utility is a fun and useful addition for environments that need quick SSH key rotation. Addressing the points above—especially the eval removal, permission handling, and expanding the test coverage—will make the script production‑ready and the PR more robust.

[polsala]

🤖 Review by GEMINI Agent

✅ What's solid

• Self-contained utility: All code, documentation, and tests reside within bash-utils/nightly-nightly-ssh-key-rotator-5, ensuring no external dependencies or side-effects on other modules.
• Robust Bash options: The use of set -euo pipefail in both the main script and the test suite enhances reliability by preventing common Bash pitfalls like unset variables and silent failures.
• Clear CLI: The script provides well-defined command-line options (--key-dir, --backup-dir, --dry-run, -h/--help) with a robust parsing mechanism, making it user-friendly.
• Effective Dry-Run Mode: The run() function elegantly implements a dry-run feature, allowing users to preview actions without modifying the filesystem, which is crucial for a utility touching system-critical files.
• Comprehensive Test Isolation: The test suite leverages mktemp -d and trap cleanup EXIT to create and manage temporary directories, ensuring tests are isolated, reproducible, and clean up after themselves.
• Explicit Key File List: The key_files array clearly enumerates the SSH host key files targeted for rotation, making the script's scope transparent and easily extensible.
• Graceful SSHD Restart: The script attempts to restart the SSH daemon using systemctl or service based on availability, and provides a warning if it cannot determine the appropriate method.

🧪 Tests

• Expand ed25519 key rotation coverage: The current tests verify that ed25519 keys are not created if absent. It would be beneficial to also include a test case where ssh_host_ed25519_key and ssh_host_ed25519_key.pub are initially present and successfully rotated.
  • Actionable feedback: Add a test scenario that pre-populates ssh_host_ed25519_key and ssh_host_ed25519_key.pub in the temporary KEY_DIR and asserts their successful backup and replacement with placeholders.
• Validate error handling for non-existent KEY_DIR: The script correctly exits with an error if the specified --key-dir does not exist. This behavior should be explicitly tested.
  • Actionable feedback: Introduce a test case that invokes the script with a non-existent --key-dir and verifies that the script exits with a non-zero status code and outputs the expected error message to stderr.
• Verify dry-run mode's non-modification guarantee: While the run function implements dry-run, the test suite does not explicitly confirm that no filesystem changes occur when --dry-run is active.
  • Actionable feedback: Add a test that runs the script with --dry-run and asserts that the original key files remain untouched in KEY_DIR and that no backup directory is created.

🔒 Security

• Improve placeholder key generation: The script currently uses touch to create empty placeholder key files. Empty files are not valid SSH keys and can cause sshd to fail or behave unexpectedly. The README suggests replacing this with ssh-keygen.
  • Actionable feedback: Integrate ssh-keygen directly into the script to generate actual (even if temporary) new host keys. This would make the utility more robust and immediately functional post-rotation. If ssh-keygen is not available, fall back to touch with a prominent warning.
  • Code snippet idea:

    # Inside the loop, replace 'run "touch \"$src\""' with:
    if command -v ssh-keygen >/dev/null 2>&1; then
      local key_type=""
      if [[ "$key" == *"rsa"* ]]; then key_type="rsa";
      elif [[ "$key" == *"ecdsa"* ]]; then key_type="ecdsa";
      elif [[ "$key" == *"ed25519"* ]]; then key_type="ed25519";
      fi
      if [[ -n "$key_type" && "$key" != *.pub ]]; then
        echo "Generating new $key using ssh-keygen..."
        run "ssh-keygen -t $key_type -f \"$src\" -N ''" # -N '' for no passphrase
      else
        echo "Creating empty placeholder for $key (public key or unknown type)."
        run "touch \"$src\""
      fi
    else
      echo "Warning: ssh-keygen not found. Creating empty placeholder for $key. Please generate a real key manually."
      run "touch \"$src\""
    fi

• Explicitly set permissions for new keys: When touch is used for placeholders, the default umask might result in insecure permissions for private key files. ssh-keygen typically handles this automatically.
  • Actionable feedback: After creating any new private key file (e.g., ssh_host_rsa_key), explicitly set its permissions to 0600 using chmod to ensure it's only readable by the owner.
  • Code snippet idea:

    # After creating a new placeholder (or generating a key):
    if [[ "$key" != *.pub ]]; then # Only for private keys
      run "chmod 600 \"$src\""
    fi

• Refactor eval "$@" in run(): The use of eval can introduce security vulnerabilities if the input is not perfectly controlled. While the current usage appears safe, it's a best practice to avoid eval in favor of direct execution.
  • Actionable feedback: Modify the run function to execute commands directly using "$@" instead of eval "$@". This requires adjusting how commands are passed to run (e.g., run command arg1 arg2 instead of run "command arg1 arg2").
  • Code snippet idea:

    run() {
      if $DRY_RUN; then
        echo "[dry-run] $*"
      else
        "$@" # Execute the command directly
      fi
    }
    # Example usage change:
    # From: run "mkdir -p \"$BACKUP_DIR\""
    # To:   run mkdir -p "$BACKUP_DIR"

• Add root privilege check for default KEY_DIR: The script defaults to /etc/ssh, which requires root privileges. While the README mentions sudo, the script itself doesn't explicitly check for root permissions when targeting this critical directory.
  • Actionable feedback: Implement an early check in the script to warn or exit if it's attempting to operate on /etc/ssh (or other system-critical paths) without root privileges (EUID -eq 0).
  • Code snippet idea:

    # After argument parsing and KEY_DIR validation:
    if [[ "$KEY_DIR" == "/etc/ssh" && $EUID -ne 0 ]]; then
      echo "Error: Running with default key directory '$KEY_DIR' requires root privileges. Please use 'sudo' or specify a custom directory with '--key-dir'." >&2
      exit 1
    fi

🧩 Docs/DX

• Align PR description with utility's purpose: The PR title and body describe the utility as "whimsical." Given that it performs a critical security operation, this description might be misleading.
  • Actionable feedback: Update the PR title and body to reflect the serious and functional nature of SSH host key rotation, removing "whimsical" to maintain a professional tone.
• Elevate ssh-keygen guidance in README: The README mentions that placeholder generation can be replaced with ssh-keygen. This is a crucial step for a fully functional and secure setup.
  • Actionable feedback: Provide more prominent guidance or a clear example in the README on how to integrate ssh-keygen into a post-rotation workflow, or, ideally, integrate it directly into the script as suggested in the Security section.
• Enhance output clarity for skipped keys: While "Skipping $key (not found)" is clear, for a security utility, a summary of all actions (processed, skipped, backed up) at the end could be beneficial, especially in verbose mode.
  • Actionable feedback: Consider adding a summary report at the end of the script's execution, detailing which keys were rotated, which were skipped, and where backups were stored.
• Clarify default backup directory implications: The default BACKUP_DIR is $KEY_DIR/backup. If KEY_DIR is /etc/ssh, backups will reside in /etc/ssh/backup. This location might not be subject to standard backup rotation policies or system cleanup.
  • Actionable feedback: In the README, highlight the implications of using the default backup location within /etc/ssh and recommend considering a more standard backup path (e.g., /var/backups/ssh_keys) for long-term storage and management.

🧱 Mocks/Fakes

• The utility's nature as a Bash script interacting with the filesystem means that traditional "mocks" or "fakes" in the software engineering sense are not directly applicable. The test suite's use of mktemp -d to create isolated temporary directories effectively serves as a robust "fake" filesystem environment, which is the appropriate and highly effective approach for testing this type of utility. No further actionable feedback is required in this section.