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

fix: use the non-root atlantis user instead of root #3886

Merged
merged 20 commits into from
Nov 9, 2023
Merged

fix: use the non-root atlantis user instead of root #3886

merged 20 commits into from
Nov 9, 2023

Conversation

bschaatsbergen
Copy link
Contributor

@bschaatsbergen bschaatsbergen commented Oct 21, 2023

what

  • Make standard use of the atlantis user and not root.
  • Removed gosu, as we're using the atlantis user anyways
  • Set DOCKER_CONTENT_TRUST=1 anywhere we build

why

  • gosu has various security issues reported (see reported issues)
  • it's not a good practice to use the root user by default

tests

references

@bschaatsbergen
Copy link
Contributor Author

TODO: needs testing locally

@bschaatsbergen bschaatsbergen changed the title feat: use Atlantis user by default and get rid of gosu (as per vulns) address Docker container security issues Oct 21, 2023
@bschaatsbergen bschaatsbergen changed the title address Docker container security issues fix: address Docker container security issues Oct 21, 2023
@bschaatsbergen bschaatsbergen marked this pull request as ready for review October 21, 2023 01:57
@bschaatsbergen bschaatsbergen requested a review from a team as a code owner October 21, 2023 01:57
@github-actions github-actions bot added build Relating to how we build Atlantis github-actions labels Oct 21, 2023
@bschaatsbergen
Copy link
Contributor Author

@nitrocode, @GenPage, @jamengual if you have some time later this week - I consider this quite an impactful change and it would require thorough testing before shipping it.

Dockerfile Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
@nitrocode
Copy link
Member

@bschaatsbergen I see the tests pass which is great but it would be nice to test this out in your setup and verify the following

  • atlantis project autodiscovery
  • atlantis website for locks and plans
  • atlantis init/plan/apply workflow
  • atlantis downloading terraform binaries
  • anything else that uses local storage?

@nitrocode
Copy link
Member

One thing I'm curious about is why there do not need to be any chmod or chown changes as part of this pr?

@bschaatsbergen
Copy link
Contributor Author

One thing I'm curious about is why there do not need to be any chmod or chown changes as part of this pr?

Yes, I think we do - as we've removed the line adduser atlantis root the chmod g=u ... would be useless, as the atlantis user is no longer in the root group. I'll work on this too.

@nitrocode
Copy link
Member

Thanks @bschaatsbergen. Please also rerun dockle and show the output in the pr summary. If we can reduce that to zero and still have atlantis running as usual then that would fully close our the associated issue.

@bschaatsbergen
Copy link
Contributor Author

bschaatsbergen commented Oct 21, 2023

@bschaatsbergen I see the tests pass which is great but it would be nice to test this out in your setup and verify the following

  • atlantis project autodiscovery
  • atlantis website for locks and plans
  • atlantis init/plan/apply workflow
  • atlantis downloading terraform binaries
  • anything else that uses local storage?

Tested:

✅ atlantis website for locks and plans
✅ atlantis init/plan/apply workflow
✅ atlantis downloading terraform binaries
✅ cloning of the git repository
✅ being able to mount a persistent data disk and use that as atlantis' home directory
✅ writing to local storage using Terraform

@bschaatsbergen
Copy link
Contributor Author

dockle seems to be happy overall, note solving CIS-DI-0008 is quite hard and as it's an info level check I think that we can skip it and address it in the nearby future if it ever posses a problem?

$ dockle ghcr.io/bschaatsbergen/atlantis:dev-alpine-2d7ba9e
$ dockle ghcr.io/bschaatsbergen/atlantis:dev-debian-2d7ba9e
INFO - CIS-DI-0008: Confirm safety of setuid/setgid files

  • setuid file: urwxr-xr-x usr/bin/mount
  • setuid file: urwxr-xr-x usr/lib/openssh/ssh-keysign
  • setuid file: urwxr-xr-x usr/bin/newgrp
  • setuid file: urwxr-xr-x usr/bin/chfn
  • setgid file: grwxr-xr-x usr/bin/ssh-agent
  • setgid file: grwxr-xr-x usr/sbin/unix_chkpwd
  • setuid file: urwxr-xr-x usr/bin/passwd
  • setuid file: urwxr-xr-x usr/bin/gpasswd
  • setuid file: urwxr-xr-x usr/bin/su
  • setuid file: urwxr-xr-x usr/bin/umount
  • setgid file: grwxr-xr-x usr/bin/wall
  • setgid file: grwxr-xr-x usr/bin/expiry
  • setgid file: grwxr-xr-x usr/bin/chage
  • setuid file: urwxr-xr-x usr/bin/chsh

@bschaatsbergen
Copy link
Contributor Author

This is ready for review @nitrocode, @jamengual, @GenPage - I've extensively tested the alpine image (besides autodiscovery) and I'm unable to test out the debian image - if one of you could check the debian image out that would be appreciated.

I've published both an alpine as well as a debian image:

  • ghcr.io/bschaatsbergen/atlantis:dev-alpine-2d7ba9e
  • ghcr.io/bschaatsbergen/atlantis:dev-debian-2d7ba9e

Dockerfile Show resolved Hide resolved
chmod g=u /home/atlantis/ && \
chmod g=u /etc/passwd
chmod u+rwx /home/atlantis/ && \
chmod u+rw /etc/passwd

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come the chmod of /etc/passwd needs to change? If the line is removed, can atlantis still function?

Copy link
Contributor Author

@bschaatsbergen bschaatsbergen Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the previous chmod g=u sets the group permissions for the /home/atlantis/ directory to be the same as the user's permissions.

The new chmod sets the user's permissions for the /home/atlantis/ directory to read, write (and execute).

So, the change here is that the original command was changing group permissions to match user permissions, and it has been modified to give the user explicit permissions instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right but I don't understand why /etc/passwd perms need to be changed. Can we get away with only changing /home/atlantis ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I didn't want to break anything. Therefor I just gave it read write permissions. We can't use g=u anymore though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok no worries then, can always tackle it later. Thank you!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok but the more changes made, the more likely something can break. I'm all for more security, provided @bschaatsbergen has the appetite to keep going 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really the case? I'm happy to add it, but couldn't find it anywhere. I know that users start from 1000 though. Is there a particular reason we should explicitly set this? Omitting it would prevent a uid and gid conflict right? @jamengual

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've set uid/gid 100:1000 and removed the passwd entry lines.

Copy link

@arohter arohter Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alpine and debian have different uid's it appears: #3317 (comment)
alpine: uid=100, debian: uid=1000

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arohter it's removed again, to stay inline with what's in the Dockerfile now. to avoid touching the uid and gid at all.

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
@GenPage GenPage added the waiting-on-response Waiting for a response from the user label Nov 4, 2023
GenPage
GenPage previously approved these changes Nov 4, 2023
Copy link
Member

@GenPage GenPage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, all of my concerns were already addressed. Is there anything left to do @bschaatsbergen based on your last comment?

@jamengual

This comment was marked as resolved.

@bschaatsbergen
Copy link
Contributor Author

Approved, all of my concerns were already addressed. Is there anything left to do @bschaatsbergen based on your last comment?

there is one more comment to resolve, which is about the passwd file changes.

Fixing this today, been a hectic week!

@bschaatsbergen bschaatsbergen dismissed stale reviews from GenPage and nitrocode via 378b857 November 4, 2023 19:36
@jamengual
Copy link
Contributor

/cherry-pick release-0.26

@jamengual jamengual merged commit e9c0d72 into runatlantis:main Nov 9, 2023
23 checks passed
jamengual added a commit that referenced this pull request Nov 9, 2023
* feat: use Atlantis user by default and get rid of gosu

* chore: set `DOCKER_CONTENT_TRUST=1`

* chore: fix chmod and chown

* feat: add a healthcheck to the debian and alpine images

* feat: removing setuid and setgid permissions prevents container privilege escalation and improve comments

* chore: remove setgid/setuid as we chown an entire directory

* chore: keep deps comment generic

* chore: grammar

* chore: remove redundant comment

* chore: rm DOCKER_CONTENT_TRUST

* chore: set uid and gid and remove passwd entry

* chore: revert gid and uid set as it's conflicting

---------

Co-authored-by: PePe Amengual <jose.amengual@gmail.com>
GenPage pushed a commit that referenced this pull request Nov 9, 2023
* feat: use Atlantis user by default and get rid of gosu

* chore: set `DOCKER_CONTENT_TRUST=1`

* chore: fix chmod and chown

* feat: add a healthcheck to the debian and alpine images

* feat: removing setuid and setgid permissions prevents container privilege escalation and improve comments

* chore: remove setgid/setuid as we chown an entire directory

* chore: keep deps comment generic

* chore: grammar

* chore: remove redundant comment

* chore: rm DOCKER_CONTENT_TRUST

* chore: set uid and gid and remove passwd entry

* chore: revert gid and uid set as it's conflicting

---------

Co-authored-by: PePe Amengual <jose.amengual@gmail.com>
GenPage pushed a commit that referenced this pull request Nov 10, 2023
* feat: use Atlantis user by default and get rid of gosu

* chore: set `DOCKER_CONTENT_TRUST=1`

* chore: fix chmod and chown

* feat: add a healthcheck to the debian and alpine images

* feat: removing setuid and setgid permissions prevents container privilege escalation and improve comments

* chore: remove setgid/setuid as we chown an entire directory

* chore: keep deps comment generic

* chore: grammar

* chore: remove redundant comment

* chore: rm DOCKER_CONTENT_TRUST

* chore: set uid and gid and remove passwd entry

* chore: revert gid and uid set as it's conflicting

---------

Co-authored-by: PePe Amengual <jose.amengual@gmail.com>
GenPage pushed a commit that referenced this pull request Nov 11, 2023
* feat: use Atlantis user by default and get rid of gosu

* chore: set `DOCKER_CONTENT_TRUST=1`

* chore: fix chmod and chown

* feat: add a healthcheck to the debian and alpine images

* feat: removing setuid and setgid permissions prevents container privilege escalation and improve comments

* chore: remove setgid/setuid as we chown an entire directory

* chore: keep deps comment generic

* chore: grammar

* chore: remove redundant comment

* chore: rm DOCKER_CONTENT_TRUST

* chore: set uid and gid and remove passwd entry

* chore: revert gid and uid set as it's conflicting

---------

Co-authored-by: Bruno Schaatsbergen <58337159+bschaatsbergen@users.noreply.github.com>
@bschaatsbergen bschaatsbergen deleted the docker-use-atlantis-user-and-remove-gosu branch November 14, 2023 09:37
@nitrocode nitrocode added the bug Something isn't working label Dec 11, 2023
@Alex-Mussell
Copy link
Contributor

Asked to comment: https://atlantis-community.slack.com/archives/C5MGGAV0C/p1702475459555389 here as there probably needs to be some documentation in the release notes regarding this update. But here is the lowdown.

We use the Atlantis image as a base image and added some extra functionality to the container like so:

BEFORE:

FROM ghcr.io/runatlantis/atlantis:v0.27.0-alpine
RUN apk add --no-cache python3 python3-dev py3-pip libc-dev libffi-dev gcc jq yq
RUN pip3 install --upgrade pip
RUN pip3 install --no-cache-dir \
  'msal==1.4.3' \
  'awscli'

etc.etc.

And this worked fine prior to v0.27.0. However we started to get an error: #8 0.951 ERROR: Unable to lock database: Permission denied , having looked around I resolved this by then doing the following (notice how I also had to create a virtualenv to be able to upgrade pip once the above error was resolved):

AFTER:

FROM ghcr.io/runatlantis/atlantis:v0.27.0-alpine
USER root   <------ ADDED

ENV VIRTUAL_ENV=/opt/venv
RUN apk add --no-cache python3 python3-dev py3-pip libc-dev libffi-dev gcc jq yq
RUN python3 -m venv $VIRTUAL_ENV
## We can replace source <venv>/activate by setting the appropriate environment variables: Docker's ENV command applies both subsequent RUNs as well as to the CMD.
ENV PATH="$VIRTUAL_ENV/bin:$PATH"
RUN pip3 install --upgrade pip
RUN pip3 install --no-cache-dir \
  'msal==1.4.3' \
  'awscli'

etc.etc.

USER atlantis   <----- ADDED

Next we deploy this container via Ansible onto an EC2 instance, but there needed to be some changes in some tasks as we are now the atlantis user. Any time a task that requires you to be root within the container on the host, you now need to explicitly define the root user as the executor of the task. For example, we have an AWS config that needs to be readable by the atlantis user.

BEFORE:
- name: Make AWS conf readable by Atlantis
  community.docker.docker_container_exec:
    container: atlantis
    command: /bin/bash -c "chown atlantis:atlantis /home/atlantis/.aws/"

AFTER:
- name: Make AWS conf readable by Atlantis
  community.docker.docker_container_exec:
    user: root
    container: atlantis
    command: /bin/bash -c "chown atlantis:atlantis /home/atlantis/.aws/"

ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024
* feat: use Atlantis user by default and get rid of gosu

* chore: set `DOCKER_CONTENT_TRUST=1`

* chore: fix chmod and chown

* feat: add a healthcheck to the debian and alpine images

* feat: removing setuid and setgid permissions prevents container privilege escalation and improve comments

* chore: remove setgid/setuid as we chown an entire directory

* chore: keep deps comment generic

* chore: grammar

* chore: remove redundant comment

* chore: rm DOCKER_CONTENT_TRUST

* chore: set uid and gid and remove passwd entry

* chore: revert gid and uid set as it's conflicting

---------

Co-authored-by: PePe Amengual <jose.amengual@gmail.com>
ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024
* feat: use Atlantis user by default and get rid of gosu

* chore: set `DOCKER_CONTENT_TRUST=1`

* chore: fix chmod and chown

* feat: add a healthcheck to the debian and alpine images

* feat: removing setuid and setgid permissions prevents container privilege escalation and improve comments

* chore: remove setgid/setuid as we chown an entire directory

* chore: keep deps comment generic

* chore: grammar

* chore: remove redundant comment

* chore: rm DOCKER_CONTENT_TRUST

* chore: set uid and gid and remove passwd entry

* chore: revert gid and uid set as it's conflicting

---------

Co-authored-by: PePe Amengual <jose.amengual@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working build Relating to how we build Atlantis security waiting-on-response Waiting for a response from the user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix dockle container security issues Run docker container as atlantis instead of root
6 participants