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

Introduce SSH support thru ssh-agent #177

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

moo1210
Copy link

@moo1210 moo1210 commented Oct 4, 2023

Currently, Wally doesn't support ssh-agent for authentication, so SSH Git URLs aren't really usable. Additionally, git2 has a bug on the current version that Wally was using that would cause an error loading known_hosts: ; class=Ssh (23) on Windows, so I also bumped that to a newer version.

@@ -27,6 +27,11 @@ fn make_credentials_callback(
let mut token_tried = false;

move |url, username, allowed_types| {
if allowed_types.contains(CredentialType::SSH_KEY) {
let username = username.unwrap();
Copy link

Choose a reason for hiding this comment

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

We probably shouldn't unwrap here, but rather return some sort of error.

Copy link
Author

Choose a reason for hiding this comment

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

Cargo's implementation doesn't, having a comment saying it doesn't get called if there is no username as the reasoning. So, I just followed Cargo's implementation since that's what all of Wally's was based on, to begin with.
https://github.com/rust-lang/cargo/blob/79b397d72c557eb6444a2ba0dc00a211a226a35a/src/cargo/sources/git/utils.rs#L637-L646

@OverHash
Copy link

OverHash commented Oct 4, 2023

Looks like rust-lang/git2-rs#935 (merged into git2 0.17.1) should solve the issue of error loading known_hosts: ; class=Ssh (23) based off rust-lang/git2-rs#937.

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