Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update verbs used to check SAR for users when k8s components are used #1050

Merged
merged 1 commit into from
Feb 22, 2023

Conversation

amisevsk
Copy link
Collaborator

What does this PR do?

Update SAR checks for user permissions in webhook server to check whether a user can get/create/update/delete the resource rather than checking for '' permissions. This is required as even if the user has the admin rolebinding, they do not have '' permissions from the perspective of the cluster.

What issues does this PR fix or reference?

Closes #1049

Is it tested? How?

Full testing would include verifying that

  • Webhooks server can manage load of doing 4 SAR checks instead of 1, and this doesn't overtax the cluster API
  • There's minimal risk of privilege escalation in checking only get, create, update, delete instead of *
    However, the risks of issues with the points above is minimal:
  • DWO only performs SAR checks when the resource is created or updated to include modified k8s components, so unless the component is being modified the checks should only be performed once per DevWorkspace
  • While DWO checks whether it has * permissions, it only performs the get, create, update, and delete verbs.

Basic testing:

  1. Set up user in an OpenShift cluster without any permissions
  2. Create a namespace as the new user
  3. Verify that user has admin permissions but not * permissions for pods: oc auth can-i '*' pods -n <namespace> should print "no"
  4. Create a DevWorkspace that uses a Kubernetes component containing a pod:
    kind: DevWorkspace
    apiVersion: workspace.devfile.io/v1alpha2
    metadata:
      name: plain-devworkspace
    spec:
      started: true
      routingClass: 'basic'
      template:
        components:
          - name: web-terminal
            container:
              image: quay.io/wto/web-terminal-tooling:next
              memoryLimit: 512Mi
              mountSources: true
              command:
                - "tail"
                - "-f"
                - "/dev/null"
          - name: test-k8s-component
            kubernetes:
              deployByDefault: true
              inlined: |
                apiVersion: v1
                kind: Pod
                metadata:
                  name: test-pod
                spec:
                  containers:
                    - image: quay.io/wto/web-terminal-tooling:next
                      name: k8s-component-pod
                      command:
                        - "tail"
                        - "-f"
                        - "/dev/null"
                      ports:
                        - name: test-port
                          containerPort: 8080

PR Checklist

  • E2E tests pass (when PR is ready, comment /test v8-devworkspace-operator-e2e, v8-che-happy-path to trigger)
    • v8-devworkspace-operator-e2e: DevWorkspace e2e test
    • v8-che-happy-path: Happy path for verification integration with Che

Update SAR checks for user permissions in webhook server to check
whether a user can get/create/update/delete the resource rather than
checking for '*' permissions. This is required as even if the user has
the admin rolebinding, they do not have '*' permissions from the
perspective of the cluster.

Signed-off-by: Angel Misevski <[email protected]>
@codecov
Copy link

codecov bot commented Feb 21, 2023

Codecov Report

Base: 50.98% // Head: 50.76% // Decreases project coverage by -0.23% ⚠️

Coverage data is based on head (e47c5ed) compared to base (2cdd390).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1050      +/-   ##
==========================================
- Coverage   50.98%   50.76%   -0.23%     
==========================================
  Files          75       75              
  Lines        6470     6477       +7     
==========================================
- Hits         3299     3288      -11     
- Misses       2907     2923      +16     
- Partials      264      266       +2     
Impacted Files Coverage Δ
webhook/workspace/handler/kubernetes.go 0.00% <0.00%> (ø)
controllers/workspace/status.go 75.58% <0.00%> (-2.91%) ⬇️
controllers/workspace/devworkspace_controller.go 60.45% <0.00%> (-1.23%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@AObuchow AObuchow left a comment

Choose a reason for hiding this comment

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

Code looks good to me.

I followed the basic testing instructions on OpenShift 4.12:

  • created a user without any permissions, and made a "test-project" namespace/project as the user
  • The result of oc auth can-i '*' pods -n test-project was no
  • Created the DevWorkspace that uses a Kubernetes component containing a pod
  • Checked I was able to also oc get, oc describe, oc edit and oc delete the workspace

Webhooks server can manage load of doing 4 SAR checks instead of 1, and this doesn't overtax the cluster API

I may be wrong, but isn't this being tested by the fact the updated code is working as intended? (I didn't see any error logs regarding this on the webhook server or the controller manager)

@openshift-ci
Copy link

openshift-ci bot commented Feb 22, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, AObuchow, ibuziuk

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:

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

@amisevsk
Copy link
Collaborator Author

I may be wrong, but isn't this being tested by the fact the updated code is working as intended? (I didn't see any error logs regarding this on the webhook server or the controller manager)

The question is whether this becomes a problem when there are e.g. 1000 workspaces across 1000 namespaces. The webhook server is now making 5 SAR requests (4 SAR, 1 SSAR) instead of 2. However, we're (hopefully) careful to only check for RBAC when k8s components are added/modified, so it should still be a minimal amount of checks overall.

@amisevsk amisevsk merged commit 66c3f3f into devfile:main Feb 22, 2023
@amisevsk amisevsk deleted the rbac-checking branch February 22, 2023 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Checking permissions for using Kubernetes/OpenShift components fails for regular users
3 participants