Skip to content

feat: Allow multiple AuthMethods#107

Open
Wykiki wants to merge 9 commits into
Miyoshi-Ryota:mainfrom
Wykiki:main
Open

feat: Allow multiple AuthMethods#107
Wykiki wants to merge 9 commits into
Miyoshi-Ryota:mainfrom
Wykiki:main

Conversation

@Wykiki

@Wykiki Wykiki commented Dec 18, 2025

Copy link
Copy Markdown

Currently, if the SSH server requires multiple authentication methods in order to validate authentication, it is not possible to connect using async-ssh2-tokio.

The goal of this change is to provide several authentication methods, that can be applied in order against the SSH server.

The first iteration just adds a new enum variant to allow a combination of private key and password auth.

The PR should be merged, if approved, once a mechanism allowing several different auth methods exists, like :

AuthMethod::with_password("foobar")
  .with_private_key("/path/to/priv/key");

@Wykiki

Wykiki commented Dec 19, 2025

Copy link
Copy Markdown
Author

I kinda landed a bomb with the latest commits, if you have any idea regarding the error management, I'd gladly update the way it is handled.

@Miyoshi-Ryota

Copy link
Copy Markdown
Owner

Thank you for your work, and sorry for the late reply.

After taking a quick look at the implementation, it seems to be many major breaking changes to the public interface (including error handling). Given the impact on existing users, I'm not sure what the best approach is at the moment.

However, judging from the thumbs-up reactions from other users, there does seem to be demand for this feature. I'd like to think about how I can make it even better when I have time. I've been busy lately so it may take a while.

@Wykiki

Wykiki commented Jan 5, 2026

Copy link
Copy Markdown
Author

Thanks for your feedback !

Regarding public interface, I think we may either find a way to kinda accept both a single AuthMethod and a vector of it, or create an alternative method. Having an alternative method would be the easiest, but I think it would make the API messy and increase its maintainability cost.

Regarding the error handling, we may be less verbose, therefor not breaking the API at all, but the current error handling is hiding some important information, like the server exposing other authentication methods.
While developing the multiple auth mechanism, I encountered hard auth failure exposed by this crate, while the underlying SFTP implementation were returning information regarding alternative authentication methods.
In this PR case, if we simplify the error handling to the existing behaviour, it would be hard to guess which AuthMethod is in error in case there is an error.

The first iteration (commit) of this PR simply added another AuthMethod variant, do you think this approach would be better ? It greatly simplifies the PR, but it covers a very narrow case, lacks versatility.

I'm totally open to implement another version of this feature, if you have any ideas, feel free to share, I might have time to code it !

@Wykiki

Wykiki commented Jan 8, 2026

Copy link
Copy Markdown
Author

With a trait, I was able to keep the existing signature for methods having AuthMethod in arguments.

Regarding errors, I don't have another implementation yet.

@Wykiki

Wykiki commented Feb 9, 2026

Copy link
Copy Markdown
Author

Do you have any idea on how we can handle the errors, regarding the new implementation ?
I might implement it if you give me enough details.

I still cannot figure out how to avoid breaking the error enum variants without loosing information about why the auth failed.

Thanks !

@Wykiki

Wykiki commented Mar 31, 2026

Copy link
Copy Markdown
Author

Hello !

Do you plan to accept this PR, or give a thought about the error handling ? I need to know how I will proceed with this dependency, either maintaining my fork, or be able to maintain upstream.

It's totally ok for me to maintain something on my side, I just need to be fixed about the upcoming of this PR.

Thanks !

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.

2 participants