Skip to content

Allow certificate and credential callbacks to decline to act - #4879

Merged
pks-t merged 2 commits into
masterfrom
ethomson/defer_cert_cred_cb
Nov 28, 2018
Merged

Allow certificate and credential callbacks to decline to act#4879
pks-t merged 2 commits into
masterfrom
ethomson/defer_cert_cred_cb

Conversation

@ethomson

@ethomson ethomson commented Nov 6, 2018

Copy link
Copy Markdown
Member

Custom transports may want to ask libgit2 to invoke a configured credential or certificate callback; however they likely do not know if a callback was actually configured. Return a sentinal value (greater than zero) if there is no callback configured instead of crashing.

With this, we want to define what > 0 means for these callbacks, because they themselves might return it. Credential callbacks are already defined to return > 0 to defer these checks, but they were only inconsistently implemented (winhttp supported this, http and ssh transports did not). Add this support consistently to all transports for both credential callbacks and certificate checking.

@tiennou

tiennou commented Nov 6, 2018

Copy link
Copy Markdown
Contributor

Did a quick review, so LGTM.

This looks like it's related to #3440 (there's also a test case in #4042 you might be able to reuse).

Comment thread include/git2/sys/transport.h Outdated
Comment thread src/transports/http.c Outdated
Custom transports may want to ask libgit2 to invoke a configured
credential or certificate callback; however they likely do not know if a
callback was actually configured.  Return a sentinal value
(GIT_PASSTHROUGH) if there is no callback configured instead of crashing.
@ethomson
ethomson force-pushed the ethomson/defer_cert_cred_cb branch from f25053c to dab7cbd Compare November 15, 2018 09:19
@ethomson

Copy link
Copy Markdown
Member Author

@pks-t I agree that GIT_PASSTHROUGH is the more sensible value. I've fixed this up to use that instead.

@tiennou Sorry, it looks like I'm stepping on your toes a bit with #4042. 😰 I'll take a look and see how these two might interact.

@tiennou

tiennou commented Nov 15, 2018

Copy link
Copy Markdown
Contributor

I don't really mind. I hope you're going to fix it right here, but in any case I think it's easier to do so after your cleanup. #4042 is about ignoring a callback's presence in case GIT_EPASSTHROUGH is returned.

Comment thread src/transports/ssh.c Outdated
Allow credential and certificate checking callbacks to return
GIT_PASSTHROUGH, indicating that they do not want to act.
Introduce this to support in both the http and ssh callbacks.
Additionally, enable the same mechanism for certificate validation.

This is most useful to disambiguate any meaning in the publicly exposed
credential and certificate functions (`git_transport_smart_credentials`
and `git_transport_smart_certificate_check`) but it may be more
generally useful for callers to be able to defer back to libgit2.
@ethomson
ethomson force-pushed the ethomson/defer_cert_cred_cb branch from dab7cbd to a2e6e0e Compare November 21, 2018 23:06
@pks-t
pks-t merged commit c97d302 into master Nov 28, 2018
@pks-t

pks-t commented Nov 28, 2018

Copy link
Copy Markdown
Member

Thanks Ed!

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.

3 participants