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

Fix ssh config resolution #69

Merged
merged 1 commit into from
Oct 27, 2021
Merged

Fix ssh config resolution #69

merged 1 commit into from
Oct 27, 2021

Conversation

clemyan
Copy link

@clemyan clemyan commented Oct 9, 2021

Fixes ssh config resolution.

When resolving ssh config, ssh-config's .find method is currently used. However from ssh-config's documentation:

NOTICE: [.find] is provided to find the corresponding section in the parsed config for config manipulation. It is NOT intended to compute config of certain Host. For latter case, use .compute(host) instead.

This PR changes ssh config resolution to use .compute instead as suggested.


In particular, this allow this extension to handle:

# Blocks with multiple hosts
Host test-a test-b
    HostName 127.0.0.1

# Multiple blocks
Host test-c
    HostName 127.0.0.2
Host test-c
    Port 23

# Wildcards
Host test-*
    HostName 127.0.0.3

Unfortunately, ssh2 does not support attempting multiple private keys OOTB, so I matched the current behavior of only using the first key.

@Natizyskunk Natizyskunk changed the base branch from master to fix-ssh-config October 27, 2021 01:49
@Natizyskunk Natizyskunk merged commit e0d2bcb into Natizyskunk:fix-ssh-config Oct 27, 2021
@Natizyskunk Natizyskunk mentioned this pull request Nov 12, 2021
1 task
@Natizyskunk
Copy link
Owner

@clemyan,

a bug has seems to be introduced since your pull request.
you fill find more info on this issue here.

Can you take a look at it please ?

@Natizyskunk Natizyskunk mentioned this pull request Nov 12, 2021
Natizyskunk added a commit that referenced this pull request Nov 20, 2021
@Natizyskunk
Copy link
Owner

@clemyan,

I had to revert your changes since it introduce the bug for to much people. We can keep in touch to find another solution.

@clemyan
Copy link
Author

clemyan commented Nov 26, 2021

@Natizyskunk

I had to revert your changes since it introduce the bug for to much people.

Understandable. Though I feel like this is a quirk of ssh2's implementation that vscode-sftp should workaround or find a different library.

We can keep in touch to find another solution.

Would you like me to create a new issue to track this?

@Natizyskunk Natizyskunk self-assigned this Sep 27, 2022
Copy link
Owner

@Natizyskunk Natizyskunk left a comment

Choose a reason for hiding this comment

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

Perfect

src/core/fileService.ts Show resolved Hide resolved
src/core/fileService.ts Show resolved Hide resolved
@Natizyskunk
Copy link
Owner

@Natizyskunk

I had to revert your changes since it introduce the bug for to much people.

Understandable. Though I feel like this is a quirk of ssh2's implementation that vscode-sftp should workaround or find a different library.

We can keep in touch to find another solution.

Would you like me to create a new issue to track this?

Hello @clemyan, Would be awesome if you can still take a look at it. Is it possible for you ? 🙂

@clemyan
Copy link
Author

clemyan commented Aug 1, 2023

@Natizyskunk

Sorry for the late reply.

I have spent some time looking into this again, but I don't think any of the things blocking this has been addressed.

Problem statement

What this PR aimed to solve is this:

The way vscode-sftp parses SSH config files is not compliant with the OpenSSH client configuration file format

The said format is specified in the ssh_config(5) man page. In particular, these config sections are not resolved correctly

# Blocks with multiple hosts
Host test-a test-b
    HostName 127.0.0.1

# Multiple blocks
Host test-c
    HostName 127.0.0.2
Host test-c
    Port 23

# Wildcards
Host test-*
    HostName 127.0.0.3

# Negated
Host test-* !test-d
    User clemyan

This in turn is because vscode-sftp uses sshConfig.find (from the ssh-config package), which its documentation specifically warns against using it for this purpose

NOTICE: This method is provided to find the corresponding section in the parsed config for config manipulation. It is NOT intended to compute config of certain Host. For latter case, use .compute(host) instead.

Attempted Solution (this PR)

The natural solution would be using .compute and that should solve everything, right? Turns out there is a limitation in the ssh2 library (which is what vscode-sftp is using as the SSH client).

A common SSH setup is like this:

  • Server allows both publickey and password authentication
  • Client uses a single publickey with passphrase

In this setup, with most SSH clients (notably, the OpenSSH client), this happens:

  1. Client connects to the SSH server
  2. Client queries server for available authentication methods
  3. Client selects publickey authentication
  4. Client reads the specified private key file and offers to use the public key (if key file is missing or invalid, skip to 7)
  5. If server accepts the key, client prompts the user for the passphrase (if server rejects the key, skip to 7)
  6. If the key can be decrypted with the provided passpharse, client actually performs the publickey authentication.
  7. If publickey authentication fails for whatever reason, client attempts password authentication and prompts the user for the password

Thus, people that have a SSH config with a wrong/invalid private key may never have any trouble using common SSH clients because they simply fallback to password authentication. The user is never even prompted for the passphrase.

However, ssh2, when supplied with a passphrased private key, just throws, without even attempting to connect to the server. For those people, before merging this PR, the fact that SSH config resolution is non-compliant may have isolated them from ssh2's behavior because vscode-sftp did not find a private key. Password authentication would be attempted if a password is set in sftp.json.

After this PR is merged, vscode-sftp finds a passphrased private key and errors, even if a password is provided in sftp.json, because ssh2 throws before even connecting to the server, let alone attempt password authentication.

Solution

So, what can be done? I see a few possibilities

  • Workaround ssh2's behavior (e.g. implement fallbacks)
  • Ditch ssh2 and choose another SSH client library
  • Merge this PR again, marking it as a breaking change and release a major version

I started this PR because it would have improved my workflow and it is a small change. My team has since changed our workflow so we aren't using SFTP anymore. So, honestly I don't care anymore and I won't have the time or bandwidth to write and test large refactors. The changes I made here should still work so if you go the breaking change route I don't think you need help from me either.

I am open to discussing and exploring other solutions if you have ideas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants