Code review comment for lp:~jelmer/bzr/urllib-verifies-ssl-certs

Revision history for this message
Martin Pool (mbp) wrote :

On 4 November 2011 14:03, Jelmer Vernooij <email address hidden> wrote:
> Jelmer Vernooij has proposed merging lp:~jelmer/bzr/urllib-verifies-ssl-certs into lp:bzr.
>
> Requested reviews:
>  bzr-core (bzr-core)
> Related bugs:
>  Bug #125055 in Bazaar: "defaulting to pycurl doesn't make sense anymore"
>  https://bugs.launchpad.net/bzr/+bug/125055
>  Bug #651161 in Bazaar: "bzr fails to verify ssl validity in https connections - by default --> as pycurl isn't a dep only a suggestion "
>  https://bugs.launchpad.net/bzr/+bug/651161
>
> For more details, see:
> https://code.launchpad.net/~jelmer/bzr/urllib-verifies-ssl-certs/+merge/81227
>
> Add support for verifying SSL certificates when using urllib.

That's great, thanks.

> This branch does actually work, but it's got some open ends:
>
> * The location with the CAs to trust differs per system. Rather than hardcoding
>  a list, I think we should have a configuration option that allows the user
>  to specify what CA list they want to verify against.
>
>  This option could then also be pointed at a user-specific list
>  (rather than a system wide list). And packagers can put in the default
>  for their distribution.
>
>  Where do I obtain this option from, which config stack ?

[needsfixing] I think the global stack would be a good place to start
- it would seem like a kind of contrived case to want different values
on a smaller scope. We can always change it later.

> * We probably want some way to specify that self-signed certificates are ok.
>  Probably also a new option? We should probably still warn the user
>  when a self-signed certificate is being trusted.

+1 to a new option and to always warning.

Though perhaps it would be equivalent and easier to just have an
option saying "don't do validation."

> * There are no additional tests. There probably should be some, at least for
>  e.g. the match_hostname helper.

It would be nice, if it's not too hard, to actually test against an
untrusted cert, or at least to do monkeypatching to make it look like
we got an invalid cert.

[needsfixing] I'd like a unit test for match_hostname covering the
various cases.

> * I have the feeling that I'm missing something. This was way too easy.
>  What is it?

I haven't read the rfc but it looks plausible. You could wait for a
review from vila, who has probably touched this most, or robert who is
an http nerd.

I wonder if this needs a change to Windows or Mac packaging to make
sure the certs are there. Or, possibly we can get them from the
system's keyring in some way.

« Back to merge proposal