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

Extend workspace SA permissions to all secrets/configmaps in namespace #1165

Merged
merged 2 commits into from
Oct 3, 2023

Conversation

amisevsk
Copy link
Collaborator

What does this PR do?

Update the default permissions granted to DevWorkspaces to allow read/write access for all secrets/configmaps in the workspace's namespace.

We should decide if the privilege escalation here represents an issue before merging this PR. With this change, permissions to create DevWorkspaces = permissions to read/write all secrets/configmaps in the current namespace.

What issues does this PR fix or reference?

No issue currently.

Is it tested? How?

Changes are pushed to quay.io/amisevsk/devworkspace-controller:workspace-permissions

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 the default permissions granted to DevWorkspaces to allow
read/write access for all secrets/configmaps in the workspace's
namespace.

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.02% ⚠️

Comparison is base (65bdb9e) 52.53% compared to head (3d7b38a) 52.52%.

❗ Current head 3d7b38a differs from pull request most recent head 33dc72c. Consider uploading reports for the commit 33dc72c to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1165      +/-   ##
==========================================
- Coverage   52.53%   52.52%   -0.02%     
==========================================
  Files          82       82              
  Lines        7460     7458       -2     
==========================================
- Hits         3919     3917       -2     
- Misses       3260     3261       +1     
+ Partials      281      280       -1     
Files Changed Coverage Δ
pkg/provision/workspace/rbac/role.go 95.14% <100.00%> (-0.10%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@RomanNikitenko
Copy link

RomanNikitenko commented Aug 21, 2023

@amisevsk
I still have HTTP request failed error when I use this.k8sConfig.loadFromCluster(); to get git-credentials secret

image

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Copy link
Collaborator Author

Looking Roman's changes for testing, the issue is that the new permissions still don't contain the list verb; an in-cluster config should be able to only get, create, patch, and delete secrets.

I've pushed an updated image to quay.io/amisevsk/devworkspace-controller:workspace-permissions with additional update and list permissions.

@RomanNikitenko
Copy link

@amisevsk
you are right, Angel, I've missed that you've provided permissions for get, create, patch, and delete, but I was testing list, sorry...

I've tested it again and can confirm that now it's possible to get git-credentials secret from a VS Code extension side using this.k8sConfig.loadFromCluster(); - thank you!

@RomanNikitenko
Copy link

RomanNikitenko commented Sep 18, 2023

@amisevsk @l0rd
Can it be opened for the review according to eclipse-che/che#22139 (comment)?

@amisevsk amisevsk changed the title [WIP] Extend workspace SA permissions to all secrets/configmaps in namespace Extend workspace SA permissions to all secrets/configmaps in namespace Oct 2, 2023
@amisevsk
Copy link
Collaborator Author

amisevsk commented Oct 2, 2023

After discussion, we've decided to go ahead with this change. @l0rd @AObuchow @RomanNikitenko please review this PR when you have time.

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.

LGTM: my (potentially naive) thoughts are that all workspaces created by Che are in a user-owned namespace, and that any resources in that namespace are available to the user anyhow.

If someone, were to grant another user access to their workspace (maybe through some pair-programming extension) then this could be potentially abused.

@openshift-ci
Copy link

openshift-ci bot commented Oct 3, 2023

@RomanNikitenko: changing LGTM is restricted to collaborators

In response to this:

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.

@openshift-ci
Copy link

openshift-ci bot commented Oct 3, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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 amisevsk merged commit 96b9c65 into devfile:main Oct 3, 2023
1 check passed
@amisevsk amisevsk deleted the workspace-permissions branch October 3, 2023 20:50
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.

3 participants