feat(groq): An Ansible playbook that rotates SSH keys on target hosts, ensuring old keys are revoked and new keys are distributed securely.

[polsala]

Implementation Summary

• Utility: nightly-ansible-ssh-key-rotator
• Provider: groq
• Location: ansible-playbooks/nightly-nightly-ansible-ssh-key-rota-5
• Files Created: 4
• Description: An Ansible playbook that rotates SSH keys on target hosts, ensuring old keys are revoked and new keys are distributed securely.

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 ansible-playbooks/nightly-nightly-ansible-ssh-key-rota-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 ansible-playbooks/nightly-nightly-ansible-ssh-key-rota-5/README.md
• Run tests located in ansible-playbooks/nightly-nightly-ansible-ssh-key-rota-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

• Clear purpose & isolation – The playbook lives in its own nightly‑... folder and does not touch any existing codebase, making it easy to reason about impact.
• Idempotent key generation – Using creates: "{{ new_key_path }}" ensures the RSA pair is only generated once per host.
• Leverages built‑in Ansible modules – authorized_key and lineinfile are the right choices for managing authorized_keys.
• README gives a quick‑start command – The one‑liner ansible-playbook … -e "new_key_path=… old_key_fingerprint=…" is helpful for users.
• Test harness runs without a real inventory – The inventory.ini points to localhost with ansible_connection=local, allowing the test suite to execute on any CI runner.

---

🧪 Tests

Observation	Recommendation
Tests only mock subprocess.run and never invoke Ansible or the playbook logic.	• Use Molecule or ansible-test to spin up a temporary container/VM and run the playbook against a real (or mocked) host. • At minimum, assert that the expected files (new_key_path, new_key_path.pub) are created and that authorized_keys contains the new key after the playbook runs.
The “missing ssh‑keygen” test simulates a failure by returning a non‑zero exit code, but it does not verify that the playbook actually aborts when ssh-keygen is absent.	• Add a unit test that runs the playbook with ssh-keygen deliberately removed from $PATH (e.g., env -u PATH) and checks that the task fails with a clear error message.
No coverage for the optional old_key_fingerprint removal path.	• Write a test that seeds an authorized_keys file with a dummy line containing a known fingerprint, runs the playbook with old_key_fingerprint set, and asserts that the line is removed.
Test suite uses unittest while the rest of the repo (if any) may be using pytest.	• Consider converting the tests to pure pytest style (functions with fixtures) for consistency and easier parametrisation.

Example snippet for a Molecule test (pytest‑style):

# molecule/default/molecule.yml
driver:
  name: docker
platforms:
  - name: instance
    image: python:3.11-slim
    privileged: true   # needed for become
provisioner:
  name: ansible
  playbooks:
    converge: ../../src/rotate_ssh_keys.yml

# tests/test_molecule.py
def test_key_rotated(host):
    # Ensure new key exists
    assert host.file("~/.ssh/id_rsa_new").exists
    # Verify authorized_keys contains the public key
    pub = host.file("~/.ssh/id_rsa_new.pub").content_string
    auth = host.file("~/.ssh/authorized_keys").content_string
    assert pub in auth

---

🔒 Security

• Passphrase‑less private key – The playbook generates a key with -N "". This is convenient but weakens security, especially if the private key is stored on a shared CI runner.
  • Action: Add an optional new_key_passphrase variable and pass it to ssh-keygen (-N "{{ new_key_passphrase | default('') }}"). Document the security implications in the README.
• File permissions – ssh-keygen creates the private key with 0600, but the playbook does not explicitly enforce permissions on the generated key or on authorized_keys.
  • Action: Add a file task after key generation:

    - name: Secure private key permissions
      file:
        path: "{{ new_key_path }}"
        mode: '0600'
        owner: "{{ ansible_user | default('root') }}"
        group: "{{ ansible_user | default('root') }}"

• Privilege escalation – become: true runs the entire playbook as root. If the only privileged operation is updating authorized_keys, consider limiting become to that specific task to reduce the attack surface.
  • Action: Move become: true into the authorized_key and lineinfile tasks only.
• Fingerprint matching – Using the raw fingerprint string as a regular expression can lead to false positives (e.g., if the fingerprint appears as a comment). Consider anchoring the regex or using a more precise pattern:

  regexp: '^.*{{ old_key_fingerprint }}.*$'

---

🧩 Docs / Developer Experience

• Variable documentation – The README lists the variables but does not specify types, defaults, or examples of a fingerprint format.
  • Action: Add a table with columns: Variable, Required, Default, Description, Example.
• Prerequisites – Mention that the host running the playbook must have ssh-keygen and ansible installed, and that the user executing the playbook needs sudo rights on the target hosts.
• Error handling guidance – Explain what a user should do if the playbook fails at the “Ensure ssh-keygen is available” step (e.g., install openssh-client).
• Cleanup instructions – Provide a short note on how to delete the generated temporary key pair after rotation, or how to rotate again safely.
• Link to CI – If the repository has a CI pipeline that runs the tests, add a badge (e.g., ![Tests](https://github.com/.../actions/workflows/ci.yml/badge.svg)) to the README for visibility.

---

🧱 Mocks / Fakes

• Current approach – The test suite patches subprocess.run, which is fine for unit testing the wrapper script but does not validate Ansible behavior.
• Improvement – Replace the blanket mock with ansible‑runner or ansible‑test fixtures that can simulate module results (ssh-keygen missing, key already present, etc.) without invoking the real binary.
• Example using unittest.mock on Ansible modules:

  @mock.patch('ansible.modules.files.authorized_key')
  def test_authorized_key_called(mock_auth):
      # Run the playbook via ansible-runner
      runner = ansible_runner.run(private_data_dir='.', playbook='src/rotate_ssh_keys.yml')
      mock_auth.assert_called_once()

• Future-proofing – If additional utilities are added to this folder, consider creating a small test harness (e.g., a tests/conftest.py) that provides common fixtures for inventory, temporary key directories, and mock Ansible callbacks.

---

TL;DR Action List

1. Add passphrase support and enforce file permissions on generated keys.
2. Scope become to privileged tasks only.
3. Strengthen fingerprint regex to avoid accidental removals.
4. Expand test coverage with Molecule/ansible-test, covering key creation, authorized‑key insertion, and fingerprint removal.
5. Enrich README with variable table, prerequisites, error‑handling tips, and CI badge.
6. Replace generic subprocess mock with Ansible‑aware testing utilities for more realistic validation.

Implementing these suggestions will make the utility more secure, reliable, and user‑friendly while providing a robust test foundation for future enhancements.

[polsala]

🤖 Review by GEMINI Agent

✅ What's solid

• Clear purpose and isolation: The utility is well-contained within its own directory, making its purpose clear and minimizing potential side effects on other parts of the codebase.
• Idempotent key generation: The creates: "{{ new_key_path }}" argument in the ssh-keygen task correctly ensures that a new key pair is only generated if it doesn't already exist, preventing unnecessary re-generation on subsequent runs.
• Leverages built-in Ansible modules: The use of authorized_key and lineinfile modules is appropriate for managing SSH keys and authorized_keys files, promoting Ansible best practices.
• become: true: The playbook correctly uses become: true for tasks that modify system-level files like authorized_keys, ensuring necessary permissions.

🧪 Tests

• Limited scope of tests: The current test suite primarily verifies that subprocess.run is called and returns expected results (success/failure for ssh-keygen availability). It does not actually execute the Ansible playbook or verify its side effects on a real or mocked filesystem.
  • Consider adding integration tests that run the playbook against a local Docker container or a temporary directory to verify:
    • A new key pair is actually created at new_key_path.
    • The public key is correctly added to ~/.ssh/authorized_keys on the target host.
    • The old key (if old_key_fingerprint is provided) is correctly removed from ~/.ssh/authorized_keys.
    • The playbook handles cases where authorized_keys does not exist initially.
• ansible_user default in tests: The playbook uses user: "{{ ansible_user | default('root') }}". The current tests don't explicitly set ansible_user or test scenarios where it might be different from root.
  • If the playbook is intended to run for non-root users, the tests should include a scenario where ansible_user is set to a different user, and verify the key rotation works correctly for that user.
• Missing negative test for new_key_path: The playbook has new_key_path as a required variable in the README, but provides a default in the playbook itself. The tests don't cover scenarios where new_key_path might be invalid or lead to permission issues.
  • Add a test case that simulates an invalid new_key_path (e.g., a read-only directory) to ensure the playbook fails gracefully.

🔒 Security

• become: true usage: The playbook uses become: true, which grants root privileges. While necessary for modifying authorized_keys, it's crucial to ensure all tasks executed under become are strictly necessary and secure.
  • Review all tasks under become: true to confirm they are essential for key rotation and do not introduce unintended privilege escalation or arbitrary command execution.
• old_key_fingerprint regex vulnerability: The lineinfile module uses regexp: "{{ old_key_fingerprint }}". If old_key_fingerprint is user-controlled and not properly sanitized, a malicious user could inject a regex that matches more than just the intended key, potentially removing other legitimate keys or lines from authorized_keys.
  • Escape the old_key_fingerprint variable when used in the regexp parameter to prevent regex injection. For example, use regexp: "{{ old_key_fingerprint | regex_escape }}" if such a filter is available, or manually escape special regex characters. Alternatively, consider using the key parameter of authorized_key with state: absent if the full public key content of the old key is available, which is safer than a fingerprint regex.
• ssh-keygen command injection: The command: ssh-keygen -t rsa -b 4096 -f "{{ new_key_path }}" -N "" task uses new_key_path directly in a shell command. While creates helps, if new_key_path contains shell metacharacters (e.g., ;, |, &), it could lead to command injection.
  • Use the ansible.builtin.openssh_keypair module instead of command: ssh-keygen for generating SSH keys. This module is designed for this purpose and handles path sanitization and key generation securely.
  • Example:

    - name: Generate new SSH key pair if not exists
      ansible.builtin.openssh_keypair:
        path: "{{ new_key_path }}"
        type: rsa
        size: 4096
        state: present
        force: false # Only create if it doesn't exist
      when: not lookup('file', new_key_path) # Still useful for initial check

• Default ansible_user to root: The authorized_key module defaults user to root if ansible_user is not defined. This might lead to keys being added to the root user's authorized_keys when a different user was intended.
  • Explicitly define ansible_user in the inventory or as an extra var, or make user a required variable for the playbook, to avoid unintended key placement.

🧩 Docs/DX

• Clarity on new_key_path: The README states new_key_path is "required", but the playbook provides a default value (~/.ssh/id_rsa_new). This creates a discrepancy.
  • Align the documentation with the playbook's behavior. If a default is provided, clarify that it's optional in the README, or remove the default from the playbook if it's truly intended to be required.
• old_key_fingerprint format: The README example uses SHA256:oldfingerprint. It would be helpful to specify the exact format expected by the regexp (e.g., SHA256:<fingerprint_hash> or MD5:<fingerprint_hash>).
  • Clarify the expected format for old_key_fingerprint in the README, perhaps with an example of how to obtain it (e.g., ssh-keygen -lf ~/.ssh/id_rsa).
• Local key generation vs. remote: The playbook generates the new key pair locally on the Ansible control node (or wherever ansible-playbook is run, if localhost is the target). The README implies this but could be more explicit about where the new_key_path refers to.
  • Add a note in the "How it works" section of the README to explicitly state that the new SSH key pair is generated on the Ansible control node (or the host running the playbook if ansible_connection=local).
• Error handling for slurp: If {{ new_key_path }}.pub does not exist (e.g., ssh-keygen failed silently or with a non-zero exit code that was ignored), the slurp task would fail.
  • Add a failed_when condition to the slurp task to provide a more specific error message if the public key file is not found after generation.

🧱 Mocks/Fakes

• Mocking subprocess.run: The current tests mock subprocess.run to simulate Ansible playbook execution. This is a common approach for unit testing, but it doesn't verify the actual Ansible logic or module interactions.
  • While the current mocking is suitable for basic unit tests of the Python wrapper, consider using Ansible's own testing frameworks (e.g., ansible-test or Molecule) for more robust integration testing. These frameworks can provision real or virtual environments and execute playbooks against them, providing a higher fidelity test of the playbook's functionality.
• Limited mock scenarios: The mocks only cover successful execution and ssh-keygen missing.
  • Expand mock scenarios to cover other potential failures that subprocess.run might encounter when executing ansible-playbook, such as syntax errors in the playbook, connection failures, or permission issues on the target host. This would involve mocking stderr with relevant Ansible error messages.