Skip to content

Add builtin proxy support for the http transport#4870

Merged
ethomson merged 28 commits intomasterfrom
ethomson/proxy
Nov 28, 2018
Merged

Add builtin proxy support for the http transport#4870
ethomson merged 28 commits intomasterfrom
ethomson/proxy

Conversation

@ethomson
Copy link
Copy Markdown
Member

Provide proxy support for the HTTP transport.

  • When we're using a proxy, we now connect directly to the proxy, instead of trying to connect to the remote (git server).
  • For connections to an HTTP remote, we can simply change the request URL that we send to the proxy to be a fully-qualified URL (eg GET https://github.com/libgit2/libgit2/...)
  • For connections to an HTTPS remote, we need to request that the proxy build a tunnel to the remote, using the CONNECT verb.
    • At that point, we can begin speaking HTTPS to the remote over the socket we've built to the proxy.
    • I've introduce git_tls_stream_wrap which 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

@ethomson
Copy link
Copy Markdown
Member Author

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.

@ethomson ethomson force-pushed the ethomson/proxy branch 6 times, most recently from fb1708c to a735bdd Compare October 30, 2018 14:23
@ethomson
Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Contributor

@tiennou tiennou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread include/git2/sys/stream.h Outdated
Comment thread include/git2/sys/stream.h Outdated
Comment thread src/streams/tls.c Outdated
Comment thread src/streams/tls.h Outdated
Comment thread src/transports/http.c
Comment thread src/streams/mbedtls.c Outdated
Comment thread src/streams/openssl.c Outdated
Comment thread src/streams/stransport.c Outdated
Comment thread src/transports/http.c Outdated
Comment thread src/transports/http.c Outdated
@ethomson
Copy link
Copy Markdown
Member Author

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. 😢

Comment thread src/transports/http.c Outdated
Comment thread src/transports/http.c Outdated
Comment thread src/transports/http.c Outdated
Comment thread src/transports/http.c Outdated
Comment thread src/transports/http.c Outdated
Comment thread src/streams/tls.c Outdated
Comment thread tests/core/stream.c Outdated
Comment thread src/transports/http.c Outdated
Comment thread src/transports/http.c
Comment thread src/transports/http.c Outdated
@pks-t
Copy link
Copy Markdown
Member

pks-t commented Nov 2, 2018

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.

@ethomson ethomson force-pushed the ethomson/proxy branch 16 times, most recently from ee28c0e to 4cace3c Compare November 18, 2018 22:27
@ethomson
Copy link
Copy Markdown
Member Author

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 GET https://host/path) through the CONNECT tunnel instead of regular requests (eg GET /path). Fixed in 66c78cb.

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.

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.
@ethomson
Copy link
Copy Markdown
Member Author

Rebased this to fix up conflicts after the certificate GIT_PASSTHROUGH changes.

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