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 SSH interactive authentication #869

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

Conversation

exactly-one-kas
Copy link

For lifetime reasons, Cred now doesn't always carry a git_cred and therefore Cred::unwrap() may now panic if called on a Cred created using Cred::ssh_interactive(); this is the only backwards-incompatible change

Without leaking memory or memorizing created handlers in RemoteCallbacks it's not possible to have individual handler for each SSH interactive Cred, so I compromized and put the handler directly into RemoteCallbacks

Also, since libgit2 only ships with dummy typedefs for LIBSSH2_USERAUTH_KBDINT_PROMPT and LIBSSH2_USERAUTH_KBDINT_PROMPT, libssh2-sys is now a direct dependency for the ssh feature

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Sorry for the long delay on the review.

I just wanted to confirm, the ssh2 library does not provide any way to indicate an error in the callback? Is the intended way to handle an error is to print a message and just not fill in the responses?

Comment on lines +560 to +562
let text = malloc(response_bytes.len());
response.text = text as *mut c_char;
response.length = response_bytes.len() as u32;
Copy link
Contributor

Choose a reason for hiding this comment

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

There seems to be an issue here. The response is never copied into the buffer created via malloc.

I think this also needs a check for a null return from malloc.

I'm only partially confident this is the correct way to handle this. From what I can tell, ssh2 has its own allocation routines that are part of the session, but I don't think the rust side exposes that, and I think it always defaults to libc malloc/free, so I don't think it is a problem. However, I'm a bit nervous about this manual memory handling with APIs that don't really document the behavior.

let response = &mut *responses.offset(i as isize);
let response_bytes = wrapped_responses[i as usize].as_bytes();

// libgit2 frees returned strings
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the ssh2 library that is freeing the memory, not git2 right?

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