Add builtin proxy support for the http transport#4870
Conversation
|
Note: I'm seeing periodic 400's on proxied connections. As best I can tell, this might actually be a problem with the proxy itself (or perhaps this new code's interaction with our particular test proxy), since I only see them when connecting through our test proxy, not with a debugging proxy like Charles. I'll have to dig in further but since I think the problem lies with the proxy, I'm going to go ahead and open this for review. |
fb1708c to
a735bdd
Compare
|
Note: we don't support speaking to a proxy over SSL on Windows; at least not fully. It seems that there's no mechanism for handling cert failures there. I'm going to leave this as-is but I would like to revisit this problem in the future. |
tiennou
left a comment
There was a problem hiding this comment.
Gave a thorough look at it, now I feel like a broken compiler ranting about indentation 😉.
It's a really nice cleanup overall, and removes one dependency, so 👏 for taking care of it.
|
Thanks @tiennou - I'll give a pass on indentation. I've been using about 4 different editors on about 4 different systems so something put in soft tabs. 😢 |
|
Thanks for working on this! I really like the direction -- removing dependencies is a nice thing, and I've really been bothered by libcurl in the past due to Ubuntu not backporting fixes to security-related issues. There's also quite a bunch of nice cleanups in here. |
ee28c0e to
4cace3c
Compare
|
Thanks @pks-t and @tiennou for the feedback. All good comments and I've updated this accordingly. I tracked down the 400 errors - in fact it was not an error with the proxy, we were removing our internal proxy configuration state by reloading the user's proxy configuration, causing us to send proxy-looking requests (eg I've also removed the ability to connect to a proxy over SSL (we still allow users to connect to an SSL remote through a proxy, using CONNECT, but we connect to the proxy itself over HTTP only). The SSL tunnel over SSL connection is showing warnings in valgrind in Ubuntu 14.04. It's unclear whether we're calling OpenSSL incorrectly or if it's an OpenSSL error. But we need not resolve that immediately, we can instead forego support for connecting to proxies over SSL. This is not a regression - our curl proxy support also had this limitation. I think this is a bad limitation, obviously, but I'd like to ship this to unblock some LibGit2Sharp scenarios by removing the libcurl dependency. So I think that this PR is a strict improvement: it does not reduce any functionality, and it removes the libcurl dependency. Therefore, I'd like to merge this and come back for the SSL proxy connection support in a separate PR. |
4cace3c to
33c9936
Compare
Store the error message from the underlying TLS library before calling the certificate callback. If it refuses to act (demonstrated by returning GIT_PASSTHROUGH) then restore the error message. Otherwise, if the callback does not set an error message, set a sensible default that implicates the callback itself.
Give the proxy tests a proxy certificate callback, and allow self-signed certificates when the `GITTEST_REMOTE_PROXY_SELFSIGNED` environment variable is set (to anything). In that case, simply compare the hostname from the callback to the hostname that we connected to.
Rename credential callback to proxy_cred_cb to match new cert callback.
Introduce `git_tls_stream_wrap` which will take an existing `stream` with an already connected socket and begin speaking TLS on top of it. This is useful if you've built a connection to a proxy server and you wish to begin CONNECT over it to tunnel a TLS connection. Also update the pluggable TLS stream layer so that it can accept a registration structure that provides an `init` and `wrap` function, instead of a single initialization function.
Natively support HTTPS connections through proxies by speaking CONNECT to the proxy and then adding a TLS connection on top of the socket.
The implementations of git_openssl_stream_new and git_mbedtls_stream_new have callers protected by #ifdefs and are never called unless compiled in. There's no need for a dummy implementation. Remove them.
We previously used cURL to support HTTP proxies. Now that we've added this support natively, we can remove the curl dependency.
For testing, we may wish to use a man-in-the-middle proxy that can inspect the CONNECT traffic to our test endpoints. For this, we will need to accept the proxy's certificate, which will not be valid for the true endpoint. Add a new environment variable, GITTEST_REMOTE_SSL_NOVERIFY to disable https certificate validation for the tests.
The `-Wdocumentation-deprecated-sync` option will warn when there is a doxygen `\deprecated` tag but there is no corresponding deprecation attribute on the function. We want to encourage users to not use particular APIs by marking them deprecated in the documentation without necessarily raising a compiler warning by marking an item as deprecated.
Don't allow servers to send us multiple Content-Type, Content-Length or Location headers.
Update the new stream registration API to be `git_stream_register` which takes a registration structure and a TLS boolean. This allows callers to register non-TLS streams as well as TLS streams. Provide `git_stream_register_tls` that takes just the init callback for backward compatibliity.
Only load the proxy configuration during connection; we need this data when we're going to connect to the server, however we may mutate it after connection (connecting through a CONNECT proxy means that we should send requests like normal). If we reload the proxy configuration but do not actually reconnect (because we're in a keep-alive session) then we will reload the proxy configuration that we should have mutated. Thus, only load the proxy configuration when we know that we're going to reconnect.
Temporarily disallow SSL connections to a proxy until we can understand the valgrind warnings when tunneling OpenSSL over OpenSSL.
Accept an enum (`git_stream_t`) during custom stream registration that indicates whether the registration structure should be used for standard (non-TLS) streams or TLS streams.
Reset the replay_count upon a successful connection. It's possible that we could encounter a situation where we connect successfully but need to replay a request - for example, a connection and initial request succeeds without authentication but a subsequent call does require authentication. Reset the replay count upon any successful request to afford subsequent replays room to manuever.
214eaf8 to
30ac46a
Compare
|
Rebased this to fix up conflicts after the certificate |
co-authored-by: Ian Hattendorf <ianh@axosoft.com>
Provide proxy support for the HTTP transport.
GET https://github.com/libgit2/libgit2/...)CONNECTverb.git_tls_stream_wrapwhich will create a TLS stream on top of an existing stream (instead of trying to connect a socket).I've also updated our test proxy to support speaking HTTPS, so I've updated the tests to support that as well and introduced a second proxy test pass: one that speaks HTTP to the proxy and one that speaks HTTPS.
With our own proxy support, this removes the need for us to use libcurl. It's been removed from the codebase.
Fixed #4515 #4207