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

Make non-privileged user owner of setup scripts #216

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

lunkwill42
Copy link
Collaborator

If the host system is running with a restrictive umask (which mine does), these scripts may be copied with a mode of 700. Owned by root, they would not be executable by the www-data user inside a container.

@codecov
Copy link

codecov bot commented Nov 9, 2021

Codecov Report

Merging #216 (23f89c2) into develop (33dd4c8) will not change coverage.
The diff coverage is n/a.

❗ Current head 23f89c2 differs from pull request most recent head 2025808. Consider uploading reports for the commit 2025808 to get more accurate results

@@           Coverage Diff            @@
##           develop     #216   +/-   ##
========================================
  Coverage    59.38%   59.38%           
========================================
  Files           61       61           
  Lines         6180     6180           
========================================
  Hits          3670     3670           
  Misses        2510     2510           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -52,7 +52,7 @@ RUN mkdir -p /opt/cnaas/templates /opt/cnaas/settings /opt/cnaas/venv \
&& chown www-data:www-data /opt/cnaas/templates /opt/cnaas/settings /opt/cnaas/venv

# Copy cnaas scripts
COPY --chown=root:www-data cnaas-setup.sh createca.sh exec-pre-app.sh nosetests.sh /opt/cnaas/
Copy link

Choose a reason for hiding this comment

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

I wonder why this one line has a different owner. Originally a thinko?

Copy link
Collaborator Author

@lunkwill42 lunkwill42 Nov 23, 2021

Choose a reason for hiding this comment

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

Probably. There are even more small issues with the Dockerfile that will only be triggered on a Linux host using a strict umask, so more PRs may come...

Copy link
Member

Choose a reason for hiding this comment

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

The basic principle is to let www-data have write access to as absolutely little as possible, best case www-data only has write access to settings and templates and not much else. Scripts and source-code does not need to be modified at runtime so www-data should not be owner or have write access. The reason we have the www-data user and don't run everything as root is so we can separate privileges like this. If everything runs as www-data and everything is owned by www-data it's the same as everything is run by root and everything is owned by root, which we got some complaints about is not a secure solution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Then the best course of action is to ensure a chmod is run to give the correct privilege bits to the file, imho

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've force-pushed changes in line with the last comment.

If host system is running with a restrictive umask, these scripts may be
copied with a mode of 700. Owned by root, they would not be executable
by the www-data user inside a container.
@lunkwill42 lunkwill42 force-pushed the uninett.fix_docker_script_privileges branch from 23f89c2 to 2025808 Compare September 14, 2022 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants