Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert commit 3ba3d39. #342

Closed
wants to merge 1 commit into from
Closed

Conversation

bfabio
Copy link
Contributor

@bfabio bfabio commented Feb 21, 2017

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).

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
Copy link
Contributor Author

bfabio commented Feb 21, 2017

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

@drybjed
Copy link
Member

drybjed commented Feb 21, 2017

@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
Copy link
Member

ypid commented Feb 21, 2017

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 debops/ansible-php#20 (comment)? 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
Copy link
Contributor Author

bfabio commented Feb 21, 2017

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 debops/ansible-apache#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
Copy link
Member

ypid commented Feb 21, 2017

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! ansible/ansible#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
Copy link
Contributor Author

bfabio commented Feb 21, 2017

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
Copy link
Member

ypid commented Feb 21, 2017

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
Copy link
Contributor Author

bfabio commented Feb 21, 2017

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
Copy link
Member

ypid commented Feb 21, 2017

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
Copy link
Contributor Author

bfabio commented Feb 23, 2017

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
Copy link
Member

drybjed commented Feb 27, 2017

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
Copy link
Contributor Author

bfabio commented Feb 27, 2017

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

@drybjed
Copy link
Member

drybjed commented Feb 27, 2017

@bfabio Sounds like a good idea.

@drybjed
Copy link
Member

drybjed commented Feb 27, 2017

@bfabio Does #344 sound good?

@bfabio
Copy link
Contributor Author

bfabio commented Feb 27, 2017

@drybjed Perfect.

@bfabio bfabio closed this Feb 27, 2017
@ypid
Copy link
Member

ypid commented Feb 27, 2017

@drybjed Sounds good to me. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants