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

Do not run containers as root and do not create root owned files #234

Open
jeremyestein opened this issue Jan 24, 2024 · 7 comments · May be fixed by #400
Open

Do not run containers as root and do not create root owned files #234

jeremyestein opened this issue Jan 24, 2024 · 7 comments · May be fixed by #400
Assignees
Labels
bug Something isn't working security Working on this will improve security
Milestone

Comments

@jeremyestein
Copy link
Contributor

jeremyestein commented Jan 24, 2024

Definition of Done / Acceptance Criteria

  • Containers are no longer run as root.
  • Files created by containers on mounted file systems are no longer owned by root, and thus they are readable/writable/deletable by a normal user.

The criteria above will both be fixed if the container no longer runs as root. If for some reason you only implement a fix for the file permissions issue (eg. by chowning the files after creating them), this issue should be split so the running as root issue is not lost.

Testing

System test should check which user each container is running as after bringing them up. Note that running on a non-linux host will falsely cause such a test to pass, as the special docker linux kernel on these platforms doesn't map container root to host root in the first place.

Any test that checks the presence and contents of an output file should also check the file's ownership. For an example, see check_radiology_parquet.py where these tests exist but have been temporarily demoted to logging only.

Documentation

Depends on implementation as to whether user will have to do anything different because of this (eg. set an environment var)

Dependencies

Some possible solutions would need a reconfiguration of the GAE via Atos, which could take some time.

Details and Comments

Rationale

  • Running as root is a security hazard. If one of the services gets compromised then the attacker can do more bad stuff to the GAE than they would otherwise.
  • Files owned by root cannot be deleted/moved/etc by a normal user (or indeed a system test).

Possible implementations

  • Run docker daemon in "rootless" mode - will require some changes via Atos?
  • User namespaces (userns_mode ) - also requires admin?
  • Set the user directly with the user property of the docker compose service spec.

Someone may have to play around with a Docker install on Linux to see what works so we know what (if anything) we want to ask Atos for.

@jeremyestein jeremyestein added the bug Something isn't working label Jan 24, 2024
@jeremyestein jeremyestein added this to the 100-days milestone Jan 24, 2024
@milanmlft
Copy link
Member

milanmlft commented Jan 25, 2024

Yeahhh, I encountered this issue as well when trying to tear down the export directories created by tests for #226, which ran fine locally but then ran into errors on the GHA because indeed the Docker volumes were root-owned and so any files created there could not be deleted... I ended up working around this by ignoring errors from shutil.rmtree(), meaning the files won't actually get deleted on the GHA runner 🤷

Turns out though that this is a longstanding issue with Docker-compose named volumes. There are some workarounds but none of them are trivial.
Just running the containers as non-root, for example, does not seem to resolve the issue, though haven't tried this myself.

@stefpiatek stefpiatek modified the milestones: 100-days, VOXL Jan 29, 2024
@milanmlft milanmlft added dev-velocity Working on the issue will increase development speed in the long run and removed developer QoL labels Jan 29, 2024
@stefpiatek stefpiatek added security Working on this will improve security and removed dev-velocity Working on the issue will increase development speed in the long run labels Jan 29, 2024
@stefpiatek
Copy link
Contributor

stefpiatek commented Feb 12, 2024

Part of wider work on best practices on GAEs.

  • secret managment
  • failover
  • logging and monitoring

@dram1964 dram1964 self-assigned this Feb 12, 2024
@dram1964
Copy link

as per #239 this is an issue that although not solved by, can still be monitored by 'docker scout'. Am investigating this to see if it is a viable approach.

@dram1964
Copy link

I've created an SOP for using Docker on the FlowEHR-Operations-Manual repo and will link to SLAB once it's been through it's PR review

@dram1964
Copy link

Check for privileged running with: docker container inspect --format '{{.Config.User}}' $container_id. Blank output suggests running as root.

@stefpiatek
Copy link
Contributor

Currently have: guidance for GAE running, collect auditing data

  • Should create another issue to track using the audit data - @dram1964

@skeating
Copy link

@jeremyestein Moving this to in progress, could you add additional tickets for the ones you haven't done. Thank you

@jeremyestein jeremyestein linked a pull request May 23, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working security Working on this will improve security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants