Skip to content

Allow callers to select an exact SSH config - #791

Open
sdmcclain wants to merge 2 commits into
MisterTea:masterfrom
sdmcclain:feature/exact-ssh-config
Open

Allow callers to select an exact SSH config#791
sdmcclain wants to merge 2 commits into
MisterTea:masterfrom
sdmcclain:feature/exact-ssh-config

Conversation

@sdmcclain

@sdmcclain sdmcclain commented Aug 13, 2026

Copy link
Copy Markdown

Summary

  • Add --ssh-config <absolute-path|none> so callers can select one SSH configuration policy.
  • Add --no-ssh-config as an alias for --ssh-config none.
  • Use the selected policy for ET destination and jumphost resolution, and pass the same -F value to both SSH subprocess shapes.
  • Preserve the existing ambient user/system configuration behavior when neither option is present.

Motivation

ET currently parses user and system SSH configuration before starting SSH. A managed client that already owns its SSH policy cannot prevent ambient HostName, ProxyJump, LocalForward, or SetEnv directives from changing the target or session behavior.

An exact configuration root lets such clients supply a private, auditable SSH policy without modifying user dotfiles or inheriting unrelated settings.

Safety and behavior

An explicit file must be absolute, readable, regular, non-symlinked, and use a conservative path alphabet (ASCII letters, digits, slash, dot, underscore, and hyphen). Metadata is checked before opening the file, so non-regular paths such as FIFOs fail without blocking. The alphabet restriction matters because OpenSSH constructs its implicit ProxyJump command through a shell and propagates -F without quoting its value.

The selected file is used by ET for destination and jumphost lookup and is passed to the destination bootstrap and direct jumphost SSH processes. OpenSSH propagates that -F setting to the implicit ProxyJump child. The none form skips ET config parsing and invokes SSH with -F none.

Validation

  • Ubuntu 22.04 arm64 no-vcpkg build of et and et-test passed.
  • Focused [SshSetupHandler] suite passed: 69 assertions in 10 test cases.
  • CLI probes passed for exact-file selection, none, relative paths, unsafe paths, missing files, symlinks, directories, FIFOs, and mutually exclusive options. The FIFO probe used a timeout and confirmed immediate status 1 rejection rather than an open/block.
  • clang-format 18.1.8 dry-run passed on every changed C++ file.
  • git diff --check passed.

Compatibility with #790

This change has no semantic dependency on #790 and intentionally does not change the existing --ssh-option behavior. Its tests use no destination SSH options, so their expectations are valid before or after #790. As a textual compatibility check, the #790 patch applies cleanly on top of this branch with every hunk succeeding. Either PR can land first.

@sdmcclain
sdmcclain force-pushed the feature/exact-ssh-config branch 2 times, most recently from 1ef964f to 12d880e Compare August 13, 2026 17:44
@codecov

codecov Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.64865% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 74.08%. Comparing base (3e8db00) to head (1d373f7).

Files with missing lines Patch % Lines
src/terminal/SshSetupHandler.cpp 95.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #791      +/-   ##
==========================================
+ Coverage   73.83%   74.08%   +0.24%     
==========================================
  Files          97       97              
  Lines        8900     8970      +70     
  Branches     5855     5918      +63     
==========================================
+ Hits         6571     6645      +74     
+ Misses       1720     1718       -2     
+ Partials      609      607       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sdmcclain and others added 2 commits September 1, 2026 13:29
ET currently reads the user and system SSH configuration before launching its own SSH subprocesses. In managed jumphost clients this can rewrite an authenticated target through HostName, import LocalForward entries, or apply ambient ProxyJump and SetEnv behavior before the caller-owned SSH policy runs.

Add --ssh-config <absolute-path|none> and --no-ssh-config. An exact path becomes ET's sole configuration input for destination and jumphost resolution and is passed as -F to both spawned SSH shapes; none disables configuration entirely. Preserve the existing ambient behavior when neither option is present.

Fail closed on missing, non-regular, symlinked, relative, or shell-unsafe paths. The conservative path alphabet is required because OpenSSH renders the implicit -J child through a shell without quoting its propagated -F value.

Tests record both destination and direct-jump argv, verify exact -F propagation, and cover the disabled-config shape.
IsSshConfigPathSafeForProxyJump treated "starts with /" as absolute and
rejected drive letters and backslashes, so native Windows config files
could never pass. Use std::filesystem::path::is_absolute() and allow
':' and '\\' only on Windows, where OpenSSH and CreateProcess need them.

Co-authored-by: Cursor <cursoragent@cursor.com>
@MisterTea
MisterTea force-pushed the feature/exact-ssh-config branch from 12d880e to 1d373f7 Compare September 1, 2026 18:34
@MisterTea

Copy link
Copy Markdown
Owner

Rebased onto current master and added Windows support for --ssh-config.

--ssh-config now uses std::filesystem::path::is_absolute() instead of requiring a leading /, and on Windows the ProxyJump-safe alphabet also allows : and \ so drive and UNC paths work. Spaces are still rejected (OpenSSH interpolates -F unquoted; Windows CreateProcess also concatenates argv without quoting).

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.

2 participants