Skip to content

Conversation

cole-h
Copy link
Member

@cole-h cole-h commented Oct 3, 2025

This broke invocations like:

NIX_SSHOPTS='-p2222 -oUserKnownHostsFile=/dev/null -oStrictHostKeyChecking=no' nix copy /nix/store/......-foo --to ssh-ng://root@localhost

In Nix 2.30.2, fakeSSH was enabled when the "thing I want to connect to" was plain old "localhost". Previously, this check was written as:

     , fakeSSH(host == "localhost")

Given the above invocation, host would have been root@localhost, and thus fakeSSH would be false because root@localhost != localhost.

However, since 49ba061, authority.host returned just the host (localhost, no user) and erroneously enabled fakeSSH in this case, causing NIX_SSHOPTS to be ignored (since, when fakeSSH is true, SSHMaster::startCommand doesn't call addCommonSSHOpts).

authority.to_string() accurately returns the expected root@localhost format (given the above invocation), fixing this.

Motivation

Context


ref NixOS#14150

Summary by CodeRabbit

  • Bug Fixes
    • Improved detection of localhost in SSH connections, ensuring local-connection behavior is applied consistently.
    • Corrected conditions that determine when SSH master sessions are used, aligning behavior with localhost handling.
    • Enhances reliability for workflows that rely on SSH to localhost, reducing unexpected fallbacks or mismatches in connection mode.

This broke invocations like:

    NIX_SSHOPTS='-p2222 -oUserKnownHostsFile=/dev/null -oStrictHostKeyChecking=no' nix copy /nix/store/......-foo --to ssh-ng://root@localhost

In Nix 2.30.2, fakeSSH was enabled when the "thing I want to connect to"
was plain old "localhost". Previously, this check was written as:

         , fakeSSH(host == "localhost")

Given the above invocation, `host` would have been `root@localhost`, and
thus `fakeSSH` would be `false` because `root@localhost` != `localhost`.

However, since 49ba061, `authority.host`
returned _just_ the host (`localhost`, no user) and erroneously enabled
`fakeSSH` in this case, causing `NIX_SSHOPTS` to be ignored (since,
when `fakeSSH` is `true`, `SSHMaster::startCommand` doesn't call
`addCommonSSHOpts`).

`authority.to_string()` accurately returns the expected `root@localhost`
format (given the above invocation), fixing this.
Copy link

coderabbitai bot commented Oct 3, 2025

Walkthrough

The localhost detection logic in src/libstore/ssh.cc was updated to compare authority.to_string() to "localhost" instead of only authority.host. This changes when fakeSSH (and thus useMaster) is enabled. No other logic, signatures, or exports were modified.

Changes

Cohort / File(s) Summary
SSH localhost check adjustment
src/libstore/ssh.cc
Replace localhost check from authority.host == "localhost" to authority.to_string() == "localhost", altering the condition for fakeSSH and downstream useMaster evaluation.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Caller
    participant SSH as SSH Config

    Caller->>SSH: configureConnection(authority)
    note right of SSH: Determine localhost

    rect rgba(200,230,255,0.4)
    SSH->>SSH: fakeSSH = (authority.to_string() == "localhost")
    end

    alt fakeSSH is true
        SSH->>SSH: useMaster = false (or dependent logic)
        note right of SSH: Behavior tied to fakeSSH
    else fakeSSH is false
        SSH->>SSH: useMaster computed normally
    end

    SSH-->>Caller: connection parameters
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I twitch my ears at localhost’s string,
A subtle shift—what will it bring?
From host to whole, the check is clean,
Small hops adjust the SSH scene.
With paws on keys, I softly cheer—
One nibble of logic, behavior clear. 🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.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 title succinctly identifies the scope (libstore) and the nature of the change (fixup of the fakeSSH check), directly matching the PR’s purpose of restoring correct behavior for SSH options by adjusting that condition. It is concise, specific, and free of noise or generic terms.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fixup-fakessh-check

📜 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 e930555 and 044797a.

📒 Files selected for processing (1)
  • src/libstore/ssh.cc (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build_aarch64-darwin / build
  • GitHub Check: build_x86_64-linux / build
🔇 Additional comments (1)
src/libstore/ssh.cc (1)

81-81: Approve SSH fakeSSH fix
The comparison now correctly distinguishes "user@localhost" vs "localhost", restoring NIX_SSHOPTS usage. No other authority.host equality checks found.


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

Copy link

github-actions bot commented Oct 3, 2025

@github-actions github-actions bot temporarily deployed to pull request October 3, 2025 19:25 Inactive
@colemickens
Copy link
Member

Curious for other reviewers - what is fakeSSH exactly? Sniffing out localhost seems a bit fraught. I suspect that in this case of localhost:2222, the connection is not to the same machine.

Does this code (w/ this patch), account for this?

@edolstra edolstra added this pull request to the merge queue Oct 7, 2025
Merged via the queue into main with commit ba6e34b Oct 7, 2025
35 checks passed
@edolstra edolstra deleted the fixup-fakessh-check branch October 7, 2025 15:43
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.

3 participants