debops / debops-playbooks

Ansible playbooks used by DebOps project
GNU General Public License v3.0
489 stars 88 forks source link

Revert commit 3ba3d39. #342

Closed bfabio closed 7 years ago

bfabio commented 7 years ago

Don't check for vulnerable ansible versions, it's not debops' job to make sure other packages don't have known vulnerabilities, but the distribution's.

Furthermore, some distributions backport the security fixes without changing the version, resulting in a false positive (Debian itself being one of those).

bfabio commented 7 years ago

I know the heart is in the right place, but I don't think this is the right solution.

drybjed commented 7 years ago

@ypid We discussed this a bit on the IRC channel, but since it was your idea I suggested a thread about this on GitHub instead.

From my point of view this is an issue focused on Ansible Controller host. DebOps doesn't really do much about this host since it can be used on on other OSes than Debian/Ubuntu. My assumption is that the system administrator knows about security issues of the software he/she uses and knows how to maintain it. Therefore I would assume that the Ansible Controller is a secure and trusted environment and DebOps itself doesn't need to enforce security on it, unlike on the remote hosts.

On the other hand the Ansible CVE issues were related to attacks against Ansible Controller from remote hosts, so that shouldn't be taken into account. Is doing this through the main playbook and checking Ansible version a proper way to do it? I'm not really sure. Currently the playbook allows to sidestep this check via the Ansible --skip-tags option, but perhaps that's not enough?

Perhaps a separate playbook that manages Ansible Controller, OS-independent and focused on Ansible deployment might be a better solution? This could be a supplement to the debops.debops role, but also support other OSes like MacOS X.

@ganto might also be interested in this thread.

ypid commented 7 years ago

Furthermore, some distributions backport the security fixes without changing the version, resulting in a false positive (Debian itself being one of those).

Well, Debian usually does this but not in this case (example: CVE-2016-8614). That is one reason why I added the assertion.

My assumption is that the system administrator knows about security issues of the software he/she uses and knows how to maintain it.

I don’t agree with your assumption. Are you really sure about this? Debian and Ubuntu both have vulnerable Ansible versions in their default repos and sysadmins seem to use them without much checking. Sure, that should be fixed on the distribution level but currently it is not so why not help users until then?

Remember https://github.com/debops/ansible-php/pull/20#issuecomment-276629927? Also, with projects like debops-wordpress our target audience is not necessarily the topical sysadmin reading the Security Bug Tracker and so on all day. We should not make such assumptions.

I guess most DebOps users run DebOps on their main machine without effective isolation. One run against your random VPS would be enough to compromise your entire digital life/work environment. This all can be made much more difficult if people would stop running versions with known vulnerability (plus effective isolation, just to be on the safe side)! Seems like the assertion is a small price to play for that?

About DebOps not managing the Ansible controller by default. Sure, the assertion does not change that, it just checks if the basic requirements are met. This is necessary so that DebOps can comply with its goals (see also CII Best Practices). The thing with moving it to a separate playbook or to debops.debops or so, that is just optimizing and can be discussed. But note that the way the assertion is implement is to terminate Ansible before it should even connect to remote hosts which could exploit it.

@bfabio You proposed the revert. What exactly is your problem with it? You can already skip the assertion if you have a good reason to do so. What would be a good reason to remove the assertion all together in the light of the notes I wrote in the changelog and the related discussions? Which Ansible version are you running?

Related to: #338

bfabio commented 7 years ago

Well, Debian usually does this but not in this case (example: CVE-2016-8614). That is one reason why I added the assertion.

I think the reason is it would be too much work to backport the fix to jessie, since its ansible version is ancient (and it can't be upgraded being Debian stable). To work around this, the package's maintainer is providing a backported package: https://packages.debian.org/jessie-backports/ansible

@bfabio What would be a good reason to remove the assertion all together in the light of the notes I wrote in the changelog and the related discussions?

The reason is because of this bug (which is also the issue in https://github.com/debops/ansible-apache/issues/12), a Debian user can't use debops, and the safe option for stable, testing and sid to downgrade to the ansible 2.2.0 package (which does contain the security fixes) is not viable because of the security check in debops.

ypid commented 7 years ago

To work around this, the package's maintainer is providing a backported package

I am aware of that.

Debian user can't use debops, and the safe option for stable, testing and sid to downgrade to the ansible 2.2.0?

That is not a good reason! https://github.com/ansible/ansible/issues/20253 is fixed already.

So you are actually proposing that (Debian and Ubuntu) users should run 2.2.0? Oh well, then those users will have to install manually as already mentioned in changelog. Still better than what you are proposing.

bfabio commented 7 years ago

That is not a good reason! ansible/ansible#20253 is fixed already.

Fixed in master, but not even released yet. We can't expect everyone to live on ansible@master.

So you are actually proposing that (Debian and Ubuntu) users should run 2.2.0? Oh well, then those users will have to install manually as already mentioned in changelog. Still better than what you are proposing.

I'm proposing a Debian user should be able to use the ansible packages, which I think it's better to force them to install stuff from source (even if they are not the last packages).

Also consider the security check is factually wrong with those Debian packages. 2.2.0 on Debian is not vulnerable.

ypid commented 7 years ago

Fixed in master, but not even released yet. We can't expect everyone to live on ansible@master.

But we can expect users to run vulnerable versions, exploitable from managed remote hosts? Yeah, sounds much better. Anyway, I don’t expect that. As suggested, stable-2.1 is sufficient.

I'm proposing a Debian user should be able to use the ansible packages, which I think it's better to force them to install stuff from source (even if they are not the last packages).

Agreed where possible.

Also consider the security check is factually wrong with those Debian packages. 2.2.0 on Debian is not vulnerable.

Makes sense because 2.2.0 is not in Debian, it has already been upgraded to 2.2.1 which is not vulnerable. Or am I missing something?

https://packages.debian.org/search?searchon=names&keywords=ansible

bfabio commented 7 years ago

But we can expect users to run vulnerable versions, exploitable from managed remote hosts? Yeah, sounds much better.

Are we going to check for vulnerable python versions as well? What about libc?

BTW, I don't get the snark and the hostility at all. This pull request was meant to start the conversation about the issue, it was not a personal attack. You're not your code.

I exposed the whole rationale behind the proposal, you can close it if has no merit.

ypid commented 7 years ago

Are we going to check for vulnerable python versions as well? What about libc?

I get your point but those are properly handled by the distributions already.

BTW, I don't get the snark and the hostility at all.

Sorry for that. I do not interpret this as a personal attack, don’t worry. I just think there are good reasons for the assertion which I might explained/defended a bit to much. I always try to be reasonable and open to discussion.

bfabio commented 7 years ago

Sorry for that. I do not interpret this as a personal attack, don’t worry

No offense taken, I'm happy to hear that.

drybjed commented 7 years ago

So, to resolve this before moving with other PRs for this repository, how about this solution? The current check for Ansible version stays in the playbook because it already can be skipped if necessary via Ansible --skip-tags option. Ansible is direct dependency for debops-playbooks, so relying on its version for security sounds good to me. Python is an indirect dependency for the playbook (not the scripts, obviously), so not checking for it in the playbook sounds reasonable, OS should handle that.

@ypid, @bfabio, do you agree?

bfabio commented 7 years ago

@drybjed That's fine with me, but I would at least add a mention to --skip-tags in the error message.

drybjed commented 7 years ago

@bfabio Sounds like a good idea.

drybjed commented 7 years ago

@bfabio Does https://github.com/debops/debops-playbooks/pull/344 sound good?

bfabio commented 7 years ago

@drybjed Perfect.

ypid commented 7 years ago

@drybjed Sounds good to me. Thanks.