Merge lp:~gz/bzr/https_proxy_host_matching_944696 into lp:bzr/2.5

[Martin Packman]

Use correct host name for checking certificates when using a proxy for https

Fixes the issue of checking against the wrong host when tunnelling https through an http proxy. The proxied_host attribute contains the right thing in that case, it just wants the port taken off.

Any ideas on unit testing this? I've confirmed it works per my reproduction steps in the bug.

[Jelmer Vernooij (jelmer)]

This looks good. I think this needs a release notes entry.

I'm not sure what the best way is to test this either. Ideally we'd set up an actual proxy, but you mentioned on IRC that it's hard to do with twisted's servers?

review: Approve

[Vincent Ladeuil (vila)]

As far as testing goes, we already have a case where we need to setup an external test server (bug #866111) and there is also the bzr-local-test-server which could probably do with an added config for tinyproxy.

The change looks fine but I wonder if it will address the (unorthodox but still valid) use case of an https proxy used to talk to a real https server. In other words, relying on self.proxied_host being None may be brittle and may be 'host' should just be a method parameter and let callers set it knowingly.

No need to block on that but doing a quick check or filing a bug for the https -> https case may be good.

[Martin Packman (gz)]

Release notes added.

As discussed on IRC, the only proxy case that matters here is tunnelling HTTPS over HTTP. That's all the code explicitly handles with the proxied_host variable, and the only kind of HTTPS proxying bzr wants to support.

[Martin Packman (gz)]

sent to pqm by email

=== modified file 'bzrlib/transport/http/_urllib2_wrappers.py'
--- bzrlib/transport/http/_urllib2_wrappers.py	2012-02-21 12:16:02 +0000
+++ bzrlib/transport/http/_urllib2_wrappers.py	2012-03-12 17:22:18 +0000
@@ -476,9 +476,12 @@
         # FIXME JRV 2011-12-18: Use location config here?
         config_stack = config.GlobalStack()
         cert_reqs = config_stack.get('ssl.cert_reqs')
+        if self.proxied_host is not None:
+            host = self.proxied_host.split(":", 1)[0]
+        else:
+            host = self.host
         if cert_reqs == ssl.CERT_NONE:
-            trace.warning("Not checking SSL certificate for %s: %d",
-                self.host, self.port)
+            trace.warning("Not checking SSL certificate for %s", host)
             ca_certs = None
         else:
             if self.ca_certs is None:
@@ -503,7 +506,7 @@
             raise
         if cert_reqs == ssl.CERT_REQUIRED:
             peer_cert = ssl_sock.getpeercert()
-            match_hostname(peer_cert, self.host)
+            match_hostname(peer_cert, host)
 
         # Wrap the ssl socket before anybody use it
         self._wrap_socket_for_reporting(ssl_sock)

=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2012-03-12 14:44:51 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2012-03-12 17:22:18 +0000
@@ -32,6 +32,10 @@
 .. Fixes for situations where bzr would previously crash or give incorrect
    or undesirable results.
 
+* Connecting with HTTPS via HTTP now correctly uses the host name of the
+  destination rather than the proxy when checking certificates.
+  (Martin Packman, #944696)
+
 * Fixed merge tool availability checking and invocation to search the
   Windows App Path registry in addition to the PATH. (Gordon Tyler, #939605)