Skip to content
This repository has been archived by the owner on Jun 29, 2022. It is now read-only.

PSP: Rename restricted to zz-minimal #293

Merged
merged 3 commits into from
Apr 22, 2020

Conversation

surajssd
Copy link
Member

@surajssd surajssd commented Apr 15, 2020

  • This PR renames the PSP restricted to zz-minimal, so that it is the last one in the list of PSPs but before the privileged PSP. restricted is applicable to all the workloads in the cluster. If a component already ships a PSP then that specific PSP should be the one that is applied to the workload, not the restricted PSP. So keeping it last in the list makes sure that pod is not filtered under restricted PSP. This was discovered when restricted PSP was interfering with the rook PSP.

  • This PR renames the PSP privileged to zz-privileged. This is done in order to avoid the situation where privileged PSP is picked up accidently even though a specific, small scoped (in terms of permissions) PSP exists.

  • openebs: Rename PSP usage. Use policy zz-privileged and not privileged.

@surajssd surajssd requested a review from invidian April 15, 2020 11:38
@surajssd surajssd force-pushed the surajssd/update-restricted-psp-name branch from d37e78f to 982eba1 Compare April 15, 2020 11:38
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

Wait, now that I think about it, shouldn't privileged be renamed to zz-privileged, so it is being picked up as a last effort?

@surajssd surajssd force-pushed the surajssd/update-restricted-psp-name branch 2 times, most recently from d449faa to c88cfb4 Compare April 15, 2020 14:59
@iaguis
Copy link
Contributor

iaguis commented Apr 17, 2020

CI is failing, can you retrigger?

@surajssd
Copy link
Member Author

I have a hunch that openebs fails and am currently testing this issue locally.

@invidian
Copy link
Member

@surajssd shouldn't restricted be named aa-restricted or something? So if it's compatible with with some pod, it will be used?

@surajssd
Copy link
Member Author

@surajssd shouldn't restricted be named aa-restricted or something? So if it's compatible with with some pod, it will be used?

No, it will cause problems. So right now any workload in the cluster has at least one policy that it can go through i.e. restricted PSP. And it happens because we have this binding here:

---
kind: ClusterRoleBinding
apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: restricted-psp-system-authenticated
roleRef:
kind: ClusterRole
name: restricted-psp
apiGroup: rbac.authorization.k8s.io
subjects:
- kind: Group
name: system:authenticated
apiGroup: rbac.authorization.k8s.io

Now imagine a workload which has it's own PSP. Now that workload is eligible for two PSPs one is its own and another is restricted one. Now the problem here is if restricted PSP takes precedence(in alphabetical order) for the application then that workload might not work reliably. Because the PSP that was shipped with the component is not applied but restricted PSP is applied, which greatly hinders what an app in the pod can do.

So there are two ways to solve this problem either we remove the aforementioned binding:

subjects:
- kind: Group
name: system:authenticated
apiGroup: rbac.authorization.k8s.io

By removing this binding we withdraw the ability of an application workload to work out of the box in Lokomotive, especially those that don't ship PSP. So we will have to create Rolebinding as needed. That is provide restricted PSP on a case by case basis. Like someone has to create a Rolebinding when they create a new namespace as follows:

kind: RoleBinding
apiVersion: rbac.authorization.k8s.io/v1
metadata:
  name: restricted-psp-mywebapp
  namespace: mywebapp
roleRef:
  kind: ClusterRole
  name: restricted-psp
  apiGroup: rbac.authorization.k8s.io
subjects:
- kind: ServiceAccount
  name: default
  namespace: mywebapp

OR we solve the problem by making sure the ordering is correct in the PSP names like it is done in this PR.


Now we have to make a choice to be specific a trade off between usability and security here.

@surajssd surajssd requested a review from ipochi as a code owner April 17, 2020 12:35
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

I got one more idea.

@@ -1,7 +1,7 @@
apiVersion: policy/v1beta1
kind: PodSecurityPolicy
metadata:
name: restricted
name: zz-restricted
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could name it to zz-minimal, so then it goes before zz-privileged?

@surajssd surajssd force-pushed the surajssd/update-restricted-psp-name branch 2 times, most recently from 7979d51 to 4b76f92 Compare April 20, 2020 08:24
@surajssd surajssd mentioned this pull request Apr 20, 2020
@surajssd surajssd force-pushed the surajssd/update-restricted-psp-name branch 4 times, most recently from 07ad278 to cc28d84 Compare April 21, 2020 07:42
@surajssd surajssd requested a review from invidian April 21, 2020 11:37
invidian
invidian previously approved these changes Apr 21, 2020
This commit renames the PSP `restricted` to `zz-minimal`, so that it
is the last one in the list of PSPs but before the `privileged` PSP.
`restricted` is applicable to all the workloads in the cluster.

If a component already ships a PSP then that specific PSP should be the
one that is applied to the workload, not the `restricted` PSP. So
keeping it last in the list makes sure that pod is not filtered under
`restricted` PSP.

This was discovered when `restricted` PSP was interfering with the
`rook` PSP.

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
This commit renames the PSP `privileged` to `zz-privileged`. This is
done in order to avoid the situation where `privileged` PSP is picked up
accidently even though a specific, small scoped (in terms of
permissions) PSP exists.

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
@surajssd surajssd force-pushed the surajssd/update-restricted-psp-name branch 4 times, most recently from 052b0f3 to 2d2e275 Compare April 22, 2020 08:28
@surajssd surajssd force-pushed the surajssd/update-restricted-psp-name branch from 2d2e275 to 0999143 Compare April 22, 2020 09:37
Use policy `zz-privileged` and not `privileged`.

Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
@surajssd surajssd force-pushed the surajssd/update-restricted-psp-name branch from 0999143 to 2e35e94 Compare April 22, 2020 10:41
@surajssd surajssd requested a review from invidian April 22, 2020 12:29
@surajssd surajssd changed the title PSP: Rename restricted to zz-restricted PSP: Rename restricted to zz-minimal Apr 22, 2020
@surajssd surajssd merged commit 6e3b6df into master Apr 22, 2020
@surajssd surajssd deleted the surajssd/update-restricted-psp-name branch April 22, 2020 13:06
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.

3 participants