feat: rust-native ssh support

[plaflamme]

This is a very basic implementation of a rust-native SSH transport (#1246). I've got as far as a successful handshake method call.

The approach used is:

• depend on russh and declare a new feature to enable it
• implement the async version of the Transport trait
• bridge futures_io AsyncRead/Write traits to Tokio's AsyncRead/Write (this was pretty involved, maybe there's a simpler way)
• trivially supports SSH Agent-based identities by iterating over all of them and trying each one

The PR lacks the following:

• tests: we could spin up a local SSH daemon or tests, but this will be pretty involved, so perhaps it can be deferred to another PR
• more auth methods: we'd want to be able to specify an ssh private key from a file or from memory
• configuration: there's no way to configure any of the many configuration options one would one to set on their SSH connection, we could wire up an (mostly empty) Option struct for now and let people add options as they need them
• error / channel updates handling: error handling is pretty trivial and I'm not quite sure if were supposed to handle certain messages in the Handler implementation, things like detecting disconnects or channel failures of some kind

[Byron]

This is just WOW! I am truly amazed by this being tackled as a contribution, and hope to have some time to take a look at the code soon.Also thanks so much for the detailed PR notes, I found them very helpful to get a first understanding on what's planned and how.

First of all, I agree that it might be best to get this to work as proof-of-concept, but also think that this means it has to be wired up with gix so it can be tested on the command-line by hand.

Regarding futures_io and tokio integration, it seems odd that this isn't already available via a crate or a feature toggle. I'd think plenty of people have a similar problem.

[plaflamme]

This is just WOW! I am truly amazed by this being tackled as a contribution, and hope to have some time to take a look at the code soon.Also thanks so much for the detailed PR notes, I found them very helpful to get a first understanding on what's planned and how.

Super happy to help. gix is a huge effort, you'll need all the help you can get!

Regarding futures_io and tokio integration, it seems odd that this isn't already available via a crate or a feature toggle. I'd think plenty of people have a similar problem.

It's available in tokio-utils, and I mostly copy-pasted the code from there, but there were complications with the ownership model:

• GitConnection requires ownership of one AsyncRead and one AsyncWrite (separately)
• This requires that we pass something like Compat(tokio_async_read) and Compat(tokio_async_write)
• This requires that russh provides an "owned" AsyncRead / AsyncWrite split, which it does here
• but, the make_reader call captures the Channel's lifetime which means we cannot call make_writer without cloning, but russh's types are not clone
• so we'd have to wrap the read/write halves in Arc<Mutex> anyway and then re-implement tokio::io::AsyncRead/Write for them
• so instead, I used the ChannelStream which is both AsyncRead and AsyncWrite and then implemented futures_io::AsyncRead/Write directly for that

I think the solution would be to have GitConnection take a single parameter that is both AsyncRead and AsyncWrite, but I'm not 100% sure that makes sense.

First of all, I agree that it might be best to get this to work as proof-of-concept, but also think that this means it has to be wired up with gix so it can be tested on the command-line by hand.

Nice, yeah I'd be happy to do that... if you can provide some guidance and I can try to tackle that.

[Byron]

Thanks for elaborating.

I think the solution would be to have GitConnection take a single parameter that is both AsyncRead and AsyncWrite, but I'm not 100% sure that makes sense.

It's generally fine to adjust the API to be more suitable. The reason it is what it is is merely that the requirements allows for it, and it was easiest to do it that way. Having just one argument there would probably shift the complexity elsewhere in the code, but it might still be preferable if mostly sync code is affected (i.e. less complexity overall).

Besides that, I don't know if anything can be done about the cargo-deny error, pulling in a 'broken' dependency without mitigation doesn't seem great. Maybe there is a Cargo feature to toggle it off?

Nice, yeah I'd be happy to do that... if you can provide some guidance and I can try to tackle that.

it looks like in order to use this transport, the caller would have to pass in a custom one to the gix::Remote, which is cumbersome and very special.
Instead, the is a system to create a connection automatically by selecting it via the scheme, so a custom scheme could be made-up that can select the new transport. That way, it should just work as PoC, without any configuration support.

[plaflamme]

It's generally fine to adjust the API to be more suitable.

Indeed, though in this case it seems a bit challenging to remove this required split. In fact, it looks like it might come from the blocking implementation due to the stdin vs stdout split. Unclear that it would lead to simpler implementations.

Besides that, I don't know if anything can be done about the cargo-deny error, pulling in a 'broken' dependency without mitigation doesn't seem great. Maybe there is a Cargo feature to toggle it off?

AFAICT, there's no feature for turning-on/off RSA support. Perhaps one could be contributed. It would remove the ability to use RSA-based SSH keys leaving only ECDSA-based keys, which seems fine to me. That said, there has been recent updates on the underlying issue here; maybe this won't be a problem for very long.

Does this cargo-deny error prevent this PR from being merged? If so, I might try to contribute that feature to russh.

it looks like in order to use this transport, the caller would have to pass in a custom one to the gix::Remote, which is cumbersome and very special.

The issue I see with integrating it with gix by default is configuration: there are so many configuration options for ssh that it seems almost unlikely that it would be usable out of the box without supporting some of them.

It looks like russh has another crate that supports parsing ssh_config files and respect some of the most common options. The upside of that is it would be quick to add, the downside is supporting new options would require going through a dependency upgrade. I think it's worth adding this crate, but perhaps you feel otherwise?

Also, for supporting this in gix, how about instead of a different protocol, we use a different Variant in gix's config? Something like ssh = native which would parse into e.g.:

enum SshVariant {
  Program(ProgramKind),
  Native,
}

The upside is that it wouldn't require "inventing" a protocol making this a bit more idiomatic. The downside is that it would no longer be per-remote (AFAICT), but per-repo. WDYT?

[Byron]

Indeed, though in this case it seems a bit challenging to remove this required split. In fact, it looks like it might come from the blocking implementation due to the stdin vs stdout split. Unclear that it would lead to simpler implementations.

Yes, that's correct, and I remember it as helpful when implementing the 'first read, then write' semantics.

AFAICT, there's no feature for turning-on/off RSA support. Perhaps one could be contributed. It would remove the ability to use RSA-based SSH keys leaving only ECDSA-based keys, which seems fine to me. That said, there has been recent updates on the underlying issue here; maybe this won't be a problem for very long.

That's great to see, it really does look like it will be fixed soon enough to not be in the way of a merge here.

Does this cargo-deny error prevent this PR from being merged? If so, I might try to contribute that feature to russh.

Maybe cargo deny can be tuned here as well. I am a bit surprised it picks up something behind a feature gate, with nobody downstream enabling it yet.
My hope would be that it would just ignore it while the feature is disabled, but in all fairness, that's probably not how it works and for good reason.

I could also imagine to ignore the error per deny.toml, and avoid turning the feature on in official builds. That would still allow testing once it's wired up in gix (CLI). Warnings could be added to the respective feature toggles as well. There will probably be a solution to this that doesn't involve you adding a feature toggle upstream.

The issue I see with integrating it with gix by default is configuration: there are so many configuration options for ssh that it seems almost unlikely that it would be usable out of the box without supporting some of them.

Probably out of ignorance I thought that having possibly hacky PoC behaviour would suffice, all tuned to allow manual testing on the command-line. This could be trying all keys available to the agent, or setting an identity through an environment variable.
For me feeling comfortable enough to merge would mean I am able to run it myself in some capacity.
From there, journey tests also don't seem to be too far off, even if run only on CI as to not mess with user keys and/or break isolation.

It looks like russh has another crate that supports parsing ssh_config files and respect some of the most common options. The upside of that is it would be quick to add, the downside is supporting new options would require going through a dependency upgrade. I think it's worth adding this crate, but perhaps you feel otherwise?

Ultimately, if the implementation should be more than niche with the potential to replace calling out to the ssh program, supporting ssh configuration is a must. Also, this feature would be very interesting to Cargo, which tries hard to not be relying on the presence of any other binary.

The upside is that it wouldn't require "inventing" a protocol making this a bit more idiomatic. The downside is that it would no longer be per-remote (AFAICT), but per-repo. WDYT?

That's a great idea and I love being (then) able to set this using the git configuration. Doing so would probably only be temporary anyway as I'd imagine this transport to then be an optional replacement for the current ssh (program) based transport, similar to what's happening when building with different HTTPS implementations.