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

russh-config: Opinion: deprecate parse_home() and add crate documentation that it parses an OpenSSH-like format #284

Open
zegelin opened this issue May 14, 2024 · 1 comment

Comments

@zegelin
Copy link

zegelin commented May 14, 2024

russh-config parses OpenSSH-style config files but with very limited functionality compared to OpenSSH. This means that trying to parse a even mildly complex ~/.ssh/config file can lead to unexpected behavior and the wrong options being applied, which to some might even be a security concern.

Excluding any OpenSSH exclusive options (i.e., an equivalent feature doesn't exist in russh), many options are missing or don't handle all the edge-cases. A select few examples:

  • Host can accept the wildcard (Host *). All option definitions in that block are used as defaults.
  • Host can accept more than one host name, and host names can be patterns: e.g. Host rack-a-* rack-b-*
  • Various options can have token placeholders (e.g., %h) in their values. e.g., Hostname, IdentityFile. Only ProxyCommand has partial support for this in russh-config.
  • Various options support environment variable expansion.
  • IdentityFile exists however UserKnownHostsFile doesn't.

I started adding SSH support to my app and decided to go with russh rather than spawning ssh and wrangling sub-processes. I hoped to handle various types of SSH setups, such as proxies, and saw the russh documentation mention russh-config. The parse_home() function, which reads ~/.ssh/config (an OpenSSH config file), lead me to assume that russh has support for OpenSSH config files. However, after testing against my own config I realised that most options aren't implemented.

IMO any app that uses parse_home() is subtly broken. Users of these apps may assume their existing OpenSSH config will work, yet the config will likely be parsed incorrectly, and connections may happen with options that differ from what's set in the config.

Unless russh wants to implement support for the full OpenSSH config format with all the nuances (which would also require russh to implement all the OpenSSH client features and nuances!), it is my opinion that the parse_home() function be marked deprecated, and the crate documentation be updated to mention that it parses an "OpenSSH-like" config format.

Another way to put it: russh isn't OpenSSH. Don't make it easy for developers, and by extension their end-uses, to pretend/assume it is.

@packetsource
Copy link
Contributor

Some enhancements to the Host directive for supporting multiple entries and and glob matching patterns now in #306, but your other concerns certainly on compatibility still stand

EpicEric pushed a commit to EpicEric/russh that referenced this issue Nov 19, 2024
* Pin golang to major.minor.patch

* Update package deps
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

No branches or pull requests

2 participants