Skip to content

feat: refactor to native ssh binary backend - #397

Open
jeanp413 wants to merge 18 commits into
masterfrom
native-ssh-helper-backend
Open

jeanp413 wants to merge 18 commits into
masterfrom
native-ssh-helper-backend

Conversation

@jeanp413

Copy link
Copy Markdown
Owner

This PR refactors the current work done in src/ssh/nativeClient.ts, so the ssh connection is done using the native ssh binary for all cases not just when GSSAPI is specified in the ssh config.

It introduces a new setting remote.SSH.connectionBackend so the user can manually opt-in using this new connection mode while we achieve feature parity with the current connection mode using the ssh2 js library and fix any remaining bugs.

The plan is that the ssh2 connection mode is deprecated and removed and only the native ssh mode exist in the near future.

@jeanp413

Copy link
Copy Markdown
Owner Author

cc @daiyam

@daiyam

daiyam commented Oct 27, 2026

Copy link
Copy Markdown
Collaborator

@darkbasic @melutovich Can you take a look? Thx

@melutovich

melutovich commented Oct 27, 2026 •

Copy link
Copy Markdown

@darkbasic @melutovich Can you take a look? Thx

Sorry very busy this week and glad to see darkbasic is giving it a look.

@jeanp413 Something to think about if you did not already - perhaps a config option to use a single session to avoid the disadvantage of "The current Windows backend opens separate connections for operations, potentially causing repeated authentication." that darkbasic pointed out.

Very glad that this is being given attention!

@darkbasic darkbasic mentioned this pull request Oct 28, 2026
@darkbasic

darkbasic commented Oct 28, 2026 •

Copy link
Copy Markdown

The main concerns are failures in ordinary connection setup and a regression in the shared installer that also affects the default ssh2 backend.

These are my findings ordered by priority:

  1. Restore termination on fatal installation errors.

    The changed failure paths call print_install_results(), which now only prints. Installation therefore continues after failure to acquire the installation lock, download errors and validation failures. Continuing without the lock permits concurrent processes to modify the same installation. This affects the default ssh2 backend as well as native mode.

    Restore an effective abort path, including when the script runs inside the native wrapper. The existing readiness test hides this regression because its replacement result printer still exits. A focused regression test should exercise the real failure behavior. src/scripts/server-setup.sh:97

  2. Stop deleting the bundled helper during native startup.

    spawnFresh() passes serverScriptPath to unlinkStaleSocket(). On Linux and macOS, that unlinks lib/native/nativeSshServer.js immediately before Node tries to execute it. The first connection fails, and later attempts encounter the missing helper. This was reproduced with the actual method and a disposable helper file.

    Remove the cleanup call. The newly generated random socket path does not require stale-socket cleanup. src/native/nativeSshManager.ts:537

  3. Deliver stderr through the correct callback argument.

    The stderr subscription invokes predicate(line, undefined), making every diagnostic look like stdout. This breaks error handling and prevents the Windows platform probe from recognizing a missing uname command through its stderr branch. The misrouting was reproduced using the actual CommandRunner.

    Change this call to predicate(undefined, line). src/native/nativeSshParser.ts:153

  4. Use remote readiness instead of requiring an authentication diagnostic.

    waitAuthSucceeded() waits indefinitely for specific authentication text. When OpenSSH reuses an existing ControlMaster connection, the new process emits multiplexing diagnostics without either accepted authentication message. A real localhost test successfully executed a remote command while the authentication regex matched nothing.

    Use the existing remote echo round trip as readiness evidence. Its deadline must accommodate authentication interaction, including PIN entry and touching a security key. src/native/nativeSshManager.ts:171

  5. Give server installation an appropriate timeout.

    The complete download, extraction, extension installation and server startup use the command runner's default ten-second deadline. The remote script itself allows thirty seconds for lock acquisition and twenty seconds for server readiness. An ordinary first installation can therefore fail while still progressing normally.

    Allow installation to supply an appropriate operation timeout while keeping short deadlines for simple probes. src/native/nativeSshManager.ts:431, src/native/nativeSshParser.ts:112

  6. Release the installation lock when setup completes.

    The installer releases its lock only through an EXIT trap. Native mode executes it inside a function in a persistent shell, so returning from the function leaves the lock held. Another installation through a different host alias or editor version can then block for the lifetime of the original connection. This was reproduced with the actual locking fragment inside the new wrapper shape.

    Explicitly release the installation lock after setup, independently of the SSH session's lifetime. src/scripts/server-setup.sh:93-117

  7. Resolve shared-helper ownership, or defer reuse between windows.

    A second window can adopt the first window's helper, but disposing the original manager unconditionally terminates that helper. Closing the first window therefore disconnects the second despite its active keepalives.

    Removing the termination call alone is insufficient to establish a reliable shared lifetime: the helper's stdin, stdout and stderr are still piped to its launching window. If supporting independent lifetimes substantially expands this PR, defer reuse between windows. One persistent SSH connection per window already avoids separate authentication for individual setup operations and tunnels. src/native/nativeSshManager.ts:191-199, process setup, disposal

  8. Fix Windows remote installation, or explicitly defer Windows remotes.

    The installer sends a PowerShell method call directly into the current login shell even when the detected shell is cmd.exe. Within PowerShell, the path argument is unquoted. A successful WriteAllBytes call also produces no output to satisfy the completion predicate.

    Use an explicit PowerShell context, a correctly quoted path and a completion marker. Alternatively, reject Windows remotes before bootstrap until this path is supported. These findings come from code inspection; Windows execution was not tested. src/native/nativeSshManager.ts:491-505

  9. Preserve the forwarded agent environment.

    The installation script prints requested environment values such as SSH_AUTH_SOCK, but the native result returns only fixed fields. The resolver consequently retains its null socket value and passes that to the remote extension host, losing access to the forwarded agent.

    Include the requested environment values in the native result, as the existing parseInstallOutput() does. This is a focused compatibility fix. src/native/nativeSshManager.ts:466-475

  10. Encode the authority before using it in a Windows directory name.

    The registry path directly includes destination.toString(). An explicit port produces a component containing a colon, such as ssh-host-user@host:2222-..., which is invalid as a Windows directory name. This affects Windows clients even when connecting to Linux remotes.

    Encode or hash the authority before using it as a filesystem component. src/native/nativeSshManager.ts:96

  11. Handle the unsupported remote Unix-socket configuration before installation.

    Native mode passes remoteServerListenOnSocket into installation, but openNativeTunnel() only handles numeric ports. With the setting enabled, setup creates a server the backend cannot reach and then throws.

    Supporting socket forwarding can be deferred. Rejecting this incompatible setting clearly before installation is a reasonable interim fix for the opt-in backend. src/authResolver.ts:681-718

I don't have write access to your branch so I've created another PR with the fixes: #398

@daiyam

daiyam commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator

Functionality wise, I would change:

  • remote.SSH.connectionBackend to be per host.
  • remove the isGssapiAuthenticationRequested check, users will need to select the native backend to support a connection with GSSAPI.

@melutovich

Copy link
Copy Markdown

@jeanp413 you saw in the comment by darkbasic:

  • I don't have write access to your branch so I've created another PR with the fixes: Native ssh #398

@daiyam

daiyam commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator

@jeanp413 you saw in the comment by darkbasic:

  • I don't have write access to your branch so I've created another PR with the fixes: Native ssh #398

Ya, normal since it's the main repo. This is why I rebase @darkbasic's PR onto this PR. So, @jeanp413 can merge it into this one, if he want.

@jeanp413

Copy link
Copy Markdown
Owner Author

Thanks @darkbasic I've ported most if not all of your suggested fixes to this branch.

remote.SSH.connectionBackend to be per host.

I want to avoid people staying on the old connection mode if they forget to change it in the setting for some host, native mode should work in all current cases plus all additional auth methods supported by ssh. Will keep the simple flag for now but I am open to change it depending on user feedback. For the next release ssh2 will be default mode.

I've finished testing on Linux. Next I will test on Windows and do cleanup.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants