Skip to content

Conversation

@nalind
Copy link
Member

@nalind nalind commented Mar 17, 2021

In the build controller, we never actually called setupContainersNodeStorage(), so remove it, and stop setting the environment variable to force storage to use the overlay driver, which currently requires privileges, leaning on openshift/builder#220 and openshift/builder#291 to have the builder container figure that out for itself.

Make the mode which we apply to secret and configuration map volumes in build pods something that the build controller can pass down to its helper functions, so that it can be adjusted based on the strategy. Continue to default them to 0600.

Set the seccomp profile for build pods to "unconfined", since we depend on unshare() and CRI-O is attempting to move away from allowing it to be used in its default seccomp profile.

When the Build object's environment includes "BUILD_PRIVILEGED=false" or "BUILD_PRIVILEGED=0":

  • turn off the "privileged" flag for build pods
  • set "io.openshift.builder=" and "io.kubernetes.cri-o.Devices=/dev/fuse:rwm" annotations
  • set "io.kubernetes.cri-o.userns-mode=auto:size=65536"

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 17, 2021
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nalind
To complete the pull request process, please assign bparees after the PR has been reviewed.
You can assign the PR to them by writing /assign @bparees in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@nalind nalind force-pushed the unprivileged branch 3 times, most recently from a279a97 to a19460f Compare March 18, 2021 18:54
@nalind
Copy link
Member Author

nalind commented Mar 23, 2021

/retest

@nalind
Copy link
Member Author

nalind commented Apr 29, 2021

/retest

@nalind
Copy link
Member Author

nalind commented Aug 25, 2021

/retest

@nalind nalind force-pushed the unprivileged branch 2 times, most recently from 5297fb4 to 5e11e89 Compare September 16, 2021 17:10
@nalind nalind force-pushed the unprivileged branch 2 times, most recently from 16754f5 to 3251751 Compare October 19, 2021 14:39
@nalind
Copy link
Member Author

nalind commented Oct 20, 2021

/retest

@nalind nalind force-pushed the unprivileged branch 2 times, most recently from 4ba3553 to 3ac5109 Compare December 3, 2021 19:34
@nalind
Copy link
Member Author

nalind commented Dec 5, 2021

/retest-required

@nalind
Copy link
Member Author

nalind commented Dec 7, 2021

/retest

@nalind
Copy link
Member Author

nalind commented May 2, 2022

/retest

@nalind
Copy link
Member Author

nalind commented May 4, 2022

/retest

1 similar comment
@nalind
Copy link
Member Author

nalind commented May 4, 2022

/retest

@nalind
Copy link
Member Author

nalind commented May 6, 2022

  • Have reverted to making securityContext.RunAsUser and securityContext.RunAsGroup implicit for unprivileged builds - if they're set explicitly, the service account token's permissions become 0600 instead of 0644, and the builder can't read them.
  • Setting up a user namespace with the current set of UIDs/GIDs mapped to themselves (or more specifically, UID 0) to get CAP_SYS_ADMIN over their own new mount namespaces requires CAP_SETFCAP starting in upstream kernel 5.12 and RHEL kernel 4.18.0-312.el8. Added it to the unprivileged build security context.

@nalind
Copy link
Member Author

nalind commented May 6, 2022

  • When the pull secret is owned by root with mode 0o600, an unprivileged builder can't read it. Tweaked pkg/build/controller/strategy.mountVolume() to use mode 0o644 when the security context isn't privileged.

@nalind
Copy link
Member Author

nalind commented May 10, 2022

/retest

@nalind
Copy link
Member Author

nalind commented May 25, 2022

/retest

@adambkaplan
Copy link
Contributor

@coreydaley if you'd like, I can set up this repo for "no-feature-freeze" so you don't need a BZ to merge.

@coreydaley
Copy link

@adambkaplan That would be great, thanks!

If the BUILD_PRIVILEGED variable is set to "false" or 0 as an integer,
set the "Privileged" flag on docker/sti/custom build pod
SecurityContexts to false, and put CAP_MKNOD and CAP_KILL in the list of
capabilities to drop.  If it is set to "true" or non-zero, the
"Privileged" flag will be set to true.  If it is not set, we still
default to privileged.

Stop setting BUILD_STORAGE_DRIVER, since we're going to expect the
builder image to figure this out on its own.

Mount an emptyDir volume at /var/run/containers in
setupContainersStorage(), so that we can ensure its permissions will be
set so that the build user will always be able to write to it.

Move the emptyDir volume for layers from /var/lib/containers/storage to
/var/lib/containers, so that the build user can create the storage
directory inside of it with exactly the permissions that the build user
wants.

When the pod security context is not privileged, configure the build pod
by setting annotations that should instruct CRI-O to run the pod in a
user namespace using mappings that CRI-O selects, and to provide the
build pod with a working /dev/fuse.

Set the seccomp profile for build pods to "unconfined", since we depend
on unshare() and CRI-O is attempting to move away from allowing it to be
used in its default seccomp profile.

Signed-off-by: Nalin Dahyabhai <[email protected]>
@nalind
Copy link
Member Author

nalind commented Jun 13, 2022

/retest

2 similar comments
@nalind
Copy link
Member Author

nalind commented Jun 13, 2022

/retest

@nalind
Copy link
Member Author

nalind commented Jun 14, 2022

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 14, 2022

@nalind: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@coreydaley
Copy link

/lgtm
/label docs-approved
/label px-approved
/label qe-approved
/approve

@openshift-ci openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Jul 18, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 18, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 18, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, coreydaley, nalind, RickJWagner, rolfedh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coreydaley
Copy link

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 18, 2022
@openshift-ci openshift-ci bot merged commit a419bbd into openshift:master Jul 18, 2022
@nalind nalind deleted the unprivileged branch July 18, 2022 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. docs-approved Signifies that Docs has signed off on this PR lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants