PROD-1331 #4421

pattisdr · 2023-11-11T04:52:11Z

Closes #PROD-1331

Description Of Changes

Run fides as non-root user.

Code Changes

Creates a non root user to run the application but switches back and forth when root user needs to install something.
Switch to non-root user in CI_ARGS and CI_ARGS_EXEC

Steps to Confirm

Run fides, whoami is fidesuser. Confirm you can run privacy requests.

Pre-Merge Checklist

All CI Pipelines Succeeded
Documentation:
- documentation complete, PR opened in fidesdocs
- documentation issue created in fidesdocs
Issue Requirements are Met
Relevant Follow-Up Issues Created
Update CHANGELOG.md
For API changes, the Postman collection has been updated

…forth when root user needs to install something.

vercel · 2023-11-11T04:52:19Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
fides-plus-nightly	⬜️ Ignored (Inspect)	Visit Preview		Nov 20, 2023 3:00pm

…k to fides user

cypress · 2023-11-11T05:37:52Z

Passing run #5295 ↗︎

0	4	0	0	0
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

Details:

Merge `cd5559c` into `141a528`...
Project: fides	Commit: `6b0f9f888e ℹ️`
Status: Passed	Duration: 00:32 💡
Started: Nov 20, 2023 3:10 PM	Ended: Nov 20, 2023 3:11 PM

Review all test suite changes for PR #4421 ↗︎

codecov · 2023-11-11T10:26:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (141a528) 87.10% compared to head (cd5559c) 87.10%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4421   +/-   ##
=======================================
  Coverage   87.10%   87.10%           
=======================================
  Files         329      329           
  Lines       20366    20366           
  Branches     2623     2623           
=======================================
  Hits        17739    17739           
  Misses       2163     2163           
  Partials      464      464

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pattisdr · 2023-11-13T14:52:27Z

noxfiles/constants_nox.py

+# 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"


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.

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?

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

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

Dockerfile

daveqnet · 2023-11-13T15:49:42Z

Looks good to me, @pattisdr! This PR meets the security requirement:

~/projects/github/ethyca/fides PROD-1331_run_as_non_root ❯ docker ps | grep -i webserver
3be5d034e8a5   ethyca/fides:local                  "fides webserver"        30 seconds ago   Up 13 seconds (health: starting)   0.0.0.0:8080->8080/tcp     fides
~/projects/github/ethyca/fides PROD-1331_run_as_non_root ❯ docker exec -it 3be5d034e8a5 /bin/bash
fidesuser@3be5d034e8a5:/fides$ whoami
fidesuser
fidesuser@3be5d034e8a5:/fides$ exit
exit
~/projects/github/ethyca/fides PROD-1331_run_as_non_root ❯ docker exec -u 0 -it 3be5d034e8a5 /bin/bash
root@3be5d034e8a5:/fides# apt-get update && apt-get install procps
...
root@3be5d034e8a5:/fides# ps aux
USER       PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
fidesus+     1  6.9  4.4 1122584 355832 ?      Ssl  15:41   0:10 /opt/fides/bin/python3 /opt/fides/bin/fides webserver
fidesus+    27  0.0  0.0   5844  3356 pts/0    Ss+  15:41   0:00 /bin/bash
root        84  0.0  0.0   5844  3352 pts/1    Ss   15:43   0:00 /bin/bash
root       445  0.0  0.0   8332  2860 pts/1    R+   15:43   0:00 ps aux
root@3be5d034e8a5:/fides# cat /etc/passwd
root:x:0:0:root:/root:/bin/bash
daemon:x:1:1:daemon:/usr/sbin:/usr/sbin/nologin
bin:x:2:2:bin:/bin:/usr/sbin/nologin
sys:x:3:3:sys:/dev:/usr/sbin/nologin
sync:x:4:65534:sync:/bin:/bin/sync
games:x:5:60:games:/usr/games:/usr/sbin/nologin
man:x:6:12:man:/var/cache/man:/usr/sbin/nologin
lp:x:7:7:lp:/var/spool/lpd:/usr/sbin/nologin
mail:x:8:8:mail:/var/mail:/usr/sbin/nologin
news:x:9:9:news:/var/spool/news:/usr/sbin/nologin
uucp:x:10:10:uucp:/var/spool/uucp:/usr/sbin/nologin
proxy:x:13:13:proxy:/bin:/usr/sbin/nologin
www-data:x:33:33:www-data:/var/www:/usr/sbin/nologin
backup:x:34:34:backup:/var/backups:/usr/sbin/nologin
list:x:38:38:Mailing List Manager:/var/list:/usr/sbin/nologin
irc:x:39:39:ircd:/run/ircd:/usr/sbin/nologin
gnats:x:41:41:Gnats Bug-Reporting System (admin):/var/lib/gnats:/usr/sbin/nologin
nobody:x:65534:65534:nobody:/nonexistent:/usr/sbin/nologin
_apt:x:100:65534::/nonexistent:/usr/sbin/nologin
fidesuser:x:1001:65534::/home/fidesuser:/usr/sbin/nologin
root@3be5d034e8a5:/fides# exit
exit
~/projects/github/ethyca/fides PROD-1331_run_as_non_root ❯

galvana

These changes make sense, the only part that threw me off was the CI_ARGS/CI_ARGS_EXEC portion but I tagged @ThomasLaPiana to comment if he remembers. The only change that would be a nice to have is locking down the fides_uploads to the fidesuser like you did in fidesplus.

Dockerfile

galvana · 2023-11-16T20:42:25Z

noxfiles/constants_nox.py

+# 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"


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?

Dockerfile

…g ownership.

pattisdr · 2023-11-17T17:52:26Z

OK @galvana thanks for your review! The changes I've made as a result: I'm chowning the fides directory but leaving the fides user is as-is.

Creates a non root user to run the application but switches back and …

3f207ba

…forth when root user needs to install something.

pattisdr changed the title ~~Creates a non root user to run the application but switches back and …~~ PROD-1331 Nov 11, 2023

Switch to root for more installation and removal before switching bac…

fd434df

…k to fides user

Switch to non-root user in CI_ARGS and CI_ARGS_EXEC

066a94c

pattisdr commented Nov 13, 2023

View reviewed changes

pattisdr requested review from ThomasLaPiana, daveqnet and NevilleS November 13, 2023 15:26

galvana self-requested a review November 14, 2023 16:27

galvana approved these changes Nov 17, 2023

View reviewed changes

pattisdr added 3 commits November 17, 2023 09:59

Add system group, and switch to user before copying fides and changin…

50253a6

…g ownership.

Revert change to change ownership of fides directory.

f28a397

Try changing owner but matching privacy center id.

e56a98b

ThomasLaPiana approved these changes Nov 20, 2023

View reviewed changes

pattisdr added 2 commits November 20, 2023 08:58

Merge branch 'main' into PROD-1331_run_as_non_root

4717d28

Update changelog

cd5559c

pattisdr merged commit 031f5aa into main Nov 20, 2023
42 checks passed

pattisdr deleted the PROD-1331_run_as_non_root branch November 20, 2023 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PROD-1331 #4421

PROD-1331 #4421

pattisdr commented Nov 11, 2023 •

edited by ThomasLaPiana

Loading

vercel bot commented Nov 11, 2023 •

edited

Loading

cypress bot commented Nov 11, 2023 •

edited

Loading

codecov bot commented Nov 11, 2023 •

edited

Loading

pattisdr Nov 13, 2023 •

edited

Loading

galvana Nov 16, 2023

ThomasLaPiana Nov 20, 2023

pattisdr Nov 20, 2023

daveqnet commented Nov 13, 2023

galvana left a comment

galvana Nov 16, 2023

pattisdr commented Nov 17, 2023

PROD-1331 #4421

PROD-1331 #4421

Conversation

pattisdr commented Nov 11, 2023 • edited by ThomasLaPiana Loading

Description Of Changes

Code Changes

Steps to Confirm

Pre-Merge Checklist

vercel bot commented Nov 11, 2023 • edited Loading

cypress bot commented Nov 11, 2023 • edited Loading

Passing run #5295 ↗︎

Review all test suite changes for PR #4421 ↗︎

codecov bot commented Nov 11, 2023 • edited Loading

Codecov Report

pattisdr Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

galvana Nov 16, 2023

Choose a reason for hiding this comment

ThomasLaPiana Nov 20, 2023

Choose a reason for hiding this comment

pattisdr Nov 20, 2023

Choose a reason for hiding this comment

daveqnet commented Nov 13, 2023

galvana left a comment

Choose a reason for hiding this comment

galvana Nov 16, 2023

Choose a reason for hiding this comment

pattisdr commented Nov 17, 2023

pattisdr commented Nov 11, 2023 •

edited by ThomasLaPiana

Loading

vercel bot commented Nov 11, 2023 •

edited

Loading

cypress bot commented Nov 11, 2023 •

edited

Loading

codecov bot commented Nov 11, 2023 •

edited

Loading

pattisdr Nov 13, 2023 •

edited

Loading