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

Proposed by Martin Packman
Status: Merged
Approved by: Martin Packman
Approved revision: 6490
Merged at revision: 6491
Proposed branch: lp:~gz/bzr/https_proxy_host_matching_944696
Merge into: lp:bzr/2.5
Diff against target: 41 lines (+10/-3)
2 files modified
bzrlib/transport/http/_urllib2_wrappers.py (+6/-3)
doc/en/release-notes/bzr-2.5.txt (+4/-0)
To merge this branch: bzr merge lp:~gz/bzr/https_proxy_host_matching_944696
Reviewer Review Type Date Requested Status
Jelmer Vernooij (community) Approve
Review via email: mp+96803@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

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
Revision history for this message
Vincent Ladeuil (vila) wrote :

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.

6490. By Martin Packman

Add release notes

Revision history for this message
Martin Packman (gz) wrote :

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.

Revision history for this message
Martin Packman (gz) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
476 # FIXME JRV 2011-12-18: Use location config here?476 # FIXME JRV 2011-12-18: Use location config here?
477 config_stack = config.GlobalStack()477 config_stack = config.GlobalStack()
478 cert_reqs = config_stack.get('ssl.cert_reqs')478 cert_reqs = config_stack.get('ssl.cert_reqs')
479 if self.proxied_host is not None:
480 host = self.proxied_host.split(":", 1)[0]
481 else:
482 host = self.host
479 if cert_reqs == ssl.CERT_NONE:483 if cert_reqs == ssl.CERT_NONE:
480 trace.warning("Not checking SSL certificate for %s: %d",484 trace.warning("Not checking SSL certificate for %s", host)
481 self.host, self.port)
482 ca_certs = None485 ca_certs = None
483 else:486 else:
484 if self.ca_certs is None:487 if self.ca_certs is None:
@@ -503,7 +506,7 @@
503 raise506 raise
504 if cert_reqs == ssl.CERT_REQUIRED:507 if cert_reqs == ssl.CERT_REQUIRED:
505 peer_cert = ssl_sock.getpeercert()508 peer_cert = ssl_sock.getpeercert()
506 match_hostname(peer_cert, self.host)509 match_hostname(peer_cert, host)
507510
508 # Wrap the ssl socket before anybody use it511 # Wrap the ssl socket before anybody use it
509 self._wrap_socket_for_reporting(ssl_sock)512 self._wrap_socket_for_reporting(ssl_sock)
510513
=== 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 @@
32.. Fixes for situations where bzr would previously crash or give incorrect32.. Fixes for situations where bzr would previously crash or give incorrect
33 or undesirable results.33 or undesirable results.
3434
35* Connecting with HTTPS via HTTP now correctly uses the host name of the
36 destination rather than the proxy when checking certificates.
37 (Martin Packman, #944696)
38
35* Fixed merge tool availability checking and invocation to search the39* Fixed merge tool availability checking and invocation to search the
36 Windows App Path registry in addition to the PATH. (Gordon Tyler, #939605)40 Windows App Path registry in addition to the PATH. (Gordon Tyler, #939605)
3741

Subscribers

People subscribed via source and target branches