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

authorization: clarify/fix service accounts and workspace initialization #1261

Merged
merged 4 commits into from
Jun 17, 2022

Conversation

s-urbaniak
Copy link
Contributor

@s-urbaniak s-urbaniak commented Jun 13, 2022

This PR adds:

  • handling of service accounts. These are are granted only to workspaces they are declared in.
  • handling of root workspace access, only service accounts defined in root and any authenticated user is granted
  • handling of initialising workspaces: only subjects granted admin access will be granted.
  • unit tests for the workspace authorizer

follow-up for #996

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 13, 2022

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

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 13, 2022
@openshift-ci openshift-ci bot requested review from stevekuznetsov and sttts June 13, 2022 15:21
@s-urbaniak
Copy link
Contributor Author

/test all

Copy link
Contributor

@stevekuznetsov stevekuznetsov left a comment

Choose a reason for hiding this comment

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

We're about to rip out how the initializer bits work, maybe not worth the time to mess around with this?


By default, workspaces are only accessible to a user if they are in phse `Ready`. Workspaces that are initializing
can be access only by users that are granted `initialize` verb on the `clusterworkspaces/content` resource in the
parent workspace.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is necessarily what we want ...

  1. some RBAC (where???) gives you global access to the initializingworkspaces virtual workspace, where you can see all workspaces that need your initializer
  2. as a first pass, actions through that workspace impersonate the user who created the workspace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as discussed OOB: handling service accounts from parent workspaces won't be handled in this PR.

docs/authorization.md Outdated Show resolved Hide resolved

Kubernetes service accounts is granted access to the workspaces they are defined in and that are ready.

E.g. a service account "default" in `root:org:ws:ws` is granted access to `root:org:ws:ws`, and through the
Copy link
Contributor

Choose a reason for hiding this comment

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

Why double-ws?

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2022
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 15, 2022
@s-urbaniak s-urbaniak force-pushed the pr-996 branch 2 times, most recently from b0b3c5c to 9d88afb Compare June 15, 2022 10:54
@s-urbaniak s-urbaniak changed the title WIP: authorization: clarify/fix service accounts and workspace initialization authorization: clarify/fix service accounts and workspace initialization Jun 15, 2022
@s-urbaniak s-urbaniak marked this pull request as ready for review June 15, 2022 10:54
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 15, 2022
@openshift-ci openshift-ci bot requested a review from ncdc June 15, 2022 10:55
@s-urbaniak s-urbaniak force-pushed the pr-996 branch 3 times, most recently from 80d7f75 to 518d7e1 Compare June 17, 2022 11:43
### Initializing Workspaces

By default, workspaces are only accessible to a user if they are in `Ready` phase. Workspaces that are initializing
can be access only by users that are granted `admin` verb on the `clusterworkspaces/content` resource in the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
can be access only by users that are granted `admin` verb on the `clusterworkspaces/content` resource in the
can be accessed only by users that are granted `admin` verb on the `clusterworkspaces/content` resource in the

cluster, err := genericapirequest.ValidClusterFrom(ctx)
if err != nil {
return authorizer.DecisionNoOpinion, fmt.Sprintf("%q workspace access not permitted", cluster.Name), err
}
if cluster == nil || cluster.Name.Empty() {
// empty or non-root based workspaces have no meaning in the context of authorizing workspace content.
if cluster == nil || cluster.Name.Empty() || !cluster.Name.HasPrefix(tenancyv1alpha1.RootCluster) {
return authorizer.DecisionNoOpinion, fmt.Sprintf("%q workspace access not permitted", cluster.Name), nil
Copy link
Member

Choose a reason for hiding this comment

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

follow-up: workspaceAccessNotPermittedReason everywhere.

@sttts
Copy link
Member

sttts commented Jun 17, 2022

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 17, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: s-urbaniak, sttts

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 17, 2022
@openshift-ci openshift-ci bot merged commit 58918ab into kcp-dev:main Jun 17, 2022
@s-urbaniak s-urbaniak deleted the pr-996 branch June 17, 2022 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants