breaking!(security): 💥 run as nobody user by default, supports read-only RootFS and control packages content
#17
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… over versions) Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
… the bottom of Dockerfile Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
dduportal
changed the title
nobody user by default, supports read-only RootFS and control packages content
dduportal
added
documentation
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
Signed-off-by: Damien Duportal <damien.duportal@gmail.com>
dduportal
commented
Sep 4, 2023
Comment on lines
+16
to
+25
| ARG RSYNCD_DIR=/rsyncd | ||
| ENV RSYNCD_DIR="${RSYNCD_DIR}" | ||
| RUN mkdir -p "${RSYNCD_DIR}/run" "${RSYNCD_DIR}/data" /etc/rsyncd.d && \ | ||
| chown -R nobody:nogroup "${RSYNCD_DIR}" | ||
|
|
||
| COPY rsyncd.conf /etc/rsyncd.conf | ||
|
|
||
| WORKDIR /rsyncd/data | ||
|
|
||
| VOLUME ["/rsyncd/run","/rsyncd/data","/tmp"] |
There was a problem hiding this comment.
Post-review: the build arg RSYNCD_DIR is not valuable since it cannot be consumed by the VOLUME:
- either find a way to consume it for the
WORKDIRandVOLUMEto avoid repetition (I failed with multi platform builds while it works on single platform: smells like buildx issue) - or do not complexify code and duplicate paths
dduportal
commented
Sep 4, 2023
|
|
||
| CMD [ "/usr/bin/rsync","--no-detach","--daemon","--config","/etc/rsyncd.conf" ] | ||
| CMD ["/usr/bin/rsync", "--no-detach","--daemon","--config","/etc/rsyncd.conf"] |
There was a problem hiding this comment.
Post-review: incoherent formatting with spaces
dduportal
commented
Sep 4, 2023
Comment on lines
+24
to
+28
|
|
||
| # Merge any /etc/rsyncd.d/*.inc files (for global values that should stay in effect), | ||
| &merge /etc/rsyncd.d | ||
| # And then include any /etc/rsyncd.d/*.conf files (defining modules without any global-value cross-talk). | ||
| &include /etc/rsyncd.d |
There was a problem hiding this comment.
Post review: add a comment about the & prefixes (link to rsyncd.conf reference?)
dduportal
commented
Sep 4, 2023
| @@ -0,0 +1,15 @@ | |||
| # Start this "lightly hardened" test with the command "docker compose up -d --build" | |||
| # Then check the result with the command "rsync -av rsync://localhost:1873/jenkins" | |||
There was a problem hiding this comment.
Post review: long flags
Suggested change
| # Then check the result with the command "rsync -av rsync://localhost:1873/jenkins" | |
| # Then check the result with the command "rsync --archive --verbose rsync://localhost:1873/jenkins" |
dduportal
commented
Sep 4, 2023
| @@ -0,0 +1,3 @@ | |||
| [jenkins] | |||
| path = /rsyncd/data/jenkins | |||
There was a problem hiding this comment.
Post-review: comment why the subdir (ref. stanza name which is an rsync module)
dduportal
commented
Sep 4, 2023
| - ./rsyncd.d:/etc/rsyncd.d:ro | ||
| - ./jenkins.motd:/etc/jenkins.motd:ro | ||
| ports: | ||
| - 1873:873 |
There was a problem hiding this comment.
Post review: note about why a >1024 port in this test case
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While working on jenkins-infra/helpdesk#2649, I realized that this image was long forgotten.
I'm opening this PR to start a (long term) work on improving security of our containers in production.
This is a first tentative, still a lot of work (documenting and explaining practises, adding more testing, etc.). The goal is to deliver this new image version on the (not yet used) rsyncd service deployed for the new update center in jenkins-infra/helpdesk#2649.
It introduces the following changes:
feat: Pinned the Debian version used as base image. We do not want surprise updates which would change the behavior of the image without testing it.
nitpick: Improved the build efficiency (speed and caching) by moving metadatas (
EXPOSE,VOLUMEetc.) at the endfix: Allow building on
arm64(initially for macOS Silicon Docker Desktop but also for Proposal for application in publick8s to migrate to arm64 helpdesk#3619) by installingtiniinside aRUNinstruction where it is easier to determine the target architecture with the exact value (in the case of tini it is eitherarm64oramd64while theTARGETPLARFORMbuild argument would have beenlinux/aarch64orlinux/arm64orlinux/amd64).feat: Decrease attack vectors by removing unused packages
💥 (breaking) Allow image to run with a read-only rootfs by defining volumes where writing is required by default
💥 (breaking) set default container user as
nobody:nogroup(65534:65534with numerical UID and GID)/rsyncd/dataand is owned by the default usernobody:nogroup/rsyncd/runand is owned by the default usernobody:nogroup💥 (breaking) : Changed the default
rsyncdconfiguration:uidandgidas the default user executing thersyncdprocess is restricted and not allowed to change uid of gid (kernel restriction)chrootas the default user executing thersyncdprocess is restricted and not allowed to clone namespace (required by chroot) neither allowed to change uid or gid/etc/rsyncdwith&includeand&mergedirectives to allow consumer to specify partial configurations for modules ("include") and to eventually override the main directives without having to rewrite the whole top level file ("merge"). This trick not only allows easier configurations in helm charts, but it also enabled the "non root" default user without risk of complex breakages (such as/etc/rsyncdnot readabale)jenkins.motd, the whole "read only" setup and the "hosts allow" => it has to be specified in a "merge" (/etc/rsyncd.d/*.inc) partial configuration file/etc/rsyncd.d/*.conf) partial configuration file/rsyncd/run/rsyncd.pidbut can be changed with a "merge" partialc configuration file (allows non root user in a read-only rootfs)🛠️ Tests: added 2 kinds of tests to ensure the above changes are present and protect from further regressions
cst.ymlfiletiniupdatecli manifest is required to update the newcst.ymlfile along theDockerfiledocker-compose.ymlfile which builds an run the image in read only + not root example reproductible scenariogoss(with its wrappersdgoss,dcgossandkgoss) but this manual acceptance is sufficient as the next step will be the helm chart!