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

connection/docker: add support for privilege escalation #55816

Merged
merged 1 commit into from May 9, 2019

Conversation

Projects
None yet
6 participants
@larsks
Copy link
Contributor

commented Apr 26, 2019

SUMMARY

As described in #53385 (and #31759), the docker connection driver did not support privilege escalation. This commit is a shameless cut-and-paste of the privilege escalation support from the local connection plugin into the docker plugin.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

docker connection plugin

ADDITIONAL INFORMATION

@larsks larsks force-pushed the larsks:bug/53385 branch from a8e99fa to 5fdf821 Apr 26, 2019

@larsks larsks changed the title connection/docker: fail with error message on sudo attempt connection/docker: add support for privilege escalation Apr 26, 2019

@bcoca bcoca removed the needs_triage label Apr 26, 2019

@larsks larsks force-pushed the larsks:bug/53385 branch from 5fdf821 to 5fef490 Apr 26, 2019

@felixfontein

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

Thanks for working on this! I have two things I'm not sure about right now:

  1. I'm not sure whether this is the best approach. The reason is that we can execute something in a container as any user we want, without having to use sudo (which might not be installed at all in the container!) or su or anything else. Simply pass --user <name|uid>[:<group|gid>] to the docker exec call. I don't really know much about connection plugins and privilege escalation with Ansible, so I'm not sure what's the best path to proceed. @agaffney @bcoca any opinions on this?

  2. I'm not sure whether this is actually a bugfix (what you claim), or a new feature. Bugfixes can be backported, new features can't.

@larsks

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

I'm not sure whether this is actually a bugfix

I really don't think there's any doubt about that. Previously, tasks attempting to use privilege escalation via become would fail and would fail in a way that failed to provide a useful error messages. That's two bugs, actually (one for the functionality and one for the bad UX).

I'm not sure whether this is the best approach.

For my purposes -- and I think the purposes of some of the folks on the linked bug reports -- being able to test privilege escalation via sudo in a containerized environment is exactly what I was looking for. For running as a different user, one can just set ansible_user and the connection driver will do the appropriate thing via the --user argument.

@WojciechowskiPiotr

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

Hi @larsks
I have a few questions:
Does it require manual entering of password for privilege escalation?
Has it been tested while connecting both to the local socket and via TCP (both SSL and non-SSL)?
Has it been tested for containers running in userspace?

@felixfontein

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

I've been going through old versions of Ansible (2.0.0, 2.1.0, 2.2.0, 2.3.0, 2.4.0, 2.5.x, 2.6.x, 2.7.x), and for none this ever worked. What probably worked (and still works) is password-less sudo (or any other become method not requiring a password).

@ansibot ansibot added the stale_ci label May 5, 2019

@felixfontein

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

@WojciechowskiPiotr local socket and TCP (both TLS and non-TLS) should make absolutely no difference, since this is on a higher level of the protocol (docker CLI is doing the talking).

@felixfontein

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

I've tested this, it seems to work. First, I created a container with the following Dockerfile called docker-user-test:latest (i.e. docker build -t docker-user-test:latest /dir/which/contains/Dockerfile):

FROM ubuntu:latest
RUN apt-get update && \ 
  apt-get -y install sudo && \
  apt-get -y dist-upgrade && \
  apt-get -y autoremove && \
  apt-get clean
RUN apt-get install -y python
RUN useradd -ms /bin/bash test_user && \
	echo "test_user:password" | chpasswd && \
	adduser test_user sudo
USER test_user
CMD /bin/bash

Then I ran the following playbook with ansible-playbook test.yml -K, password password:

- hosts: localhost
  tasks:
    - docker_container:
        image: docker-user-test:latest
        name: test
        command: /bin/sleep 1h
      register: container
    - add_host:
        name: test
- hosts: test
  connection: docker
  tasks:
    - name: Who am I? (1)
      command: whoami
      remote_user: root
      register: res
    - debug: var=res
    - name: Who am I? (2)
      become: yes
      become_method: sudo
      become_user: root
      command: whoami
      register: res
    - debug: var=res
@felixfontein

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

(Backporting this to stable-2.7 will probably involve some more work, since become plugins have only been added for 2.8.)

connection/docker: add privilege escalation support
As described in #53385 (and #31759), the docker connection driver did
not support privilege escalation. This commit is a shameless
cut-and-paste of the privilege escalation support from the `local`
connection plugin into the `docker` plugin.

Closes: #53385

@larsks larsks force-pushed the larsks:bug/53385 branch from 5fef490 to 5f0b159 May 5, 2019

@ansibot ansibot removed the stale_ci label May 5, 2019

larsks added a commit to larsks/ansible that referenced this pull request May 5, 2019

connection/docker: add privilege escalation support
As described in ansible#53385 (and ansible#31759), the docker connection driver did
not support privilege escalation. This commit is a shameless
cut-and-paste of the privilege escalation support from the `local`
connection plugin into the `docker` plugin.

This is a backport to stable-2.7 of ansible#55816.
@larsks

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

@felixfontein the backport to 2.7 is substantially similar and looks like this.

@larsks

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

@WojciechowskiPiotr:

Does it require manual entering of password for privilege escalation?

This implements support for privilege escalation via sudo similar to how it is handled in the local plugin. Whether or not it requires one to enter a password depends entirely on your sudo configuration.

Has it been tested while connecting both to the local socket and via TCP (both SSL and non-SSL)?

The mechanism is not dependent on how the docker clients connect to the server.

Has it been tested for containers running in userspace?

I'm not sure what you're asking here.

@WojciechowskiPiotr

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

@larsks I meant user namespace. But it should not be a problem more I thing about it.

@felixfontein
Copy link
Contributor

left a comment

I think this is ready for merging. It works (at least for what I tested) and definitely improves the current situation.

@WojciechowskiPiotr

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

shipit

1 similar comment
@felixfontein

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

shipit

@ansibot ansibot merged commit 61e476b into ansible:devel May 9, 2019

1 check passed

Shippable Run 121780 status is SUCCESS.
Details
@felixfontein

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

@larsks thanks for fixing this!
@agaffney @WojciechowskiPiotr thanks for reviewing this!

I'll create a backport for stable-2.8. @larsks since you already prepared that, can you create a backport for stable-2.7?

felixfontein added a commit to felixfontein/ansible that referenced this pull request May 9, 2019

connection/docker: add privilege escalation support (ansible#55816)
As described in ansible#53385 (and ansible#31759), the docker connection driver did
not support privilege escalation. This commit is a shameless
cut-and-paste of the privilege escalation support from the `local`
connection plugin into the `docker` plugin.

Closes: ansible#53385
(cherry picked from commit 61e476b)
@larsks

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

@felixfontein sure, I'll post the 2.7 backport.

larsks added a commit to larsks/ansible that referenced this pull request May 9, 2019

connection/docker: add privilege escalation support
As described in ansible#53385 (and ansible#31759), the docker connection driver did
not support privilege escalation. This commit is a shameless
cut-and-paste of the privilege escalation support from the `local`
connection plugin into the `docker` plugin.

This is a backport to stable-2.7 of ansible#55816.

larsks added a commit to larsks/ansible that referenced this pull request May 9, 2019

connection/docker: add privilege escalation support
As described in ansible#53385 (and ansible#31759), the docker connection driver did
not support privilege escalation. This commit is a shameless
cut-and-paste of the privilege escalation support from the `local`
connection plugin into the `docker` plugin.

This is a backport to stable-2.7 of ansible#55816.

larsks added a commit to larsks/ansible that referenced this pull request May 10, 2019

connection/docker: add privilege escalation support
As described in ansible#53385 (and ansible#31759), the docker connection driver did
not support privilege escalation. This commit is a shameless
cut-and-paste of the privilege escalation support from the `local`
connection plugin into the `docker` plugin.

This is a backport to stable-2.7 of ansible#55816.

mnecas added a commit to mnecas/ansible that referenced this pull request May 13, 2019

connection/docker: add privilege escalation support (ansible#55816)
As described in ansible#53385 (and ansible#31759), the docker connection driver did
not support privilege escalation. This commit is a shameless
cut-and-paste of the privilege escalation support from the `local`
connection plugin into the `docker` plugin.

Closes: ansible#53385

@larsks larsks deleted the larsks:bug/53385 branch May 17, 2019

abadger added a commit that referenced this pull request May 21, 2019

[WIP] [2.8] connection/docker: add support for privilege escalation (#…
…56277)

* connection/docker: add privilege escalation support (#55816)

As described in #53385 (and #31759), the docker connection driver did
not support privilege escalation. This commit is a shameless
cut-and-paste of the privilege escalation support from the `local`
connection plugin into the `docker` plugin.

Closes: #53385
(cherry picked from commit 61e476b)

* docker connection plugin: make privilege escalation code more similar to local.py (#56288)

* Make more similar to local.py

* Fix typo.

(cherry picked from commit 708bda0)

abadger added a commit that referenced this pull request May 21, 2019

connection/docker: add privilege escalation support
As described in #53385 (and #31759), the docker connection driver did
not support privilege escalation. This commit is a shameless
cut-and-paste of the privilege escalation support from the `local`
connection plugin into the `docker` plugin.

This is a backport to stable-2.7 of #55816.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.