search

jhaals / ansible-vault

ansible lookup plugin for secrets stored in Vault(by HashiCorp)
BSD 3-Clause "New" or "Revised" License
347 stars 65 forks source link

don't create an SSL context if not needed #26

Closed richfromm closed 8 years ago

richfromm commented 8 years ago

Specifically, don't create one (and don't use it) if neither VAULT_CACERT or VAULT_CAPATH are defined.

This narrows the Python >= 2.7.9 requirement. Now earlier versions work if there is no SSL context, because we're not calling ssl.create_default_context(), and we're not passing a context attribute to urllib2.urlopen(), which is also new as of 2.7.9: https://docs.python.org/2/library/urllib2.html#urllib2.urlopen

richfromm commented 8 years ago

This replaces #21

richfromm commented 8 years ago

Will you have a chance to review this soonish? I have two more pending pull requests that are waiting on resolution of this.

richfromm commented 8 years ago

The tl;dr answer is slightly yes, but mostly no.

Now for a longer answer...

The primary purpose of this commit is exactly what it says, "don't create an SSL context if not needed." If neither VAULT_CACERT nor VAULT_CAPATH are set, just don't do it. I suppose you might be able to create a context where cafile and capath are both None, and pass that to urllib2.urlopen() and have it work, but I'm not sure. I haven't tested that.

To answer what might be a question of yours as to why would you have neither VAULT_CACERT or VAULT_CAPATH, one possibility is if the Vault instance is sitting behind a load balancer, and only the load balancer directly takes connections from the outside. In this case, while the connection to the load balancer will be using HTTPS, it's entirely possible to have TLS be disabled between the load balancer and Vault, and just be straight HTTP. Yes, you will get some additional security by encrypting this as well, but it's not strictly needed.

I view the fact that now part of this works with Python < 2.7.9 as a side effect, and not the primary rationale. Note that when I added the 2.7.9 check (in #24), I did not just add a blanket version check then fail. Instead, it catches the specific narrow case that will happen with Python < 2.7.9, and improves the error reporting for that case. What's going on here is that the scenario under which that can happen is now narrowed. So sometimes it will work with Python < 2.7.9, if you happen to not be setting either VAULT_CACERT or VAULT_CAPATH.

As to what to or not to support, I can certainly see your argument that a piece of community supported free software has limited resources and can't support everything. But there is a difference, IMHO, between not wanting to jump through extra hoops to support older versions, vs. actively working to limit support of those old versions.

And Python < 2.7.9 isn't necessarily that old depending on your perspective. Ubuntu 14.04 LTS has packages for 2.7.5 and 2.7.6 only. While it may not be the most recent LTS release (that would be 16.04), it's only one removed, and it's also not the oldest still supported LTS release (that would be 12.04, which will still be supported until April 2017). 14.04 will be supported until April 2019, which is quite a while from now. I'm not saying that you necessarily need to be supporting it for that entire time, but it's not unreasonable for large organizations to not yet be on the most recent Ubuntu release yet, so I don't think considering it for the sake of support is so out of the question. (Yes, I am aware of other non-system based means of updating Python, like pyenv.)

I guess the bottom line is that I think there are multiple reasons for considering the patch, and I'm not sure if "2.7.9 is old" is a good enough reason to reject it. But obviously it's ultimately your call.

jhaals commented 8 years ago

Such a long answer :)

I'm sure we can work this out with some modifications. I hope you're okay with a short reply to your text. I think python 2.7.9 is old from a desktop perspective where most people run ansible however some people might use ansible-pull to trigger runs. It's always a problem supporting old versions and I hope ansible start bundling the python interpreter in the future similar to what chef/puppet is doing.

richfromm commented 8 years ago

See comments at https://github.com/jhaals/ansible-vault/pull/26/files/3f0e06c59c17306c3dd35a3a1e47acf3ef10965f and two new revs.

Just to give you a little perspective on where I'm coming from, my desktop is Ubuntu 14.04 LTS because that matches the state of the majority of our servers in production. We will eventually move to 16.04, but we're nowhere close to that yet. One of the difficulties is that the 14.04 => 16.04 transition includes systemd, and accomodating that creates some amount of work. Since 14.04 will still be supported for many years to come, we're in no rush.

jhaals commented 8 years ago

Thanks for the update and clarification