Entirely Internal Connection #1610
Conversation
This makes it easier for the ssh keywords to be overridden.
This makes it a configurable option if the internal ssh keywords are used to override the ones in the ssh config.
WalkthroughThis pull request introduces several modifications across multiple files to enhance SSH connection handling and configuration management. The changes primarily focus on improving flexibility and robustness in SSH-related functionality. Key modifications include the addition of a new optional property The modifications span four files: Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/remote/sshclient.go (1)
882-933: mergeKeywords function is comprehensive.
Each pointer-based SSH field is replaced if non-nil. The function is consistent, though consider also reviewing ConnOverrideConfig if relevant here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/types/gotypes.d.ts(1 hunks)pkg/remote/connutil.go(2 hunks)pkg/remote/sshclient.go(15 hunks)pkg/wshrpc/wshrpctypes.go(2 hunks)
🔇 Additional comments (22)
pkg/remote/sshclient.go (16)
156-156: Validate agent availability with caution.
Ensuring that keys are only added to the SSH agent when it is genuinely available can help avoid unintended errors or security risks.
170-170: Confirm default behavior for batch mode.
Relying on the default value from safeDeref is good, but confirm the fallback logic if the user explicitly sets an unexpected value for SshBatchMode.
199-199: Same key-adding pattern as at line 156.
This code repeats the logic for adding keys to the SSH agent. Make sure to keep both in sync if logic changes.
555-561: safeDeref utility is well-implemented.
This approach neatly avoids nil pointer dereferences by returning a zero value when pointers are nil.
562-565: ptr utility for generic pointer creation.
This straightforward function is useful for succinct pointer creation, especially for complex struct fields.
596-598: Appropriate conditional logic for SSH authentication methods.
Tying pubkey, password, and keyboard-interactive authentication to batch mode and booleans is a valid approach.
665-671: Parsed SSH user/port from opts.
This is a clear approach: only set fields when non-empty, preventing unintended overrides.
674-675: Reading full config for internal SSH keywords.
No concurrency or error checks are performed, but it may be acceptable if fullConfig is guaranteed.
677-677: Initialize empty config if not found.
Creating a new ConnKeywords struct is a safe fallback.
680-684: Merging internal config conditionally.
The approach of only merging config when ConnOverrideConfig is true is consistent with user expectations.
686-688: Sequential merging approach.
Merging flags into partialMerged and then parsedKeywords is straightforward for applying layered overrides.
689-696: Appending identity files.
The reversed order for appended identity files is intentional. Ensure this aligns with user expectations.
721-721: Same port-check concern as line 619.
See prior note about an empty or malformed SshPort leading to a trailing colon in the address.
575-575: Check empty SSH agent path.
When safeDeref(sshKeywords.SshIdentityAgent) is empty, net.Dial may fail. Ensure it’s caught gracefully.
✅ Verification successful
Let me verify how the empty SSH agent path is handled by examining the code context around this line.
Let me check the implementation of safeDeref with a different pattern since the previous one didn't yield results.
Empty SSH agent path is handled gracefully
The code already handles empty SSH agent path appropriately:
safeDerefreturns a zero value when the pointer is nil- If
net.Dialfails (due to empty path or any other reason), the error is logged and the code continues execution - The authentication process falls back to other methods (public key, keyboard-interactive, password) as seen in the subsequent code
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Let's check the context around the net.Dial call and how safeDeref is implemented
rg -B 10 -A 10 'conn, err := net\.Dial\("unix", safeDeref\(sshKeywords\.SshIdentityAgent\)\)' pkg/remote/sshclient.go
Length of output: 1183
Script:
#!/bin/bash
# Let's also check the implementation of safeDeref
ast-grep --pattern 'func safeDeref($_) {
$$$
}'
Length of output: 51
Script:
#!/bin/bash
# Let's see how sshKeywords.SshIdentityAgent is used elsewhere
rg "SshIdentityAgent" pkg/remote/sshclient.go
Length of output: 318
Script:
#!/bin/bash
# Let's find safeDeref implementation with a simpler pattern
rg -B 2 -A 5 "func safeDeref" pkg/remote/sshclient.go
Length of output: 152
Script:
#!/bin/bash
# Let's also see where the error from net.Dial is used in the function
rg -B 5 -A 10 "Failed to open Identity Agent Socket:" pkg/remote/sshclient.go
Length of output: 830
619-619: Ensure chosenPort is set.
If chosenPort is empty, networkAddr might become "hostname:". Verify that a default port is set elsewhere in the flow.
✅ Verification successful
Let me verify how chosenPort is handled by examining the code.
Let me check how sshKeywords.SshPort is handled and if there's a default value set.
Let me check one more thing to see how the SSH port is handled in the configuration and if there's a default port setting.
Port handling is properly implemented with fallback to default port 22
From the code analysis, I can confirm that the port handling is safe:
- The code shows that when
SSHPortis empty or "22", it's handled gracefully in theString()method:
if opts.SSHPort != "22" && opts.SSHPort != "" {
stringRepr = stringRepr + ":" + fmt.Sprint(opts.SSHPort)
}-
The port is obtained through SSH config using
WaveSshConfigUserSettings().GetStrict(hostPattern, "Port"), which will return the standard SSH port 22 if not explicitly set. -
Even if
chosenPortis empty in the network address formation (networkAddr := chosenHostName + ":" + chosenPort), the SSH client library will default to port 22, which is the standard SSH port.
Therefore, the original concern is not an issue as the SSH port handling is implemented with proper defaults.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# First, let's look at the file content around the usage of chosenPort
rg -B 10 -A 5 "chosenPort" pkg/remote/sshclient.go
# Also check where chosenPort is set or initialized
ast-grep --pattern 'chosenPort := $_'
ast-grep --pattern 'chosenPort = $_'
Length of output: 1789
Script:
#!/bin/bash
# Look for the definition of SshPort in the ConnKeywords struct
rg -B 5 -A 5 "type ConnKeywords struct"
# Check for any default port assignments
rg -B 3 -A 3 "SshPort.*=.*"
# Look for the safeDeref function implementation
ast-grep --pattern 'func safeDeref($_) $_'
Length of output: 2004
Script:
#!/bin/bash
# Look for SSH default port definitions or configurations
rg -B 5 -A 5 "22" pkg/remote/sshclient.go
# Check for port validation or default port handling
rg -B 5 -A 5 "port" pkg/remote/sshclient.go --ignore-case
# Look for the complete struct definition to see SshPort field
rg -A 20 "SshPort.*\*string"
Length of output: 6504
568-571: Verify behavior of non-numeric ports.
If SshPort contains a non-numeric or empty string, confirm if downstream usage handles it gracefully.
✅ Verification successful
Let me gather information about how the port value is used and validated in the codebase.
Let me search for more specific information about port handling and network connection establishment.
Let me check one final aspect - the actual network connection handling and any port validation that might occur in the Go SSH package usage.
Based on the gathered information, I can now provide a final response about the port handling behavior:
Non-numeric ports will cause connection failures at the network dial stage
The code shows that:
- The port string is directly concatenated into the network address (
networkAddr := chosenHostName + ":" + chosenPort) - This address is used in
net.Dialer.DialContext(ctx, "tcp", networkAddr) - Invalid ports will fail at the TCP dial stage with standard Go network errors
- The error is properly wrapped in a
ConnectionErrorstructure and propagated to the caller
The behavior is safe because:
- Empty or non-numeric ports will fail early during TCP connection establishment
- Errors are properly handled and propagated through the connection stack
- No risk of undefined behavior or security issues
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for usages of SshPort and port validation
rg -A 5 "SshPort"
# Search for usages of the remoteName variable to see how it's used downstream
rg "remoteName :="
# Look for any port validation or parsing functions
rg -A 5 "port.*validation|validatePort|parsePort"
# Search for xknownhosts.Normalize to understand its behavior with invalid ports
rg -A 5 "xknownhosts.Normalize"
Length of output: 4317
Script:
#!/bin/bash
# Look for the connectInternal function implementation to see how the port is used
ast-grep --pattern 'func connectInternal($$$) {
$$$
}'
# Search for net.Dial usage to see how the network address is handled
rg -A 5 'net.Dial\('
# Look for the implementation of safeDeref function
ast-grep --pattern 'func safeDeref($$$) {
$$$
}'
# Search for any error handling related to connection failures
rg -B 2 -A 5 'ConnectionError'
Length of output: 5663
Script:
#!/bin/bash
# Look for the actual SSH connection establishment
ast-grep --pattern 'func connectInternal($$$) {
$$$
}'
# Search for any SSH dial or connection configuration
rg -B 2 -A 5 'ssh.Dial|ssh.Client'
# Look for any port-related error handling
rg -B 2 -A 5 'dial.*error|connection.*refused|invalid.*port'
Length of output: 6851
pkg/remote/connutil.go (1)
29-29: Port handling now directly relies on the regex capture.
Replacing integer conversion with a string-based approach simplifies parsing but verify that invalid ports (e.g., alphabetic) are handled as intended.
pkg/wshrpc/wshrpctypes.go (4)
464-464: Added ConnOverrideConfig field.
Allows ignoring or overwriting default configs if set to true. This is a useful extension for advanced connection scenarios.
474-476: Pointer-based approach for user, hostname, and port.
These fields elegantly support optional or unset values; safeDeref usage is consistent with these changes.
478-481: Pointer-based boolean fields for SSH authentication.
This design allows explicit differentiation between “false” and “unset.”
483-484: SshAddKeysToAgent and SshIdentityAgent as pointers.
Enables flexible control over agent usage. The new pointer type approach aligns with existing safeDeref patterns.
frontend/types/gotypes.d.ts (1)
293-293: New "conn:overrideconfig" field in ConnKeywords.
Ensures TypeScript type definitions mirror the new Go struct field for configuration overrides.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/remote/sshclient.go (3)
564-564: Failure logging on IdentityAgent.
A log is printed if the socket isn't found. Consider including more context or ways to recover if the agent is essential.
663-664: Fetching fullConfig.
This is a straightforward config initialization. Suggest verifying presence of any newly introduced fields.
871-922: mergeKeywords function.
This merges new keyword pointers into an existing struct. The approach is correct; ensure references are not inadvertently shared.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/remote/sshclient.go(16 hunks)pkg/util/utilfn/utilfn.go(1 hunks)
🔇 Additional comments (31)
pkg/util/utilfn/utilfn.go (2)
945-956: Ensure thorough test coverage for SafeDeref.
The new SafeDeref function is well-defined for generic pointer safety. However, consider unit tests for corner cases, such as nil pointers, zero values, and reference types.
958-966: Ptr function is concise and clear.
This utility cleanly creates pointers without requiring intermediate variables. Good addition for improved readability and maintainability.
pkg/remote/sshclient.go (29)
30-30: Import addition seems appropriate.
Importing “github.com/wavetermdev/waveterm/pkg/util/utilfn” expands utility usage. Ensure there are no cyclical dependencies.
157-157: Use of SafeDeref for SshAddKeysToAgent.
This safely dereferences the pointer. Ensure that the default value is correct in all usage contexts.
171-171: SafeDeref usage for SshBatchMode.
This is correct for preventing nil pointer issues. Confirm that skipping passphrase input is the desired default when SshBatchMode is nil.
200-200: Additional SafeDeref check for SshAddKeysToAgent.
This pattern is consistent. No major issues.
557-560: Check default SSH user, host, or port values carefully.
At lines 557-560, SafeDeref ensures we don’t panic on nil. Make sure any fallback logic is correct if these fields are unset.
585-587: Batch mode concurrency check.
The combined conditions ensure keyboard-interactive or password authentication is skipped in batch mode. This is logically sound.
608-608: networkAddr usage.
Make sure that host and port are combined precisely, especially for IPv6 addresses.
610-610: Explicit user field assignment.
Line 610 sets ssh.ClientConfig.User to the derived user. No issues spotted.
654-654: Initialize local parsedKeywords.
This pointer-based struct usage is consistent with the rest of the code.
655-657: Conditional user assignment.
If opts.SSHUser is non-empty, store it in parsedKeywords. Matches desired logic.
658-660: Conditional port assignment.
Similar logic for port. Helps unify the final merged config.
666-666: Defaulting to empty ConnKeywords.
If no internal config is found, we create an empty struct. Good to prevent nil references.
669-673: mergeKeywords usage #1.
The cascading approach is clear. Ensure the final merges preserve user overrides properly.
675-677: mergeKeywords usage #2.
Continuing to merge in user flag overrides. This chain-of-responsibility pattern is logically consistent.
678-682: Appending identity files #1.
Appending to SshIdentityFile in reverse order is intentional for prioritization. No issues spotted.
683-685: Appending identity files #2.
Combines sshConfigKeywords last. This is consistent with the comment describing the order.
710-710: networkAddr usage in connectInternal.
Combining SshHostName + SshPort through SafeDeref. Carefully handle blank values if the struct is partially nil.
736-744: User resolution fallback logic.
If no user is specified by config, it defaults to current OS user. This is standard SSH behavior. Good approach.
750-756: HostName fallback logic.
If HostName is missing, fallback uses the original host pattern. Ensure the normalized pattern is correct for multi-part host specs.
762-762: Port assignment with Ptr(...)
The usage of utilfn.Ptr ensures correct conversion to pointer. Straightforward.
774-774: BatchMode assignment.
This approach sets the boolean correctly based on “yes” or “no.” Looks good.
781-781: PubkeyAuthentication defaults to true.
In openSSH config, “yes” is typical. Non-‘no’ strings become true. This matches existing patterns.
787-787: PasswordAuthentication logic.
Similar “!= no” condition used. Maintains consistent default.
793-793: KbdInteractiveAuthentication logic.
Again, consistent with the “!= no” pattern.
806-806: AddKeysToAgent detection.
Sets SshAddKeysToAgent to true if the config is “yes.” Matches openSSH usage.
821-821: ExpandHomeDir.
This ensures the IdentityAgent path is properly expanded. Make sure no edge cases for unusual HOME.
830-830: Set SshIdentityAgent pointer.
For explicit IdentityAgent settings, the code is consistent.
856-856: SSHPort type changed to string.
Switching from int to string broadens input flexibility. Thoroughly test for invalid port strings.
865-865: Conditional port display on String().
This logic only appends the port if it’s not 22 and not empty. Standard approach.
This change allows the user to specify all connection details in an internal connection rather than forcing them to rely on the ssh config for certain keywords.