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 support for keyboard-interactie auth. #63

Merged
merged 3 commits into from
Jul 23, 2024

Conversation

mateuszkj
Copy link
Contributor

@mateuszkj mateuszkj commented Jul 1, 2024

What

This change adds support for the keyboard-interactive authentication method described in RFC 4256.

Implementation

This solution introduces the AuthKeyboardInteractive builder, which can be converted into an AuthMethod.

let kbi = AuthKeyboardInteractive::new()
    .with_response("Password: ", "abt")
    .with_response("OTP: ", "12345");

let auth = AuthMethod::with_keyboard_interactive(kbi);
...

This implementation allows defining responses to known and exact prompts.

Implementation Considerations

Although my solution addresses my problem, it might be too limited for others. Here are some considerations:

  • Should prompts be trimmed of white spaces before comparison?
  • Should prompt comparisons be case-insensitive?
  • Should we add an async handler so API users can implement custom responses (e.g., asking SMS services for OTPs)?

If the answer to any of these questions is yes, I am willing to implement those changes.

@mateuszkj mateuszkj changed the title [WIP] Add support for keyboard-interactie auth. Add support for keyboard-interactie auth. Jul 2, 2024
@Miyoshi-Ryota Miyoshi-Ryota self-requested a review July 10, 2024 08:38
Copy link
Owner

@Miyoshi-Ryota Miyoshi-Ryota left a comment

Choose a reason for hiding this comment

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

@mateuszkj
Sorry for the late response. I'm caught up in a death march at my main job and couldn't find time.

Thanks for the excellent changes. LGTM!

I'll go ahead and merge it.

I'll just fix some minor lint warnings in the CI and then release it. I'll release your contributions to crates.io this weekend.

Implementation Considerations

I'm a bit interested in the third point, but we can address it when the need arises. I prioritize releasing what's needed now over waiting for a perfect solution.

Thank you for your perfect pull request!

@Miyoshi-Ryota Miyoshi-Ryota merged commit 19f7073 into Miyoshi-Ryota:main Jul 23, 2024
1 of 2 checks passed
@mateuszkj
Copy link
Contributor Author

@Miyoshi-Ryota Thanks for merging it.

If you need help with any implementation, feel free to contact me. I'm also considering adding SSH-agent support, so there might be a need for an async credential provider then.

@Miyoshi-Ryota Miyoshi-Ryota mentioned this pull request Jul 28, 2024
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