feat: username and password support for SSH connection type #1126

smorrisj · 2024-08-01T13:26:42Z

Summary

Adds support for username and password to the 9.4 Remote (SSH) connection type.
Refactor SSH auth process to use a handler that can support use of agent, manually specifying a private keyfile (with or without a passphrase) in the connection profile, using a traditional username password, or using keyboard-interactive mode, depending on how the ssh server is configured.
Run initialization code to set working dir to the WORK lib path to autocleanup ODS results on session close
Add keepalive configuration to ssh client to prevent idle disconnects for long-running programs

Todos:

Testing

Connection Profile Prompts:

Create new connection profile should offer private key path as an optional prompt and create the profile with the value that the user specifies

Auth Refactor:
Users can successfully authenticate to the server to run sas programs using the following auth methods:

SSH Agent (See current doc)
Private key specified in privateKeyFilePath field in connection profile (no passphrase)
Public key specified in privateKeyFilePath field in connection profile (with a passphrase)
Using a traditional username/password
Using Keyboard Interactive (RHEL disables this by default in modern versions, will need to enable it in sshd config on the server and set the AuthenticationMethods field such that this method is used instead of password)

Work Directory Detection

ODS SAS result html files are no longer written out to the users home directory on the SSH Server
Verify that the SAS Log Output contains an output entry for the working dir getting set to the WORK dir generated.
User can change the work directory by passing -WORK [path] option as a sasOption on the connection profile options array

Keepalive Changes:

Verify that long running program (using sleep function) does not result in the ssh server disconnecting the client

smorrisj · 2024-08-01T16:56:21Z

Pushed a super rough draft of auth handler logic this morning. Very rough around the edges but wanted to get thoughts down in some (semi) organized manner. Will further test and refine as time allows.

chunky · 2024-08-02T04:41:54Z

First up, thank you for working on this. It'll be a big improvement for us.

A few random thoughts:

You can find a conversation I had with a maintainer of ssh2 about authhandlers here: Request: Delay client password entry until password actually needed mscdex/ssh2#1400 . Things that might be relevant from your comments and code so far:
1. The authhandler is only called with a list of methods that would work for that server
2. "agent" and "private key" are different, in the authhandler callback.
3. Note this from the ssh2 doc you link: "Valid method names are: ''none', 'password', 'publickey', 'agent', 'keyboard-interactive', 'hostbased'"
I feel pretty strongly that attempting all non-interactive options on the table first, and only then trying the interactive ones is the right choice. If agent would be accepted, and I have a running agent, then obviously that should be tried before asking users for a password, for example.
Don't forget keyboard-interactive, as separate from password. Some server configurations ask for "keyboard-interactive" authentication and ask you for a password in a message, rather than asking for "password" authentication. [set sshd config "ChallengeResponseAuthentication yes" to see this]
Affording use of an unshielded private key as a separate config item, and proffering it if "private key" method is available, would really help
I have no strong feelings about password storage in secrets. The main concern is, users have to rotate passwords regularly; what happens if the password in storage is no longer the same one that works for the server? Honestly for the first pass, asking for it each time doesn't seem unreasonable.
1. users can set up public key auth if they don't like it

From your top message: "Users should still be able to specify an identify file for keys that have a passphrase." ; do you mean "for keys that don't have a passphrase"? For keys that do, you would need to develop the ability to de-shield them after loading them from disk, to pass to the private key method. Probably safer to just expect users to set up ssh-agent, if that's where they're at. [although it's true that that is unfortunate on windows]

smorrisj · 2024-08-02T11:18:24Z

client/src/connection/ssh/index.ts


  constructor(c?: Config) {
    super();
    this._config = c;
-    this.conn = new Client();
+    this._conn = new Client();
+    this._authMethods = ["publickey", "password", "keyboard-interactive"];


Attempt non-interactive methods first, falling back to interactive ones.
@chunky in a few test scenarios in house, I'm seeing the auth method for agent and public key coming back from the server simply as "publickey". I'm wondering if the library is adding in agent based on set config options?

Essentially, these strings are well known auth methods that are supported by ssh itself. I think it would be great to get these from the server by initially sending "none" up to prompt for a response. We need to then roll through the list of methods that we can support in the extension (which should be most of them except for the kerberos methods) and then make sure that before we try it, that it's in the "server supported list".

smorrisj · 2024-08-02T11:21:50Z

client/src/connection/ssh/index.ts

+              parsedKeyResult instanceof Error &&
+              parsedKeyResult.message ===
+                "Encrypted OpenSSH private key detected, but no passphrase given"
+            ) {


The ssh2 library has a function that can assist with detecting that the key file specified on the config property is encrypted, which is a nice help to allow users to specify shielded and unshielded key paths on the connection profile (if they dont want to / cant use use agent). Right now with the code below if we detect a passphrase we'd prompt for it before sending it on. Generally I'm against storing these kinds of passphrases in a secret for reasons mentioned above. If a user wants passphrase persistence then perhaps agent is the ideal solution.

smorrisj · 2024-08-02T11:24:17Z

client/src/connection/ssh/index.ts

+                });
+              }
+            }
+          } else if (process.env.SSH_AUTH_SOCK) {


Pretty straightforward, this is the agent method that we supported in the before state. Users can still elect not to specify a key file on the connection profile and have the agent do all of the work (if they have set it up).

smorrisj · 2024-08-02T11:30:27Z

client/src/connection/ssh/index.ts

+            });
+          }
+          break;
+        }


Support both password and keyboard-interactive. Prompt user "just in time".

smorrisj · 2024-08-02T11:39:29Z

First up, thank you for working on this. It'll be a big improvement for us.

A few random thoughts:

You can find a conversation I had with a maintainer of ssh2 about authhandlers here: Request: Delay client password entry until password actually needed mscdex/ssh2#1400 . Things that might be relevant from your comments and code so far:

The authhandler is only called with a list of methods that would work for that server

"agent" and "private key" are different, in the authhandler callback.

Note this from the ssh2 doc you link: "Valid method names are: ''none', 'password', 'publickey', 'agent', 'keyboard-interactive', 'hostbased'"

I feel pretty strongly that attempting all non-interactive options on the table first, and only then trying the interactive ones is the right choice. If agent would be accepted, and I have a running agent, then obviously that should be tried before asking users for a password, for example.

Don't forget keyboard-interactive, as separate from password. Some server configurations ask for "keyboard-interactive" authentication and ask you for a password in a message, rather than asking for "password" authentication. [set sshd config "ChallengeResponseAuthentication yes" to see this]

Affording use of an unshielded private key as a separate config item, and proffering it if "private key" method is available, would really help

I have no strong feelings about password storage in secrets. The main concern is, users have to rotate passwords regularly; what happens if the password in storage is no longer the same one that works for the server? Honestly for the first pass, asking for it each time doesn't seem unreasonable.

users can set up public key auth if they don't like it

From your top message: "Users should still be able to specify an identify file for keys that have a passphrase." ; do you mean "for keys that don't have a passphrase"? For keys that do, you would need to develop the ability to de-shield them after loading them from disk, to pass to the private key method. Probably safer to just expect users to set up ssh-agent, if that's where they're at. [although it's true that that is unfortunate on windows]

All good feedback. I generally agree with thoughts above. I've put comments around changes that I think support these items. On the issue of whether or not to store the passwords in secrets, we have some precedent for doing this in other connection types. I do agree though that generally for ssh interaction with other solutions, anytime passwordless or passphraseless interactions are intended, that I've seen docs push users towards agent or public key.

jbreitman · 2024-08-02T13:02:00Z

I would like to see the authentication method gssapi-with-mic added. This will be beneficial to those in corporate environments where Kerberos Tickets enable you to access other resources such as file systems, databases, APIs, etc ... while eliminating the need for the person to enter their credentials.

chunky · 2024-08-02T13:47:56Z

If you can figure it out, yes, the kerberos SSO stuff is great where available. That was another thing I couldn't figure out. It works on some of our servers. Another of those "if it works it's magical and absolutely preferred".

I think the distinction between "agent" and "public key" is a manifestation from the ssh2 library, not the server. In practice it might mean taking two bites at the key-based apple, which is explicitly afforded by the Auth handler mechanism

smorrisj · 2024-08-02T13:51:32Z

I think the distinction between "agent" and "public key" is a manifestation from the ssh2 library, not the server. In practice it might mean taking two bites at the key-based apple, which is explicitly afforded by the Auth handler mechanism

Yea that's what the current changeset has, inside of "publickey" we'd look for whether or not the user has set their keyfile on the connection profile, if so we'd use that file, otherwise we'd defer to agent.

I would like to see the authentication method gssapi-with-mic added. This will be beneficial to those in corporate environments where Kerberos Tickets enable you to access other resources such as file systems, databases, APIs, etc ... while eliminating the need for the person to enter their credentials.

One challenge is that the ssh2 library doesnt support GSSAPI methods yet:
mscdex/ssh2#333

chunky · 2024-08-13T14:56:02Z

@smorrisj I missed this comment from earlier: "I'm seeing the auth method for agent and public key coming back from the server simply as "publickey". I'm wondering if the library is adding in agent based on set config options?"

From the server's perspective, these are the same thing; it just wants to do a key-based handshake. But on the client, they're different beasts. With ssh2's 'agent' they go get it from a daemon running on a socket, while 'publickey' is just a thing where you hand ssh2 the ready-to-use bytes.

smorrisj · 2024-09-05T16:01:11Z

client/src/connection/ssh/const.ts

+// SPDX-License-Identifier: Apache-2.0
+
+const SECOND = 1000;
+export const KEEPALIVE_INTERVAL = 60 * SECOND; //How often (in milliseconds) to send SSH-level keepalive packets to the server. Set to 0 to disable.


@chunky any thoughts on going with these values for keepalive and max unanswered?

@smorrisj 60 seconds is a reasonable figure - I think I usually set it to 30, but I have no strong feelings as long as it's well inside 90.
The unanswered_threshold, I confess to be unsure about appropriate behaviour. If the comment is right, it's measured in "pings" not in "time". So if you're aiming for 12 hours, it should be (12 * HOUR / KEEPALIVE_INTERVAL). Either way, because this is happening at the SSH/networking level, then 12 hours is probably way too long. Maybe 15 minutes?

Thanks @chunky. Must have forgot my morning coffee when making the first pass on that value =) See 04444a7.

scnwwu · 2024-09-19T07:56:16Z

website/docs/Configurations/Profiles/sas9ssh.md

@@ -118,3 +129,26 @@ Host host.machine.name
 ```

 6. Add the public part of the keypair to the SAS server. Add the contents of the key file to the ~/.ssh/authorized_keys file.
+
+### Connection Profile


It should be 4th level (####) right?
I'm thinking the title might be better to be something like "Private Key File Path"? As the agent way also involves profile and has profile sample above.

Fixed in ea17b75

scnwwu · 2024-09-19T08:18:28Z

client/src/connection/ssh/types.ts

+  privateKeyFilePath: string;
+}
+
+export const LogLineTypes: LogLineTypeEnum[] = [


nit: where is it used?

Thanks. Relic from the initial prototype / approach. Removed in 3e1f267.

scnwwu · 2024-09-19T08:27:52Z

client/src/connection/ssh/index.ts

+
+  private resolveSystemVars = (): void => {
+    const code = `%let workDir = %sysfunc(pathname(work));
+    %put ${WORK_DIR_START_TAG};


so looks like it will suffer #1119 right?

I noticed this in testing. One "chunk" of data written to the stream actually contained multiple long lines with breaks on my test environment (I used VA). In testing, this code was able to parse out the actual unix path from the input strings I was seeing come across. Take a look and let me know what you think?

const match = foundWorkDir.match(/\/[^\s\r]+/); this._workDirectory = match ? match[0] : "";

scnwwu · 2024-09-19T08:30:29Z

client/src/connection/LineParser.ts

+// Copyright © 2024, SAS Institute Inc., Cary, NC, USA.  All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+export class LineParser {


do we want to remove the itc/LineParser.ts in favor of this?

Thanks for catching. I had originally promoted LineParser up higher. A merge must have errantly added it back. Fixed in 76a271f.

scnwwu · 2024-09-19T08:31:33Z

client/src/components/profile.ts

+    title: l10n.t("Private Key File Path (optional)"),
+    placeholder: l10n.t("Enter the private key file path"),
+    description: l10n.t(
+      "Enter the local path of the private key file. Leave empty to use SSH Agent.",


nit: do we want to say Leave empty to use SSH Agent or password.?

I was already unsure about having such a long description. I think we could shorten it a bit. See 7a2c0b4. Will that work?

scnwwu · 2024-09-19T09:01:07Z

website/docs/Configurations/Profiles/sas9ssh.md

cc @jennifert-sas for review

@scnwwu For "Leave empty to use SSH Agent or password." I would suggest:
"To use the ssh-agent or a password, leave blank."
Not entirely sure if it should be "SSH Agent" or "ssh-agent" - i see both in the GitHub doc.

ssh-agent typically refers to the running process in the context of running in the operating system, whereas we generally have been using SSH Agent for a more general reference, regardless of operating system. I updated the prompt to use "SSH Agent" in 5f9ec57, since in this case we were referring to it more as a general reference?

Sounds good - thanks!

scnwwu

Looks good to me. Thanks.

Zhirong2022 · 2024-09-24T08:50:41Z

1.Create a SSH connection profile and specify an invalid privateKeyFilePath, run some code, it will prompt error message as expected.
2.Update the profile to correct the file path, run code, it has the same error as before.
The only way to restart VSC to use the updated profile.

Zhirong2022 · 2024-09-25T09:02:50Z

1.Create a SSH connection profile, specify privateKeyFilePath field in connection profile (with a passphrase).
2.Run some code, it will ask the user to enter the passphrase for the private key
3.Cancel the dialog or type an invalid password in such scenario
Result: It keeps showing 'Connecting to SAS session' until it gets error message 'Could not connect to the SAS server.'
Suggestion: If the user cancel or type with an invalid password, it can stop the process right now without having to wait for some time.

Zhirong2022 · 2024-09-25T09:18:11Z

It cannot create the profile successfully if leave the 'Enter the local private key file path' as blank. For others fields have been filled correctly.

smorrisj · 2024-09-26T21:15:48Z

It cannot create the profile successfully if leave the 'Enter the local private key file path' as blank. For others fields have been filled correctly.

Fixed in d3c9cd2

smorrisj · 2024-09-30T13:56:17Z

1.Create a SSH connection profile and specify an invalid privateKeyFilePath, run some code, it will prompt error message as expected. 2.Update the profile to correct the file path, run code, it has the same error as before. The only way to restart VSC to use the updated profile.

Fixed in 6d28a43

smorrisj · 2024-09-30T14:13:40Z

1.Create a SSH connection profile, specify privateKeyFilePath field in connection profile (with a passphrase). 2.Run some code, it will ask the user to enter the passphrase for the private key 3.Cancel the dialog or type an invalid password in such scenario Result: It keeps showing 'Connecting to SAS session' until it gets error message 'Could not connect to the SAS server.' Suggestion: If the user cancel or type with an invalid password, it can stop the process right now without having to wait for some time.

In this setup, we're trying as much as possible to let the SSH client and server negotiate auth based on the server configuration. If you're seeing that message, it means that the SSH client received a disconnect from the server side to close the connection before auth could finish. This can happen in a number of auth setups, for example, if max auth tries are exceeded etc. I think to support the largest number of customer configured auth setups, we should keep this the way it is.

feat: auth handler skeleton

482f2f2

smorrisj linked an issue Aug 1, 2024 that may be closed by this pull request

[SSH connection] Add password authentication #294

Open

smorrisj mentioned this pull request Aug 1, 2024

Feat: Provision for password entry in SSH #1081

Closed

smorrisj added 3 commits August 1, 2024 09:45

fix: type on authsLeft

bda97f1

chore: progress on auth handler

86840c7

fix: brain working faster than fingers

7404025

smorrisj commented Aug 2, 2024

View reviewed changes

snlwih mentioned this pull request Aug 13, 2024

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

Closed

Merge branch 'main' into feat/ssh-userpass

54d8d66

smorrisj mentioned this pull request Aug 28, 2024

[SSH Connection] Better results file handling #1176

Open

smorrisj added 6 commits September 4, 2024 13:48

Merge branch 'main' into feat/ssh-userpass

36a778c

feat: progress on userpass

69b6664

refactor: auth state cleanup

cc667a4

refactor: move auth details to separate module

38a575a

feat: keepalive and max unanswered settings

f0016cb

use const for keepalive

21284e4

smorrisj commented Sep 5, 2024

View reviewed changes

smorrisj added 3 commits September 5, 2024 16:42

refactor: remove redundant timeout test

8aa0c23

chore: copyright header

91a6d0f

refactor for testability, auth tests

0607d64

smorrisj added 4 commits September 17, 2024 11:18

fix: minor cleanup

debafaf

fix: bug fixes found during mfa testing

e0e26dd

doc: ssh auth refactor

51ce78a

Merge branch 'main' into feat/ssh-userpass

9879746

smorrisj marked this pull request as ready for review September 18, 2024 18:12

smorrisj requested review from scnwwu and scottdover September 18, 2024 18:12

scnwwu reviewed Sep 19, 2024

View reviewed changes

smorrisj added 4 commits September 19, 2024 08:08

rename Connection Profile to Private Key File Path

ea17b75

refactor: remove unused types

3e1f267

refactor: use one LineParser

76a271f

refactor: shorter prompting for private key file path

7a2c0b4

scnwwu approved these changes Sep 20, 2024

View reviewed changes

smorrisj added the verification-needed label Sep 20, 2024

smorrisj assigned enzhihpp Sep 20, 2024

fix: remove console logging for debug msgs

430f80b

enzhihpp assigned Zhirong2022 and unassigned enzhihpp Sep 23, 2024

Zhirong2022 added testing Test the pull requests and removed verification-needed labels Sep 23, 2024

smorrisj added 2 commits September 23, 2024 11:30

fix: use a more reasonable unanswered threshhold

04444a7

doc: update prompt wording

5f9ec57

fix: only write private key file key if there is an input value

d3c9cd2

fix: connection close lifecycle bug

6d28a43

feat: username and password support for SSH connection type #1126

Are you sure you want to change the base?

feat: username and password support for SSH connection type #1126

Conversation

smorrisj commented Aug 1, 2024 • edited Loading

smorrisj commented Aug 1, 2024

chunky commented Aug 2, 2024

smorrisj Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

smorrisj Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smorrisj commented Aug 2, 2024

jbreitman commented Aug 2, 2024

chunky commented Aug 2, 2024

smorrisj commented Aug 2, 2024

chunky commented Aug 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scnwwu left a comment

Choose a reason for hiding this comment

Zhirong2022 commented Sep 24, 2024

Zhirong2022 commented Sep 25, 2024

Zhirong2022 commented Sep 25, 2024

smorrisj commented Sep 26, 2024

smorrisj commented Sep 30, 2024

smorrisj commented Sep 30, 2024

smorrisj commented Aug 1, 2024 •

edited

Loading

smorrisj Aug 2, 2024 •

edited

Loading

smorrisj Aug 2, 2024 •

edited

Loading