-
Notifications
You must be signed in to change notification settings - Fork 400
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
base: master
Are you sure you want to change the base?
Add support for SSH interactive authentication #869
Conversation
Add missing documentation
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.
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?
let text = malloc(response_bytes.len()); | ||
response.text = text as *mut c_char; | ||
response.length = response_bytes.len() as u32; |
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 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 |
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.
This is the ssh2 library that is freeing the memory, not git2 right?
☔ The latest upstream changes (possibly f095112) made this pull request unmergeable. Please resolve the merge conflicts. |
For lifetime reasons,
Cred
now doesn't always carry agit_cred
and thereforeCred::unwrap()
may now panic if called on aCred
created usingCred::ssh_interactive()
; this is the only backwards-incompatible changeWithout leaking memory or memorizing created handlers in
RemoteCallbacks
it's not possible to have individual handler for each SSH interactiveCred
, so I compromized and put the handler directly intoRemoteCallbacks
Also, since
libgit2
only ships with dummytypedef
s forLIBSSH2_USERAUTH_KBDINT_PROMPT
andLIBSSH2_USERAUTH_KBDINT_PROMPT
,libssh2-sys
is now a direct dependency for thessh
feature