Lock down package server runtime environment #1073

javanthropus · 2019-10-12T02:55:15Z

Description of the change:
This PR modifes the package server component's deployment manifest's pods to:

Run as non-root (uid 1000) by default
Mount a emptyDir volume to /tmp so that the root filesystem can be read-only

Motivation for the change:
At Indeed we're using restrictive pod security policies that among other things prevent pods running as root and marks them to have read-only root filesystems.

Reviewer Checklist

Implementation matches the proposed design, or proposal is updated to match implementation
Sufficient unit test coverage
Sufficient end-to-end test coverage
Docs updated or added to /docs
Commit messages sensible and descriptive

openshift-ci-robot · 2019-10-12T02:55:35Z

Hi @javanthropus. Thanks for your PR.

I'm waiting for a operator-framework or openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

jpeeler · 2019-10-12T15:12:31Z

/ok-to-test

ecordell · 2019-10-13T22:34:38Z

/retest

deploy/chart/templates/_packageserver.deployment-spec.yaml

ecordell · 2019-10-14T15:34:57Z

Thanks @javanthropus !

/lgtm

ecordell · 2019-10-14T15:37:14Z

/hold

Sorry, I thought you had removed the default runAsUser. Could you remove that as well?

If you can explain more about your deploy target we can maybe find a better place to put those values? It might make sense for our deploy/upstream/values.yaml?

(OpenShift randomizes uids on deployment, so setting this is actually not what we want for OCP releases)

javanthropus · 2019-10-14T19:23:01Z

The clusters we're building have a default pod security policy applied such that pods are not allowed to run as root and their root filesystems are mounted read-only. Another way to address this problem would have been to apply a pod security policy and corresponding role and role binding for packageserver to use; however, since pod security policies are still a beta resource, I figured that locking down packageserver was the safer bet. Plus, running it this way is a bit safer should there be a vulnerability in it.

If there is a different file in which to place the default value, I'll move it over there. I'm not familiar enough with this project to know which is the best to choose.

javanthropus · 2019-10-14T21:07:06Z

I've been doing more testing against our clusters, and it actually looks like this change may not be necessary in the first place. The package server pods run with the olm-operator-serviceaccount service account, and that service account is basically granted full admin access over the cluster, which allows it to use all available pod security policies.

At this point, I'm not sure anymore what was broken in my test cluster that made me think I needed to make this change. I'm going to do more testing, and I'll close this PR if it really proves to be unnecessary, unless you would like to keep the changes for the security benefits alone.

javanthropus · 2019-10-15T13:45:29Z

I tested things out with the stock 0.12.0 release on a fresh cluster, and none of my changes were necessary to work around the pod security policies. Sadly, our most permissive policy is used, but that's a result of how the policy admission controller works and the broad permissions granted to the service account used by packageserver.

I would still like to see this MR accepted since it still helps reduce the attack surface of packageserver. If you agree, let me know where to move that default runAsUser setting; otherwise, please feel free to close this out.

deploy/chart/values.yaml

njhale · 2019-10-29T14:19:48Z

/retest

javanthropus · 2019-10-30T16:22:50Z

I do not believe the changes in this PR should cause the test failures being reported. Please let me know if I'm wrong about that though.

jpeeler · 2019-10-31T02:55:10Z

/retest

jpeeler · 2019-12-06T16:54:49Z

/hold cancel
/lgtm

jpeeler · 2019-12-06T19:22:18Z

/retest

jpeeler · 2019-12-11T05:34:56Z

/retest

openshift-ci-robot · 2020-03-14T14:20:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ecordell, javanthropus, jpeeler

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ecordell]

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

openshift-bot · 2020-03-14T17:36:20Z

/retest