Conversation
Introduce an Authentication value built once per host and reused for the
initial dial and every self-heal redial, so a reconnect never falls back to
a different method or re-reads the key. A host may now authenticate with a
private key (plaintext or passphrase-encrypted) via ssh_key_path/
ssh_key_passphrase in addition to a password.
- internal/sshx/auth.go: Authentication, PasswordAuth, PrivateKeyAuth, and
newClientConfig (the single *ssh.ClientConfig builder). The key is read
via filepath.EvalSymlinks + os.Root containment (openContained), so a
symlinked key works and the read needs no #nosec. The signer is parsed
once; encrypted-no-passphrase and wrong-passphrase map to literal errors
that never echo key bytes or the passphrase.
- internal/sshx/{client,retry,pool}.go: stash the Authentication on the
Client and rebuild the redial config from it; DialBoth/DialDest select
the method with AuthFromHost.
- internal/config: HostConfig gains ssh_key_path/ssh_key_passphrase;
validate enforces exactly one of password/key (passphrase requires a
key); relative key paths resolve against the config-file directory; debug
logs use a non-secret auth label only.
- internal/migrate: reject a database apply from a key-authenticated source
fail-fast (the source MySQL credential derives from the source ssh_pass,
which a key source cannot provide); file/mail and dry-run runs are
unaffected.
Tests cover plaintext/encrypted keys, wrong/missing passphrase, no secret
leak in errors, symlinked keys, dial/redial parity against a
password-rejecting server (key deleted after the initial dial), a
key-authenticated destination, and the DB-apply guard wired into Run.
Reviewer's GuideThis release adds SSH private key authentication support alongside passwords, wires it into config loading and the SSH client/pool, and introduces guards and tests around key usage and database migration behavior, ensuring dial/redial parity and non-leaky error handling. Sequence diagram for SSH client dial using AuthenticationsequenceDiagram
actor Operator
participant Config as config.Load
participant Pool as sshx.DialBoth
participant Auth as sshx.AuthFromHost
participant Dial as sshx.Dial
participant NewCfg as sshx.newClientConfig
participant SSH as dialAttempts
Operator->>Config: Load(path)
Config->>Config: resolveKeyPath for SSHKeyPath
Config->>Config: HostConfig.validate("src"/"dest")
Config-->>Pool: Config with HostConfig
Pool->>Auth: AuthFromHost(cfg.Src)
Auth-->>Pool: Authentication (password or private_key)
Pool->>Dial: Dial(ctx, "source", addr, cfg.Src.SSHUser, Authentication,...)
Dial->>NewCfg: newClientConfig(user, Authentication, hostKeyCB, timeout)
NewCfg-->>Dial: *ssh.ClientConfig
Dial->>SSH: dialAttempts(ctx, name, addr, timeout, ClientConfig)
SSH-->>Dial: *ssh.Client
Dial-->>Pool: *Client with auth stored
Note over Pool,Client: Client.heal uses same Authentication via newClientConfig for redial
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughChangesSSH configuration and connection handling now support private-key authentication alongside passwords. Keys are resolved and parsed once, reused during redials, and selected for source and destination connections. Database apply rejects key-authenticated sources before dialing. SSH key authentication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Config
participant AuthResolver
participant SSHClient
participant SSHServer
Config->>AuthResolver: provide key or password settings
AuthResolver-->>SSHClient: return Authentication
SSHClient->>SSHServer: dial using selected authentication
SSHServer-->>SSHClient: establish SSH connection
SSHClient->>SSHServer: redial with cached authentication
SSHServer-->>SSHClient: reestablish connection
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Several tests assert exact error strings (e.g., in PrivateKeyAuth and validateSourceAuthForDBApply); consider matching via substring or sentinel error values to avoid brittleness if wording changes.
- validateSourceAuthForDBApply currently checks cfg.Src.SSHKeyPath directly; using cfg.Src.AuthMethod() or a shared helper for “is key-auth” would keep DB-auth logic consistent with the rest of the config/auth selection code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several tests assert exact error strings (e.g., in PrivateKeyAuth and validateSourceAuthForDBApply); consider matching via substring or sentinel error values to avoid brittleness if wording changes.
- validateSourceAuthForDBApply currently checks cfg.Src.SSHKeyPath directly; using cfg.Src.AuthMethod() or a shared helper for “is key-auth” would keep DB-auth logic consistent with the rest of the config/auth selection code.
## Individual Comments
### Comment 1
<location path="internal/config/auth_test.go" line_range="48-68" />
<code_context>
+ }
+}
+
+func TestLoad_RelativeKeyPathResolvedAgainstConfigDir(t *testing.T) {
+ dir := t.TempDir()
+ yaml := "src:\n" +
+ " ip: 1.2.3.4\n" +
+ " port: 22\n" +
+ " ssh_user: u\n" +
+ " ssh_key_path: keys/id_ed25519\n" +
+ " timeout: 10s\n"
+ cfgPath := filepath.Join(dir, "host.yaml")
+ if err := os.WriteFile(cfgPath, []byte(yaml), 0600); err != nil {
+ t.Fatal(err)
+ }
+ cfg, err := Load(cfgPath)
+ if err != nil {
+ t.Fatalf("Load: %v", err)
+ }
+ want := filepath.Join(dir, "keys", "id_ed25519")
+ if cfg.Src.SSHKeyPath != want {
+ t.Errorf("SSHKeyPath = %q, want %q (resolved against the config dir)", cfg.Src.SSHKeyPath, want)
+ }
</code_context>
<issue_to_address>
**suggestion (testing):** Extend Load key-path tests to cover destination resolution and the empty-key case.
`Load` now resolves both `Src` and `Dest` via `resolveKeyPath`. Please also add: (1) a test for `Dest.SSHKeyPath` to verify it’s resolved against the config directory, and (2) a test that confirms an empty `ssh_key_path` remains empty. This will fully exercise `resolveKeyPath` and guard against regressions affecting the destination host or empty-key handling.
```suggestion
func TestLoad_RelativeKeyPathResolvedAgainstConfigDir(t *testing.T) {
dir := t.TempDir()
yaml := "src:\n" +
" ip: 1.2.3.4\n" +
" port: 22\n" +
" ssh_user: u\n" +
" ssh_key_path: keys/id_ed25519\n" +
" timeout: 10s\n"
cfgPath := filepath.Join(dir, "host.yaml")
if err := os.WriteFile(cfgPath, []byte(yaml), 0600); err != nil {
t.Fatal(err)
}
cfg, err := Load(cfgPath)
if err != nil {
t.Fatalf("Load: %v", err)
}
want := filepath.Join(dir, "keys", "id_ed25519")
if cfg.Src.SSHKeyPath != want {
t.Errorf("SSHKeyPath = %q, want %q (resolved against the config dir)", cfg.Src.SSHKeyPath, want)
}
}
func TestLoad_RelativeDestKeyPathResolvedAgainstConfigDir(t *testing.T) {
dir := t.TempDir()
yaml := "src:\n" +
" ip: 1.2.3.4\n" +
" port: 22\n" +
" ssh_user: srcuser\n" +
" ssh_key_path: keys/src_id_ed25519\n" +
" timeout: 10s\n" +
"dest:\n" +
" ip: 5.6.7.8\n" +
" port: 22\n" +
" ssh_user: destuser\n" +
" ssh_key_path: keys/dest_id_ed25519\n" +
" timeout: 10s\n"
cfgPath := filepath.Join(dir, "host.yaml")
if err := os.WriteFile(cfgPath, []byte(yaml), 0600); err != nil {
t.Fatal(err)
}
cfg, err := Load(cfgPath)
if err != nil {
t.Fatalf("Load: %v", err)
}
want := filepath.Join(dir, "keys", "dest_id_ed25519")
if cfg.Dest.SSHKeyPath != want {
t.Errorf("Dest.SSHKeyPath = %q, want %q (resolved against the config dir)", cfg.Dest.SSHKeyPath, want)
}
}
func TestLoad_EmptySSHKeyPathRemainsEmpty(t *testing.T) {
dir := t.TempDir()
yaml := "src:\n" +
" ip: 1.2.3.4\n" +
" port: 22\n" +
" ssh_user: srcuser\n" +
" ssh_key_path: \"\"\n" +
" timeout: 10s\n" +
"dest:\n" +
" ip: 5.6.7.8\n" +
" port: 22\n" +
" ssh_user: destuser\n" +
" ssh_key_path: \"\"\n" +
" timeout: 10s\n"
cfgPath := filepath.Join(dir, "host_empty_key.yaml")
if err := os.WriteFile(cfgPath, []byte(yaml), 0600); err != nil {
t.Fatal(err)
}
cfg, err := Load(cfgPath)
if err != nil {
t.Fatalf("Load: %v", err)
}
if cfg.Src.SSHKeyPath != "" {
t.Errorf("Src.SSHKeyPath = %q, want empty string", cfg.Src.SSHKeyPath)
}
if cfg.Dest.SSHKeyPath != "" {
t.Errorf("Dest.SSHKeyPath = %q, want empty string", cfg.Dest.SSHKeyPath)
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+48
to
+68
| func TestLoad_RelativeKeyPathResolvedAgainstConfigDir(t *testing.T) { | ||
| dir := t.TempDir() | ||
| yaml := "src:\n" + | ||
| " ip: 1.2.3.4\n" + | ||
| " port: 22\n" + | ||
| " ssh_user: u\n" + | ||
| " ssh_key_path: keys/id_ed25519\n" + | ||
| " timeout: 10s\n" | ||
| cfgPath := filepath.Join(dir, "host.yaml") | ||
| if err := os.WriteFile(cfgPath, []byte(yaml), 0600); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| cfg, err := Load(cfgPath) | ||
| if err != nil { | ||
| t.Fatalf("Load: %v", err) | ||
| } | ||
| want := filepath.Join(dir, "keys", "id_ed25519") | ||
| if cfg.Src.SSHKeyPath != want { | ||
| t.Errorf("SSHKeyPath = %q, want %q (resolved against the config dir)", cfg.Src.SSHKeyPath, want) | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (testing): Extend Load key-path tests to cover destination resolution and the empty-key case.
Load now resolves both Src and Dest via resolveKeyPath. Please also add: (1) a test for Dest.SSHKeyPath to verify it’s resolved against the config directory, and (2) a test that confirms an empty ssh_key_path remains empty. This will fully exercise resolveKeyPath and guard against regressions affecting the destination host or empty-key handling.
Suggested change
| func TestLoad_RelativeKeyPathResolvedAgainstConfigDir(t *testing.T) { | |
| dir := t.TempDir() | |
| yaml := "src:\n" + | |
| " ip: 1.2.3.4\n" + | |
| " port: 22\n" + | |
| " ssh_user: u\n" + | |
| " ssh_key_path: keys/id_ed25519\n" + | |
| " timeout: 10s\n" | |
| cfgPath := filepath.Join(dir, "host.yaml") | |
| if err := os.WriteFile(cfgPath, []byte(yaml), 0600); err != nil { | |
| t.Fatal(err) | |
| } | |
| cfg, err := Load(cfgPath) | |
| if err != nil { | |
| t.Fatalf("Load: %v", err) | |
| } | |
| want := filepath.Join(dir, "keys", "id_ed25519") | |
| if cfg.Src.SSHKeyPath != want { | |
| t.Errorf("SSHKeyPath = %q, want %q (resolved against the config dir)", cfg.Src.SSHKeyPath, want) | |
| } | |
| } | |
| func TestLoad_RelativeKeyPathResolvedAgainstConfigDir(t *testing.T) { | |
| dir := t.TempDir() | |
| yaml := "src:\n" + | |
| " ip: 1.2.3.4\n" + | |
| " port: 22\n" + | |
| " ssh_user: u\n" + | |
| " ssh_key_path: keys/id_ed25519\n" + | |
| " timeout: 10s\n" | |
| cfgPath := filepath.Join(dir, "host.yaml") | |
| if err := os.WriteFile(cfgPath, []byte(yaml), 0600); err != nil { | |
| t.Fatal(err) | |
| } | |
| cfg, err := Load(cfgPath) | |
| if err != nil { | |
| t.Fatalf("Load: %v", err) | |
| } | |
| want := filepath.Join(dir, "keys", "id_ed25519") | |
| if cfg.Src.SSHKeyPath != want { | |
| t.Errorf("SSHKeyPath = %q, want %q (resolved against the config dir)", cfg.Src.SSHKeyPath, want) | |
| } | |
| } | |
| func TestLoad_RelativeDestKeyPathResolvedAgainstConfigDir(t *testing.T) { | |
| dir := t.TempDir() | |
| yaml := "src:\n" + | |
| " ip: 1.2.3.4\n" + | |
| " port: 22\n" + | |
| " ssh_user: srcuser\n" + | |
| " ssh_key_path: keys/src_id_ed25519\n" + | |
| " timeout: 10s\n" + | |
| "dest:\n" + | |
| " ip: 5.6.7.8\n" + | |
| " port: 22\n" + | |
| " ssh_user: destuser\n" + | |
| " ssh_key_path: keys/dest_id_ed25519\n" + | |
| " timeout: 10s\n" | |
| cfgPath := filepath.Join(dir, "host.yaml") | |
| if err := os.WriteFile(cfgPath, []byte(yaml), 0600); err != nil { | |
| t.Fatal(err) | |
| } | |
| cfg, err := Load(cfgPath) | |
| if err != nil { | |
| t.Fatalf("Load: %v", err) | |
| } | |
| want := filepath.Join(dir, "keys", "dest_id_ed25519") | |
| if cfg.Dest.SSHKeyPath != want { | |
| t.Errorf("Dest.SSHKeyPath = %q, want %q (resolved against the config dir)", cfg.Dest.SSHKeyPath, want) | |
| } | |
| } | |
| func TestLoad_EmptySSHKeyPathRemainsEmpty(t *testing.T) { | |
| dir := t.TempDir() | |
| yaml := "src:\n" + | |
| " ip: 1.2.3.4\n" + | |
| " port: 22\n" + | |
| " ssh_user: srcuser\n" + | |
| " ssh_key_path: \"\"\n" + | |
| " timeout: 10s\n" + | |
| "dest:\n" + | |
| " ip: 5.6.7.8\n" + | |
| " port: 22\n" + | |
| " ssh_user: destuser\n" + | |
| " ssh_key_path: \"\"\n" + | |
| " timeout: 10s\n" | |
| cfgPath := filepath.Join(dir, "host_empty_key.yaml") | |
| if err := os.WriteFile(cfgPath, []byte(yaml), 0600); err != nil { | |
| t.Fatal(err) | |
| } | |
| cfg, err := Load(cfgPath) | |
| if err != nil { | |
| t.Fatalf("Load: %v", err) | |
| } | |
| if cfg.Src.SSHKeyPath != "" { | |
| t.Errorf("Src.SSHKeyPath = %q, want empty string", cfg.Src.SSHKeyPath) | |
| } | |
| if cfg.Dest.SSHKeyPath != "" { | |
| t.Errorf("Dest.SSHKeyPath = %q, want empty string", cfg.Dest.SSHKeyPath) | |
| } | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sshx/auth.go`:
- Around line 96-108: Update AuthFromHost to validate authentication fields
before selecting a method: reject configurations that set both SSHKeyPath and
SSHPass, and reject configurations that provide SSHKeyPassphrase without
SSHKeyPath or alongside password authentication. Preserve the existing
PrivateKeyAuth, PasswordAuth, and missing-method behavior for valid
configurations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb787df7-0278-4c82-b478-e948d54821d3
📒 Files selected for processing (17)
internal/config/auth_test.gointernal/config/config.gointernal/migrate/auth_guard_test.gointernal/migrate/runner.gointernal/sshtest/sshtest.gointernal/sshx/auth.gointernal/sshx/auth_redial_test.gointernal/sshx/auth_test.gointernal/sshx/client.gointernal/sshx/client_dial_test.gointernal/sshx/client_redial_test.gointernal/sshx/client_run_test.gointernal/sshx/pool.gointernal/sshx/pr2_regression_test.gointernal/sshx/retry.gointernal/sshx/retry_dial_test.gointernal/sshx/stream_bridge_test.go
Address review feedback on the private-key auth change. - AuthFromHost now rejects a HostConfig with both ssh_pass and ssh_key_path set, and a passphrase paired with a password (no key), mirroring config.validate. Production callers always go through config.Load (which already rejects these), so this is defense-in-depth for a HostConfig built programmatically without Load: it can no longer silently select the key when both methods are set, or silently drop a passphrase. - Add tests: the destination ssh_key_path resolves against the config-file directory, and an empty ssh_key_path stays empty after Load.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated release PR for
v2.4.0.Summary by Sourcery
Introduce SSH key-based authentication across configuration, migration, and SSH client layers, including path resolution and validation, while enforcing safety around database applies and expanding test coverage for the new auth behavior.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
This PR adds SSH private-key authentication throughout configuration, migration, and SSH connection handling. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Config as Configuration participant Pool as SSH Pool participant Auth as Authentication participant Client as SSH Client Config->>Config: Resolve key paths and validate auth Pool->>Auth: Build password or private-key credential Auth-->>Pool: Parsed credential Pool->>Client: Initial dial Client->>Client: Store credential Client->>Client: Reuse credential on redial%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Config as Configuration participant Pool as SSH Pool participant Auth as Authentication participant Client as SSH Client Config->>Config: Resolve key paths and validate auth Pool->>Auth: Build password or private-key credential Auth-->>Pool: Parsed credential Pool->>Client: Initial dial Client->>Client: Store credential Client->>Client: Reuse credential on redialReviews (2): Last reviewed commit: "fix(sshx): reject ambiguous SSH auth con..." | Re-trigger Greptile