Skip to content

fix(admin-user): grant traversal ACL on /etc/ssh for agenix HM#1722

Merged
johnae merged 1 commit into
mainfrom
push-ymtsnzwumvsu
May 16, 2026
Merged

fix(admin-user): grant traversal ACL on /etc/ssh for agenix HM#1722
johnae merged 1 commit into
mainfrom
push-ymtsnzwumvsu

Conversation

@johnae
Copy link
Copy Markdown
Owner

@johnae johnae commented May 16, 2026

No description provided.

@johnae johnae merged commit 050a1f4 into main May 16, 2026
2 of 4 checks passed
@johnae johnae deleted the push-ymtsnzwumvsu branch May 16, 2026 04:28
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 16, 2026

Claude finished @johnae's task —— View job


Code Review for PR #1722

Todo List:

  • Read the changed file to understand the modification
  • Analyze code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security implications
  • Consider test coverage needs
  • Provide comprehensive review feedback

Review Summary

This PR adds ACL (Access Control List) permissions to grant the admin user traversal rights on /etc/ssh directory and read access to the SSH host key. This is necessary for Home Manager's agenix decryption functionality.

Code Quality and Best Practices ✅

The implementation follows NixOS best practices:

  • Uses systemd.tmpfiles.rules for managing ACLs, which is the idiomatic approach
  • Properly handles both regular and ephemeral root configurations
  • Clear and informative comments explaining the necessity of the traversal ACL
  • Consistent code style matching the rest of the codebase

Potential Issues 🔍

No critical issues found. The change is minimal and focused on solving a specific permission problem.

Minor observation:

  • The file appears to be newly created rather than modified. If there was existing home-manager configuration elsewhere, ensure this doesn't duplicate or conflict with it.

Performance Considerations ⚡

No performance concerns. The ACL rules are applied once during system activation via systemd-tmpfiles, which has negligible overhead.

Security Assessment 🔒

The security implications are well-considered:

Positive aspects:

  • Grants minimal necessary permissions (only x for directory traversal, r for the key file)
  • Permissions are scoped to a specific user (the admin user)
  • Only affects the ED25519 host key, not all SSH keys
  • The comment correctly notes that some systems ship /etc/ssh with restrictive 0700 permissions

Security considerations:

  • Granting read access to SSH host keys should be done cautiously. In this case, it's justified for agenix Home Manager functionality
  • The admin user gaining read access to the host key is acceptable since they typically have elevated privileges anyway
  • No write permissions are granted, maintaining key integrity

Test Coverage Suggestions 🧪

Since this is a NixOS configuration change, traditional unit tests don't apply. However, consider:

  1. Manual verification: After deployment, verify that:

    • The admin user can successfully decrypt agenix secrets in Home Manager
    • The ACLs are correctly applied: getfacl /etc/ssh and getfacl /etc/ssh/ssh_host_ed25519_key
    • Systems with ephemeral roots work correctly with /keep/etc/ssh paths
  2. NixOS test framework: If the project uses NixOS tests, consider adding a test that:

    • Verifies the ACLs are applied correctly
    • Confirms agenix decryption works for the admin user

Additional Observations

The broader context shows this is part of a well-structured NixOS configuration:

  • The file integrates cleanly with the existing home-manager setup in profiles/home-manager.nix
  • The Git configuration section is unrelated to the ACL fix but appears well-configured for a multi-context development environment

Conclusion

Approved - This is a focused, necessary fix that properly addresses the permission issue for agenix Home Manager decryption. The implementation is clean, secure, and follows NixOS best practices.

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.

1 participant