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

Add binding for agent forwarding #24

Merged
merged 1 commit into from
May 8, 2024
Merged

Conversation

Riatre
Copy link
Contributor

@Riatre Riatre commented Apr 27, 2024

channel_open_request_auth_agent_callback is required for implementing ssh agent forward as unlike X11 forward, there is no other way to establish a forwarding channel.

In libssh:

  1. Callback is triggered while handling protocol packets in other libssh call.
  2. The callback creates a new channel and prepare for bidirectional forwarding between it and ssh agent.
  3. The callback then returns a borrow of the newly created channel for libssh to make reply to the remote side.

However, the callback-based flow does not really fit our Rust binding design: during callback we have SessionHolder locked, so it's really hard to do anything without introducing lock re-entrancy issues, plus that it demands us to return a temporary borrow of something owned by Rust side whose lifetime is tricky to model.

Instead, we try to turn the callback-based style back to something resembling ssh_channel_accept_x11 by buffering pending channels and let users fetch them later in a saner context.

@Riatre
Copy link
Contributor Author

Riatre commented Apr 27, 2024

I have a draft for using this to implement ssh agent forward in wezterm: wez/wezterm#5345

Copy link
Owner

@wez wez left a comment

Choose a reason for hiding this comment

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

Thanks for this! I'd love to be able to use agent forwarding with wezterm!
I think there are some smallish tweaks that are needed to make this safe

libssh-rs/src/lib.rs Outdated Show resolved Hide resolved
libssh-rs/src/lib.rs Outdated Show resolved Hide resolved
libssh-rs/src/channel.rs Outdated Show resolved Hide resolved
libssh-rs/src/error.rs Outdated Show resolved Hide resolved
libssh-rs/src/lib.rs Outdated Show resolved Hide resolved
@Riatre
Copy link
Contributor Author

Riatre commented May 6, 2024

Thanks for the review! I've addressed most comments, but have further comments on the safety discussion.

In addition, I wonder why do we need std::panic::catch_unwind in bridge_*_callback-s? I just followed suit here.

Edit: nomicon mentioned:

If you are writing Rust code that may panic, and you don't wish to abort the process if it panics, you must use catch_unwind

But we don't have UB here even if we don't catch it: panic will be turned into abort in extern "C" functions. My question holds: do we need to eat the panic and return error code instead of bring down the entire process?

Edit 2: nvm, c_unwind is not in stable yet.

@Riatre Riatre force-pushed the auth-agent-take2 branch 4 times, most recently from a6df87b to 9b92bfc Compare May 7, 2024 16:09
channel_open_request_auth_agent_callback is required for implementing
ssh agent forward as unlike X11 forward, there is no other way to
establish a forwarding channel.

In libssh:

1. Callback is triggered while handling protocol packets in other libssh
   call.
2. The callback creates a new channel and prepare for bidirectional
   forwarding between it and ssh agent.
3. The callback then returns a borrow of the newly created channel for
   libssh to make reply to the remote side.

However, the callback-based flow does not really fit our Rust binding
design: during callback we have SessionHolder locked, so it's really
hard to do anything without introducing lock re-entrancy issues, plus
that it demands us to return a temporary borrow of something owned by
Rust side whose lifetime is tricky to model.

Instead, we try to turn the callback-based style back to something
resembling `ssh_channel_accept_x11` by buffering pending channels and
let users fetch them later in a saner context.
@Riatre Riatre changed the title Add binding for channel_open_request_auth_agent_callback Add binding for agent forwarding May 8, 2024
@wez wez merged commit f334115 into wez:main May 8, 2024
6 checks passed
@wez
Copy link
Owner

wez commented May 8, 2024

Thank you!

@wez
Copy link
Owner

wez commented May 8, 2024

Published to crates.io as 0.3.1

@Riatre Riatre deleted the auth-agent-take2 branch May 8, 2024 16:59
@tomtastic
Copy link

I'd also love to see agent forwarding in WezTerm, but small side-note that the Windows OpenSSH agent (and specifically ssh-add) is broken currently for users of SSH certificates.

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