Skip to content

Commit 7035800

Browse files
s-urbaniaks-urbaniak
committed
pkg/virtual/apiexport: impersonate requests
Currently, privilege escalation can be provoked when creating APIBindings for exported resources. This fixes it by impersonating virtual API export requests with a service account that is bound to the requested workspace.
1 parent 2ed4948 commit 7035800

File tree

10 files changed

+279
-32
lines changed

10 files changed

+279
-32
lines changed

‎pkg/virtual/apiexport/builder/build.go

+37-3
Original file line numberDiff line numberDiff line change
@@ -27,14 +27,17 @@ import (
2727
"github.com/kcp-dev/logicalcluster/v3"
2828

2929
"k8s.io/apimachinery/pkg/labels"
30+
"k8s.io/apiserver/pkg/authentication/serviceaccount"
3031
"k8s.io/apiserver/pkg/authorization/authorizer"
3132
genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
3233
genericapiserver "k8s.io/apiserver/pkg/server"
34+
"k8s.io/client-go/rest"
3335
"k8s.io/client-go/tools/cache"
3436
"k8s.io/klog/v2"
3537

3638
apisv1alpha1 "github.com/kcp-dev/kcp/pkg/apis/apis/v1alpha1"
3739
"github.com/kcp-dev/kcp/pkg/authorization"
40+
"github.com/kcp-dev/kcp/pkg/authorization/bootstrap"
3841
kcpclientset "github.com/kcp-dev/kcp/pkg/client/clientset/versioned/cluster"
3942
kcpinformers "github.com/kcp-dev/kcp/pkg/client/informers/externalversions"
4043
virtualapiexportauth "github.com/kcp-dev/kcp/pkg/virtual/apiexport/authorizer"
@@ -53,8 +56,8 @@ const VirtualWorkspaceName string = "apiexport"
5356

5457
func BuildVirtualWorkspace(
5558
rootPathPrefix string,
59+
cfg *rest.Config,
5660
kubeClusterClient, deepSARClient kcpkubernetesclientset.ClusterInterface,
57-
dynamicClusterClient kcpdynamic.ClusterInterface,
5861
kcpClusterClient kcpclientset.ClusterInterface,
5962
cachedKcpInformers kcpinformers.SharedInformerFactory,
6063
) ([]rootapiserver.NamedVirtualWorkspace, error) {
@@ -86,6 +89,37 @@ func BuildVirtualWorkspace(
8689
}),
8790

8891
BootstrapAPISetManagement: func(mainConfig genericapiserver.CompletedConfig) (apidefinition.APIDefinitionSetGetter, error) {
92+
dynamicClient, err := kcpdynamic.NewForConfig(cfg)
93+
if err != nil {
94+
return nil, fmt.Errorf("error creating privileged dynamic kcp client: %w", err)
95+
}
96+
97+
impersonatedDynamicClientGetter := func(ctx context.Context) (kcpdynamic.ClusterInterface, error) {
98+
cluster, err := genericapirequest.ValidClusterFrom(ctx)
99+
if err != nil {
100+
return nil, fmt.Errorf("error getting valid cluster from context: %w", err)
101+
}
102+
103+
// Wildcard requests cannot be impersonated against a concrete cluster.
104+
if cluster.Wildcard {
105+
return dynamicClient, nil
106+
}
107+
108+
impersonationConfig := rest.CopyConfig(cfg)
109+
impersonationConfig.Impersonate = rest.ImpersonationConfig{
110+
UserName: "system:serviceaccount:default:rest",
111+
Groups: []string{bootstrap.SystemKcpAdminGroup},
112+
Extra: map[string][]string{
113+
serviceaccount.ClusterNameKey: {cluster.Name.Path().String()},
114+
},
115+
}
116+
impersonatedClient, err := kcpdynamic.NewForConfig(impersonationConfig)
117+
if err != nil {
118+
return nil, fmt.Errorf("error generating dynamic client: %w", err)
119+
}
120+
return impersonatedClient, nil
121+
}
122+
89123
apiReconciler, err := apireconciler.NewAPIReconciler(
90124
kcpClusterClient,
91125
cachedKcpInformers.Apis().V1alpha1().APIResourceSchemas(),
@@ -100,7 +134,7 @@ func BuildVirtualWorkspace(
100134
})
101135
}
102136

103-
storageBuilder := provideDelegatingRestStorage(ctx, dynamicClusterClient, identityHash, wrapper)
137+
storageBuilder := provideDelegatingRestStorage(ctx, impersonatedDynamicClientGetter, identityHash, wrapper)
104138
def, err := apiserver.CreateServingInfoFor(mainConfig, apiResourceSchema, version, storageBuilder)
105139
if err != nil {
106140
cancelFn()
@@ -112,7 +146,7 @@ func BuildVirtualWorkspace(
112146
}, nil
113147
},
114148
func(ctx context.Context, clusterName logicalcluster.Name, apiExportName string) (apidefinition.APIDefinition, error) {
115-
restProvider, err := provideAPIExportFilteredRestStorage(ctx, dynamicClusterClient, clusterName, apiExportName)
149+
restProvider, err := provideAPIExportFilteredRestStorage(ctx, impersonatedDynamicClientGetter, clusterName, apiExportName)
116150
if err != nil {
117151
return nil, err
118152
}

‎pkg/virtual/apiexport/builder/forwarding.go

+4-5
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ import (
2020
"context"
2121
"fmt"
2222

23-
kcpdynamic "github.com/kcp-dev/client-go/dynamic"
2423
"github.com/kcp-dev/logicalcluster/v3"
2524

2625
"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
@@ -39,7 +38,7 @@ import (
3938
registry "github.com/kcp-dev/kcp/pkg/virtual/framework/forwardingregistry"
4039
)
4140

42-
func provideAPIExportFilteredRestStorage(ctx context.Context, clusterClient kcpdynamic.ClusterInterface, clusterName logicalcluster.Name, exportName string) (apiserver.RestProviderFunc, error) {
41+
func provideAPIExportFilteredRestStorage(ctx context.Context, dynamicClusterClientFunc registry.DynamicClusterClientFunc, clusterName logicalcluster.Name, exportName string) (apiserver.RestProviderFunc, error) {
4342
labelSelector := map[string]string{
4443
apisv1alpha1.InternalAPIBindingExportLabelKey: permissionclaims.ToAPIBindingExportLabelValue(clusterName, exportName),
4544
}
@@ -48,11 +47,11 @@ func provideAPIExportFilteredRestStorage(ctx context.Context, clusterClient kcpd
4847
return nil, fmt.Errorf("unable to create a selector from the provided labels")
4948
}
5049

51-
return registry.ProvideReadOnlyRestStorage(ctx, clusterClient, registry.WithStaticLabelSelector(requirements), nil)
50+
return registry.ProvideReadOnlyRestStorage(ctx, dynamicClusterClientFunc, registry.WithStaticLabelSelector(requirements), nil)
5251
}
5352

5453
// provideDelegatingRestStorage returns a forwarding storage build function, with an optional storage wrapper e.g. to add label based filtering.
55-
func provideDelegatingRestStorage(ctx context.Context, clusterClient kcpdynamic.ClusterInterface, apiExportIdentityHash string, wrapper registry.StorageWrapper) apiserver.RestProviderFunc {
54+
func provideDelegatingRestStorage(ctx context.Context, dynamicClusterClientFunc registry.DynamicClusterClientFunc, apiExportIdentityHash string, wrapper registry.StorageWrapper) apiserver.RestProviderFunc {
5655
return func(resource schema.GroupVersionResource, kind schema.GroupVersionKind, listKind schema.GroupVersionKind, typer runtime.ObjectTyper, tableConvertor rest.TableConvertor, namespaceScoped bool, schemaValidator *validate.SchemaValidator, subresourcesSchemaValidator map[string]*validate.SchemaValidator, structuralSchema *structuralschema.Structural) (mainStorage rest.Storage, subresourceStorages map[string]rest.Storage) {
5756
statusSchemaValidate, statusEnabled := subresourcesSchemaValidator["status"]
5857

@@ -86,7 +85,7 @@ func provideDelegatingRestStorage(ctx context.Context, clusterClient kcpdynamic.
8685
nil,
8786
tableConvertor,
8887
nil,
89-
clusterClient,
88+
dynamicClusterClientFunc,
9089
nil,
9190
wrapper,
9291
)

‎pkg/virtual/apiexport/options/options.go

+1-6
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ package options
1919
import (
2020
"path"
2121

22-
kcpdynamic "github.com/kcp-dev/client-go/dynamic"
2322
kcpkubernetesclientset "github.com/kcp-dev/client-go/kubernetes"
2423
"github.com/spf13/pflag"
2524

@@ -67,14 +66,10 @@ func (o *APIExport) NewVirtualWorkspaces(
6766
if err != nil {
6867
return nil, err
6968
}
70-
dynamicClusterClient, err := kcpdynamic.NewForConfig(config)
71-
if err != nil {
72-
return nil, err
73-
}
7469
deepSARClient, err := kcpkubernetesclientset.NewForConfig(authorization.WithDeepSARConfig(rest.CopyConfig(config)))
7570
if err != nil {
7671
return nil, err
7772
}
7873

79-
return builder.BuildVirtualWorkspace(path.Join(rootPathPrefix, builder.VirtualWorkspaceName), kubeClusterClient, deepSARClient, dynamicClusterClient, kcpClusterClient, cachedKcpInformers)
74+
return builder.BuildVirtualWorkspace(path.Join(rootPathPrefix, builder.VirtualWorkspaceName), config, kubeClusterClient, deepSARClient, kcpClusterClient, cachedKcpInformers)
8075
}

‎pkg/virtual/framework/forwardingregistry/rest.go

+5-7
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,6 @@ package forwardingregistry
1919
import (
2020
"context"
2121

22-
kcpdynamic "github.com/kcp-dev/client-go/dynamic"
23-
2422
structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema"
2523
"k8s.io/apiextensions-apiserver/pkg/registry/customresource"
2624
"k8s.io/apimachinery/pkg/api/validation/path"
@@ -71,7 +69,7 @@ func NewStorage(
7169
categories []string,
7270
tableConvertor rest.TableConvertor,
7371
replicasPathMapping fieldmanager.ResourcePathMappings,
74-
dynamicClusterClient kcpdynamic.ClusterInterface,
72+
dynamicClusterClientFunc DynamicClusterClientFunc,
7573
patchConflictRetryBackoff *wait.Backoff,
7674
wrapper StorageWrapper,
7775
) (mainStorage, statusStorage *StoreFuncs) {
@@ -99,7 +97,7 @@ func NewStorage(
9997
factory, listFactory, destroyer,
10098
strategy, tableConvertor,
10199
resource, apiExportIdentityHash, categories,
102-
dynamicClusterClient, []string{}, *patchConflictRetryBackoff, ctx.Done(),
100+
dynamicClusterClientFunc, []string{}, *patchConflictRetryBackoff, ctx.Done(),
103101
)
104102
if wrapper != nil {
105103
wrapper.Decorate(resource.GroupResource(), store)
@@ -110,7 +108,7 @@ func NewStorage(
110108
factory, listFactory, destroyer,
111109
statusStrategy, tableConvertor,
112110
resource, apiExportIdentityHash, categories,
113-
dynamicClusterClient, []string{"status"}, *patchConflictRetryBackoff, ctx.Done(),
111+
dynamicClusterClientFunc, []string{"status"}, *patchConflictRetryBackoff, ctx.Done(),
114112
)
115113
delegateUpdate := statusStore.UpdaterFunc
116114
statusStore.UpdaterFunc = func(ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, createValidation rest.ValidateObjectFunc, updateValidation rest.ValidateObjectUpdateFunc, forceAllowCreate bool, options *metav1.UpdateOptions) (runtime.Object, bool, error) {
@@ -126,7 +124,7 @@ func NewStorage(
126124

127125
// ProvideReadOnlyRestStorage returns a commonly used REST storage that forwards calls to a dynamic client,
128126
// but only for read-only requests.
129-
func ProvideReadOnlyRestStorage(ctx context.Context, clusterClient kcpdynamic.ClusterInterface, wrapper StorageWrapper, identities map[schema.GroupResource]string) (apiserver.RestProviderFunc, error) {
127+
func ProvideReadOnlyRestStorage(ctx context.Context, dynamicClusterClientFunc DynamicClusterClientFunc, wrapper StorageWrapper, identities map[schema.GroupResource]string) (apiserver.RestProviderFunc, error) {
130128
return func(
131129
resource schema.GroupVersionResource,
132130
kind schema.GroupVersionKind,
@@ -162,7 +160,7 @@ func ProvideReadOnlyRestStorage(ctx context.Context, clusterClient kcpdynamic.Cl
162160
nil,
163161
tableConvertor,
164162
nil,
165-
clusterClient,
163+
dynamicClusterClientFunc,
166164
nil,
167165
wrapper,
168166
)

‎pkg/virtual/framework/forwardingregistry/rest_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ func newStorage(t *testing.T, clusterClient kcpdynamic.ClusterInterface, apiExpo
112112
nil,
113113
table,
114114
nil,
115-
clusterClient,
115+
func(ctx context.Context) (kcpdynamic.ClusterInterface, error) { return clusterClient, nil },
116116
patchConflictRetryBackoff,
117117
forwardingregistry.StorageWrapperFunc(func(_ schema.GroupResource, store *forwardingregistry.StoreFuncs) {
118118
}))

‎pkg/virtual/framework/forwardingregistry/store.go

+18-5
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,8 @@ type Strategy interface {
6565
rest.ResetFieldsStrategy
6666
}
6767

68+
type DynamicClusterClientFunc func(ctx context.Context) (kcpdynamic.ClusterInterface, error)
69+
6870
func DefaultDynamicDelegatedStoreFuncs(
6971
factory FactoryFunc,
7072
listFactory ListFactoryFunc,
@@ -74,13 +76,13 @@ func DefaultDynamicDelegatedStoreFuncs(
7476
resource schema.GroupVersionResource,
7577
apiExportIdentityHash string,
7678
categories []string,
77-
dynamicClusterClient kcpdynamic.ClusterInterface,
79+
dynamicClusterClientFunc DynamicClusterClientFunc,
7880
subResources []string,
7981
patchConflictRetryBackoff wait.Backoff,
8082
stopWatchesCh <-chan struct{},
8183
) *StoreFuncs {
82-
client := clientGetter(dynamicClusterClient, strategy.NamespaceScoped(), resource, apiExportIdentityHash)
83-
listerWatcher := listerWatcherGetter(dynamicClusterClient, strategy.NamespaceScoped(), resource, apiExportIdentityHash)
84+
client := clientGetter(dynamicClusterClientFunc, strategy.NamespaceScoped(), resource, apiExportIdentityHash)
85+
listerWatcher := listerWatcherGetter(dynamicClusterClientFunc, strategy.NamespaceScoped(), resource, apiExportIdentityHash)
8486
s := &StoreFuncs{}
8587
s.FactoryFunc = factory
8688
s.ListFactoryFunc = listFactory
@@ -263,18 +265,24 @@ func DefaultDynamicDelegatedStoreFuncs(
263265
return s
264266
}
265267

266-
func clientGetter(dynamicClusterClient kcpdynamic.ClusterInterface, namespaceScoped bool, resource schema.GroupVersionResource, apiExportIdentityHash string) func(ctx context.Context) (dynamic.ResourceInterface, error) {
268+
func clientGetter(dynamicClusterClientFunc DynamicClusterClientFunc, namespaceScoped bool, resource schema.GroupVersionResource, apiExportIdentityHash string) func(ctx context.Context) (dynamic.ResourceInterface, error) {
267269
return func(ctx context.Context) (dynamic.ResourceInterface, error) {
268270
cluster, err := genericapirequest.ValidClusterFrom(ctx)
269271
if err != nil {
270272
return nil, apiErrorBadRequest(err)
271273
}
274+
272275
gvr := resource
273276
clusterName := cluster.Name
274277
if apiExportIdentityHash != "" {
275278
gvr.Resource += ":" + apiExportIdentityHash
276279
}
277280

281+
dynamicClusterClient, err := dynamicClusterClientFunc(ctx)
282+
if err != nil {
283+
return nil, fmt.Errorf("error generating dynamic client: %w", err)
284+
}
285+
278286
if namespaceScoped {
279287
if namespace, ok := genericapirequest.NamespaceFrom(ctx); ok {
280288
return dynamicClusterClient.Cluster(clusterName.Path()).Resource(gvr).Namespace(namespace), nil
@@ -292,7 +300,7 @@ type listerWatcher interface {
292300
Watch(ctx context.Context, opts metav1.ListOptions) (watch.Interface, error)
293301
}
294302

295-
func listerWatcherGetter(dynamicClusterClient kcpdynamic.ClusterInterface, namespaceScoped bool, resource schema.GroupVersionResource, apiExportIdentityHash string) func(ctx context.Context) (listerWatcher, error) {
303+
func listerWatcherGetter(dynamicClusterClientFunc DynamicClusterClientFunc, namespaceScoped bool, resource schema.GroupVersionResource, apiExportIdentityHash string) func(ctx context.Context) (listerWatcher, error) {
296304
return func(ctx context.Context) (listerWatcher, error) {
297305
cluster, err := genericapirequest.ValidClusterFrom(ctx)
298306
if err != nil {
@@ -304,6 +312,11 @@ func listerWatcherGetter(dynamicClusterClient kcpdynamic.ClusterInterface, names
304312
}
305313
namespace, namespaceSet := genericapirequest.NamespaceFrom(ctx)
306314

315+
dynamicClusterClient, err := dynamicClusterClientFunc(ctx)
316+
if err != nil {
317+
return nil, fmt.Errorf("error generating dynamic client: %w", err)
318+
}
319+
307320
switch {
308321
case cluster.Wildcard:
309322
if namespaceScoped && namespaceSet && namespace != metav1.NamespaceAll {

‎pkg/virtual/initializingworkspaces/builder/forwarding.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ func filteredLogicalClusterReadOnlyRestStorage(
7272

7373
return registry.ProvideReadOnlyRestStorage(
7474
ctx,
75-
clusterClient,
75+
func(ctx context.Context) (kcpdynamic.ClusterInterface, error) { return clusterClient, nil },
7676
registry.WithStaticLabelSelector(requirements),
7777
nil,
7878
)
@@ -130,7 +130,7 @@ func delegatingLogicalClusterReadOnlyRestStorage(
130130
nil,
131131
tableConvertor,
132132
nil,
133-
clusterClient,
133+
func(ctx context.Context) (kcpdynamic.ClusterInterface, error) { return clusterClient, nil },
134134
nil,
135135
&registry.StorageWrappers{
136136
registry.WithStaticLabelSelector(requirements),

‎pkg/virtual/syncer/builder/forwarding.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ func NewSyncerRestProvider(ctx context.Context, clusterClient kcpdynamic.Cluster
7171
nil,
7272
tableConvertor,
7373
nil,
74-
clusterClient,
74+
func(ctx context.Context) (kcpdynamic.ClusterInterface, error) { return clusterClient, nil },
7575
nil,
7676
wrapper,
7777
)
@@ -167,7 +167,7 @@ func NewUpSyncerRestProvider(ctx context.Context, clusterClient kcpdynamic.Clust
167167
nil,
168168
tableConvertor,
169169
nil,
170-
clusterClient,
170+
func(ctx context.Context) (kcpdynamic.ClusterInterface, error) { return clusterClient, nil },
171171
nil,
172172
wrapper,
173173
)

‎test/e2e/virtual/apiexport/authorizer_test.go

+3-1
Original file line numberDiff line numberDiff line change
@@ -479,7 +479,9 @@ func apply(t *testing.T, ctx context.Context, workspace logicalcluster.Path, cfg
479479
}()
480480

481481
mapping, err := mapper.RESTMapping(gvk.GroupKind(), gvk.Version)
482-
require.NoError(t, err)
482+
if err != nil {
483+
return fmt.Errorf("error getting REST mapping: %w", err)
484+
}
483485

484486
dynamicClient, err := kcpdynamic.NewForConfig(cfg)
485487
require.NoError(t, err)

0 commit comments

Comments
 (0)