fix(ssh): SSH agent IdentitiesOnly logic and public key parsing #2749
Conversation
- Add logSSHKeywords helper to log SSH config with privacy masking - Enhance agent connection/communication logging with blocklogger - Use INFO level for key events (connect success/failure, signer count) - Use DEBUG level for diagnostic info (config details, key fingerprints) - Mask hostname (show first 3 and last 3 chars only) - Show only basename for identity files - Add Windows-specific hint when agent connection fails
Add comprehensive privacy masking for SSH connection logs: - Add MaskString() to mask usernames, hostnames, and connection identifiers - Add maskIdentityFile() to mask paths while preserving directory structure - Add logSSHKeywords() to log SSH config with masked sensitive values - Mask remoteName and shellPath in shellcontroller.go - Mask connection name and knownhosts address in conncontroller.go - Mask networkAddr, opts.String(), and fingerprints in sshclient.go - Fix StreamCancelFn type signature to accept context.Context
…xt parameter" This reverts commit 43fb6e8.
This change fixes issues preventing SSH agent usage on Windows when `IdentitiesOnly` is enabled. - Fix Agent Logic: Always attempt agent connection even if `IdentitiesOnly` is set. - Fix Windows Defaults: Update `findSshDefaults` to use the correct named pipe. - Enhance Parsing: Support `authorized_keys` format for `.pub` files.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughChanges introduce privacy-preserving logging across three packages by replacing direct printing of sensitive values (connection names, host addresses, file paths, SSH-related strings) with masked outputs. New helpers Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 |
andya1lan
changed the title
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical issue where IdentitiesOnly=yes would prevent SSH agents from being used entirely. The fix implements smart filtering that allows agent keys matching configured identity files, aligning with OpenSSH behavior. Additionally, it improves public key parsing to handle multiple formats (.pub files, raw keys, private keys), sets correct Windows SSH agent defaults, and adds privacy-focused logging with masking of sensitive information.
Key Changes
- Removed the
IdentitiesOnlycheck from agent connection logic, now connecting to the agent regardless and filtering signers afterward - Implemented fingerprint-based matching to filter agent signers when
IdentitiesOnly=true, trying multiple key formats (authorized_key, raw, private key, and .pub file) - Added masking utilities (
MaskString,maskIdentityFile) to protect sensitive data in logs and improved SSH configuration logging
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| pkg/remote/sshclient.go | Core changes: added masking utilities, implemented IdentitiesOnly filtering with multi-format key parsing, removed agent connection gate, improved error messages and logging, added Windows named pipe default |
| pkg/remote/conncontroller/conncontroller.go | Applied masking to connection names in logging for privacy |
| pkg/blockcontroller/shellcontroller.go | Applied masking to remote names and shell paths in logging for privacy |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pubKeyData, err = os.ReadFile(pubKeyPath) | ||
| if err == nil { | ||
| pubKey, _, _, _, err = ssh.ParseAuthorizedKey(pubKeyData) |
There was a problem hiding this comment.
The variable pubKeyData is being reused in step 4 (line 279) where it's read from the .pub file. This shadows the original pubKeyData from line 246 and could cause issues if the .pub file reading fails - the subsequent ParseAuthorizedKey call would use the .pub file data instead of the original identity file data. This is likely not the intended behavior. Consider using a different variable name like pubFileData for the .pub file content to avoid shadowing.
| pubKeyData, err = os.ReadFile(pubKeyPath) | |
| if err == nil { | |
| pubKey, _, _, _, err = ssh.ParseAuthorizedKey(pubKeyData) | |
| pubFileData, err := os.ReadFile(pubKeyPath) | |
| if err == nil { | |
| pubKey, _, _, _, err = ssh.ParseAuthorizedKey(pubFileData) |
![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
🧹 Nitpick comments (2)
pkg/remote/sshclient.go (2)
92-190: LGTM: Well-designed privacy-preserving helpers.The masking functions provide good privacy protection:
MaskStringuses rune-based slicing for proper UTF-8 handlinglogSSHKeywordslogs SSH config at DEBUG level with appropriate maskingmaskIdentityFilehandles cross-platform home directory pathsThe
maskIdentityFilefunction is quite complex with its path manipulation logic. While the implementation appears sound, consider adding unit tests to verify edge cases (relative paths, symlinks, unusual path structures).
241-311: LGTM: Correct IdentitiesOnly implementation.This properly implements OpenSSH's IdentitiesOnly behavior: the SSH agent is still queried, but only keys matching configured IdentityFiles are used. The multi-format public key parsing (authorized_key, raw, private key, .pub file) ensures compatibility with various key formats including 1Password-style .pub files.
The fallback chain (authorized_key → raw public key → private key → .pub file) means some formats will be tried unnecessarily, but this ensures maximum compatibility. If performance becomes a concern with many identity files, consider optimizing by checking file extensions first.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/blockcontroller/shellcontroller.gopkg/remote/conncontroller/conncontroller.gopkg/remote/sshclient.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-22T01:28:41.417Z
Learnt from: esimkowitz
Repo: wavetermdev/waveterm PR: 1790
File: pkg/remote/fileshare/wshfs/wshfs.go:122-122
Timestamp: 2025-01-22T01:28:41.417Z
Learning: The RpcClient in pkg/remote/fileshare/wshfs/wshfs.go is initialized and handled downstream by either main-server or wshcmd-connserver, as documented in the package comment.
Applied to files:
pkg/remote/conncontroller/conncontroller.go
🧬 Code graph analysis (2)
pkg/remote/sshclient.go (4)
pkg/wconfig/settingsconfig.go (1)
ConnKeywords(305-344)pkg/blocklogger/blocklogger.go (2)
Debugf(86-92)Infof(78-84)pkg/util/utilfn/utilfn.go (1)
SafeDeref(941-947)pkg/wavebase/wavebase.go (1)
ExpandHomeDir(149-163)
pkg/remote/conncontroller/conncontroller.go (2)
pkg/blocklogger/blocklogger.go (1)
Infof(78-84)pkg/remote/sshclient.go (1)
MaskString(118-127)
⏰ 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). (3)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (go)
- GitHub Check: merge-gatekeeper
🔇 Additional comments (5)
pkg/blockcontroller/shellcontroller.go (1)
386-386: LGTM: Privacy-preserving logging.The masking of
remoteNameandShellPathprevents sensitive information from appearing in logs while retaining enough context for debugging.pkg/remote/conncontroller/conncontroller.go (1)
553-553: LGTM: Consistent privacy-safe logging.Connection names and addresses are now masked in logs, preventing exposure of sensitive information like usernames and hostnames while maintaining debuggability.
Also applies to: 751-751, 768-768
pkg/remote/sshclient.go (3)
76-80: LGTM: Error messages now privacy-safe.Connection information in error messages is masked, preventing sensitive data exposure while preserving debuggability.
783-821: LGTM: Enhanced SSH agent diagnostics with privacy.The improved agent logging provides valuable diagnostic information:
- Connection attempts and results are logged at appropriate levels (DEBUG/INFO)
- Key fingerprints are masked for privacy
- Windows-specific hints guide users when the agent service isn't running
- The agent path is reduced to basename only for privacy
The comprehensive logging will significantly help troubleshoot agent-related issues while maintaining privacy.
887-887: LGTM: Comprehensive connection logging privacy.Network addresses, hostnames, and connection identifiers are now consistently masked throughout the connection flow, protecting sensitive information while retaining diagnostic value.
Also applies to: 894-894, 906-906, 911-911, 930-930, 938-938
…directly under user home
|
Hi @sawka waveterm/pkg/remote/sshclient.go Line 972 in a07e078 won't expand $SSH_AUTH_SOCK on Unix.However, this only affects users with ignoresshconfig=true since normal people would use findSshConfigKeywords() which correctly resolves the env var via shell.Should I fix this in this PR or leave it for a follow-up? |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/remote/sshclient.go (1)
1180-1185: Unix IdentityAgent default behavior still needs resolution.As noted in previous review comments, the Unix branch uses
ssh_config.Default("IdentityAgent")which does not resolve the$SSH_AUTH_SOCKenvironment variable, unlikefindSshConfigKeywords(lines 1116-1133) which executes a shell command. This affects users withignoresshconfig=trueon Unix systems.The Wave team discussion on this behavior is still pending per the author's previous comment.
🧹 Nitpick comments (1)
pkg/remote/sshclient.go (1)
251-321: Approve with optional clarity suggestion on variable reuse.The IdentitiesOnly filtering logic is comprehensive and correctly implements OpenSSH semantics. The multi-format parsing (authorized_key, raw public key, private key, .pub file) ensures broad compatibility.
One minor clarity point: At line 289,
pubKeyDatais reassigned when reading the .pub file, shadowing the original variable from line 256. While functionally correct (step 4 is specifically for encrypted private keys), consider using a different variable name likepubFileDatafor improved readability.Optional: Improve variable naming clarity
- pubKeyData, err = os.ReadFile(pubKeyPath) + pubFileData, err := os.ReadFile(pubKeyPath) if err == nil { - pubKey, _, _, _, err = ssh.ParseAuthorizedKey(pubKeyData) + pubKey, _, _, _, err = ssh.ParseAuthorizedKey(pubFileData)
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/remote/sshclient.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/remote/sshclient.go (4)
pkg/wconfig/settingsconfig.go (1)
ConnKeywords(305-344)pkg/blocklogger/blocklogger.go (2)
Debugf(86-92)Infof(78-84)pkg/util/utilfn/utilfn.go (2)
SafeDeref(941-947)Ptr(955-957)pkg/wavebase/wavebase.go (1)
ExpandHomeDir(149-163)
🔇 Additional comments (7)
pkg/remote/sshclient.go (7)
77-79: LGTM: Privacy-preserving error messages.The masking of connection details in error messages prevents sensitive information from being exposed in logs.
92-114: LGTM: Well-designed sanitized logging.The function appropriately masks sensitive SSH configuration details while providing useful debugging information. The use of DEBUG level and secure handling of secrets (e.g., PasswordSecretName) is correct.
116-127: LGTM: Proper UTF-8-aware masking.The rune-based slicing ensures correct handling of multi-byte UTF-8 characters, and edge cases are properly covered.
129-200: LGTM: Proper handling of edge cases in path masking.The special handling for
filepath.Dir()returning "." (lines 166-170, 184-188) correctly addresses the corner case where files are placed directly in the user's home directory, preventing malformed paths like "/home/u***/." This aligns with the commit message and resolves past review concerns.
793-794: LGTM: Appropriate placement of configuration logging.Logging the SSH configuration at the start of client config creation provides useful debugging context.
803-831: LGTM: Correctly implements IdentitiesOnly logic fix.This change addresses the core issue in the PR: the agent is now contacted even when
IdentitiesOnlyis true, with filtering applied later increatePublicKeyCallback(lines 251-321). This matches OpenSSH behavior where the agent can still be used, but only for explicitly listed keys.The enhanced logging with platform-specific hints (e.g., Windows service check at line 809) and fingerprint debugging is valuable for troubleshooting.
897-921: LGTM: Consistent masking in connection logs.The masking of network addresses and host information across all connection-related log messages maintains privacy while preserving debuggability.
1 participant
Summary
This PR fixes a critical issue where enabling IdentitiesOnly would prevent the SSH agent from being used entirely.
It also corrects the default SSH agent path on Windows and fixes a type mismatch in wshrpc.
Problem
IdentitiesOnly Conflict: The previous implementation checked !sshKeywords.SshIdentitiesOnly before connecting to the agent.
waveterm/pkg/remote/sshclient.go
Lines 617 to 625 in f622c65
If IdentitiesOnly was set to yes (to restrict key usage), Wave would skip the agent entirely, leading to authentication failures.
Changes
createPublicKeyCallback. If IdentitiesOnly is true, Wave will fetch the agent's keys but filter them to only use keys that fingerprint-match the locally configured.findSshDefaultsto support Windows as well.Out of Scope
SSH Config Match Keyword: The underlying SSH configuration parser library used by Wave does not currently support the Match keyword. Consequently, settings (like IdentityFile or IdentitiesOnly) must be explicitly defined within Host blocks. Match blocks will be ignored and are not addressed by this PR.
Conflicts
This branch is based on #2748