feat(system): redo password handling for remote root, allow SSH password auth#124
Conversation
WalkthroughSSH deployment flow now supports password fallback to SSH agent, interactive PTY-based root password handling, and an EnsureRemoteRootPassword check; apply flow always prompts for activation and invokes remote-root verification when enabled. Documentation updated with a Remote Deployments section. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Apply
participant SSH as SSHSystem
participant Remote as Remote Host
participant Terminal
User->>Apply: run apply (--target-host, --remote-root?)
Apply->>SSH: Initialize SSHSystem (NewSSHSystem)
SSH->>Remote: Try SSH agent auth
alt Agent auth succeeds
Remote-->>SSH: auth OK
else Agent missing/failed
SSH->>Terminal: promptForPassword()
Terminal->>User: request SSH password
User-->>Terminal: enter password
Terminal-->>SSH: password bytes
SSH->>Remote: attempt password auth
Remote-->>SSH: auth OK
end
SSH-->>Apply: connection ready
Apply->>User: prompt activation confirmation
User-->>Apply: confirm
rect rgb(230,245,255)
Note over Apply,SSH: Remote root setup (if --remote-root)
Apply->>SSH: EnsureRemoteRootPassword(rootCmd)
SSH->>Terminal: promptForPassword() for sudo/root
Terminal->>User: request root/sudo password
User-->>Terminal: enter password
Terminal-->>SSH: root password bytes
SSH->>Remote: testRemoteRoot (sudo + password)
Remote-->>SSH: validation OK
SSH-->>Apply: remote root configured
end
Apply->>SSH: Run deployment commands
SSH->>Remote: execute (inject password if needed)
Remote-->>Apply: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/apply/apply.go(3 hunks)doc/src/overview.md(1 hunks)internal/system/ssh.go(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/apply/apply.go (2)
internal/cmd/utils/confirmation.go (1)
ConfirmationInput(11-28)internal/system/ssh.go (1)
SSHSystem(30-39)
internal/system/ssh.go (2)
internal/logger/logger.go (1)
Logger(12-27)internal/system/runner.go (1)
NewCommand(25-34)
🪛 LanguageTool
doc/src/overview.md
[style] ~94-~94: Consider a more concise word here.
Context: ...defined in the root_command setting. In order to access remote machines, prefer using an...
(IN_ORDER_TO_PREMIUM)
[style] ~96-~96: Consider using a different verb to strengthen your wording.
Context: ...t recommended, and can be buggy. If you find any issues, report them on the issue tr...
(FIND_ENCOUNTER)
⏰ 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). (1)
- GitHub Check: Build/Test
50aa35d to
75975c4
Compare
75975c4 to
f36bded
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/system/ssh.go (1)
173-185: Consider parameterizing stdin for password promptsThe function hard-codes
os.Stdinfor terminal detection and password reading. This works for the current use case but reduces flexibility if stdin needs to be redirected or mocked for testing.Consider accepting stdin as a parameter:
-func promptForPassword(username string, address string) ([]byte, error) { - fd := int(os.Stdin.Fd()) +func promptForPassword(username string, address string, stdin *os.File) ([]byte, error) { + fd := int(stdin.Fd()) if !term.IsTerminal(fd) { return nil, fmt.Errorf("cannot prompt for password: stdin is not a terminal") } - fmt.Fprintf(os.Stderr, "Password for %s@%s: ", username, address) - _ = os.Stdin.Sync() + fmt.Fprintf(os.Stderr, "Password for %s@%s: ", username, address) + _ = stdin.Sync() - bytePassword, err := term.ReadPassword(int(os.Stdin.Fd())) + bytePassword, err := term.ReadPassword(fd) fmt.Fprintln(os.Stderr) return bytePassword, err }Then update callers on lines 101, 152 to pass
os.Stdinexplicitly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/apply/apply.go(3 hunks)doc/src/overview.md(1 hunks)internal/system/ssh.go(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/apply/apply.go (2)
internal/cmd/utils/confirmation.go (1)
ConfirmationInput(11-28)internal/system/ssh.go (1)
SSHSystem(30-39)
internal/system/ssh.go (2)
internal/logger/logger.go (1)
Logger(12-27)internal/system/runner.go (1)
NewCommand(25-34)
🪛 LanguageTool
doc/src/overview.md
[style] ~94-~94: Consider a more concise word here.
Context: ...defined in the root_command setting. In order to access remote machines, prefer using an...
(IN_ORDER_TO_PREMIUM)
[style] ~96-~96: Consider using a different verb to strengthen your wording.
Context: ...t recommended, and can be buggy. If you find any issues, report them on the issue tr...
(FIND_ENCOUNTER)
⏰ 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). (1)
- GitHub Check: Build/Test
🔇 Additional comments (10)
cmd/apply/apply.go (3)
179-180: LGTM: Consistent error handlingThe removal of special-case SSH connection error handling provides consistent logging and error propagation for both target and build hosts.
Also applies to: 228-229
407-418: LGTM: Unconditional activation confirmation improves safetyThe change to always prompt for activation confirmation (regardless of deployment target) is a safer default that prevents accidental remote activations.
420-426: LGTM: Remote root password verification integrated correctlyThe integration of
EnsureRemoteRootPasswordis well-placed after successful SSH connection setup and only executes when the--remote-rootflag is specified. Error handling and logging are appropriate.doc/src/overview.md (1)
79-97: LGTM: Clear documentation of remote deployment featuresThe new "Remote Deployments" section effectively documents the SSH-based deployment workflow, root escalation requirements, and authentication methods. The caveat about password-based authentication being "potentially buggy" appropriately sets user expectations.
internal/system/ssh.go (6)
30-39: LGTM: Struct updated to support password authenticationThe addition of
addressandpasswordfields appropriately supports the new authentication flow. Thepasswordfield stores credentials in memory, which is necessary for sudo password injection but should be documented as sensitive.
79-108: Verify the silent SSH agent fallback behaviorThe authentication flow now silently falls backs to password auth when the SSH agent is unavailable (line 88-89). While this provides a better user experience, it may mask configuration issues where users expect agent-based authentication.
Consider whether a warning-level log message would be more appropriate than debug-level for the agent fallback.
141-164: LGTM: Root password verification well-implementedThe
EnsureRemoteRootPasswordmethod correctly:
- Short-circuits when password is already available
- Prompts once and stores the credential
- Verifies the password works before returning
This provides good UX by catching authentication failures early.
476-476: LGTM: Address method updated for field renameCorrectly uses
s.addressto match the struct field changes.
503-505: LGTM: Defensive resource cleanup and command filteringThe nil check before closing
s.connprevents panics, and the removal of "run0" fromisRootCommandappears intentional.Also applies to: 520-520
274-316: stdin forwarding logic is correct—no action requiredThe code properly sets
session.Stdin = cmd.Stdin(line 290) before callingrequestRootPasswordPTY(line 308), addressing the previous review concern. The password injection path uses the-Sflag with stdin correctly, and the PTY path forwards stdin after setting up the terminal.The FIXME comment (lines 305-307) documents a known PTY issue with first-byte loss as an existing limitation, not a bug requiring immediate resolution.
Description
sudoand other root command input is just broken right now due to required interactive input.As such, this changes around some SSH handling, and also allows passwords to be used as an authentication mechanism.
If a password is used for access, then this is reused when
--remote-sudois passed. Additionally, it asks for a password once, and then usessudostdin.This may be broken for
doas, anddoasstill requires command input and a PTY, but this is a step in the right direction.Also, this removes passing
stdinto SSH processes, since no remote commands should need astdinat this time. This fixes some input handling for remote confirmation as well.Summary by CodeRabbit
New Features
Improvements
Documentation