From 9b9405865e15da3a0a6ee0a67b59b36c5a973a8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 4 Jul 2014 12:45:43 +0200 Subject: [PATCH 01/19] Provide a callback for certificate validation If the certificate validation fails (or always in the case of ssh), let the user decide whether to allow the connection. The data structure passed to the user is the native certificate information from the underlying implementation, namely OpenSSL or WinHTTP. --- include/git2/errors.h | 1 + include/git2/remote.h | 8 ++++++++ include/git2/sys/transport.h | 1 + include/git2/transport.h | 37 ++++++++++++++++++++++++++++++++++++ include/git2/types.h | 12 ++++++++++++ src/netops.c | 7 ++++--- src/remote.c | 3 ++- src/transports/http.c | 28 ++++++++++++++++++++++++--- src/transports/smart.c | 2 ++ src/transports/smart.h | 1 + src/transports/ssh.c | 34 +++++++++++++++++++++++++++++++++ tests/online/push_util.h | 2 +- 12 files changed, 128 insertions(+), 8 deletions(-) diff --git a/include/git2/errors.h b/include/git2/errors.h index b91560631..2ba9924f5 100644 --- a/include/git2/errors.h +++ b/include/git2/errors.h @@ -42,6 +42,7 @@ typedef enum { GIT_ELOCKED = -14, /**< Lock file prevented operation */ GIT_EMODIFIED = -15, /**< Reference value does not match expected */ GIT_EAUTH = -16, /**< Authentication error */ + GIT_ECERTIFICATE = -17, /**< Server certificate is invalid */ GIT_PASSTHROUGH = -30, /**< Internal only */ GIT_ITEROVER = -31, /**< Signals end of iteration with iterator */ diff --git a/include/git2/remote.h b/include/git2/remote.h index c0717fa31..723147590 100644 --- a/include/git2/remote.h +++ b/include/git2/remote.h @@ -458,6 +458,14 @@ struct git_remote_callbacks { */ git_cred_acquire_cb credentials; + /** + * If cert verification fails, this will be called to let the + * user make the final decision of whether to allow the + * connection to proceed. Returns 1 to allow the connection, 0 + * to disallow it or a negative value to indicate an error. + */ + git_transport_certificate_check_cb certificate_check; + /** * During the download of new data, this will be regularly * called with the current count of progress done by the diff --git a/include/git2/sys/transport.h b/include/git2/sys/transport.h index 62ac455d3..44d41c14d 100644 --- a/include/git2/sys/transport.h +++ b/include/git2/sys/transport.h @@ -37,6 +37,7 @@ struct git_transport { git_transport *transport, git_transport_message_cb progress_cb, git_transport_message_cb error_cb, + git_transport_certificate_check_cb certificate_check_cb, void *payload); /* Connect the transport to the remote repository, using the given diff --git a/include/git2/transport.h b/include/git2/transport.h index 7090698ac..cd4429fee 100644 --- a/include/git2/transport.h +++ b/include/git2/transport.h @@ -20,6 +20,43 @@ */ GIT_BEGIN_DECL +/** + * Type of host certificate structure that is passed to the check callback + */ +typedef enum git_cert_t { + /** + * The `data` argument to the callback will be a pointer to + * OpenSSL's `X509` structure. + */ + GIT_CERT_X509_OPENSSL, + GIT_CERT_X509_WINHTTP, + /** + * The `data` argument to the callback will be a pointer to a + * `git_cert_hostkey` structure. + */ + GIT_CERT_HOSTKEY_LIBSSH2, +} git_cert_t; + +/** + * Hostkey information taken from libssh2 + */ +typedef struct { + /** + * A hostkey type from libssh2, either + * `LIBSSH2_HOSTKEY_HASH_MD5` or `LIBSSH2_HOSTKEY_HASH_SHA1` + */ + int type; + /** + * Hostkey hash. If the type is MD5, only the first 16 bytes + * will be set. + */ + unsigned char hash[20]; +} git_cert_hostkey; + +/* + *** Begin interface for credentials acquisition *** + */ + /** Authentication type requested */ typedef enum { /* git_cred_userpass_plaintext */ diff --git a/include/git2/types.h b/include/git2/types.h index 7ed1bcd4c..0009a8aa5 100644 --- a/include/git2/types.h +++ b/include/git2/types.h @@ -253,6 +253,18 @@ typedef int (*git_transfer_progress_cb)(const git_transfer_progress *stats, void */ typedef int (*git_transport_message_cb)(const char *str, int len, void *payload); + +typedef enum git_cert_t git_cert_t; + +/** + * Callback for the user's custom certificate checks. + * + * @param type The type of certificate or host info, SSH or X.509 + * @param data The data for the certificate or host info + * @param payload Payload provided by the caller + */ +typedef int (*git_transport_certificate_check_cb)(git_cert_t type, void *data, void *payload); + /** * Opaque structure representing a submodule. */ diff --git a/src/netops.c b/src/netops.c index fceb4fb74..67d49a529 100644 --- a/src/netops.c +++ b/src/netops.c @@ -384,7 +384,7 @@ on_error: cert_fail_name: OPENSSL_free(peer_cn); giterr_set(GITERR_SSL, "hostname does not match certificate"); - return -1; + return GIT_ECERTIFICATE; } static int ssl_setup(gitno_socket *socket, const char *host, int flags) @@ -494,8 +494,9 @@ int gitno_connect(gitno_socket *s_out, const char *host, const char *port, int f p_freeaddrinfo(info); #ifdef GIT_SSL - if ((flags & GITNO_CONNECT_SSL) && ssl_setup(s_out, host, flags) < 0) - return -1; + if ((flags & GITNO_CONNECT_SSL) && + (ret = ssl_setup(s_out, host, flags)) < 0) + return ret; #else /* SSL is not supported */ if (flags & GITNO_CONNECT_SSL) { diff --git a/src/remote.c b/src/remote.c index 433015f60..9c93f67a1 100644 --- a/src/remote.c +++ b/src/remote.c @@ -673,7 +673,7 @@ int git_remote_connect(git_remote *remote, git_direction direction) return error; if (t->set_callbacks && - (error = t->set_callbacks(t, remote->callbacks.sideband_progress, NULL, remote->callbacks.payload)) < 0) + (error = t->set_callbacks(t, remote->callbacks.sideband_progress, NULL, remote->callbacks.certificate_check, remote->callbacks.payload)) < 0) goto on_error; if (!remote->check_cert) @@ -1263,6 +1263,7 @@ int git_remote_set_callbacks(git_remote *remote, const git_remote_callbacks *cal return remote->transport->set_callbacks(remote->transport, remote->callbacks.sideband_progress, NULL, + remote->callbacks.certificate_check, remote->callbacks.payload); return 0; diff --git a/src/transports/http.c b/src/transports/http.c index f9df53b71..d37059bae 100644 --- a/src/transports/http.c +++ b/src/transports/http.c @@ -19,6 +19,10 @@ git_http_auth_scheme auth_schemes[] = { { GIT_AUTHTYPE_BASIC, "Basic", GIT_CREDTYPE_USERPASS_PLAINTEXT, git_http_auth_basic }, }; +#ifdef GIT_SSL +# include +#endif + static const char *upload_pack_service = "upload-pack"; static const char *upload_pack_ls_service_url = "/info/refs?service=git-upload-pack"; static const char *upload_pack_service_url = "/git-upload-pack"; @@ -524,7 +528,7 @@ static int write_chunk(gitno_socket *socket, const char *buffer, size_t len) static int http_connect(http_subtransport *t) { - int flags = 0; + int flags = 0, error; if (t->connected && http_should_keep_alive(&t->parser) && @@ -546,8 +550,26 @@ static int http_connect(http_subtransport *t) flags |= GITNO_CONNECT_SSL_NO_CHECK_CERT; } - if (gitno_connect(&t->socket, t->connection_data.host, t->connection_data.port, flags) < 0) - return -1; + 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) { + X509 *cert = SSL_get_peer_certificate(t->socket.ssl.ssl); + int allow; + + allow = t->owner->certificate_check_cb(GIT_CERT_X509_OPENSSL, cert, t->owner->message_cb_payload); + if (allow < 0) { + error = allow; + } else if (!allow) { + error = GIT_ECERTIFICATE; + } else { + error = 0; + } + } +#else + if (error < 0) + return error; +#endif t->connected = 1; return 0; diff --git a/src/transports/smart.c b/src/transports/smart.c index a5c3e82dc..d0f9c90e8 100644 --- a/src/transports/smart.c +++ b/src/transports/smart.c @@ -53,12 +53,14 @@ static int git_smart__set_callbacks( git_transport *transport, git_transport_message_cb progress_cb, git_transport_message_cb error_cb, + git_transport_certificate_check_cb certificate_check_cb, void *message_cb_payload) { transport_smart *t = (transport_smart *)transport; t->progress_cb = progress_cb; t->error_cb = error_cb; + t->certificate_check_cb = certificate_check_cb; t->message_cb_payload = message_cb_payload; return 0; diff --git a/src/transports/smart.h b/src/transports/smart.h index b29faae44..44e241adc 100644 --- a/src/transports/smart.h +++ b/src/transports/smart.h @@ -137,6 +137,7 @@ typedef struct { int flags; git_transport_message_cb progress_cb; git_transport_message_cb error_cb; + git_transport_certificate_check_cb certificate_check_cb; void *message_cb_payload; git_smart_subtransport *wrapped; git_smart_subtransport_stream *current_stream; diff --git a/src/transports/ssh.c b/src/transports/ssh.c index fff81661a..fa7dca7c3 100644 --- a/src/transports/ssh.c +++ b/src/transports/ssh.c @@ -517,10 +517,44 @@ static int _git_ssh_setup_conn( if (error < 0) goto on_error; + if (t->owner->certificate_check_cb != NULL) { + git_cert_hostkey cert; + const char *key; + int allow; + + cert.type = LIBSSH2_HOSTKEY_HASH_SHA1; + key = libssh2_hostkey_hash(session, LIBSSH2_HOSTKEY_HASH_SHA1); + if (key != NULL) { + memcpy(&cert.hash, key, 20); + } else { + cert.type = LIBSSH2_HOSTKEY_HASH_MD5; + key = libssh2_hostkey_hash(session, LIBSSH2_HOSTKEY_HASH_MD5); + if (key != NULL) + memcpy(&cert.hash, key, 16); + } + + if (key == NULL) { + giterr_set(GITERR_SSH, "unable to get the host key"); + return -1; + } + + allow = t->owner->certificate_check_cb(GIT_CERT_HOSTKEY_LIBSSH2, &cert, t->owner->message_cb_payload); + if (allow < 0) { + error = allow; + goto on_error; + } + + if (!allow) { + error = GIT_ECERTIFICATE; + goto on_error; + } + } + channel = libssh2_channel_open_session(session); if (!channel) { error = -1; ssh_error(session, "Failed to open SSH channel"); + error = -1; goto on_error; } diff --git a/tests/online/push_util.h b/tests/online/push_util.h index a7207c49e..7736912d6 100644 --- a/tests/online/push_util.h +++ b/tests/online/push_util.h @@ -12,7 +12,7 @@ extern const git_oid OID_ZERO; * @param data pointer to a record_callbacks_data instance */ #define RECORD_CALLBACKS_INIT(data) \ - { GIT_REMOTE_CALLBACKS_VERSION, NULL, NULL, cred_acquire_cb, NULL, record_update_tips_cb, data } + { GIT_REMOTE_CALLBACKS_VERSION, NULL, NULL, cred_acquire_cb, NULL, NULL, record_update_tips_cb, data } typedef struct { char *name; From ec1ce4584a6a8ec2b5b227301a918548907a2b02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Sun, 10 Aug 2014 17:06:53 +0200 Subject: [PATCH 02/19] http: send the DER-encoded cert to the callback Instead of the parsed data, we can ask OpenSSL to give us the DER-encoded version of the certificate, which the user can then parse and validate. --- include/git2/transport.h | 5 ++--- include/git2/types.h | 3 ++- src/transports/http.c | 27 ++++++++++++++++++++++++-- src/transports/ssh.c | 41 +++++++++++++++++++++------------------- 4 files changed, 51 insertions(+), 25 deletions(-) diff --git a/include/git2/transport.h b/include/git2/transport.h index cd4429fee..7365cffdf 100644 --- a/include/git2/transport.h +++ b/include/git2/transport.h @@ -26,10 +26,9 @@ GIT_BEGIN_DECL typedef enum git_cert_t { /** * The `data` argument to the callback will be a pointer to - * OpenSSL's `X509` structure. + * the DER-encoded data. */ - GIT_CERT_X509_OPENSSL, - GIT_CERT_X509_WINHTTP, + GIT_CERT_X509, /** * The `data` argument to the callback will be a pointer to a * `git_cert_hostkey` structure. diff --git a/include/git2/types.h b/include/git2/types.h index 0009a8aa5..b574d2945 100644 --- a/include/git2/types.h +++ b/include/git2/types.h @@ -261,9 +261,10 @@ 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 payload Payload provided by the caller */ -typedef int (*git_transport_certificate_check_cb)(git_cert_t type, void *data, void *payload); +typedef int (*git_transport_certificate_check_cb)(git_cert_t type, void *data, size_t len, void *payload); /** * Opaque structure representing a submodule. diff --git a/src/transports/http.c b/src/transports/http.c index d37059bae..ab477547a 100644 --- a/src/transports/http.c +++ b/src/transports/http.c @@ -555,9 +555,32 @@ static int http_connect(http_subtransport *t) #ifdef GIT_SSL if (error == GIT_ECERTIFICATE && t->owner->certificate_check_cb != NULL) { X509 *cert = SSL_get_peer_certificate(t->socket.ssl.ssl); - int allow; + int allow, len; + unsigned char *guard, *encoded_cert; + + /* Retrieve the length of the certificate first */ + len = i2d_X509(cert, NULL); + if (len < 0) { + giterr_set(GITERR_NET, "failed to retrieve certificate information"); + return -1; + } + + + encoded_cert = git__malloc(len); + GITERR_CHECK_ALLOC(encoded_cert); + /* i2d_X509 makes 'copy' point to just after the data */ + guard = encoded_cert; + + len = i2d_X509(cert, &guard); + if (len < 0) { + git__free(encoded_cert); + giterr_set(GITERR_NET, "failed to retrieve certificate information"); + return -1; + } + + allow = t->owner->certificate_check_cb(GIT_CERT_X509, encoded_cert, len, t->owner->message_cb_payload); + git__free(encoded_cert); - allow = t->owner->certificate_check_cb(GIT_CERT_X509_OPENSSL, cert, t->owner->message_cb_payload); if (allow < 0) { error = allow; } else if (!allow) { diff --git a/src/transports/ssh.c b/src/transports/ssh.c index fa7dca7c3..ba549ff3b 100644 --- a/src/transports/ssh.c +++ b/src/transports/ssh.c @@ -517,28 +517,31 @@ static int _git_ssh_setup_conn( if (error < 0) goto on_error; - if (t->owner->certificate_check_cb != NULL) { - git_cert_hostkey cert; - const char *key; - int allow; + if (t->owner->certificate_check_cb != NULL) { + git_cert_hostkey cert; + const char *key; + int allow; + size_t certlen; - cert.type = LIBSSH2_HOSTKEY_HASH_SHA1; - key = libssh2_hostkey_hash(session, LIBSSH2_HOSTKEY_HASH_SHA1); - if (key != NULL) { - memcpy(&cert.hash, key, 20); - } else { - cert.type = LIBSSH2_HOSTKEY_HASH_MD5; - key = libssh2_hostkey_hash(session, LIBSSH2_HOSTKEY_HASH_MD5); - if (key != NULL) - memcpy(&cert.hash, key, 16); - } + cert.type = LIBSSH2_HOSTKEY_HASH_SHA1; + key = libssh2_hostkey_hash(session, LIBSSH2_HOSTKEY_HASH_SHA1); + if (key != NULL) { + certlen = 20; + memcpy(&cert.hash, key, certlen); + } else { + cert.type = LIBSSH2_HOSTKEY_HASH_MD5; + key = libssh2_hostkey_hash(session, LIBSSH2_HOSTKEY_HASH_MD5); + certlen = 16; + if (key != NULL) + memcpy(&cert.hash, key, certlen); + } - if (key == NULL) { - giterr_set(GITERR_SSH, "unable to get the host key"); - return -1; - } + if (key == NULL) { + giterr_set(GITERR_SSH, "unable to get the host key"); + return -1; + } - allow = t->owner->certificate_check_cb(GIT_CERT_HOSTKEY_LIBSSH2, &cert, t->owner->message_cb_payload); + allow = t->owner->certificate_check_cb(GIT_CERT_HOSTKEY_LIBSSH2, &cert, certlen, t->owner->message_cb_payload); if (allow < 0) { error = allow; goto on_error; From 85acc56262b20567c40a90c0996115060655112c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 29 Aug 2014 17:07:07 +0200 Subject: [PATCH 03/19] remote: add tests for the certificate callback --- tests/online/clone.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/online/clone.c b/tests/online/clone.c index 1ccaf2773..2e8db2b64 100644 --- a/tests/online/clone.c +++ b/tests/online/clone.c @@ -469,3 +469,38 @@ void test_online_clone__url_with_no_path_returns_EINVALIDSPEC(void) cl_git_fail_with(git_clone(&g_repo, "http://github.com", "./foo", &g_options), GIT_EINVALIDSPEC); } + +static int fail_certificate_check(git_cert_t type, void *data, size_t len, void *payload) +{ + GIT_UNUSED(type); + GIT_UNUSED(data); + GIT_UNUSED(len); + GIT_UNUSED(payload); + + return GIT_EUSER; +} + +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); +} + +static int succeed_certificate_check(git_cert_t type, void *data, size_t len, void *payload) +{ + GIT_UNUSED(type); + GIT_UNUSED(data); + GIT_UNUSED(len); + GIT_UNUSED(payload); + + return 0; +} + +void test_online_clone__certificate_valid(void) +{ + g_options.remote_callbacks.certificate_check = succeed_certificate_check; + + cl_git_pass(git_clone(&g_repo, "http://github.com/libgit2/TestGitRepository", "./foo", &g_options)); +} 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 04/19] 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) From 2f5864c50c3c82d01837570cb0b7e629295c65cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 29 Aug 2014 21:15:36 +0200 Subject: [PATCH 05/19] ssh: do ssh cert info before asking for credentials We know the host's key as soon as we connect, so we should perform the check as soon as we can, before we bother with the user's credentials. --- src/transports/ssh.c | 93 +++++++++++++++++++++----------------------- tests/online/clone.c | 3 ++ 2 files changed, 48 insertions(+), 48 deletions(-) diff --git a/src/transports/ssh.c b/src/transports/ssh.c index 8344714f3..a25ab6315 100644 --- a/src/transports/ssh.c +++ b/src/transports/ssh.c @@ -467,56 +467,12 @@ static int _git_ssh_setup_conn( GITERR_CHECK_ALLOC(port); } - /* we need the username to ask for auth methods */ - if (!user) { - if ((error = request_creds(&cred, t, NULL, GIT_CREDTYPE_USERNAME)) < 0) - goto on_error; - - user = git__strdup(((git_cred_username *) cred)->username); - cred->free(cred); - cred = NULL; - if (!user) - goto on_error; - } else if (user && pass) { - if ((error = git_cred_userpass_plaintext_new(&cred, user, pass)) < 0) - goto on_error; - } - if ((error = gitno_connect(&s->socket, host, port, 0)) < 0) goto on_error; if ((error = _git_ssh_session_create(&session, s->socket)) < 0) goto on_error; - if ((error = list_auth_methods(&auth_methods, session, user)) < 0) - goto on_error; - - error = GIT_EAUTH; - /* if we already have something to try */ - if (cred && auth_methods & cred->credtype) - error = _git_ssh_authenticate_session(session, cred); - - while (error == GIT_EAUTH) { - if (cred) { - cred->free(cred); - cred = NULL; - } - - if ((error = request_creds(&cred, t, user, auth_methods)) < 0) - goto on_error; - - if (strcmp(user, git_cred__username(cred))) { - giterr_set(GITERR_SSH, "username does not match previous request"); - error = -1; - goto on_error; - } - - error = _git_ssh_authenticate_session(session, cred); - } - - if (error < 0) - goto on_error; - if (t->owner->certificate_check_cb != NULL) { git_cert_hostkey cert; const char *key; @@ -554,11 +510,54 @@ static int _git_ssh_setup_conn( } } + /* we need the username to ask for auth methods */ + if (!user) { + if ((error = request_creds(&cred, t, NULL, GIT_CREDTYPE_USERNAME)) < 0) + goto on_error; + + user = git__strdup(((git_cred_username *) cred)->username); + cred->free(cred); + cred = NULL; + if (!user) + goto on_error; + } else if (user && pass) { + if ((error = git_cred_userpass_plaintext_new(&cred, user, pass)) < 0) + goto on_error; + } + + if ((error = list_auth_methods(&auth_methods, session, user)) < 0) + goto on_error; + + error = GIT_EAUTH; + /* if we already have something to try */ + if (cred && auth_methods & cred->credtype) + error = _git_ssh_authenticate_session(session, cred); + + while (error == GIT_EAUTH) { + if (cred) { + cred->free(cred); + cred = NULL; + } + + if ((error = request_creds(&cred, t, user, auth_methods)) < 0) + goto on_error; + + if (strcmp(user, git_cred__username(cred))) { + giterr_set(GITERR_SSH, "username does not match previous request"); + error = -1; + goto on_error; + } + + error = _git_ssh_authenticate_session(session, cred); + } + + if (error < 0) + goto on_error; + channel = libssh2_channel_open_session(session); if (!channel) { error = -1; ssh_error(session, "Failed to open SSH channel"); - error = -1; goto on_error; } @@ -634,10 +633,8 @@ static int ssh_receivepack_ls( { const char *cmd = t->cmd_receivepack ? t->cmd_receivepack : cmd_receivepack; - if (_git_ssh_setup_conn(t, url, cmd, stream) < 0) - return -1; - return 0; + return _git_ssh_setup_conn(t, url, cmd, stream); } static int ssh_receivepack( diff --git a/tests/online/clone.c b/tests/online/clone.c index 0e9a17663..66e614e15 100644 --- a/tests/online/clone.c +++ b/tests/online/clone.c @@ -487,6 +487,9 @@ void test_online_clone__certificate_invalid(void) cl_git_fail_with(git_clone(&g_repo, "http://github.com/libgit2/TestGitRepository", "./foo", &g_options), GIT_ECERTIFICATE); + + cl_git_fail_with(git_clone(&g_repo, "ssh://github.com/libgit2/TestGitRepository", "./foo", &g_options), + GIT_ECERTIFICATE); } static int succeed_certificate_check(git_cert_t type, void *data, size_t len, int valid, void *payload) From 23ca0ad5ebcba3173ba3ff51e8114c33f795e62a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Fri, 29 Aug 2014 21:25:13 +0200 Subject: [PATCH 06/19] Bring certificate check back to the normal return code Returning 0 lets the certificate check succeed. An error code is bubbled up to the user. --- src/transports/http.c | 18 +++++++++--------- src/transports/ssh.c | 17 +++++++---------- tests/online/clone.c | 4 ++-- 3 files changed, 18 insertions(+), 21 deletions(-) diff --git a/src/transports/http.c b/src/transports/http.c index f49242e3b..3f74bd149 100644 --- a/src/transports/http.c +++ b/src/transports/http.c @@ -555,7 +555,7 @@ static int http_connect(http_subtransport *t) #ifdef GIT_SSL if ((!error || error == GIT_ECERTIFICATE) && t->owner->certificate_check_cb != NULL) { X509 *cert = SSL_get_peer_certificate(t->socket.ssl.ssl); - int allow, len, is_valid; + int len, is_valid; unsigned char *guard, *encoded_cert; /* Retrieve the length of the certificate first */ @@ -578,17 +578,17 @@ static int http_connect(http_subtransport *t) return -1; } + giterr_clear(); is_valid = error != GIT_ECERTIFICATE; - allow = t->owner->certificate_check_cb(GIT_CERT_X509, encoded_cert, len, is_valid, t->owner->message_cb_payload); + error = 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) { - error = allow; - } else if (!allow) { - error = GIT_ECERTIFICATE; - } else { - error = 0; - } + if (error < 0) { + if (!giterr_last()) + giterr_set(GITERR_NET, "user cancelled certificate check"); + + return error; + } } #endif if (error < 0) diff --git a/src/transports/ssh.c b/src/transports/ssh.c index a25ab6315..8ea4a25d7 100644 --- a/src/transports/ssh.c +++ b/src/transports/ssh.c @@ -476,7 +476,6 @@ static int _git_ssh_setup_conn( if (t->owner->certificate_check_cb != NULL) { git_cert_hostkey cert; const char *key; - int allow; size_t certlen; cert.type = LIBSSH2_HOSTKEY_HASH_SHA1; @@ -498,16 +497,14 @@ static int _git_ssh_setup_conn( } /* 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; - } + giterr_clear(); + error = t->owner->certificate_check_cb(GIT_CERT_HOSTKEY_LIBSSH2, &cert, certlen, 0, t->owner->message_cb_payload); + if (error < 0) { + if (!giterr_last()) + giterr_set(GITERR_NET, "user cancelled hostkey check"); - if (!allow) { - error = GIT_ECERTIFICATE; - goto on_error; - } + goto on_error; + } } /* we need the username to ask for auth methods */ diff --git a/tests/online/clone.c b/tests/online/clone.c index 66e614e15..a880d47d9 100644 --- a/tests/online/clone.c +++ b/tests/online/clone.c @@ -478,7 +478,7 @@ static int fail_certificate_check(git_cert_t type, void *data, size_t len, int v GIT_UNUSED(valid); GIT_UNUSED(payload); - return 0; + return GIT_ECERTIFICATE; } void test_online_clone__certificate_invalid(void) @@ -500,7 +500,7 @@ static int succeed_certificate_check(git_cert_t type, void *data, size_t len, in GIT_UNUSED(valid); GIT_UNUSED(payload); - return 1; + return 0; } void test_online_clone__certificate_valid(void) From 08545d366b24186631b08e3c232f35222d9de5ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Sat, 30 Aug 2014 00:40:37 +0200 Subject: [PATCH 07/19] winhttp: credential check on successful connect On successful connection, still ask the user whether they accept the server's certificate, indicating that WinHTTP would let it though. --- src/transports/winhttp.c | 41 ++++++++++++++++++++++++++++++++++++++++ tests/online/clone.c | 6 ++++-- 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/src/transports/winhttp.c b/src/transports/winhttp.c index 6523086da..3ddf49d60 100644 --- a/src/transports/winhttp.c +++ b/src/transports/winhttp.c @@ -16,6 +16,8 @@ #include "remote.h" #include "repository.h" +#include +#pragma comment(lib, "crypt32") #include #pragma comment(lib, "winhttp") @@ -203,6 +205,31 @@ static int fallback_cred_acquire_cb( return error; } +static int certificate_check(winhttp_stream *s, int valid) +{ + int error; + winhttp_subtransport *t = OWNING_SUBTRANSPORT(s); + PCERT_CONTEXT cert_ctx; + DWORD cert_ctx_size = sizeof(cert_ctx); + + if (t->owner->certificate_check_cb == NULL) + return 0; + + if (!WinHttpQueryOption(s->request, WINHTTP_OPTION_SERVER_CERT_CONTEXT, &cert_ctx, &cert_ctx_size)) { + giterr_set(GITERR_OS, "failed to get server certificate"); + return -1; + } + + giterr_clear(); + error = t->owner->certificate_check_cb(GIT_CERT_X509, cert_ctx->pbCertEncoded, cert_ctx->cbCertEncoded, valid, t->owner->cred_acquire_payload); + CertFreeCertificateContext(cert_ctx); + + if (error < 0 && !giterr_last()) + giterr_set(GITERR_NET, "user cancelled certificate check"); + + return error; +} + static int winhttp_stream_connect(winhttp_stream *s) { winhttp_subtransport *t = OWNING_SUBTRANSPORT(s); @@ -384,6 +411,8 @@ static int winhttp_stream_connect(winhttp_stream *s) goto on_error; } + /* set up the certificate failure callback */ + /* We've done everything up to calling WinHttpSendRequest. */ error = 0; @@ -537,6 +566,7 @@ static int winhttp_stream_read( winhttp_subtransport *t = OWNING_SUBTRANSPORT(s); DWORD dw_bytes_read; char replay_count = 0; + int error; replay: /* Enforce a reasonable cap on the number of replays */ @@ -566,6 +596,9 @@ replay: s->sent_request = 1; } + if ((error = certificate_check(s, 1)) < 0) + return error; + if (s->chunked) { assert(s->verb == post_verb); @@ -815,6 +848,7 @@ static int winhttp_stream_write_single( winhttp_stream *s = (winhttp_stream *)stream; winhttp_subtransport *t = OWNING_SUBTRANSPORT(s); DWORD bytes_written; + int error; if (!s->request && winhttp_stream_connect(s) < 0) return -1; @@ -835,6 +869,9 @@ static int winhttp_stream_write_single( s->sent_request = 1; + if ((error = certificate_check(s, 1)) < 0) + return error; + if (!WinHttpWriteData(s->request, (LPCVOID)buffer, (DWORD)len, @@ -954,6 +991,7 @@ static int winhttp_stream_write_chunked( { winhttp_stream *s = (winhttp_stream *)stream; winhttp_subtransport *t = OWNING_SUBTRANSPORT(s); + int error; if (!s->request && winhttp_stream_connect(s) < 0) return -1; @@ -978,6 +1016,9 @@ static int winhttp_stream_write_chunked( s->sent_request = 1; } + if ((error = certificate_check(s, 1)) < 0) + return error; + if (len > CACHED_POST_BODY_BUF_SIZE) { /* Flush, if necessary */ if (s->chunk_buffer_len > 0) { diff --git a/tests/online/clone.c b/tests/online/clone.c index a880d47d9..cebe3b2ba 100644 --- a/tests/online/clone.c +++ b/tests/online/clone.c @@ -485,11 +485,13 @@ 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), + cl_git_fail_with(git_clone(&g_repo, "https://github.com/libgit2/TestGitRepository", "./foo", &g_options), GIT_ECERTIFICATE); +#ifdef GIT_SSH cl_git_fail_with(git_clone(&g_repo, "ssh://github.com/libgit2/TestGitRepository", "./foo", &g_options), GIT_ECERTIFICATE); +#endif } static int succeed_certificate_check(git_cert_t type, void *data, size_t len, int valid, void *payload) @@ -507,5 +509,5 @@ void test_online_clone__certificate_valid(void) { g_options.remote_callbacks.certificate_check = succeed_certificate_check; - cl_git_pass(git_clone(&g_repo, "http://github.com/libgit2/TestGitRepository", "./foo", &g_options)); + cl_git_pass(git_clone(&g_repo, "https://github.com/libgit2/TestGitRepository", "./foo", &g_options)); } From 5f2cf732ab8ecbfdf69cad48a52c14483c7aa15f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Sat, 30 Aug 2014 13:12:33 +0200 Subject: [PATCH 08/19] winhttp: only do certificate check for SSL If we're not using SSL, don't call the user's certificate check callback. --- src/transports/winhttp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/transports/winhttp.c b/src/transports/winhttp.c index 3ddf49d60..e6bd1c9e1 100644 --- a/src/transports/winhttp.c +++ b/src/transports/winhttp.c @@ -212,7 +212,7 @@ static int certificate_check(winhttp_stream *s, int valid) PCERT_CONTEXT cert_ctx; DWORD cert_ctx_size = sizeof(cert_ctx); - if (t->owner->certificate_check_cb == NULL) + if (t->owner->certificate_check_cb == NULL || !t->connection_data.use_ssl) return 0; if (!WinHttpQueryOption(s->request, WINHTTP_OPTION_SERVER_CERT_CONTEXT, &cert_ctx, &cert_ctx_size)) { From 67c84e06f3cef3e555ddb74a1de9affdc3f0688f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Sat, 30 Aug 2014 14:04:57 +0200 Subject: [PATCH 09/19] winhttp: bring together request sending We need to call WinHttpSendRequest() in three different places. Unify all in a single function to have a single place for the certificate check. --- src/transports/winhttp.c | 76 +++++++++++++++++++++++----------------- 1 file changed, 44 insertions(+), 32 deletions(-) diff --git a/src/transports/winhttp.c b/src/transports/winhttp.c index e6bd1c9e1..14afce282 100644 --- a/src/transports/winhttp.c +++ b/src/transports/winhttp.c @@ -212,6 +212,10 @@ static int certificate_check(winhttp_stream *s, int valid) PCERT_CONTEXT cert_ctx; DWORD cert_ctx_size = sizeof(cert_ctx); + /* If there is no override, we should fail if WinHTTP doesn't think it's fine */ + if (t->owner->certificate_check_cb == NULL && !valid) + return GIT_ECERTIFICATE; + if (t->owner->certificate_check_cb == NULL || !t->connection_data.use_ssl) return 0; @@ -411,8 +415,6 @@ static int winhttp_stream_connect(winhttp_stream *s) goto on_error; } - /* set up the certificate failure callback */ - /* We've done everything up to calling WinHttpSendRequest. */ error = 0; @@ -556,6 +558,38 @@ on_error: return error; } +static int send_request(winhttp_stream *s, size_t len, int ignore_length) +{ + int request_failed = 0, cert_valid = 1, error = 0; + + if (ignore_length) { + if (!WinHttpSendRequest(s->request, + WINHTTP_NO_ADDITIONAL_HEADERS, 0, + WINHTTP_NO_REQUEST_DATA, 0, + WINHTTP_IGNORE_REQUEST_TOTAL_LENGTH, 0)) { + request_failed = 1; + } + } else { + if (!WinHttpSendRequest(s->request, + WINHTTP_NO_ADDITIONAL_HEADERS, 0, + WINHTTP_NO_REQUEST_DATA, 0, + len, 0)) { + request_failed = 1; + } + } + + if (request_failed && GetLastError() == ERROR_WINHTTP_SECURE_FAILURE) + cert_valid = 0; + + giterr_clear(); + if ((error = certificate_check(s, cert_valid)) < 0) { + if (!giterr_last()) + giterr_set(GITERR_OS, "failed to send request"); + } + + return error; +} + static int winhttp_stream_read( git_smart_subtransport_stream *stream, char *buffer, @@ -583,22 +617,16 @@ replay: DWORD status_code, status_code_length, content_type_length, bytes_written; char expected_content_type_8[MAX_CONTENT_TYPE_LEN]; wchar_t expected_content_type[MAX_CONTENT_TYPE_LEN], content_type[MAX_CONTENT_TYPE_LEN]; + int request_failed = 0, cert_valid = 1; if (!s->sent_request) { - if (!WinHttpSendRequest(s->request, - WINHTTP_NO_ADDITIONAL_HEADERS, 0, - WINHTTP_NO_REQUEST_DATA, 0, - s->post_body_len, 0)) { - giterr_set(GITERR_OS, "Failed to send request"); - return -1; - } + + if ((error = send_request(s, s->post_body_len, 0)) < 0) + return error; s->sent_request = 1; } - if ((error = certificate_check(s, 1)) < 0) - return error; - if (s->chunked) { assert(s->verb == post_verb); @@ -859,19 +887,11 @@ static int winhttp_stream_write_single( return -1; } - if (!WinHttpSendRequest(s->request, - WINHTTP_NO_ADDITIONAL_HEADERS, 0, - WINHTTP_NO_REQUEST_DATA, 0, - (DWORD)len, 0)) { - giterr_set(GITERR_OS, "Failed to send request"); - return -1; - } + if ((error = send_request(s, len, 0)) < 0) + return error; s->sent_request = 1; - if ((error = certificate_check(s, 1)) < 0) - return error; - if (!WinHttpWriteData(s->request, (LPCVOID)buffer, (DWORD)len, @@ -1005,20 +1025,12 @@ static int winhttp_stream_write_chunked( return -1; } - if (!WinHttpSendRequest(s->request, - WINHTTP_NO_ADDITIONAL_HEADERS, 0, - WINHTTP_NO_REQUEST_DATA, 0, - WINHTTP_IGNORE_REQUEST_TOTAL_LENGTH, 0)) { - giterr_set(GITERR_OS, "Failed to send request"); - return -1; - } + if ((error = send_request(s, 0, 1)) < 0) + return error; s->sent_request = 1; } - if ((error = certificate_check(s, 1)) < 0) - return error; - if (len > CACHED_POST_BODY_BUF_SIZE) { /* Flush, if necessary */ if (s->chunk_buffer_len > 0) { From 7c8acc54be390face2f8b519a828a7f2860bb5b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Sat, 30 Aug 2014 14:26:57 +0200 Subject: [PATCH 10/19] winhttp: set ignore security flags on user command If the user returns 0 from the certificate check and we had certificate issues, set the options to ignore certificate errors and resend the request. --- src/transports/winhttp.c | 48 +++++++++++++++++++++++++++++++++++----- 1 file changed, 42 insertions(+), 6 deletions(-) diff --git a/src/transports/winhttp.c b/src/transports/winhttp.c index 14afce282..039399790 100644 --- a/src/transports/winhttp.c +++ b/src/transports/winhttp.c @@ -558,7 +558,7 @@ on_error: return error; } -static int send_request(winhttp_stream *s, size_t len, int ignore_length) +static int do_send_request(winhttp_stream *s, size_t len, int ignore_length) { int request_failed = 0, cert_valid = 1, error = 0; @@ -567,26 +567,62 @@ static int send_request(winhttp_stream *s, size_t len, int ignore_length) WINHTTP_NO_ADDITIONAL_HEADERS, 0, WINHTTP_NO_REQUEST_DATA, 0, WINHTTP_IGNORE_REQUEST_TOTAL_LENGTH, 0)) { - request_failed = 1; + return -1; } } else { if (!WinHttpSendRequest(s->request, WINHTTP_NO_ADDITIONAL_HEADERS, 0, WINHTTP_NO_REQUEST_DATA, 0, len, 0)) { - request_failed = 1; + return -1; } } - if (request_failed && GetLastError() == ERROR_WINHTTP_SECURE_FAILURE) - cert_valid = 0; + return 0; +} + +static int send_request(winhttp_stream *s, size_t len, int ignore_length) +{ + int request_failed = 0, cert_valid = 1, error = 0; + DWORD ignore_flags; + + if ((error = do_send_request(s, len, ignore_length)) < 0) + request_failed = 1; + + if (request_failed) { + if (GetLastError() != ERROR_WINHTTP_SECURE_FAILURE) { + giterr_set(GITERR_OS, "failed to send request"); + return -1; + } else { + cert_valid = 0; + } + } giterr_clear(); if ((error = certificate_check(s, cert_valid)) < 0) { if (!giterr_last()) - giterr_set(GITERR_OS, "failed to send request"); + giterr_set(GITERR_OS, "user cancelled certificate check"); + + return error; } + /* if neither the request nor the certificate check returned errors, we're done */ + if (!request_failed) + return 0; + + ignore_flags = + SECURITY_FLAG_IGNORE_CERT_CN_INVALID | + SECURITY_FLAG_IGNORE_CERT_DATE_INVALID | + SECURITY_FLAG_IGNORE_UNKNOWN_CA; + + if (!WinHttpSetOption(s->request, WINHTTP_OPTION_SECURITY_FLAGS, &ignore_flags, sizeof(ignore_flags))) { + giterr_set(GITERR_OS, "failed to set security options"); + return -1; + } + + if ((error = do_send_request(s, len, ignore_length)) < 0) + giterr_set(GITERR_OS, "failed to send request"); + return error; } From 2aee4642ef9c0cffcebc443e81a706f3e458906f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Sun, 31 Aug 2014 23:16:19 +0200 Subject: [PATCH 11/19] transport: move the cert type enum to types.h This should make the mingw compiler happy. --- include/git2/transport.h | 16 ---------------- include/git2/types.h | 17 ++++++++++++++++- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/include/git2/transport.h b/include/git2/transport.h index 7365cffdf..06d090f4a 100644 --- a/include/git2/transport.h +++ b/include/git2/transport.h @@ -20,22 +20,6 @@ */ GIT_BEGIN_DECL -/** - * Type of host certificate structure that is passed to the check callback - */ -typedef enum git_cert_t { - /** - * The `data` argument to the callback will be a pointer to - * the DER-encoded data. - */ - GIT_CERT_X509, - /** - * The `data` argument to the callback will be a pointer to a - * `git_cert_hostkey` structure. - */ - GIT_CERT_HOSTKEY_LIBSSH2, -} git_cert_t; - /** * Hostkey information taken from libssh2 */ diff --git a/include/git2/types.h b/include/git2/types.h index 51f058867..3544037a4 100644 --- a/include/git2/types.h +++ b/include/git2/types.h @@ -254,7 +254,22 @@ typedef int (*git_transfer_progress_cb)(const git_transfer_progress *stats, void typedef int (*git_transport_message_cb)(const char *str, int len, void *payload); -typedef enum git_cert_t git_cert_t; + +/** + * Type of host certificate structure that is passed to the check callback + */ +typedef enum git_cert_t { + /** + * The `data` argument to the callback will be a pointer to + * the DER-encoded data. + */ + GIT_CERT_X509, + /** + * The `data` argument to the callback will be a pointer to a + * `git_cert_hostkey` structure. + */ + GIT_CERT_HOSTKEY_LIBSSH2, +} git_cert_t; /** * Callback for the user's custom certificate checks. From 41698f22f683d3452ef83de3b3e82f5cb178b0b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Thu, 11 Sep 2014 10:04:05 +0200 Subject: [PATCH 12/19] net: remove support for outright ignoring certificates This option make it easy to ignore anything about the server we're connecting to, which is bad security practice. This was necessary as we didn't use to expose detailed information about the certificate, but now that we do, we should get rid of this. If the user wants to ignore everything, they can still provide a callback which ignores all the information passed. --- include/git2/remote.h | 8 -------- include/git2/sys/transport.h | 3 --- src/netops.c | 7 ++----- src/netops.h | 4 ---- src/remote.c | 20 +++----------------- src/remote.h | 1 - src/transports/http.c | 3 --- 7 files changed, 5 insertions(+), 41 deletions(-) diff --git a/include/git2/remote.h b/include/git2/remote.h index 723147590..d2cc3e8e7 100644 --- a/include/git2/remote.h +++ b/include/git2/remote.h @@ -410,14 +410,6 @@ GIT_EXTERN(int) git_remote_supported_url(const char* url); */ GIT_EXTERN(int) git_remote_list(git_strarray *out, git_repository *repo); -/** - * Choose whether to check the server's certificate (applies to HTTPS only) - * - * @param remote the remote to configure - * @param check whether to check the server's certificate (defaults to yes) - */ -GIT_EXTERN(void) git_remote_check_cert(git_remote *remote, int check); - /** * Argument to the completion callback which tells it which operation * finished. diff --git a/include/git2/sys/transport.h b/include/git2/sys/transport.h index 44d41c14d..1e8f4e4ed 100644 --- a/include/git2/sys/transport.h +++ b/include/git2/sys/transport.h @@ -23,9 +23,6 @@ GIT_BEGIN_DECL typedef enum { GIT_TRANSPORTFLAGS_NONE = 0, - /* If the connection is secured with SSL/TLS, the authenticity - * of the server certificate should not be verified. */ - GIT_TRANSPORTFLAGS_NO_CHECK_CERT = 1 } git_transport_flags_t; typedef struct git_transport git_transport; diff --git a/src/netops.c b/src/netops.c index 67d49a529..43b8c5311 100644 --- a/src/netops.c +++ b/src/netops.c @@ -387,7 +387,7 @@ cert_fail_name: return GIT_ECERTIFICATE; } -static int ssl_setup(gitno_socket *socket, const char *host, int flags) +static int ssl_setup(gitno_socket *socket, const char *host) { int ret; @@ -406,9 +406,6 @@ static int ssl_setup(gitno_socket *socket, const char *host, int flags) if ((ret = SSL_connect(socket->ssl.ssl)) <= 0) return ssl_set_error(&socket->ssl, ret); - if (GITNO_CONNECT_SSL_NO_CHECK_CERT & flags) - return 0; - return verify_server_cert(&socket->ssl, host); } #endif @@ -495,7 +492,7 @@ int gitno_connect(gitno_socket *s_out, const char *host, const char *port, int f #ifdef GIT_SSL if ((flags & GITNO_CONNECT_SSL) && - (ret = ssl_setup(s_out, host, flags)) < 0) + (ret = ssl_setup(s_out, host)) < 0) return ret; #else /* SSL is not supported */ diff --git a/src/netops.h b/src/netops.h index dfb4ab7b4..beb0e0760 100644 --- a/src/netops.h +++ b/src/netops.h @@ -47,10 +47,6 @@ typedef struct gitno_buffer gitno_buffer; enum { /* Attempt to create an SSL connection. */ GITNO_CONNECT_SSL = 1, - - /* Valid only when GITNO_CONNECT_SSL is also specified. - * Indicates that the server certificate should not be validated. */ - GITNO_CONNECT_SSL_NO_CHECK_CERT = 2, }; /** diff --git a/src/remote.c b/src/remote.c index 9c93f67a1..46a610c3a 100644 --- a/src/remote.c +++ b/src/remote.c @@ -80,6 +80,8 @@ static int ensure_remote_name_is_valid(const char *name) return error; } +#if 0 +/* We could export this as a helper */ static int get_check_cert(int *out, git_repository *repo) { git_config *cfg; @@ -105,6 +107,7 @@ static int get_check_cert(int *out, git_repository *repo) *out = git_config__get_bool_force(cfg, "http.sslverify", 1); return 0; } +#endif static int create_internal(git_remote **out, git_repository *repo, const char *name, const char *url, const char *fetch) { @@ -121,9 +124,6 @@ static int create_internal(git_remote **out, git_repository *repo, const char *n remote->repo = repo; remote->update_fetchhead = 1; - if (get_check_cert(&remote->check_cert, repo) < 0) - goto on_error; - if (git_vector_init(&remote->refs, 32, NULL) < 0) goto on_error; @@ -274,7 +274,6 @@ int git_remote_dup(git_remote **dest, git_remote *source) remote->transport_cb_payload = source->transport_cb_payload; remote->repo = source->repo; remote->download_tags = source->download_tags; - remote->check_cert = source->check_cert; remote->update_fetchhead = source->update_fetchhead; if (git_vector_init(&remote->refs, 32, NULL) < 0 || @@ -369,9 +368,6 @@ int git_remote_load(git_remote **out, git_repository *repo, const char *name) remote->name = git__strdup(name); GITERR_CHECK_ALLOC(remote->name); - if ((error = get_check_cert(&remote->check_cert, repo)) < 0) - goto cleanup; - if (git_vector_init(&remote->refs, 32, NULL) < 0 || git_vector_init(&remote->refspecs, 2, NULL) < 0 || git_vector_init(&remote->active_refspecs, 2, NULL) < 0) { @@ -676,9 +672,6 @@ int git_remote_connect(git_remote *remote, git_direction direction) (error = t->set_callbacks(t, remote->callbacks.sideband_progress, NULL, remote->callbacks.certificate_check, remote->callbacks.payload)) < 0) goto on_error; - if (!remote->check_cert) - flags |= GIT_TRANSPORTFLAGS_NO_CHECK_CERT; - if ((error = t->connect(t, url, remote->callbacks.credentials, remote->callbacks.payload, direction, flags)) != 0) goto on_error; @@ -1244,13 +1237,6 @@ int git_remote_list(git_strarray *remotes_list, git_repository *repo) return 0; } -void git_remote_check_cert(git_remote *remote, int check) -{ - assert(remote); - - remote->check_cert = check; -} - int git_remote_set_callbacks(git_remote *remote, const git_remote_callbacks *callbacks) { assert(remote && callbacks); diff --git a/src/remote.h b/src/remote.h index c471756b8..f88601e9b 100644 --- a/src/remote.h +++ b/src/remote.h @@ -31,7 +31,6 @@ struct git_remote { git_transfer_progress stats; unsigned int need_pack; git_remote_autotag_option_t download_tags; - int check_cert; int update_fetchhead; }; diff --git a/src/transports/http.c b/src/transports/http.c index 3f74bd149..1bbef81b8 100644 --- a/src/transports/http.c +++ b/src/transports/http.c @@ -545,9 +545,6 @@ static int http_connect(http_subtransport *t) return -1; flags |= GITNO_CONNECT_SSL; - - if (GIT_TRANSPORTFLAGS_NO_CHECK_CERT & tflags) - flags |= GITNO_CONNECT_SSL_NO_CHECK_CERT; } error = gitno_connect(&t->socket, t->connection_data.host, t->connection_data.port, flags); From bf8756d6a2c42dc77b8a2de814a12e2ceb4487fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Mon, 15 Sep 2014 21:51:42 +0200 Subject: [PATCH 13/19] ssh: add test for host key Test that the certificate check callback gets the right fingerprint from the host we're connecting to. --- script/cibuild.sh | 5 ++++- tests/online/clone.c | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/script/cibuild.sh b/script/cibuild.sh index 981f95b7e..c7c341ce5 100755 --- a/script/cibuild.sh +++ b/script/cibuild.sh @@ -33,6 +33,9 @@ ssh-keygen -t rsa -f ~/.ssh/id_rsa -N "" -q cat ~/.ssh/id_rsa.pub >>~/.ssh/authorized_keys ssh-keyscan -t rsa localhost >>~/.ssh/known_hosts +# Get the fingerprint for localhost and remove the colons so we can parse it as a hex number +export GITTEST_REMOTE_SSH_FINGERPRINT=$(ssh-keygen -F localhost -l | tail -n 1 | cut -d ' ' -f 2 | tr -d ':') + export GITTEST_REMOTE_URL="ssh://localhost/$HOME/_temp/test.git" export GITTEST_REMOTE_USER=$USER export GITTEST_REMOTE_SSH_KEY="$HOME/.ssh/id_rsa" @@ -40,7 +43,7 @@ export GITTEST_REMOTE_SSH_PUBKEY="$HOME/.ssh/id_rsa.pub" export GITTEST_REMOTE_SSH_PASSPHRASE="" if [ -e ./libgit2_clar ]; then - ./libgit2_clar -sonline::push -sonline::clone::cred_callback && + ./libgit2_clar -sonline::push -sonline::clone::cred_callback -sonline::clone::ssh_cert && rm -rf $HOME/_temp/test.git && git init --bare $HOME/_temp/test.git && # create an empty one ./libgit2_clar -sonline::clone::ssh_with_paths diff --git a/tests/online/clone.c b/tests/online/clone.c index cebe3b2ba..2e51364f6 100644 --- a/tests/online/clone.c +++ b/tests/online/clone.c @@ -464,6 +464,38 @@ void test_online_clone__ssh_cannot_change_username(void) cl_git_fail(git_clone(&g_repo, "ssh://git@github.com/libgit2/TestGitRepository", "./foo", &g_options)); } +int ssh_certificate_check(git_cert_t type, void *data, size_t len, int valid, void *payload) +{ + git_cert_hostkey *key; + git_oid expected = {{0}}, actual = {{0}}; + const char *expected_str; + + GIT_UNUSED(len); + GIT_UNUSED(valid); + GIT_UNUSED(payload); + + expected_str = cl_getenv("GITTEST_REMOTE_SSH_FINGERPRINT"); + if (!expected_str) + cl_skip(); + + cl_git_pass(git_oid_fromstr(&expected, expected_str)); + cl_assert_equal_i(GIT_CERT_HOSTKEY_LIBSSH2, type); + + key = (git_cert_hostkey *) data; + git_oid_fromraw(&actual, key->hash); + + cl_assert(git_oid_equal(&expected, &actual)); + + return GIT_EUSER; +} + +void test_online_clone__ssh_cert(void) +{ + g_options.remote_callbacks.certificate_check = ssh_certificate_check; + + cl_git_fail_with(GIT_EUSER, git_clone(&g_repo, "ssh://localhost/foo", "./foo", &g_options)); +} + void test_online_clone__url_with_no_path_returns_EINVALIDSPEC(void) { cl_git_fail_with(git_clone(&g_repo, "http://github.com", "./foo", &g_options), From 0782fc43f809b1c2a5001453ccb064a4afcfc2b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Tue, 16 Sep 2014 01:47:30 +0200 Subject: [PATCH 14/19] net: use only structs to pass information about cert Instead of spreading the data in function arguments, some of which aren't used for ssh and having a struct only for ssh, use a struct for both, using a common parent to pass to the callback. --- include/git2/transport.h | 24 ++++++++++++++++++++++++ include/git2/types.h | 14 +++++++++++--- src/transports/http.c | 6 +++++- src/transports/ssh.c | 4 +++- src/transports/winhttp.c | 6 +++++- tests/online/clone.c | 21 ++++++++------------- 6 files changed, 56 insertions(+), 19 deletions(-) diff --git a/include/git2/transport.h b/include/git2/transport.h index 06d090f4a..4fa637292 100644 --- a/include/git2/transport.h +++ b/include/git2/transport.h @@ -24,6 +24,11 @@ GIT_BEGIN_DECL * Hostkey information taken from libssh2 */ typedef struct { + /** + * Type of certificate. Here to share the header with + * `git_cert`. + */ + git_cert_t cert_type; /** * A hostkey type from libssh2, either * `LIBSSH2_HOSTKEY_HASH_MD5` or `LIBSSH2_HOSTKEY_HASH_SHA1` @@ -36,6 +41,25 @@ typedef struct { unsigned char hash[20]; } git_cert_hostkey; +/** + * X.509 certificate information + */ +typedef struct { + /** + * Type of certificate. Here to share the header with + * `git_cert`. + */ + git_cert_t cert_type; + /** + * Pointer to the X.509 certificate data + */ + void *data; + /** + * Length of the memory block pointed to by `data`. + */ + size_t len; +} git_cert_x509; + /* *** Begin interface for credentials acquisition *** */ diff --git a/include/git2/types.h b/include/git2/types.h index 3544037a4..7ee7cc344 100644 --- a/include/git2/types.h +++ b/include/git2/types.h @@ -253,8 +253,6 @@ typedef int (*git_transfer_progress_cb)(const git_transfer_progress *stats, void */ typedef int (*git_transport_message_cb)(const char *str, int len, void *payload); - - /** * Type of host certificate structure that is passed to the check callback */ @@ -271,6 +269,16 @@ typedef enum git_cert_t { GIT_CERT_HOSTKEY_LIBSSH2, } git_cert_t; +/** + * Parent type for `git_cert_hostkey` and `git_cert_x509`. + */ +typedef struct { + /** + * Type of certificate. A `GIT_CERT_` value. + */ + git_cert_t cert_type; +} git_cert; + /** * Callback for the user's custom certificate checks. * @@ -281,7 +289,7 @@ typedef enum git_cert_t { * 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, int valid, void *payload); +typedef int (*git_transport_certificate_check_cb)(git_cert *cert, int valid, void *payload); /** * Opaque structure representing a submodule. diff --git a/src/transports/http.c b/src/transports/http.c index 1bbef81b8..7ef0b519c 100644 --- a/src/transports/http.c +++ b/src/transports/http.c @@ -552,6 +552,7 @@ static int http_connect(http_subtransport *t) #ifdef GIT_SSL if ((!error || error == GIT_ECERTIFICATE) && t->owner->certificate_check_cb != NULL) { X509 *cert = SSL_get_peer_certificate(t->socket.ssl.ssl); + git_cert_x509 cert_info; int len, is_valid; unsigned char *guard, *encoded_cert; @@ -577,7 +578,10 @@ static int http_connect(http_subtransport *t) giterr_clear(); is_valid = error != GIT_ECERTIFICATE; - error = t->owner->certificate_check_cb(GIT_CERT_X509, encoded_cert, len, is_valid, t->owner->message_cb_payload); + cert_info.cert_type = GIT_CERT_X509; + cert_info.data = encoded_cert; + cert_info.len = len; + error = t->owner->certificate_check_cb((git_cert *) &cert_info, is_valid, t->owner->message_cb_payload); git__free(encoded_cert); if (error < 0) { diff --git a/src/transports/ssh.c b/src/transports/ssh.c index a0f6dd722..298209150 100644 --- a/src/transports/ssh.c +++ b/src/transports/ssh.c @@ -484,6 +484,8 @@ static int _git_ssh_setup_conn( const char *key; size_t certlen; + cert.cert_type = GIT_CERT_HOSTKEY_LIBSSH2; + cert.type = LIBSSH2_HOSTKEY_HASH_SHA1; key = libssh2_hostkey_hash(session, LIBSSH2_HOSTKEY_HASH_SHA1); if (key != NULL) { @@ -504,7 +506,7 @@ static int _git_ssh_setup_conn( /* We don't currently trust any hostkeys */ giterr_clear(); - error = t->owner->certificate_check_cb(GIT_CERT_HOSTKEY_LIBSSH2, &cert, certlen, 0, t->owner->message_cb_payload); + error = t->owner->certificate_check_cb((git_cert *) &cert, 0, t->owner->message_cb_payload); if (error < 0) { if (!giterr_last()) giterr_set(GITERR_NET, "user cancelled hostkey check"); diff --git a/src/transports/winhttp.c b/src/transports/winhttp.c index 039399790..5c74b5672 100644 --- a/src/transports/winhttp.c +++ b/src/transports/winhttp.c @@ -211,6 +211,7 @@ static int certificate_check(winhttp_stream *s, int valid) winhttp_subtransport *t = OWNING_SUBTRANSPORT(s); PCERT_CONTEXT cert_ctx; DWORD cert_ctx_size = sizeof(cert_ctx); + git_cert_x509 cert; /* If there is no override, we should fail if WinHTTP doesn't think it's fine */ if (t->owner->certificate_check_cb == NULL && !valid) @@ -225,7 +226,10 @@ static int certificate_check(winhttp_stream *s, int valid) } giterr_clear(); - error = t->owner->certificate_check_cb(GIT_CERT_X509, cert_ctx->pbCertEncoded, cert_ctx->cbCertEncoded, valid, t->owner->cred_acquire_payload); + cert.cert_type = GIT_CERT_X509; + cert.data = cert_ctx->pbCertEncoded; + cert.len = cert_ctx->cbCertEncoded; + error = t->owner->certificate_check_cb((git_cert *) &cert, valid, t->owner->cred_acquire_payload); CertFreeCertificateContext(cert_ctx); if (error < 0 && !giterr_last()) diff --git a/tests/online/clone.c b/tests/online/clone.c index f88a4d611..2c36b3d22 100644 --- a/tests/online/clone.c +++ b/tests/online/clone.c @@ -473,13 +473,12 @@ void test_online_clone__ssh_cannot_change_username(void) cl_git_fail(git_clone(&g_repo, "ssh://git@github.com/libgit2/TestGitRepository", "./foo", &g_options)); } -int ssh_certificate_check(git_cert_t type, void *data, size_t len, int valid, void *payload) +int ssh_certificate_check(git_cert *cert, int valid, void *payload) { git_cert_hostkey *key; git_oid expected = {{0}}, actual = {{0}}; const char *expected_str; - GIT_UNUSED(len); GIT_UNUSED(valid); GIT_UNUSED(payload); @@ -487,10 +486,10 @@ int ssh_certificate_check(git_cert_t type, void *data, size_t len, int valid, vo if (!expected_str) cl_skip(); - cl_git_pass(git_oid_fromstr(&expected, expected_str)); - cl_assert_equal_i(GIT_CERT_HOSTKEY_LIBSSH2, type); + cl_git_pass(git_oid_fromstrp(&expected, expected_str)); + cl_assert_equal_i(GIT_CERT_HOSTKEY_LIBSSH2, cert->cert_type); - key = (git_cert_hostkey *) data; + key = (git_cert_hostkey *) cert; git_oid_fromraw(&actual, key->hash); cl_assert(git_oid_equal(&expected, &actual)); @@ -511,11 +510,9 @@ 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, int valid, void *payload) +static int fail_certificate_check(git_cert *cert, int valid, void *payload) { - GIT_UNUSED(type); - GIT_UNUSED(data); - GIT_UNUSED(len); + GIT_UNUSED(cert); GIT_UNUSED(valid); GIT_UNUSED(payload); @@ -535,11 +532,9 @@ void test_online_clone__certificate_invalid(void) #endif } -static int succeed_certificate_check(git_cert_t type, void *data, size_t len, int valid, void *payload) +static int succeed_certificate_check(git_cert *cert, int valid, void *payload) { - GIT_UNUSED(type); - GIT_UNUSED(data); - GIT_UNUSED(len); + GIT_UNUSED(cert); GIT_UNUSED(valid); GIT_UNUSED(payload); From ebda0970765e881feddd1879fb39ac9a3aa951a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Tue, 16 Sep 2014 02:07:39 +0200 Subject: [PATCH 15/19] script: use a parallel build on Travis --- script/cibuild.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/script/cibuild.sh b/script/cibuild.sh index dda4ead4a..abe31d0dc 100755 --- a/script/cibuild.sh +++ b/script/cibuild.sh @@ -15,7 +15,7 @@ export GITTEST_REMOTE_URL="git://localhost/test.git" mkdir _build cd _build cmake .. -DCMAKE_INSTALL_PREFIX=../_install $OPTIONS || exit $? -cmake --build . --target install || exit $? +make -j2 install || exit $? ctest -V . || exit $? # Now that we've tested the raw git protocol, let's set up ssh to we From 286369a81fc2e1d77a7a134d5c140f023af1298b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Tue, 16 Sep 2014 02:27:16 +0200 Subject: [PATCH 16/19] ssh: provide our own types for host key lengths Instead of using the libssh2 defines, provide our own, which eases usage as we do not need to check whether libgit2 was built with libssh2 or not. --- include/git2/transport.h | 14 ++++++++++++-- src/transports/ssh.c | 16 +++++++--------- tests/online/clone.c | 2 ++ 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/include/git2/transport.h b/include/git2/transport.h index 4fa637292..6c568be65 100644 --- a/include/git2/transport.h +++ b/include/git2/transport.h @@ -20,6 +20,16 @@ */ GIT_BEGIN_DECL +/** + * Type of SSH host fingerprint + */ +typedef enum { + /** MD5, 16 bytes */ + GIT_CERT_SSH_MD5, + /** SHA-1, 20 bytes */ + GIT_CERT_SSH_SHA1, +} git_cert_ssh_type ; + /** * Hostkey information taken from libssh2 */ @@ -31,9 +41,9 @@ typedef struct { git_cert_t cert_type; /** * A hostkey type from libssh2, either - * `LIBSSH2_HOSTKEY_HASH_MD5` or `LIBSSH2_HOSTKEY_HASH_SHA1` + * `GIT_CERT_SSH_MD5` or `GIT_CERT_SSH_SHA1` */ - int type; + git_cert_ssh_type type; /** * Hostkey hash. If the type is MD5, only the first 16 bytes * will be set. diff --git a/src/transports/ssh.c b/src/transports/ssh.c index 298209150..717565327 100644 --- a/src/transports/ssh.c +++ b/src/transports/ssh.c @@ -480,23 +480,21 @@ static int _git_ssh_setup_conn( goto on_error; if (t->owner->certificate_check_cb != NULL) { - git_cert_hostkey cert; + git_cert_hostkey cert = { 0 }; const char *key; - size_t certlen; cert.cert_type = GIT_CERT_HOSTKEY_LIBSSH2; - cert.type = LIBSSH2_HOSTKEY_HASH_SHA1; key = libssh2_hostkey_hash(session, LIBSSH2_HOSTKEY_HASH_SHA1); if (key != NULL) { - certlen = 20; - memcpy(&cert.hash, key, certlen); + cert.type = GIT_CERT_SSH_SHA1; + memcpy(&cert.hash, key, 20); } else { - cert.type = LIBSSH2_HOSTKEY_HASH_MD5; key = libssh2_hostkey_hash(session, LIBSSH2_HOSTKEY_HASH_MD5); - certlen = 16; - if (key != NULL) - memcpy(&cert.hash, key, certlen); + if (key != NULL) { + cert.type = GIT_CERT_SSH_MD5; + memcpy(&cert.hash, key, 16); + } } if (key == NULL) { diff --git a/tests/online/clone.c b/tests/online/clone.c index 2c36b3d22..0dd746a75 100644 --- a/tests/online/clone.c +++ b/tests/online/clone.c @@ -492,6 +492,8 @@ int ssh_certificate_check(git_cert *cert, int valid, void *payload) key = (git_cert_hostkey *) cert; git_oid_fromraw(&actual, key->hash); + cl_assert_equal_i(GIT_CERT_SSH_SHA1, key->type); + cl_assert(git_oid_equal(&expected, &actual)); return GIT_EUSER; From 1e0aa105faff1bd6987665506037f299b8e0e1cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Tue, 16 Sep 2014 03:22:09 +0200 Subject: [PATCH 17/19] ssh: expose both hashes The user may have the data hashed as MD5 or SHA-1, so we should provide both types for consumption. --- include/git2/transport.h | 25 ++++++++++++++++--------- src/transports/ssh.c | 18 +++++++++--------- tests/online/clone.c | 17 +++++++++++++---- 3 files changed, 38 insertions(+), 22 deletions(-) diff --git a/include/git2/transport.h b/include/git2/transport.h index 6c568be65..39df479c7 100644 --- a/include/git2/transport.h +++ b/include/git2/transport.h @@ -24,11 +24,11 @@ GIT_BEGIN_DECL * Type of SSH host fingerprint */ typedef enum { - /** MD5, 16 bytes */ - GIT_CERT_SSH_MD5, - /** SHA-1, 20 bytes */ - GIT_CERT_SSH_SHA1, -} git_cert_ssh_type ; + /** MD5 is available */ + GIT_CERT_SSH_MD5 = (1 << 0), + /** SHA-1 is available */ + GIT_CERT_SSH_SHA1 = (1 << 1), +} git_cert_ssh_t; /** * Hostkey information taken from libssh2 @@ -43,12 +43,19 @@ typedef struct { * A hostkey type from libssh2, either * `GIT_CERT_SSH_MD5` or `GIT_CERT_SSH_SHA1` */ - git_cert_ssh_type type; + git_cert_ssh_t type; + /** - * Hostkey hash. If the type is MD5, only the first 16 bytes - * will be set. + * Hostkey hash. If type has `GIT_CERT_SSH_MD5` set, this will + * have the MD5 hash of the hostkey. */ - unsigned char hash[20]; + unsigned char hash_md5[16]; + + /** + * Hostkey hash. If type has `GIT_CERT_SSH_SHA1` set, this will + * have the SHA-1 hash of the hostkey. + */ + unsigned char hash_sha1[20]; } git_cert_hostkey; /** diff --git a/src/transports/ssh.c b/src/transports/ssh.c index 717565327..15a45ca86 100644 --- a/src/transports/ssh.c +++ b/src/transports/ssh.c @@ -487,17 +487,17 @@ static int _git_ssh_setup_conn( key = libssh2_hostkey_hash(session, LIBSSH2_HOSTKEY_HASH_SHA1); if (key != NULL) { - cert.type = GIT_CERT_SSH_SHA1; - memcpy(&cert.hash, key, 20); - } else { - key = libssh2_hostkey_hash(session, LIBSSH2_HOSTKEY_HASH_MD5); - if (key != NULL) { - cert.type = GIT_CERT_SSH_MD5; - memcpy(&cert.hash, key, 16); - } + cert.type |= GIT_CERT_SSH_SHA1; + memcpy(&cert.hash_sha1, key, 20); } - if (key == NULL) { + key = libssh2_hostkey_hash(session, LIBSSH2_HOSTKEY_HASH_MD5); + if (key != NULL) { + cert.type |= GIT_CERT_SSH_MD5; + memcpy(&cert.hash_md5, key, 16); + } + + if (cert.type == 0) { giterr_set(GITERR_SSH, "unable to get the host key"); return -1; } diff --git a/tests/online/clone.c b/tests/online/clone.c index 0dd746a75..42682e8d8 100644 --- a/tests/online/clone.c +++ b/tests/online/clone.c @@ -488,13 +488,22 @@ int ssh_certificate_check(git_cert *cert, int valid, void *payload) cl_git_pass(git_oid_fromstrp(&expected, expected_str)); cl_assert_equal_i(GIT_CERT_HOSTKEY_LIBSSH2, cert->cert_type); - key = (git_cert_hostkey *) cert; - git_oid_fromraw(&actual, key->hash); - cl_assert_equal_i(GIT_CERT_SSH_SHA1, key->type); + /* + * We need to figure out how long our input was to check for + * the type. Here we abuse the fact that both hashes fit into + * our git_oid type. + */ + if (strlen(expected_str) == 32 && key->type & GIT_CERT_SSH_MD5) { + memcpy(&actual.id, key->hash_md5, 16); + } else if (strlen(expected_str) == 40 && key->type & GIT_CERT_SSH_SHA1) { + memcpy(&actual, key->hash_sha1, 20); + } else { + cl_fail("Cannot find a usable SSH hash"); + } - cl_assert(git_oid_equal(&expected, &actual)); + cl_assert(!memcmp(&expected, &actual, 20)); return GIT_EUSER; } From 4fe5b771b5c6c72ca33bf7593fe1aca1afd02578 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Tue, 16 Sep 2014 13:35:36 +0200 Subject: [PATCH 18/19] winhttp: get rid of the cert ignore flag This brings us back in line with the other transports. --- src/transports/winhttp.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/transports/winhttp.c b/src/transports/winhttp.c index 5c74b5672..8aef63193 100644 --- a/src/transports/winhttp.c +++ b/src/transports/winhttp.c @@ -388,13 +388,6 @@ static int winhttp_stream_connect(winhttp_stream *s) if (t->owner->parent.read_flags(&t->owner->parent, &flags) < 0) goto on_error; - - if ((GIT_TRANSPORTFLAGS_NO_CHECK_CERT & flags) && - !WinHttpSetOption(s->request, WINHTTP_OPTION_SECURITY_FLAGS, - (LPVOID)&no_check_cert_flags, sizeof(no_check_cert_flags))) { - giterr_set(GITERR_OS, "Failed to set options to ignore cert errors"); - goto on_error; - } } /* If we have a credential on the subtransport, apply it to the request */ From 52e09724fde2d46c1f31d07f6445dc7b4dee3947 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Mart=C3=ADn=20Nieto?= Date: Tue, 16 Sep 2014 17:13:58 +0200 Subject: [PATCH 19/19] ssh: skip the localhost cert check earlier Skip it before we attempt to clone, as we would exit with -1 on systems which do not have sshd running. --- tests/online/clone.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/online/clone.c b/tests/online/clone.c index 42682e8d8..f7f3aaeda 100644 --- a/tests/online/clone.c +++ b/tests/online/clone.c @@ -483,8 +483,7 @@ int ssh_certificate_check(git_cert *cert, int valid, void *payload) GIT_UNUSED(payload); expected_str = cl_getenv("GITTEST_REMOTE_SSH_FINGERPRINT"); - if (!expected_str) - cl_skip(); + cl_assert(expected_str); cl_git_pass(git_oid_fromstrp(&expected, expected_str)); cl_assert_equal_i(GIT_CERT_HOSTKEY_LIBSSH2, cert->cert_type); @@ -512,6 +511,9 @@ void test_online_clone__ssh_cert(void) { g_options.remote_callbacks.certificate_check = ssh_certificate_check; + if (!cl_getenv("GITTEST_REMOTE_SSH_FINGERPRINT")) + cl_skip(); + cl_git_fail_with(GIT_EUSER, git_clone(&g_repo, "ssh://localhost/foo", "./foo", &g_options)); }