Skip to content
This repository was archived by the owner on Aug 12, 2024. It is now read-only.

Conversation

@tylerslaton
Copy link
Contributor

@tylerslaton tylerslaton commented Aug 30, 2022

Summary

This PR brings Pod Security Admission (PSA) standards to RukPak in the form of updating the Namespace manifest (in accordance with the Kubernetes documentation) and Deployments.

Note: The namespace is running in a baseline model; However, we eventually want this to be restricted. For the time being I've implemented everything in such a way that restricted would be satisfied if we switch over except that the unpacker pod is running as root currently. This is because we cannot guarantee, at least at the moment, that the container image that we are going to unpack does not run as root. We'll need to think up a solution for this either here or in the future.

@tylerslaton tylerslaton requested a review from anik120 August 30, 2022 19:42
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 30, 2022
@joelanford
Copy link
Member

We should also document somewhere that this change will require bundle image contents to be readable by the unpack pod, which may have an unpredictable process UID.

In general, we can probably say bundle files and directories should have GID=0 and they should have permissions 0750 and 0640 at a minimum.

@tylerslaton
Copy link
Contributor Author

tylerslaton commented Aug 30, 2022

We should also document somewhere that this change will require bundle image contents to be readable by the unpack pod, which may have an unpredictable process UID.

@joelanford Is this a potential non-starter for things like the registry provisioner which is leveraging image content from pretty establish ecosystems? If I'm understanding correctly, that may require registry+v1 format Bundles to update file permissions.

Signed-off-by: Tyler Slaton <tyslaton@redhat.com>
Co-authored-by: Anik Bhattacharjee <anikbhattacharya93@gmail.com>
@joelanford
Copy link
Member

@tylerslaton, maybe that'll dictate that the registry provisioner runs in a separate namespace with less restrictive PSA (e.g. baseline)?

@tylerslaton tylerslaton force-pushed the add-psa-compliance branch 2 times, most recently from 3ac6822 to 4e72c36 Compare August 31, 2022 18:22
@tylerslaton tylerslaton force-pushed the add-psa-compliance branch 2 times, most recently from 416a77d to 978b694 Compare August 31, 2022 19:05
@tylerslaton tylerslaton removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 31, 2022
@tylerslaton tylerslaton marked this pull request as ready for review August 31, 2022 19:21
@tylerslaton tylerslaton requested a review from a team as a code owner August 31, 2022 19:21
@joelanford
Copy link
Member

joelanford commented Aug 31, 2022

As a follow-up, can we:

  1. Add a knob to the image unpacker that lets a provisioner opt-in to the more restrictive security context stuff
  2. Update the plain (and helm?) provisioners to use that knob
  3. Update the tests and testdata for plain (and helm?) images to pass with the restrictive security contexts.
  4. Update docs for the plain (and helm?) provisioners that talk about how bundle files need to be world-readable.

"And helm?" because I'm not sure what sort of file permissions Helm OCI images have.

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

Just a minor nit about adding a TODO comment. Otherwise LGTM.

Signed-off-by: Tyler Slaton <tyslaton@redhat.com>
Co-authored-by: Anik Bhattacharjee <anikbhattacharya93@gmail.com>
Copy link
Member

@anik120 anik120 left a comment

Choose a reason for hiding this comment

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

@tylerslaton I have a couple of clarifying questions otherwise looks good for the most part 🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants