From 17491f6e5660b82cb8163eea58805df6398af16e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 29 Aug 2014 17:18:23 +0200 Subject: [PATCH] transport: always call the certificate check callback We should let the user decide whether to cancel the connection or not regardless of whether our checks have decided that the certificate is fine. We provide our own assessment to the callback to let the user fall back to our checks if they so desire. --- include/git2/types.h | 4 +++- src/transports/http.c | 19 ++++++++++--------- src/transports/ssh.c | 3 ++- tests/online/clone.c | 12 +++++++----- 4 files changed, 22 insertions(+), 16 deletions(-) diff --git a/include/git2/types.h b/include/git2/types.h index b574d2945..51f058867 100644 --- a/include/git2/types.h +++ b/include/git2/types.h @@ -262,9 +262,11 @@ typedef enum git_cert_t git_cert_t; * @param type The type of certificate or host info, SSH or X.509 * @param data The data for the certificate or host info * @param len The size of the certificate or host info + * @param valid Whether the libgit2 checks (OpenSSL or WinHTTP) think + * this certificate is valid * @param payload Payload provided by the caller */ -typedef int (*git_transport_certificate_check_cb)(git_cert_t type, void *data, size_t len, void *payload); +typedef int (*git_transport_certificate_check_cb)(git_cert_t type, void *data, size_t len, int valid, void *payload); /** * Opaque structure representing a submodule. diff --git a/src/transports/http.c b/src/transports/http.c index ab477547a..f49242e3b 100644 --- a/src/transports/http.c +++ b/src/transports/http.c @@ -553,9 +553,9 @@ static int http_connect(http_subtransport *t) error = gitno_connect(&t->socket, t->connection_data.host, t->connection_data.port, flags); #ifdef GIT_SSL - if (error == GIT_ECERTIFICATE && t->owner->certificate_check_cb != NULL) { + if ((!error || error == GIT_ECERTIFICATE) && t->owner->certificate_check_cb != NULL) { X509 *cert = SSL_get_peer_certificate(t->socket.ssl.ssl); - int allow, len; + int allow, len, is_valid; unsigned char *guard, *encoded_cert; /* Retrieve the length of the certificate first */ @@ -578,7 +578,8 @@ static int http_connect(http_subtransport *t) return -1; } - allow = t->owner->certificate_check_cb(GIT_CERT_X509, encoded_cert, len, t->owner->message_cb_payload); + is_valid = error != GIT_ECERTIFICATE; + allow = t->owner->certificate_check_cb(GIT_CERT_X509, encoded_cert, len, is_valid, t->owner->message_cb_payload); git__free(encoded_cert); if (allow < 0) { @@ -589,10 +590,9 @@ static int http_connect(http_subtransport *t) error = 0; } } -#else +#endif if (error < 0) return error; -#endif t->connected = 1; return 0; @@ -653,6 +653,7 @@ replay: while (!*bytes_read && !t->parse_finished) { size_t data_offset; + int error; /* * Make the parse_buffer think it's as full of data as @@ -699,8 +700,8 @@ replay: if (PARSE_ERROR_REPLAY == t->parse_error) { s->sent_request = 0; - if (http_connect(t) < 0) - return -1; + if ((error = http_connect(t)) < 0) + return error; goto replay; } @@ -952,8 +953,8 @@ static int http_action( (ret = gitno_connection_data_from_url(&t->connection_data, url, NULL)) < 0) return ret; - if (http_connect(t) < 0) - return -1; + if ((ret = http_connect(t)) < 0) + return ret; switch (action) { case GIT_SERVICE_UPLOADPACK_LS: diff --git a/src/transports/ssh.c b/src/transports/ssh.c index ba549ff3b..8344714f3 100644 --- a/src/transports/ssh.c +++ b/src/transports/ssh.c @@ -541,7 +541,8 @@ static int _git_ssh_setup_conn( return -1; } - allow = t->owner->certificate_check_cb(GIT_CERT_HOSTKEY_LIBSSH2, &cert, certlen, t->owner->message_cb_payload); + /* We don't currently trust any hostkeys */ + allow = t->owner->certificate_check_cb(GIT_CERT_HOSTKEY_LIBSSH2, &cert, certlen, 0, t->owner->message_cb_payload); if (allow < 0) { error = allow; goto on_error; diff --git a/tests/online/clone.c b/tests/online/clone.c index 2e8db2b64..0e9a17663 100644 --- a/tests/online/clone.c +++ b/tests/online/clone.c @@ -470,14 +470,15 @@ void test_online_clone__url_with_no_path_returns_EINVALIDSPEC(void) GIT_EINVALIDSPEC); } -static int fail_certificate_check(git_cert_t type, void *data, size_t len, void *payload) +static int fail_certificate_check(git_cert_t type, void *data, size_t len, int valid, void *payload) { GIT_UNUSED(type); GIT_UNUSED(data); GIT_UNUSED(len); + GIT_UNUSED(valid); GIT_UNUSED(payload); - return GIT_EUSER; + return 0; } void test_online_clone__certificate_invalid(void) @@ -485,17 +486,18 @@ void test_online_clone__certificate_invalid(void) g_options.remote_callbacks.certificate_check = fail_certificate_check; cl_git_fail_with(git_clone(&g_repo, "http://github.com/libgit2/TestGitRepository", "./foo", &g_options), - GIT_EUSER); + GIT_ECERTIFICATE); } -static int succeed_certificate_check(git_cert_t type, void *data, size_t len, void *payload) +static int succeed_certificate_check(git_cert_t type, void *data, size_t len, int valid, void *payload) { GIT_UNUSED(type); GIT_UNUSED(data); GIT_UNUSED(len); + GIT_UNUSED(valid); GIT_UNUSED(payload); - return 0; + return 1; } void test_online_clone__certificate_valid(void)