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.

Sign up for GitHub

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

Jump to bottom

✨ Add original user/group as extra to the impersonating client used by virtual workspace #3155

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

✨ Add original user/group as extra to the impersonating client used by virtual workspace #3155

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion pkg/virtual/apiexport/builder/build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,16 @@ import (
kcpinformers "github.com/kcp-dev/kcp/sdk/client/informers/externalversions"
)

const VirtualWorkspaceName string = "apiexport"
const (
// VirtualWorkspaceName is the name of the virtual workspace.
VirtualWorkspaceName string = "apiexport"
// OriginalUserAnnotationKey is the key used in a user's "extra" to
// specify the original user of the authenticating request.
OriginalUserAnnotationKey = "authorization.kcp.io/original-username"
// OriginalGroupsAnnotationKey is the key used in a user's "extra" to
// specify the original groups of the authenticating request.
OriginalGroupsAnnotationKey = "authorization.kcp.io/original-groups"
)

func BuildVirtualWorkspace(
rootPathPrefix string,
Expand Down Expand Up @@ -113,6 +122,15 @@ func BuildVirtualWorkspace(
serviceaccount.ClusterNameKey: {cluster.Name.Path().String()},
},
}

if user, ok := genericapirequest.UserFrom(ctx); ok {
// We pass the original user and groups as extra fields to
// the impersonation config so that the receiver can make
// decisions based on the original user/groups.
impersonationConfig.Impersonate.Extra[OriginalUserAnnotationKey] = []string{user.GetName()}
impersonationConfig.Impersonate.Extra[OriginalGroupsAnnotationKey] = user.GetGroups()
Copy link
Member

Choose a reason for hiding this comment

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

trying to wrap my head around whether we actually want to preserve the original groups as part of the impersonated identity.

What we really want is this:

  • inside of the virtualized workspace this user should have the admin CRUD permissions because the authorization is done by the virtual workspace.
  • outside of the virtualized workspace (e.g. in admission webhooks) we want the actually export owner identity to be effective.

I think we have that partially somewhere called "soft impersonation". Have to double check our thinking there.

🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIR I also believe we should leave groups out. we are really only interested in the person's identity. Groups are per-workspace and not global.

Take this with a grain of salt: it's been a while.

Copy link
Author

Choose a reason for hiding this comment

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

outside of the virtualized workspace (e.g. in admission webhooks) we want the actually export owner identity to be effective.

Owner identity is both its username and groups. Even we add its groups to the list of groups, it would be partial, permissions might have been granted to its user only. So, on the admission side, I would rather rely on extra where I can find both username/group.

}

impersonatedClient, err := kcpdynamic.NewForConfig(impersonationConfig)
if err != nil {
return nil, fmt.Errorf("error generating dynamic client: %w", err)
Expand Down