[PATCH] Makes https+pycurl urls works with self-certified hosts

[John Arbash Meinel]

Vincent LADEUIL wrote:
>>>>>> "jam" == John Arbash Meinel <john at arbash-meinel.com> writes:
> 
> <snip/>
> 
>     >>  <cough> Setting HHTPS_PROXY (note caps) works. <blush>
> 
>     jam> Well, I hope it is 'HTTPS_PROXY' :)
> 

...

v- The first line of this change seems accidental, you don't really want
the extra whitespace, do you?

Also, you are again using Tabs, rather than space characters.

>          ua_str = 'bzr/%s (pycurl)' % (bzrlib.__version__,)
>          curl.setopt(pycurl.USERAGENT, ua_str)
>          curl.setopt(pycurl.HTTPHEADER, headers)
> -        curl.setopt(pycurl.FOLLOWLOCATION, 1) # follow redirect responses
> +        curl.setopt(pycurl.FOLLOWLOCATION,  1) #  follow redirect responses
> +	# FIXME: The  following allows the  use of self-certified
> +	# hosts.  This  should not be activated  without the user
> +	# understanding  the  consequences (there  is  a risk  to
> +	# connect  to  rogue  hosts).  This should  certainly  be
> +	# handled better but  that keep the relevant informations
> +	# at the right place for now.
> +	if os.getenv('BZR_ACCEPT_SELF_CERTIFIED_HOSTS') is not None:
> +	    curl.setopt(pycurl.SSL_VERIFYHOST, 1)
> +	    curl.setopt(pycurl.SSL_VERIFYPEER, False)

I think it would be better to keep a list somewhere, of what hosts have
been accepted. Rather than just blindly accepting all self-certified
hosts. This has pretty big implications for man-in-the-middle attacks.
If you allow self-certified across the board, than a man-in-the-middle
has no problem inserting themselves, and the user won't even be alerted
to the fact that the certified host's key has changed.

So I would prefer a user prompt (through bzrlib.ui.ui_factory) in the
case that the self certificate fails.

I'm not sure where you would exactly need to put this check. But I think
it would end up being around one of the get/has handlers. Because you
don't know that it fails to validate until you have actually made a
request. But then if it does fail, you catch the correct exception (I
would guess CURLE_SSL_PEER_CERTIFICATE (51) look in _pycurl_errors.py).

Now, in the short term, it might be okay to expose a big thunk like
this. But we can make it slightly better with:

BZR_ACCEPT_SELF_CERTIFIED_HOSTS = 'host1:host2:host3:host4'

And then internally you use:

hosts = os.getenv('BZR_ACCEPT_SELF_CERTIFIED_HOSTS', '').split(':')
if self._host in hosts:
  curl.setopt(...)

So final feeling:

1) You need to get rid of the tabs
2) If you use an env var, have it explicitly list the allowed hosts. It
might also be reasonable to put this into ~/.bazaar/bazaar.conf, and
then you can access it with a_config = bzrlib.config.GlobalConfig().
Though it is a small layering violation to have a Transport know about
bzrlib.config. We've never really stated how we want to handle global
state information like this.
3) I would prefer handling this on a case-by-case, with a user-prompt.
But (2) is reasonably secure, since you have to explicitly enable a host
as being okay for self certified. It still leaves you open to
man-in-the-middle on that host, but at least it doesn't open you up to
man-in-the-middle on every other host.

John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060821/5c360526/attachment.pgp