Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dynamically generate profiles from hosts in OpenSSH config files #14042

Merged
merged 10 commits into from
Dec 9, 2022

Conversation

jonthysell
Copy link
Contributor

@jonthysell jonthysell commented Sep 20, 2022

This PR adds a new dynamic profile generator which creates profiles to
quickly connect to detected SSH hosts.

This PR adds a new SshHostGenerator inbox dynamic profile generator.
When run, it looks for an install of our
Win32-OpenSSH client app
ssh.exe in all of the (official) places it gets installed. If the exe
is found, the generator then looks for and parses both the user and
system OpenSSH config files for valid SSH hosts. Each host is then
converted into a profiles to call ssh.exe and connect to those hosts.

VALIDATION
Installed OpenSSH, configured host for alt.org NetHack server, connected
and played some NetHack from the created profile.

  • When OpenSSH is not installed, don't add profiles
  • Detected when installed via Optional Features (installs in
    System32\OpenSSH, added to PATH)
  • Detected when installed via the 32-Bit OpenSSH MSI from GitHub
    (installs in Program Files (x86)\OpenSSH, not added to PATH)
  • Detected when installed via the 64-Bit OpenSSH MSI from GitHub
    (installs in Program Files\OpenSSH, not added to PATH)
  • Detected when installed via winget install Microsoft.OpenSSH.Beta (uses MSI from GitHub)
  • With "disabledProfileSources": ["Windows.Terminal.SSH"] the
    profiles are not generated

Closes #9031

@ghost ghost added Area-Extensibility A feature that would ideally be fulfilled by us having an extension model. Area-Settings Issues related to settings and customizability, for console or terminal Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Sep 20, 2022
@jonthysell jonthysell changed the title Dynamically generate profile with .ssh/config Dynamically generate profiles from .ssh/config hosts Sep 20, 2022
@jonthysell jonthysell changed the title Dynamically generate profiles from .ssh/config hosts Dynamically generate profiles from hosts in OpenSSH config files Sep 20, 2022
@jonthysell
Copy link
Contributor Author

e4c57c92-1179-482a-bb4c-90552a36109f

This PR adds a new `SshHostGenerator` inbox dynamic profile generator. When run, it looks for an install of our [Win32-OpenSSH](https://github.com/PowerShell/Win32-OpenSSH) client app `ssh.exe` in all of the (official) places it gets installed. If the exe is found, the generator then looks for and parses both the user and system OpenSSH config files for valid SSH hosts. Each host is then converted into a profiles to call `ssh.exe` and connect to those hosts.

Closes microsoft#9031
@jonthysell jonthysell marked this pull request as ready for review September 20, 2022 22:40
@jonthysell
Copy link
Contributor Author

Tests pass, but I don't see an obvious place for adding new tests (there are no existing tests for dynamic profiles).

I'm not sure what Schema needs to be updated.

Working on a paired docs PR.

Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Schema: this is probably the only thing that needs to be updated there -
    "DynamicProfileSource": {
    "enum": [
    "Windows.Terminal.Wsl",
    "Windows.Terminal.Azure",
    "Windows.Terminal.PowershellCore"
    ],
    "type": "string"
    },
  • that _ prefix thing

Other than that, this is perfect! Thank you so much!

src/cascadia/TerminalSettingsModel/SshHostGenerator.h Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/SshHostGenerator.cpp Outdated Show resolved Hide resolved
carlos-zamora added a commit that referenced this pull request Oct 8, 2022
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-- blocking for team discussion --

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 10, 2022
@DHowett DHowett added the Needs-Discussion Something that requires a team discussion before we can proceed label Oct 10, 2022
@lhecker
Copy link
Member

lhecker commented Oct 10, 2022

We've discussed this PR among the team today. We've had a history of generating too many profiles during startup which leads to usability problems when this list for instance gets to too long. For instance my list looks like that by default already when I first launch Windows Terminal:
image

Long term we'd like to improve this by introducing a nested new tab menu like this:
image

(See also #13763.)

...as well as adding a revamped settings UI which nests or groups profiles, removing them from the navigation view on the left. Finally, we've been thinking about not adding any generated profiles by default anymore and instead improve the settings UI in such a way that creating/cloning profiles will be much easier. So instead of adding all your SSH hosts all at once you can choose which ones are supposed to show up in your UI first.
While the first one should be landing fairly soon as an initial implementation, we have no design or plans ready for the latter two just yet. But we believe that this PR and the unbounded list of profiles it can generate, requires these improvements to exist first, or otherwise it would swamp users with too many profiles making UI navigation too "annoying".

As such we'd like to hide it behind a feature flag for now similar to Feature_VtPassthroughModeSettingInUI which disables it by default until we can sort out our UI/UX issues. 😕

Would you agree with our reasoning? If not, please let us know! 🙂

@ffes
Copy link

ffes commented Oct 11, 2022

Although I would LOVE to have this feature, as I asked in #9031 (comment), I am pleased with this decision. 👍🏻

With over 20 profiles in my .ssh/config I really don't want them all just added to my list of profiles. There are already 7 profiles there, and I have disabled a couple of the default ones. Having a UI to manage that would be great and having the submenus would make it perfect.

carlos-zamora added a commit that referenced this pull request Oct 11, 2022
@ghost ghost added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Oct 18, 2022
@ghost
Copy link

ghost commented Oct 18, 2022

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

@vbrozik
Copy link

vbrozik commented Oct 18, 2022

Although the idea of this feature is great it would be unusable for serious users of hosts inside OpenSSH config:

  1. As mentioned above: It is unusable for tens of hosts - it would generate too long menu if submenus are not implemented.
  2. It does not scan Include files. Includes are really useful for managing large OpenSSH configuration split for multiple subjects.

@ghost ghost removed the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Oct 18, 2022
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 24, 2022
@jonthysell
Copy link
Contributor Author

Although the idea of this feature is great it would be unusable for serious users of hosts inside OpenSSH config:

  1. As mentioned above: It is unusable for tens of hosts - it would generate too long menu if submenus are not implemented.
  2. It does not scan Include files. Includes are really useful for managing large OpenSSH configuration split for multiple subjects.

I was not aware of the use of Include files. Maybe worthy of a future enhancement to this code? As it stands it already looks like Terminal needs to address the potential menu spamming issue, as well as the dynamic profile bug I found before expanding Terminal with more dynamically generated profiles.

@carlos-zamora
Copy link
Member

Alright, we chatted about this in sync today. Outside of that issue for profile management we've been chatting about above, this PR is pretty much good to go. So we're going to review/merge it as-is, but hide generating the profiles behind a feature flag.

I'll handle adding the feature flag code, so don't worry about that. :) It'll require me to push to this branch/PR though FYI,

Excited to see this land!

@carlos-zamora carlos-zamora self-assigned this Oct 24, 2022
@carlos-zamora carlos-zamora removed the Needs-Discussion Something that requires a team discussion before we can proceed label Oct 24, 2022
@vbrozik
Copy link

vbrozik commented Oct 25, 2022

I was not aware of the use of Include files. Maybe worthy of a future enhancement to this code?

In my opinion support for Include files is necessary. Without that the functionality is just partial. Also when hierarchy of profiles is implemented I think every include file should have its directory in the hierarchy by default.

I think that the use of the Include directive is pretty common among serious users of ~/.ssh/config. I personally have no Host entries in the main configuration file. All of them are in the included files. Just few examples from quick googling:

@zadjii-msft
Copy link
Member

In my opinion support for Include files is necessary. Without that the functionality is just partial

FWIW, I'm not gonna hold this PR up over that. We should file a follow up, to track it, but let's not let perfect be the enemy of the good.

carlos-zamora added a commit that referenced this pull request Oct 25, 2022
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm cool with this behind velocity while we sort out #12584

@zadjii-msft
Copy link
Member

@DHowett I'm gonna remove your block tomorrow if you don't, cause we're merging the nested menu & filtering, AND cause carlos added the velocity flag.

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 8, 2022
@ghost
Copy link

ghost commented Dec 8, 2022

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@DHowett
Copy link
Member

DHowett commented Dec 8, 2022

Merge and iterate!

@DHowett
Copy link
Member

DHowett commented Dec 8, 2022

@jonthysell thanks so much for doing this, and putting up with our quite long review cycle!

@DHowett
Copy link
Member

DHowett commented Dec 8, 2022

@zadjii-msft @carlos-zamora somebody may need to merge main! I am not able to do so at the moment

@github-actions

This comment has been minimized.

@phil-blain
Copy link

phil-blain commented Dec 9, 2022

Hi everyone, very glad to have found this PR, thanks for working on this. I've been doing this with a Python script up to now:

https://gist.github.com/phil-blain/2e5a294b79ec26a1729aedab010a9369. This uses the paramiko Python library to parse the SSH config file. A few things I do in my script that this PR does not do (at least from my reading of the changes):

  • It works for both Host and Match host clauses in the SSH config file (with a patch to paramiko)
  • It filters out hosts that contain a wildcard character * (since these are not real host names, just "generic" names used in the config file to group settings related to similarly named hosts together)
  • It uses powershell -NoExit -Command \"ssh {host}\" as the command line, to avoid the tab being unusable if the connection closes (we are dropped back in Powershell and so can SSH back in manually)
  • It uses the OpenSSH logo as icon :D (not sure if that's what "ms-appx:///ProfileIcons/{550ce7b8-d500-50ad-8a1a-c400c3262db3}.png"; is )

Just a few suggestions :)

EDIT a question I just had: Why not also simply look for ssh.exe in $PATH ? maybe someone download the zip from Win32-OpenSSH and put it in their PATH ?

@zadjii-msft zadjii-msft added AutoMerge Marked for automatic merge by the bot when requirements are met and removed AutoMerge Marked for automatic merge by the bot when requirements are met labels Dec 9, 2022
@zadjii-msft zadjii-msft merged commit a5f9c85 into microsoft:main Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Extensibility A feature that would ideally be fulfilled by us having an extension model. Area-Settings Issues related to settings and customizability, for console or terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. zBugBash-Consider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto generate profile from .ssh/config
8 participants