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

feat: Advanced authorization #1789

Merged
merged 6 commits into from
Dec 4, 2023
Merged

feat: Advanced authorization #1789

merged 6 commits into from
Dec 4, 2023

Conversation

tolusha
Copy link
Contributor

@tolusha tolusha commented Nov 28, 2023

What does this PR do?

feat: Advanced authorization

Screenshot/screencast of this PR

N/A

What issues does this PR fix or reference?

https://issues.redhat.com/browse/CRW-4805

How to test this PR?

  1. Prepare a patch file if needed:
cat > /tmp/cr-patch.yaml <<EOF
apiVersion: org.eclipse.che/v2
kind: CheCluster
spec:
  components:
    cheServer:
      deployment:
        containers:
        - image: quay.io/eclipse/che-server:pr-619
EOF
  1. Deploy the operator:

OpenShift

# user{x}:password
cat > /tmp/test-htpasswd <<EOF
user1:{SHA}W6ph5Mm5Pz8GgiULbPgzG37mj9g=
user2:{SHA}W6ph5Mm5Pz8GgiULbPgzG37mj9g=
user3:{SHA}W6ph5Mm5Pz8GgiULbPgzG37mj9g=
user4:{SHA}W6ph5Mm5Pz8GgiULbPgzG37mj9g=
user5:{SHA}W6ph5Mm5Pz8GgiULbPgzG37mj9g=
EOF
oc create secret generic test-htpass-secret --from-file=htpasswd=/tmp/test-htpasswd -n openshift-config
oc patch oauth cluster --type=merge -p '{"spec":{"identityProviders":[{"htpasswd":{"fileData":{"name":"test-htpass-secret"}},"mappingMethod":"claim","name":"test-htpasswd","type":"HTPasswd"}]}}'

oc apply -f - <<EOF
apiVersion: user.openshift.io/v1
kind: Group
metadata:
  name: test-group-1
users:
  - user1
EOF
  
oc apply -f - <<EOF
apiVersion: user.openshift.io/v1
kind: Group
metadata:
  name: test-group-2
users:
  - user3
EOF

./build/scripts/olm/test-catalog-from-sources.sh --cr-patch-yaml /tmp/cr-patch.yaml

oc patch checluster eclipse-che -n eclipse-che --type=merge -p='{"spec":{"networking":{"auth":{"advancedAuthorization":{"allowGroups":["test-group-1"],"allowUsers":["user2"],"denyGroups":["test-group-2"],"denyUsers":["user4","user5"]}}}}}'

on Minikube

./build/scripts/minikube-tests/test-operator-from-sources.sh --cr-patch-yaml /tmp/cr-patch.yaml

kubectl patch checluster eclipse-che -n eclipse-che --type=merge -p='{"spec":{"networking":{"auth":{"advancedAuthorization":{"allowUsers":["user1", "user2"],"denyUsers":["user3", "user4","user5"]}}}}}'
  1. Validation steps
    Users user1 and user2 are allowed to access Che.
    Users user3, user4 and user5 are disallowed to access Che

screenshot-eclipse-che apps ci-ln-bfdj75b-76ef8 aws-2 ci openshift org-2023 11 29-14_36_47

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

Signed-off-by: Anatolii Bazko <[email protected]>
Copy link

openshift-ci bot commented Nov 28, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link

codecov bot commented Nov 28, 2023

Codecov Report

Attention: 40 lines in your changes are missing coverage. Please review.

Comparison is base (14eed75) 59.53% compared to head (3bde4f2) 59.46%.
Report is 2 commits behind head on main.

Files Patch % Lines
api/v2/zz_generated.deepcopy.go 0.00% 34 Missing ⚠️
pkg/deploy/server/server_configmap.go 25.00% 5 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1789      +/-   ##
==========================================
- Coverage   59.53%   59.46%   -0.08%     
==========================================
  Files          71       71              
  Lines        8605     8703      +98     
==========================================
+ Hits         5123     5175      +52     
- Misses       3131     3172      +41     
- Partials      351      356       +5     

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

Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
@tolusha tolusha marked this pull request as ready for review November 29, 2023 14:07
Signed-off-by: Anatolii Bazko <[email protected]>
Signed-off-by: Anatolii Bazko <[email protected]>
@tolusha
Copy link
Contributor Author

tolusha commented Dec 1, 2023

/retest

Signed-off-by: Anatolii Bazko <[email protected]>
Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

LGTM; haven't tested changes.

To double check -- these changes (along with the server-side changes) won't block access to the dashboard / gateway, correct?

Comment on lines +67 to +72
- apiGroups:
- user.openshift.io
resources:
- groups
verbs:
- get
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering -- does the Che Operator not use +kubebuilder:rbac markers to define controller permissions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't use annotations like this to define permissions.

Comment on lines +148 to +162
{
APIGroups: []string{""},
Resources: []string{"serviceaccounts"},
Verbs: []string{"get", "watch", "create"},
},
{
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"roles"},
Verbs: []string{"get", "create", "update"},
},
{
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"rolebindings"},
Verbs: []string{"get", "create", "update", "delete"},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand the three functions here any why they are separated; these all apply to the Che server SA, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Operator delegates permissions to a che-server SA and the che-server SA delegates permissions to a user.
But some permissions are only needed for the che-server SA and not for the user.
That's we have separation for cluster roles: some permissions for the user and some permissions only for che-server SA. I hope I was clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe some permissions are redundant since moving to the Dev Workspace engine.
But I am not sure right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 makes sense, thanks.

@tolusha
Copy link
Contributor Author

tolusha commented Dec 4, 2023

/retest

@tolusha
Copy link
Contributor Author

tolusha commented Dec 4, 2023

@amisevsk

To double check -- these changes (along with the server-side changes) won't block access to the dashboard / gateway, correct?

You are right, it doesn't block any access to dashboard/gateway.

Copy link

openshift-ci bot commented Dec 4, 2023

@tolusha: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/v12-devworkspace-happy-path f499799 link true /test v12-devworkspace-happy-path

Full PR test history. Your PR dashboard.

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.

Copy link

openshift-ci bot commented Dec 4, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

The full list of commands accepted by this bot can be found 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

@tolusha tolusha merged commit d99f892 into main Dec 4, 2023
13 checks passed
@tolusha tolusha deleted the CRW-4805 branch December 4, 2023 16:09
@devstudio-release
Copy link

Build 3.11 :: operator_3.x/327: Console, Changes, Git Data

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.11 :: copyIIBsToQuay/2258: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.11 :: sync-to-downstream_3.x/5504: SUCCESS

Build container: devspaces-operator-bundle synced; /DS_CI/get-sources-rhpkg-container-build_3.x/5383 triggered; /job/DS_CI/job/dsc_3.x triggered;

@devstudio-release
Copy link

Build 3.11 :: operator-bundle_3.x/2364: SUCCESS

Upstream sync done; /DS_CI/sync-to-downstream_3.x/5504 triggered

@devstudio-release
Copy link

Build 3.11 :: dsc_3.x/1630: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.11 :: dsc_3.x/1630: SUCCESS

3.11.0-CI

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.11 :: copyIIBsToQuay/2259: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.11 :: sync-to-downstream_3.x/5506: SUCCESS

Build container: devspaces-operator-bundle synced; /DS_CI/get-sources-rhpkg-container-build_3.x/5385 triggered; /job/DS_CI/job/dsc_3.x triggered;

@devstudio-release
Copy link

Build 3.11 :: operator-bundle_3.x/2365: SUCCESS

Upstream sync done; /DS_CI/sync-to-downstream_3.x/5506 triggered

@devstudio-release
Copy link

Build 3.11 :: dsc_3.x/1631: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.11 :: dsc_3.x/1631: SUCCESS

3.11.0-CI

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.11 :: copyIIBsToQuay/2260: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.11 :: sync-to-downstream_3.x/5509: SUCCESS

Build container: devspaces-operator-bundle synced; /DS_CI/get-sources-rhpkg-container-build_3.x/5388 triggered; /job/DS_CI/job/dsc_3.x triggered;

@devstudio-release
Copy link

Build 3.11 :: operator-bundle_3.x/2366: SUCCESS

Upstream sync done; /DS_CI/sync-to-downstream_3.x/5509 triggered

@devstudio-release
Copy link

Build 3.11 :: dsc_3.x/1632: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.11 :: dsc_3.x/1632: SUCCESS

3.11.0-CI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants