Verify HTTPS certs in OpenStack provider

Verify HTTPS certs in OpenStack provider

First pass at using the txaws cert validation logic for the OpenStack client
backend as well. It's a little messy as the client still needs cleaning up
properly, but the basic behaviours should be about right.

I've aimed at making the output a bit less spammy than the EC2 provider
equivalent. Generally if the option is unset, the user doesn't want to
hear about it four times in different ways.

This needs tests for the logging etc.

https://code.launchpad.net/~gz/juju/os_https_certs_1026102/+merge/118602

(do not edit description out of merge proposal)

[gz, 2012-08-07 17:15:59]

Please take a look.

[jimbaker, 2012-08-21 01:37:04]

Needs fixing. There appears to be a security hole in reversed logic: using curl
-k, also known as curl --insecure, when ssl-hostname-verification is true. In
addition, this branch needs some additional work on unit tests to ensure no
regressions.

https://codereview.appspot.com/6443098/diff/1/juju/environment/config.py
File juju/environment/config.py (right):

https://codereview.appspot.com/6443098/diff/1/juju/environment/config.py#newc...
juju/environment/config.py:79: "ssl-hostname-verification"]),
ssl-hostname-verification should default to true per Clint's comment earlier.
Given the other fixes that are needed, probably just best to do this change in
this branch too.

https://codereview.appspot.com/6443098/diff/1/juju/providers/openstack/_ssl.py
File juju/providers/openstack/_ssl.py (right):

https://codereview.appspot.com/6443098/diff/1/juju/providers/openstack/_ssl.p...
juju/providers/openstack/_ssl.py:19: return
VerifyingContextFactory(hostname).getContext()
Obviously some refactoring would be preferred, but I find this usage OK,
especially given that this codebase will be replaced by the Go version - not
likely any tech debt will be incurred before then given txaws stability.

https://codereview.appspot.com/6443098/diff/1/juju/providers/openstack/client.py
File juju/providers/openstack/client.py (right):

https://codereview.appspot.com/6443098/diff/1/juju/providers/openstack/client...
juju/providers/openstack/client.py:44: WebVerifyingContextFactory = None
This conditional import should be removed: we want the import to fail if there
are any problems, especially in any tests. If I simply remove the .ssl module
from juju.providers.openstack, all tests still pass. So now I don't know that we
no longer have working ssl hostname verification, perhaps because of txaws
upstream modification.

https://codereview.appspot.com/6443098/diff/1/juju/providers/openstack/client...
juju/providers/openstack/client.py:121: response = yield agent.request(method,
url, headers, body)
This along with the curl usage in juju.providers.openstack.launch is the heart
of this proposal. Two things:

1. Please say something to this effect - at first glance and without consulting
the Twisted docs, it's a bit mysterious what's going on, that this ensures
certificate verification for such agent requests. On the other hand, it's done
in one place, so that's nice.

2. We really need to verify that this verification is happening. I don't think
we need to mock out the exact SSL negotiation, that's really a function of
Twisted and specific txaws support. However, we do need to verify that
ssl-hostname-verification: true does actually employ this factory in the tests.
There's an example of such testing in juju.providers.ec2.tests.test_utils

https://codereview.appspot.com/6443098/diff/1/juju/providers/openstack/launch.py
File juju/providers/openstack/launch.py (right):

https://codereview.appspot.com/6443098/diff/1/juju/providers/openstack/launch...
juju/providers/openstack/launch.py:73: curl = "curl -k"
Shouldn't this logic be reversed?! -k (long option is --insecure) implies no
hostname verification.

https://codereview.appspot.com/6443098/diff/1/juju/providers/openstack/launch...
juju/providers/openstack/launch.py:75: curl, filestorage.get_url(id_name),))
Additionally, we should be verifying the cloud-init as rendered so ensure curl
is called as desired in the env config setup. Examples of this can be seen in
providers.ec2.tests.test_launch in usage of mock_launch, which then compares
against sample cloud-init setups (see for example launch_cloud_init).

[gz, 2012-08-21 15:30:27]

Thanks for the review Jim.

> ssl-hostname-verification should default to true per Clint's comment earlier.
> Given the other fixes that are needed, probably just best to do this change in
> this branch too.

Without also changing the EC2 provider default?

> Obviously some refactoring would be preferred, but I find this usage OK,
> especially given that this codebase will be replaced by the Go version - not
> likely any tech debt will be incurred before then given txaws stability.

The change should probably be upstreamed to txaws, but txaws should probably
upstream their cert validation code to twisted...

> This conditional import should be removed: we want the import to fail if there
> are any problems, especially in any tests. If I simply remove the .ssl module
> from juju.providers.openstack, all tests still pass. So now I don't know that
we
> no longer have working ssl hostname verification, perhaps because of txaws
> upstream modification.

This is just the lack of unit tests, no? Without the conditional import, we
can't support older txaws versions, which was apparently important when
implementing the EC2 version of this. I've asked Clint if that's still the case.

> This along with the curl usage in juju.providers.openstack.launch is the heart
> of this proposal. Two things:
> 
> 1. Please say something to this effect - at first glance and without
consulting
> the Twisted docs, it's a bit mysterious what's going on, that this ensures
> certificate verification for such agent requests. On the other hand, it's done
> in one place, so that's nice.
> 
> 2. We really need to verify that this verification is happening. I don't think
> we need to mock out the exact SSL negotiation, that's really a function of
> Twisted and specific txaws support. However, we do need to verify that
> ssl-hostname-verification: true does actually employ this factory in the
tests.
> There's an example of such testing in juju.providers.ec2.tests.test_utils

So, I have a separate refactoring branch that makes testing easier and
rearranges this twisted http code. I'll add a comment here and look at testing
again to see what progress I can make here without blocking on that merge.

> Shouldn't this logic be reversed?! -k (long option is --insecure) implies no
> hostname verification.

It's the correct way round, when ssl-hostname-verification=True 'curl' is used
which defaults to failing (unhelpfully, the way cloud-init is using it) if the
cert doesn't match.

I'll stick an attribute on the provider for this boolean rather than rechecking
the config which should make it the code a little easier to read.

> Additionally, we should be verifying the cloud-init as rendered so ensure curl
> is called as desired in the env config setup. Examples of this can be seen in
> providers.ec2.tests.test_launch in usage of mock_launch, which then compares
> against sample cloud-init setups (see for example launch_cloud_init).

Yes, there's a todo in... the bootstrap tests I think, about checking the
cloud-init stuff. I skipped over it originally as comparing against a
pre-generated text file is painful, but with a small amount of work it will be
possible to handle in a neater fashion.

[jimbaker, 2012-08-21 16:17:09]

On 2012/08/21 15:30:27, gz wrote:
> Thanks for the review Jim.

No worries, thanks for putting this important branch together!

> 
> > ssl-hostname-verification should default to true per Clint's comment
earlier.
> > Given the other fixes that are needed, probably just best to do this change
in
> > this branch too.
> 
> Without also changing the EC2 provider default?

Current plans are to change that in trunk ASAP, but not in the SRU for Precise
(unfortunately from my perspective).

> 
> > Obviously some refactoring would be preferred, but I find this usage OK,
> > especially given that this codebase will be replaced by the Go version - not
> > likely any tech debt will be incurred before then given txaws stability.
> 
> The change should probably be upstreamed to txaws, but txaws should probably
> upstream their cert validation code to twisted...

Exactly, please don't worry about it for now :)

> 
> > This conditional import should be removed: we want the import to fail if
there
> > are any problems, especially in any tests. If I simply remove the .ssl
module
> > from juju.providers.openstack, all tests still pass. So now I don't know
that
> we
> > no longer have working ssl hostname verification, perhaps because of txaws
> > upstream modification.
> 
> This is just the lack of unit tests, no? Without the conditional import, we
> can't support older txaws versions, which was apparently important when
> implementing the EC2 version of this. I've asked Clint if that's still the
case.
> 

OK, that's interesting. If the conditional import stays, we need to ensure that
the tests are not quiet without juju.providers.openstack._ssl being defined.

> > This along with the curl usage in juju.providers.openstack.launch is the
heart
> > of this proposal. Two things:
> > 
> > 1. Please say something to this effect - at first glance and without
> consulting
> > the Twisted docs, it's a bit mysterious what's going on, that this ensures
> > certificate verification for such agent requests. On the other hand, it's
done
> > in one place, so that's nice.
> > 
> > 2. We really need to verify that this verification is happening. I don't
think
> > we need to mock out the exact SSL negotiation, that's really a function of
> > Twisted and specific txaws support. However, we do need to verify that
> > ssl-hostname-verification: true does actually employ this factory in the
> tests.
> > There's an example of such testing in juju.providers.ec2.tests.test_utils
> 
> So, I have a separate refactoring branch that makes testing easier and
> rearranges this twisted http code. I'll add a comment here and look at testing
> again to see what progress I can make here without blocking on that merge.

Cool.

> 
> > Shouldn't this logic be reversed?! -k (long option is --insecure) implies no
> > hostname verification.
> 
> It's the correct way round, when ssl-hostname-verification=True 'curl' is used
> which defaults to failing (unhelpfully, the way cloud-init is using it) if the
> cert doesn't match.
> 
> I'll stick an attribute on the provider for this boolean rather than
rechecking
> the config which should make it the code a little easier to read.
> 

Sounds good, thanks for doublechecking.

> > Additionally, we should be verifying the cloud-init as rendered so ensure
curl
> > is called as desired in the env config setup. Examples of this can be seen
in
> > providers.ec2.tests.test_launch in usage of mock_launch, which then compares
> > against sample cloud-init setups (see for example launch_cloud_init).
> 
> Yes, there's a todo in... the bootstrap tests I think, about checking the
> cloud-init stuff. I skipped over it originally as comparing against a
> pre-generated text file is painful, but with a small amount of work it will be
> possible to handle in a neater fashion.

Right, I think some amount of testing along these lines is important for this
branch.

[gz, 2012-08-24 16:58:20]

Please take a look.

[jimbaker, 2012-08-25 14:31:28]

+1, with the two issues fixed in my comments, especially
ssl-hostname-verification in the case of an old txaws impl with no certificate
verification support.

However, both issues are trivial to fix.

Otherwise, looks good to me. I appreciate the work on testing.  In particular,
both curl vs curl -k and setting of the contextFactory is tested, even though as
the code notes, it's complicated due to the multiple layers impacted. The use of
mocks here seem to keep that complexity to a minimum.

https://codereview.appspot.com/6443098/diff/8001/juju/providers/openstack/tes...
File juju/providers/openstack/tests/test_provider.py (right):

https://codereview.appspot.com/6443098/diff/8001/juju/providers/openstack/tes...
juju/providers/openstack/tests/test_provider.py:12: 
The imports are following some other convention than what's been established for
Juju.

https://codereview.appspot.com/6443098/diff/8001/juju/providers/openstack/tes...
juju/providers/openstack/tests/test_provider.py:182:
self.assertIn("txaws.client.ssl unavailable", log.getvalue())
Raise an error, not just a warning, if there is no txaws support for certs (old
txaws) but ssl-hostname-verification is true. The user can always change their
setting to false to override in this case. To me, this best fulfills the intent
of the ssl-hostname-verification setting.

[gz, 2012-08-28 11:30:12]

Will push changes requested shortly.

https://codereview.appspot.com/6443098/diff/8001/juju/providers/openstack/tes...
File juju/providers/openstack/tests/test_provider.py (right):

https://codereview.appspot.com/6443098/diff/8001/juju/providers/openstack/tes...
juju/providers/openstack/tests/test_provider.py:12: 
On 2012/08/25 14:31:28, jimbaker wrote:
> The imports are following some other convention than what's been established
for
> Juju.

Grumble grumble. I need something like this, otherwise I can't do below:

    self.patch(client, "WebVerifyingContextFactory", obj)

https://codereview.appspot.com/6443098/diff/8001/juju/providers/openstack/tes...
juju/providers/openstack/tests/test_provider.py:182:
self.assertIn("txaws.client.ssl unavailable", log.getvalue())
On 2012/08/25 14:31:28, jimbaker wrote:
> Raise an error, not just a warning, if there is no txaws support for certs
(old
> txaws) but ssl-hostname-verification is true. The user can always change their
> setting to false to override in this case. To me, this best fulfills the
intent
> of the ssl-hostname-verification setting.

Okay. Again, this is a divergence from the EC2 provider behaviour though.

[gz, 2012-08-28 12:58:07]

On 2012/08/28 11:30:12, gz wrote:
> 
> Okay. Again, this is a divergence from the EC2 provider behaviour though.

Having looked at this, I'll flip the behaviour for both providers in a separate
branch after these have landed. That will make life a little easier by avoiding
conflicts with the dependent branches.