-
Notifications
You must be signed in to change notification settings - Fork 2
libstore: fixup fakeSSH check #219
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
WalkthroughThe 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
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
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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)
🔇 Additional comments (1)
Comment |
Curious for other reviewers - what is fakeSSH exactly? Sniffing out Does this code (w/ this patch), account for this? |
This broke invocations like:
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:
Given the above invocation,
host
would have beenroot@localhost
, and thusfakeSSH
would befalse
becauseroot@localhost
!=localhost
.However, since 49ba061,
authority.host
returned just the host (localhost
, no user) and erroneously enabledfakeSSH
in this case, causingNIX_SSHOPTS
to be ignored (since, whenfakeSSH
istrue
,SSHMaster::startCommand
doesn't calladdCommonSSHOpts
).authority.to_string()
accurately returns the expectedroot@localhost
format (given the above invocation), fixing this.Motivation
Context
ref NixOS#14150
Summary by CodeRabbit