-
-
Notifications
You must be signed in to change notification settings - Fork 142
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
Pageant: Implement NamedPipes-based stream #472
base: main
Are you sure you want to change the base?
Conversation
b11e29e
to
106337b
Compare
106337b
to
9953b62
Compare
@@ -11,8 +11,14 @@ | |||
clippy::panic | |||
)] | |||
|
|||
#[cfg(windows)] | |||
#[cfg(all(windows, feature = "wmmessage", not(feature = "namedpipes")))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot!
The two implementations should definitely not be mutually exclusive. They could be both exported together, with neither being a "default", e.g.:
#[cfg(all(windows, feature = "wmmessage"))]
pub use pageant_impl::PageantStream as PageantWmMessageStream;
#[cfg(all(windows, feature = "namedpipes"))]
pub use pageant_impl_namedpipes::PageantStream as PageantNamedPipeStream;
I'd really like to see a common trait for both of them since it would be weird to have to choose a specific implementation at compile time unless you know exactly which PuTTY version to expect in advance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a cost to maintaining both and why would one want to run an old pre-2020 Putty Pageant with potential security bugs in 2025? I think a compile time decision is good enough, and I would even evaluate to eventually remove the WM_COPYDATA method to simplify maintenance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because in reality, when you're writing an SSH client, you don't control what version of Putty the user might have installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P.S. I don't mind doing it myself, it will just take a bit until I have time to work on it.
Adds an implementation of the newer NamedPipes-based access to the Pageant Stream.
Right now, it can be switched to via feature, i.e. the binary crate imports
to enable it.
While the original client has some backwards-compatibility for very old Windows versions (the comments in some of the called code speak of
Win9x
andXP SP2
), I've just implemented the primary path, which should work on all platforms the current MSRV supports (7+).This is a breaking change both on the API-Surface of the Pageant crate (new needs to be async + have error handling), as well as in russh (also to introduce a
Result
return type).Alternatively to this implementation, we could also just offer the generation of the Named-Pipe path in this crate and let users create a NamedPipe-Backed agent using the existing code in Russh, although this version seemed more comfortable to me.