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
1=== modified file 'bzrlib/transport/http/_urllib2_wrappers.py'
2--- bzrlib/transport/http/_urllib2_wrappers.py 2012-02-21 12:16:02 +0000
3+++ bzrlib/transport/http/_urllib2_wrappers.py 2012-03-12 17:22:18 +0000
4@@ -476,9 +476,12 @@
5 # FIXME JRV 2011-12-18: Use location config here?
6 config_stack = config.GlobalStack()
7 cert_reqs = config_stack.get('ssl.cert_reqs')
8+ if self.proxied_host is not None:
9+ host = self.proxied_host.split(":", 1)[0]
10+ else:
11+ host = self.host
12 if cert_reqs == ssl.CERT_NONE:
13- trace.warning("Not checking SSL certificate for %s: %d",
14- self.host, self.port)
15+ trace.warning("Not checking SSL certificate for %s", host)
16 ca_certs = None
17 else:
18 if self.ca_certs is None:
19@@ -503,7 +506,7 @@
20 raise
21 if cert_reqs == ssl.CERT_REQUIRED:
22 peer_cert = ssl_sock.getpeercert()
23- match_hostname(peer_cert, self.host)
24+ match_hostname(peer_cert, host)
25
26 # Wrap the ssl socket before anybody use it
27 self._wrap_socket_for_reporting(ssl_sock)
28
29=== modified file 'doc/en/release-notes/bzr-2.5.txt'
30--- doc/en/release-notes/bzr-2.5.txt 2012-03-12 14:44:51 +0000
31+++ doc/en/release-notes/bzr-2.5.txt 2012-03-12 17:22:18 +0000
32@@ -32,6 +32,10 @@
33 .. Fixes for situations where bzr would previously crash or give incorrect
34 or undesirable results.
35
36+* Connecting with HTTPS via HTTP now correctly uses the host name of the
37+ destination rather than the proxy when checking certificates.
38+ (Martin Packman, #944696)
39+
40 * Fixed merge tool availability checking and invocation to search the
41 Windows App Path registry in addition to the PATH. (Gordon Tyler, #939605)
42

Subscribers

People subscribed via source and target branches