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

Feat: Add support for SSH and private key without requiring SSH-agent #1072

Closed
wants to merge 13 commits into from

Conversation

chunky
Copy link

@chunky chunky commented Jun 29, 2024

Summary
Patch adds an "identityFile" parameter to the SSH profile, one of the suggested alternate fixes to Issue #1059 .

Using SSH_AUTH_SOCK in our environment is too high friction to be useful, so this provides a fallback option.

Testing
I have not yet added formal tests to this. If this request is likely to be accepted, I would appreciate guidance on what an appropriate set of tests would look like.

Signed-off-by: Gary Briggs [email protected]

@scnwwu scnwwu requested a review from smorrisj July 1, 2024 01:59
@chunky
Copy link
Author

chunky commented Jul 3, 2024

@smorrisj What is the correct way to put in i18n placeholders? Should I just edit each translation file and put the English verbiage in as the translation, and let the translators deal with it?

@smorrisj
Copy link
Contributor

smorrisj commented Jul 3, 2024

@smorrisj What is the correct way to put in i18n placeholders? Should I just edit each translation file and put the English verbiage in as the translation, and let the translators deal with it?

The typical process is to just fill in the english bundle with strings. We have automation that we run near the end of a release cycle that will propagate the deltas as placeholders out to the other language bundle files. We have a group of both internal SAS employees and community members that help with translation of the non english files. We typically bundle these up in a PR for merge as we prepare to release.

@@ -88,6 +88,7 @@ export interface SSHProfile extends BaseProfile {
saspath: string;
port: number;
username: string;
identityFile: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be made as an optional field? I think having it as optional is more on brand for the overall approaches we recommend for this in the markdown changes. It should also clean up the tests a bit so that the "original" tests dont have to be burdened with keeping up with default values. In my mind we would write a separate set of tests that isolates how this new member gets passed around etc.

Copy link
Author

Choose a reason for hiding this comment

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

With apologies, this is the first time I've ever used either typescript, or VS Code. In my head this is already "optional", in that when the profile is created, you can leave it blank.

I intuit you mean "optional" like "the TS type system affords making it explicit that this field is optional", but it's not obvious to me how to implement that in a way that aligns with the rest of the codebase. I would appreciate pointers to the right path.

@smorrisj
Copy link
Contributor

smorrisj commented Jul 3, 2024

Some context around the introduction to use SSH Agent. We actually used Identify file in the beginning:
#182

@chunky
Copy link
Author

chunky commented Jul 8, 2024

I read that, and I do agree that in some grander sense it makes more sense to use agents to handle keys. Unfortunately it just isn't a friendly enough option in our environment.

I think this configuration approach is a reasonable fallback that still uses SSH keys, which users prefer post-initial-setup, to having to enter their password each time.

I have also put in a pull request, #1081, for a mechanism by which users are asked for their password each time a connection is made. Personally I would like to see both of these PR's accepted, to afford compatibility with a wider array of servers, and to give users more choices where they want them.

@snlwih
Copy link
Collaborator

snlwih commented Aug 13, 2024

@chunky , could you explain to me (using Sesamestreet language 😃 ) what the exact use case for this is?

Now that we're planning for user/pass authentication for SSH connections by storing them locally using the VS Code Secrets API (#1126 ), is there still a need for this? Reason for asking is that you mention this PR being a workaround for #1059, which is also a workaround for being able to use user/pass authentication.

@chunky
Copy link
Author

chunky commented Aug 13, 2024

@snlwih Sure. There are a few issues with password as a notional magic bullet:

  • Some places don't allow password at all [as I understand, that was even in SAS internal Security Group's recommendations].
  • Some places require 2-factor, that I believe usually implemented as a keyboard-interactive setup, so could appear to be password but isn't.
  • Passwords rotate, possibly even as single-use items.

At the other end of the spectrum is ssh-agent that you support right now. It's great when it works, but the setup is potentially complex and, in the case of our systems, impossible to complete as described on Windows because of some long-standing security policy. Even on systems where it's possible, it requires administrative privileges on the laptop, which most orgs no longer give people. I'm imagining that the typical SAS user doing analytics isn't in the category of people specially granted admin rights, at orgs where that's on the table.

Loading private keys from a file gives a middle-ground. There's the additional convenience of private keys and avoidance of password-related problems, but without any complexity or logistical-impossibility for the user to do initial setup. On my windows laptop, this would be my own personal preference. I also see it as the one easiest to handhold my users through, if they need to go one notch beyond password.

@snlwih
Copy link
Collaborator

snlwih commented Aug 20, 2024

@chunky, thanks for your patience and education 😃

  • when understand things correctly this PR is about using private key file without needing SSH-agent. Right? If so, I will rename the title of this PR.
  • as part of PR feat: username and password support for SSH connection type #1126 there will be support for using a private key file without needing SSH-agent, so are you okay with closing this PR?

@chunky
Copy link
Author

chunky commented Aug 20, 2024

Yes, please, feel free to close. Same as the other pr, I just want stuff that works!

And yes, this was for using pk without agent.

@snlwih snlwih changed the title Feat: Add support for SSH IdentityFile Feat: Add support for SSH and private key without requiring SSH-agent Aug 22, 2024
@snlwih
Copy link
Collaborator

snlwih commented Aug 22, 2024

Closing as this will be implemented as part of PR #1126

@snlwih snlwih closed this Aug 22, 2024
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.

3 participants