Enable ssl-hostname-verification config option for ec2 provider

Enable ssl-hostname-verification config option for ec2 provider

Enable ssl-hostname-verification config option for ec2 provider

https://code.launchpad.net/~clint-fewbar/juju/add-ssl-cert-verification/+merge/99772

(do not edit description out of merge proposal)

[SpamapS, 2012-03-28 16:07:39]

Please take a look.

[hazmat, 2012-03-28 16:26:45]

lgtm, thanks

https://codereview.appspot.com/5934054/diff/1/juju/providers/ec2/__init__.py
File juju/providers/ec2/__init__.py (right):

https://codereview.appspot.com/5934054/diff/1/juju/providers/ec2/__init__.py#...
juju/providers/ec2/__init__.py:12: have_txaws_ssl = False
This should import the value already defined in ec2.utils

https://codereview.appspot.com/5934054/diff/1/juju/providers/ec2/__init__.py#...
juju/providers/ec2/__init__.py:44:
self._service.ec2_endpoint.ssl_hostname_verification=True
Shouldn't this default to true

[hazmat, 2012-03-28 16:34:17]

following up from irc to be added, log if not verifying hosts, and generate new
default configs with ssl verify on.

[SpamapS, 2012-03-28 18:25:27]

Please take a look.

[hazmat, 2012-03-28 18:32:42]

the env schema work needs some verification, and some other minors, else LGTM

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

https://codereview.appspot.com/5934054/diff/13/juju/environment/config.py#new...
juju/environment/config.py:56: Constant(True),
i'm pretty sure this doesn't work, the values come in as a string not a bool,
definitely could use a test.

https://codereview.appspot.com/5934054/diff/13/juju/providers/ec2/__init__.py
File juju/providers/ec2/__init__.py (right):

https://codereview.appspot.com/5934054/diff/13/juju/providers/ec2/__init__.py...
juju/providers/ec2/__init__.py:8: from .utils import have_txaws_ssl
hmm.. so i would have this define txaws_ssl = ssl or False in utils, then you
don't have to define or import it again right below.

https://codereview.appspot.com/5934054/diff/13/juju/providers/ec2/__init__.py...
juju/providers/ec2/__init__.py:42: if have_txaws_ssl and
self.config.get("ssl-hostname-verification", False):
this also needs a check that the uri endpoint if specified has https before it
enables the cert check. if its not https, then the warnings below are to little
purpose.

https://codereview.appspot.com/5934054/diff/13/juju/providers/ec2/__init__.py...
juju/providers/ec2/__init__.py:51: if not secure_endpoints:
its more than just cloud image lookup, isn't this also turning api endpoint cert
checking, ie not passing creds to a man in the middle.

[SpamapS, 2012-03-28 19:30:54]

https://codereview.appspot.com/5934054/diff/13/juju/providers/ec2/__init__.py
File juju/providers/ec2/__init__.py (right):

https://codereview.appspot.com/5934054/diff/13/juju/providers/ec2/__init__.py...
juju/providers/ec2/__init__.py:42: if have_txaws_ssl and
self.config.get("ssl-hostname-verification", False):
On 2012/03/28 18:32:42, hazmat wrote:
> this also needs a check that the uri endpoint if specified has https before it
> enables the cert check. if its not https, then the warnings below are to
little
> purpose.

Good point. Adding that to make the warnings more representative of the reality
of why we need to warn you.

https://codereview.appspot.com/5934054/diff/13/juju/providers/ec2/__init__.py...
juju/providers/ec2/__init__.py:51: if not secure_endpoints:
On 2012/03/28 18:32:42, hazmat wrote:
> its more than just cloud image lookup, isn't this also turning api endpoint
cert
> checking, ie not passing creds to a man in the middle.

True that this log message is not clear what is not secure. Creds are actually
safe, as we're just passing the HMAC of the secret key and the message contents.
Replays are mitigated by the Expires or Timestamp portion of the signature,
which txaws always seems to make use of.

I will separate the log messages for S3, EC2, and the ubuntu cloud images so
that it is clear what is going on.

[SpamapS, 2012-03-28 19:31:43]

Please take a look.

[hazmat, 2012-03-28 22:56:58]

LGTM, some minors.

https://codereview.appspot.com/5934054/diff/4004/juju/providers/ec2/__init__.py
File juju/providers/ec2/__init__.py (right):

https://codereview.appspot.com/5934054/diff/4004/juju/providers/ec2/__init__....
juju/providers/ec2/__init__.py:42:
self._service.ec2_endpoint.ssl_hostname_verification=True
style syntax is space between equals on assignment statements. we have a make
check in the Makefile but it needs JUJU_TRUNK env defined and pyflakes & pep8
installed

https://codereview.appspot.com/5934054/diff/4004/juju/providers/ec2/tests/tes...
File juju/providers/ec2/tests/test_provider.py (right):

https://codereview.appspot.com/5934054/diff/4004/juju/providers/ec2/tests/tes...
juju/providers/ec2/tests/test_provider.py:12: have_txaws_ssl = False
The usage here is fine.. but what i was trying to get to is
having this in utils, and just have the other places (including tests) import
it.

try:
  from txaws.client import txaws_ssl
except ImportError:
  txaws_ssl = None


if also becomes easier to test the absence of the value via
self.patch(utils, "txaws_ssl", None) in a test which bypasses mocker. 

your call if you want to do it or not

https://codereview.appspot.com/5934054/diff/4004/juju/providers/ec2/tests/tes...
File juju/providers/ec2/tests/test_utils.py (right):

https://codereview.appspot.com/5934054/diff/4004/juju/providers/ec2/tests/tes...
juju/providers/ec2/tests/test_utils.py:12: from txaws.client.ssl import
VerifyingContextFactory
nitpick, it probably makes sense to import this from utils, else its requiring a
txaws with ssl support to run tests.

[SpamapS, 2012-03-29 23:34:17]

Minors all addressed in next push

https://codereview.appspot.com/5934054/diff/4004/juju/providers/ec2/__init__.py
File juju/providers/ec2/__init__.py (right):

https://codereview.appspot.com/5934054/diff/4004/juju/providers/ec2/__init__....
juju/providers/ec2/__init__.py:42:
self._service.ec2_endpoint.ssl_hostname_verification=True
On 2012/03/28 22:56:58, hazmat wrote:
> style syntax is space between equals on assignment statements. we have a make
> check in the Makefile but it needs JUJU_TRUNK env defined and pyflakes & pep8
> installed

Nice, I hadn't really looked closely into those, but will start running them
pre-commit. :)

https://codereview.appspot.com/5934054/diff/4004/juju/providers/ec2/__init__....
juju/providers/ec2/__init__.py:42:
self._service.ec2_endpoint.ssl_hostname_verification=True
On 2012/03/28 22:56:58, hazmat wrote:
> style syntax is space between equals on assignment statements. we have a make
> check in the Makefile but it needs JUJU_TRUNK env defined and pyflakes & pep8
> installed

Nice, I hadn't really looked closely into those, but will start running them
pre-commit. :)

https://codereview.appspot.com/5934054/diff/4004/juju/providers/ec2/tests/tes...
File juju/providers/ec2/tests/test_provider.py (right):

https://codereview.appspot.com/5934054/diff/4004/juju/providers/ec2/tests/tes...
juju/providers/ec2/tests/test_provider.py:12: have_txaws_ssl = False
On 2012/03/28 22:56:58, hazmat wrote:
> The usage here is fine.. but what i was trying to get to is
> having this in utils, and just have the other places (including tests) import
> it.
> 
> try:
>   from txaws.client import txaws_ssl
> except ImportError:
>   txaws_ssl = None
> 
> 
> if also becomes easier to test the absence of the value via
> self.patch(utils, "txaws_ssl", None) in a test which bypasses mocker. 
> 
> your call if you want to do it or not

Switched to import. Makes sense. I left out 'txaws' though, since it just
matters that ssl is available, not that it is specifically txaws.

https://codereview.appspot.com/5934054/diff/4004/juju/providers/ec2/tests/tes...
File juju/providers/ec2/tests/test_utils.py (right):

https://codereview.appspot.com/5934054/diff/4004/juju/providers/ec2/tests/tes...
juju/providers/ec2/tests/test_utils.py:12: from txaws.client.ssl import
VerifyingContextFactory
On 2012/03/28 22:56:58, hazmat wrote:
> nitpick, it probably makes sense to import this from utils, else its requiring
a
> txaws with ssl support to run tests.

I agree.. I hadn't really ever thought to import things that were imported to
another module before.

[SpamapS, 2012-03-29 23:35:00]

Please take a look.

[hazmat, 2012-04-05 00:36:51]

On 2012/03/29 23:35:00, superamazingdude wrote:
> Please take a look.

LGTM, i'd like to see the default config that's written include the ssl check
defaulted to on.