Skip to content

feat(system): mimic SSH known_hosts key addition process#121

Merged
water-sucks merged 1 commit intonix-community:mainfrom
water-sucks:knownhosts-more-descriptive-errors
Oct 31, 2025
Merged

feat(system): mimic SSH known_hosts key addition process#121
water-sucks merged 1 commit intonix-community:mainfrom
water-sucks:knownhosts-more-descriptive-errors

Conversation

@water-sucks
Copy link
Copy Markdown
Collaborator

@water-sucks water-sucks commented Oct 31, 2025

Description

The SSH knownhosts package does not behave like the OpenSSH client binary does currently. If a host is unknown, then an error is immediately raised and the program exits, without a chance to add the key to the known_hosts file. The only way to work around this is to add these keys to the known_hosts file beforehand.

As such, I mimic the process here using the nixos-cli logger and confirmation input, in case the key does not exist.

Summary by CodeRabbit

  • New Features
    • Interactive SSH host verification: Users are now prompted to confirm trusting new SSH hosts when connecting in an interactive terminal, with automatic known_hosts updates.
    • Host key change detection with security warnings to alert users of potential man-in-the-middle attacks.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Oct 31, 2025

Walkthrough

Modified SSH initialization to add interactive known_hosts handling. When an unknown host is encountered during TTY sessions, the system prompts for user confirmation to trust the host, then appends the host key to the known_hosts file on acceptance. Warnings are issued for modified host keys.

Changes

Cohort / File(s) Change Summary
Interactive known_hosts callback
internal/system/ssh.go
Added wrappedKnownHostsCallback helper function that wraps the original HostKeyCallback to intercept unknown hosts. When TTY is active, prompts user to confirm trusting unknown hosts and appends them to known_hosts file. Replaced HostKeyCallback usage and added import for command utilities.

Sequence Diagram

sequenceDiagram
    participant SSH as SSH Connection
    participant Callback as wrappedKnownHostsCallback
    participant KnownHosts as known_hosts File
    participant User as User/TTY

    SSH->>Callback: HostKeyCallback invoked
    alt Host Known
        Callback-->>SSH: Accept (success)
    else Host Unknown & TTY Active
        Callback->>User: Prompt: Trust this host?
        alt User Confirms
            User-->>Callback: Yes
            Callback->>KnownHosts: Append host/key pair
            Callback-->>SSH: Accept (success)
        else User Rejects
            User-->>Callback: No
            Callback-->>SSH: Error (rejected)
        end
    else Host Key Changed
        Callback-->>SSH: Warning (potential MITM)
    else Unknown Host & No TTY
        Callback-->>SSH: Error (cannot prompt)
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Focus area: Security-critical logic in wrappedKnownHostsCallback function, particularly confirmation flow and file write operations
  • Verify TTY detection logic and user input handling
  • Ensure host key change detection and warning behavior align with security expectations

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "feat(system): mimic SSH known_hosts key addition process" directly aligns with the main change described in the raw summary and PR objectives. The change introduces interactive known_hosts handling that mimics OpenSSH's behavior by prompting users to confirm and add host keys to known_hosts when encountered in a TTY, rather than erroring immediately. The title is clear, specific, concise, and accurately captures the primary objective without vague language or misleading information.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 030369d and a844405.

📒 Files selected for processing (1)
  • internal/system/ssh.go (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/system/ssh.go (2)
internal/logger/logger.go (1)
  • Logger (12-27)
internal/cmd/utils/confirmation.go (1)
  • ConfirmationInput (11-28)
🔇 Additional comments (4)
internal/system/ssh.go (4)

19-19: LGTM: Import addition is appropriate.

The import of the confirmation utility is necessary for the interactive host key verification flow.


92-98: LGTM: Clean wrapper pattern for host key handling.

The known_hosts path is correctly stored for reuse in both verification and writing operations. The wrapper approach cleanly extends the standard callback behavior.


100-104: LGTM: Correct integration of wrapped callback.

The wrapped callback is properly integrated into the SSH client configuration.


128-191: Well-structured implementation correctly mimicking OpenSSH behavior with proper port handling.

The wrappedKnownHostsCallback function correctly handles both unknown hosts and changed host keys:

Verified details:

  • The hostname parameter passed to HostKeyCallback is exactly what was given to ssh.Dial — e.g., "example.com:2222" for non-standard ports
  • knownhosts.Line() internally normalizes this by omitting default port 22, and using "[host]:port" format for non-standard ports
  • Terminal check (line 145) prevents non-interactive prompts
  • File permissions (0o600) are appropriate for SSH
  • Error handling is thorough with proper two-path logic (unknown vs changed host key)

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@water-sucks water-sucks merged commit ae050f9 into nix-community:main Oct 31, 2025
2 checks passed
@water-sucks water-sucks deleted the knownhosts-more-descriptive-errors branch November 3, 2025 10:04
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