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.

Sign up for GitHub

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

Jump to bottom

PROD-1331 #4421

Merged
merged 8 commits into from
Nov 20, 2023
Merged

PROD-1331 #4421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3f207ba
Creates a non root user to run the application but switches back and …
pattisdr Nov 11, 2023
fd434df
Switch to root for more installation and removal before switching bac…
pattisdr Nov 11, 2023
066a94c
Switch to non-root user in CI_ARGS and CI_ARGS_EXEC
pattisdr Nov 11, 2023
50253a6
Add system group, and switch to user before copying fides and changin…
pattisdr Nov 17, 2023
f28a397
Revert change to change ownership of fides directory.
pattisdr Nov 17, 2023
e56a98b
Try changing owner but matching privacy center id.
pattisdr Nov 17, 2023
4717d28
Merge branch 'main' into PROD-1331_run_as_non_root
pattisdr Nov 20, 2023
cd5559c
Update changelog
pattisdr Nov 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,11 @@ The types of changes are:

### Added
- Logging when root user and client credentials are used [#4432](https://github.com/ethyca/fides/pull/4432)


### Changed
- Improved bulk vendor adding table UX [#4425](https://github.com/ethyca/fides/pull/4425)
- Run fides with non-root user [#4421](https://github.com/ethyca/fides/pull/4421)


## [2.24.0](https://github.com/ethyca/fides/compare/2.23.3...2.24.0)
Expand Down
15 changes: 14 additions & 1 deletion Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ ARG PYTHON_VERSION="3.10.12"
#########################
FROM python:${PYTHON_VERSION}-slim-bullseye as compile_image


# Install auxiliary software
RUN apt-get update && \
apt-get install -y --no-install-recommends \
Expand Down Expand Up @@ -54,6 +55,11 @@ RUN pip install --no-cache-dir install -r dev-requirements.txt
##################
FROM python:${PYTHON_VERSION}-slim-bullseye as backend

# Add the fidesuser user but don't switch to it yet
RUN addgroup --system --gid 1001 fidesgroup
RUN adduser --system --uid 1001 fidesuser
pattisdr marked this conversation as resolved.
Show resolved Hide resolved


RUN apt-get update && \
apt-get install -y --no-install-recommends \
curl \
Expand All @@ -69,7 +75,8 @@ COPY --from=compile_image /opt/fides /opt/fides
ENV PATH=/opt/fides/bin:$PATH

# General Application Setup ##
COPY . /fides
USER fidesuser
COPY --chown=fidesuser:fidesgroup . /fides
WORKDIR /fides

# Immediately flush to stdout, globally
Expand All @@ -92,8 +99,12 @@ CMD [ "fides", "webserver" ]
#############################
FROM backend as dev

USER root

RUN pip install -e . --no-deps

USER fidesuser

###################
## Frontend Base ##
###################
Expand Down Expand Up @@ -150,9 +161,11 @@ FROM backend as prod
# Copy frontend build over
COPY --from=built_frontend /fides/clients/admin-ui/out/ /fides/src/fides/ui-build/static/admin

USER root
# Install without a symlink
RUN python setup.py sdist
RUN pip install dist/ethyca-fides-*.tar.gz

# Remove this directory to prevent issues with catch all
RUN rm -r /fides/src/fides/ui-build
USER fidesuser
8 changes: 4 additions & 4 deletions noxfiles/constants_nox.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,11 @@
# Helpful paths
CWD = getcwd()

# Disable TTY to perserve output within Github Actions logs
# Disable TTY to preserve output within Github Actions logs
# CI env variable is always set to true in Github Actions
# The else statement is required due to the way commmands are structured and is arbitrary.
CI_ARGS = "-T" if getenv("CI") else "--user=root"
CI_ARGS_EXEC = "-t" if not getenv("CI") else "--user=root"
# The else statement is required due to the way commands are structured and is arbitrary.
CI_ARGS = "-T" if getenv("CI") else "--user=fidesuser"
CI_ARGS_EXEC = "-t" if not getenv("CI") else "--user=fidesuser"
Comment on lines +55 to +57
Copy link
Contributor Author

@pattisdr pattisdr Nov 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This was causing numerous tests to fail when running:

docker exec -e ANALYTICS_OPT_OUT -e FIDES__CLI__ANALYTICS_ID --user=root fides pytest --cov-report=xml tests/ctl/ -m 'not external' so I updated it to use the new fidesuser

I'm a little confused about the previous code comment here: The else statement is required due to the way commands are structured and is arbitrary. It seems like the "else" matters, is all.

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't make sense to me either, it seems like the flags (-T or -t) control TTY settings which seems incompatible with specifying a user. The earliest occurrence of this comes from @ThomasLaPiana, maybe he can comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes this is a super hacky workaround. Due to the way that commands were injected, it broke if I tried to use an empty string, so instead I used a "dummy" flag which ended up being --user=root. As long as there is something valid in there it should be fine :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ThomasLaPiana the value seemed to matter though which is why I was confused. Once I started running this container as fidesuser and not root, a large number of tests were failing though and changing the value here to --user=fidesuser fixed.


# If FIDES__CLI__ANALYTICS_ID is set in the local environment, use its value as the analytics_id
ANALYTICS_ID_OVERRIDE = ("-e", "FIDES__CLI__ANALYTICS_ID")
Expand Down