Merge lp://staging/~vila/bzr/1538480-match-hostname into lp://staging/bzr

Proposed by Vincent Ladeuil
Status: Merged
Approved by: Richard Wilbur
Approved revision: no longer in the source branch.
Merged at revision: 6614
Proposed branch: lp://staging/~vila/bzr/1538480-match-hostname
Merge into: lp://staging/bzr
Diff against target: 364 lines (+75/-127)
4 files modified
bzrlib/errors.py (+1/-9)
bzrlib/tests/test_https_urllib.py (+32/-28)
bzrlib/transport/http/_urllib2_wrappers.py (+39/-90)
doc/en/release-notes/bzr-2.7.txt (+3/-0)
To merge this branch: bzr merge lp://staging/~vila/bzr/1538480-match-hostname
Reviewer Review Type Date Requested Status
Richard Wilbur Approve
Review via email: mp+284171@code.staging.launchpad.net

Commit message

Use ssl.match_hostname instead of our own.

Description of the change

Since match_hostname is now provided, use it instead of carrying an obsolete version.

To post a comment you must log in.
Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

Vincent, what you've done here is excellent and definitely an improvement over the contributed patch, exempli gratia, adding the not_ok function.

What I'm curious about is this patch requires python >= 2.7.9. Do we plan to bump our python version requirement from 2.6 to 2.7.9?

The other part of the patch checks for match_hostname in the ssl library and, if not found, imports it from backports.
1. Is that a worthwhile exercise in light of python version requirements?
2. Is backports normally available or would we change our dependencies to get it?

Revision history for this message
Vincent Ladeuil (vila) wrote :

> Vincent, what you've done here is excellent and definitely an improvement over
> the contributed patch, exempli gratia, adding the not_ok function.
>
> What I'm curious about is this patch requires python >= 2.7.9. Do we plan to
> bump our python version requirement from 2.6 to 2.7.9?

Ha, the difficult question :)

So I went to https://docs.python.org/2.6/ and saw 'last updated Oct 29, 2013'. I.e. python 2.6 didn't receive any security support for the last 2 years.

And I'm seeing various projects dropping support for 2.6 too including testtools that is a requirement for bzr tests.

All in all, I think this means that bzr itself cannot support security fixes for python 2.6.

So I dropped the obsolete copy of python 3.2 (not supported anymore either).

In turns this means that people using a bzr 2.6 AND wanting https security support needs to upgrade to python 2.7.

In other words, we attempted to support cert checking with py2.6 at best as we could, we cannot anymore. This specific feature is controlled via the 'ssl.cert_reqs' config option which can be disabled if needed.

So with this patch, py27 users get better security, py26 users will need to verify their certs by other means and use the option to disable bzr checks but won't be lured into a false sense of security.

Apart from that scenario, py26 is still supported (but not regularly tested to the best of my knowledge).

>
> The other part of the patch checks for match_hostname in the ssl library and,
> if not found, imports it from backports.

I've never heard about a python 'backports' library, may be that's specific to the distribution the patch author is using ?

> 1. Is that a worthwhile exercise in light of python version requirements?

I don't think so, if we need to invest into a porting effort, I'd rather see work around forward porting to py3 than back porting to py2.6 ;)

On the other hand, we may want to officially stop supporting py26 in the future.

> 2. Is backports normally available or would we change our dependencies to get
> it?

Not that I know of.

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

Could we then as part of this fix automatically disable the 'ssl.cert_reqs' config option if running under python 2.6 with a warning that the functionality is only available with python 2.x >= 2.7.9 or 3.x >= 3.3?

review: Needs Fixing
Revision history for this message
Vincent Ladeuil (vila) wrote :

> Could we then as part of this fix automatically disable the 'ssl.cert_reqs'
> config option if running under python 2.6 with a warning that the
> functionality is only available with python 2.x >= 2.7.9 or 3.x >= 3.3?

/me facepalms

Of course !

Done.

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

Thanks for that last revision. I think a descriptive warning is significantly better than an exception!
+2

review: Approve
Revision history for this message
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Revision history for this message
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.