Repository navigation
Native ssh - #398
Closed
darkbasic wants to merge 16 commits into
Closed
Native ssh#398darkbasic wants to merge 16 commits into
darkbasic wants to merge 16 commits into
Conversation
Route installation failures through the existing fail_and_exit helper so setup emits its failure result and stops instead of continuing into later installation or launch steps. Preserve the existing protocol: the result contains exitCode=1 while the shell exits with status 0. Use same-shell blocks for directory creation and archive error handlers so an exit terminates the installer rather than only a subshell. This also protects the default ssh2 backend from continuing without the install lock. Update the four existing readiness tests to run the real result callbacks and require exactly one result block. The corrected regression test fails against the original implementation. No additional test harness was added. Validation: four startup tests, targeted ESLint, Bash syntax, TypeScript compilation, and three independent reviews passed.
Remove the cleanup call that passed nativeSshServer.js to a socket-unlink helper immediately before spawning it. On Unix this deleted the installed entrypoint and broke both the initial connection and subsequent retries. Remove the now-unused unlink method; new helper sockets already receive random paths. Add one focused regression test that launches the actual helper from a temporary transpiled entrypoint, verifies the entrypoint remains intact, and checks its HTTP health endpoint. A local Node stdin reader substitutes for SSH so the test requires no remote host or credentials. Validation: helper startup regression, targeted ESLint, TypeScript compilation, and three independent reviews passed.
Deliver stderr lines as predicate(undefined, line) instead of passing them as stdout. Callers can now distinguish command output from diagnostics, including the missing-uname fallback used by the Windows platform probe. Add one minimal regression through the real LineParser and CommandRunner that verifies both output streams reach the expected predicate arguments. Validation: all six focused tests, targeted ESLint, TypeScript compilation, and three independent reviews passed.
Remove the authentication-message gate and its unused parser expression. OpenSSH clients reusing an existing ControlMaster can open a working shell without emitting either of those authentication diagnostics, leaving the previous initialization path waiting indefinitely. Use the existing echo round trip as the initial readiness check, with a two-minute deadline to allow interactive PIN/passphrase entry and token touch. Forward an optional command timeout while preserving the default for subsequent probes. Add one initialization regression using the actual helper and a local Bash session that emits no authentication messages. Stub only server installation and reuse the existing helper fixture. A watchdog cleans up a hung regression. Validation: three native manager/parser tests, targeted ESLint, TypeScript compilation, and three independent reviews passed.
Apply a five-minute deadline to the complete Bash and PowerShell installer commands. Downloads, extension installation, lock acquisition and server startup can legitimately exceed the ordinary ten-second command deadline. Keep the existing short timeout for other probes and setup commands. Add one focused regression that advances virtual time beyond ten seconds and then completes installation through the real command runner and result parser. It verifies the original failure without a slow download or remote server requirement. Validation: native manager/parser tests, targeted ESLint, source compilation, focused test type-checking, and three independent reviews passed. The stock compile:test configuration already excludes its own test directory; focused test types were checked with an ignored temporary configuration.
Explicitly unlock the installer descriptor, close it, and remove the installer's EXIT/INT/HUP traps before emitting the successful result. A native connection keeps its shell alive after setup, so waiting for shell exit previously held the installation lock for the whole session. Initialize the descriptor marker so cleanup only applies when locking was available and acquired. Add one real Bash/flock regression that executes the actual acquisition and completion fragments inside the persistent wrapper, then verifies a second process can acquire the lock before the first shell exits. The regression fails against the previous implementation. Keep failure cleanup through the existing exit trap. Validation: all 116 tests across nine non-Docker suites, production build, targeted ESLint, focused test type-checking, and Bash syntax passed. Three independent reviewers approved this fix and the cumulative six-fix diff.
Do not adopt a healthy helper created by another manager. Its SSH stdin, stdout, stderr and askpass service belong to the launching window, whose disposal terminates the helper and disconnects any adopting window. Always start a helper for the current manager and run its own bootstrap. This defers cross-window reuse while retaining one persistent SSH session per window, without introducing a new shared-process ownership protocol. Extend the existing real-helper readiness regression to initialize two managers for the same authority, dispose the first, and verify the second still answers health checks and executes commands. Validation: source compilation, targeted ESLint and all three native manager tests pass. Three independent reviewers approved the final diff.
The native Windows installer sent PowerShell syntax into the login shell, used an unquoted path and waited for output from a silent file write. Defer this unsupported path instead of adding a new upload protocol and claiming Windows support without Windows integration coverage. Reject explicitly configured Windows remotes before helper startup and autodetected CMD or PowerShell Windows sessions before installation. Recommend the existing ssh2 backend in the error and document the limit in the README and backend setting. Windows clients connecting to Unix hosts and the default ssh2 Windows installer remain supported. Remove the redundant uname probe, which could stall before the actual platform detector, and delete the now-unreachable native Windows upload and installer code. Keep the existing Bash installation path. Validation: source and focused test typechecks, targeted ESLint and six manager tests pass, including configured Windows, CMD and PowerShell rejection cases. Three independent reviewers approved the final diff.
The installer reports requested environment values, but the native result previously discarded them. The resolver consequently passed a null SSH_AUTH_SOCK to the remote extension host even when the SSH session had a forwarded agent socket. Include only the requested environment keys in the parsed native result, following the existing ssh2 installer parser. This allows the resolver to propagate the forwarded socket without exposing unrelated output fields. Extend the existing slow-installation test with the requested agent socket and an unrequested value, proving preservation and filtering while retaining the installation-timeout regression. No new fixture is needed. Validation: source and focused test typechecks, targeted ESLint and all six manager tests pass. Three independent reviewers approved the final diff.
Authorities with explicit ports contain colons, so placing them directly in directory names prevents native connections from Windows clients. IPv6 authorities also contain filesystem-reserved characters. Hash the full authority with SHA-256 and use its hexadecimal digest in the registry directory. This provides a stable, bounded filename component without collapsing different users, hosts or ports into one identity. Existing helper cleanup keeps its original path; cross-window adoption is already disabled, so no registry migration is needed. Add one focused regression using an IPv6 authority with an explicit port. Check Windows-safe characters, isolation between ports, and a real write followed by reading through a newly created registry instance. Validation: source and focused test typechecks, targeted ESLint and all seven manager tests pass. Three independent reviewers approved the diff.
Native tunnels currently require a numeric TCP port. Passing remoteServerListenOnSocket through to installation creates a server the backend cannot reach, then fails when opening the tunnel. Reject this configuration at manager initialization before acquiring a helper or running installation. Explain how to disable the setting or select the ssh2 backend, and document the limitation in the README and setting description. Socket forwarding remains deferred for native mode. Add one regression that requests socket mode with an unavailable helper and verifies the actionable configuration error takes precedence over startup. The existing TCP tests retain coverage for supported requests. Validation: all 121 non-Docker tests in nine suites pass. Source and focused test typechecks, targeted ESLint and the production webpack build pass. Three independent reviewers approved the final diff before this commit. Windows remote execution and physical security-key authentication were not exercised.
Pass an explicit 127.0.0.1 bind address to OpenSSH's dynamic forward. A bare port lets GatewayPorts=yes expose the internal unauthenticated SOCKS proxy on all interfaces, allowing other reachable machines to use the authenticated connection. The resolver already connects to this proxy over IPv4 loopback, as does the existing native fallback. Validation: a real OpenSSH connection with GatewayPorts=yes now listens only on 127.0.0.1. Loopback connects successfully and the machine's LAN address refuses the connection. Source compilation, targeted ESLint and 24 native-client/proxy-port tests pass. Three independent reviewers approved the final one-line diff before committing.
Use compact random Unix socket filenames so normal macOS temporary directories fit within the 103-byte socket path limit. Check the complete UTF-8 byte length and fall back to /tmp when a configured runtime or temporary directory is too long. Keep the existing 120-bit randomness and Windows named-pipe behavior. This avoids adding temporary-directory ownership or cleanup machinery. Both socket callers continue using their existing socket lifecycle. Add one parameterized regression covering a normal macOS-shaped temporary directory and an oversized multibyte runtime directory. Both askpass and helper paths must satisfy the byte limit and bind a real local socket. Validation: source and focused test typechecks, targeted ESLint and all 10 native connection/manager tests pass. Three independent reviewers approved the final diff before committing. Actual macOS execution was not available; path limits are checked explicitly on the Linux runner.
A failed installation left the helper and askpass listener active. Its keepalive prevented idle cleanup, and later resolution attempts overwrote the owning fields, leaving authenticated sessions and timers unreachable. Dispose resources acquired by the failing attempt before reporting its original error, and release previous native resources before replacing them. Keep per-attempt references so cleanup cannot clear a newer attempt's fields. Log disposal failures without hiding the bootstrap error, and use the same cleanup at resolver shutdown. Record helper ownership before waiting for readiness so startup failures can also terminate the child. Stop keepalives when the helper exits. Await askpass socket permissions before returning from creation, let Node remove the socket on close, and close pending HTTP connections where supported. This avoids racing a pending chmod or Node's socket unlink during cleanup. Add two focused regressions: a real resolver/helper/Bash/askpass setup with a missing installer template, and a real child disposed after a forced readiness failure. The integration test fails against the old resolver because the helper remains healthy, and passes with this fix. Correct the existing VS Code mock's cancellation-token cast for focused typechecking. Validation: 125 non-Docker tests across eleven suites pass, along with source and focused test typechecks, targeted ESLint and the production webpack build. Three independent reviewers approved the final diff before committing. Actual macOS/Windows execution and physical security-key hardware were not tested.
…ation Forward SSH_ASKPASS_PROMPT from the askpass helper to the extension. For OpenSSH's informational "none" hint, show a nonmodal notification and return an empty successful response immediately instead of opening a password input box. OpenSSH waits for the notification helper to exit when user presence completes, including before a later PIN prompt. Waiting for an unrelated input box can stall authentication until the bootstrap deadline. Keep ordinary PIN and password prompts on the existing interactive path. Add two focused cases using the real helper subprocess and askpass HTTP server: a notification whose UI promise never resolves still completes, and a normal PIN prompt returns the user's answer. Validation: 12 focused tests, source compilation, focused test typecheck, and lint passed. Three independent reviewers approved the final diff. Physical FIDO hardware was not exercised.
Pass ControlPath=none for the native backend's SSH invocation so each window owns its SSH process and dynamically allocated SOCKS listener. Document this intentional connection-sharing restriction in the README. When a user's SSH configuration enables ControlMaster and ControlPersist, terminating the foreground SSH process can leave the generated -D forward on a persistent master. New SOCKS clients can still use that abandoned listener, and repeated connection attempts accumulate additional forwards. Disabling sharing for this invocation keeps cleanup tied to the owned child process without shutting down unrelated user master connections. ControlPath=none disables both attachment to an existing master and creation of a new one. No additional ControlMaster/ControlPersist override or shared-master teardown protocol is needed. Validation: actual native helper plus OpenSSH against an isolated local SSH server completed shell commands and closed the SOCKS listener after disposal, including with ControlMaster=auto and ControlPersist=300. All 127 non-Docker tests, source compilation, focused test typecheck, touched-file lint, and the production bundle passed. Three independent reviewers approved the final diff.
Owner
|
Closing this as I manually merged the changes in the original PR, thanks @darkbasic |
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
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.
Further fixes on top of #397