From 90981b579452752b7c4cb76b23c004d77aa3653a Mon Sep 17 00:00:00 2001 From: Michael Nelson Date: Thu, 2 Feb 2023 06:59:18 +1100 Subject: [PATCH] Ensure user client with service account backup when fetching namespaces. (#5940) ### Description of the change The main change here is to ensure that the request for namespaces uses the *user* client first, then the service account client. For a detailed analysis of the issue it is fixing, see my [comment on 5755](https://github.com/vmware-tanzu/kubeapps/issues/5755#issuecomment-1406172791) While there, I have tried to clarify the code a little, so that it is clearer when the user client is being used (it now matches the comments again). I also fixed an RBAC issue in the dev environment (so that `kubeapps-user@example.com` is permitted to access, in addition to `oidc:kubeapps-user@example.com`, since the former is what pinniped uses). ### Benefits Back to expected behavior when authenticated as a non-privileged user with or without a service account token configured in Kubeapps' clusters configuration. ### Possible drawbacks ### Applicable issues - fixes #5755 ### Additional information I've tested this pretty thoroughly locally with multiple clusters using both unprivileged and privileged users (with additional log lines showing exactly what token is being used when), but only in the namespaces call-site. --------- Signed-off-by: Michael Nelson --- .../plugins/pkg/resources/namespaces.go | 26 +++++--- .../v1alpha1/namespaces_filtering.go | 30 ++++++--- .../plugins/resources/v1alpha1/server.go | 39 +++++++++--- .../plugins/resources/v1alpha1/server_test.go | 62 +++++++++---------- pkg/kube/cluster_config.go | 7 ++- .../kubeapps-local-dev-users-rbac.yaml | 14 +++++ 6 files changed, 115 insertions(+), 63 deletions(-) diff --git a/cmd/kubeapps-apis/plugins/pkg/resources/namespaces.go b/cmd/kubeapps-apis/plugins/pkg/resources/namespaces.go index 606544f3051..f606dbd1962 100644 --- a/cmd/kubeapps-apis/plugins/pkg/resources/namespaces.go +++ b/cmd/kubeapps-apis/plugins/pkg/resources/namespaces.go @@ -4,6 +4,9 @@ package resources import ( + "math" + "sync" + "github.com/vmware-tanzu/kubeapps/cmd/kubeapps-apis/plugins/pkg/clientgetter" "golang.org/x/net/context" "google.golang.org/grpc/codes" @@ -14,8 +17,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" log "k8s.io/klog/v2" - "math" - "sync" ) type checkNSJob struct { @@ -28,10 +29,10 @@ type checkNSResult struct { Error error } -// FindAccessibleNamespaces return the raw list of namespaces that the user has permission to access +// FindAccessibleNamespaces returns the raw list of namespaces that the user has permission to access // Not filtered by any status (e.g. Active), but actual access is checked. -func FindAccessibleNamespaces(clusterTypedClientGetter clientgetter.TypedClientFunc, inClusterClientGetter clientgetter.TypedClientFunc, maxWorkers int) ([]corev1.Namespace, error) { - typedClient, err := clusterTypedClientGetter() +func FindAccessibleNamespaces(userClientGetter clientgetter.TypedClientFunc, serviceAccountClientGetter clientgetter.TypedClientFunc, maxWorkers int) ([]corev1.Namespace, error) { + userClient, err := userClientGetter() if err != nil { return nil, status.Errorf(codes.Internal, "unable to get the k8s client: '%v'", err) } @@ -39,16 +40,21 @@ func FindAccessibleNamespaces(clusterTypedClientGetter clientgetter.TypedClientF backgroundCtx := context.Background() // Try to list namespaces first with the user token - namespaces, err := typedClient.CoreV1().Namespaces().List(backgroundCtx, metav1.ListOptions{}) + namespaces, err := userClient.CoreV1().Namespaces().List(backgroundCtx, metav1.ListOptions{}) if err != nil { if k8sErrors.IsForbidden(err) { - // The user doesn't have permissions to list namespaces, then use the current pod's service account - inClusterClient, err := inClusterClientGetter() + // The user doesn't have permissions to list namespaces, then use + // the provided service account client. This client will have been configured + // with either the current pod's service account, if the target + // cluster is the same one on which Kubeapps is installed, or with + // the cluster config service account otherwise. + serviceAccountClient, err := serviceAccountClientGetter() if err != nil { return nil, status.Errorf(codes.Internal, "unable to get the in-cluster k8s client: '%v'", err) } - namespaces, err = inClusterClient.CoreV1().Namespaces().List(backgroundCtx, metav1.ListOptions{}) + namespaces, err = serviceAccountClient.CoreV1().Namespaces().List(backgroundCtx, metav1.ListOptions{}) if err != nil && k8sErrors.IsForbidden(err) { + log.Errorf("Returning a forbidden error because: %+v", err) // Not even the configured kubeapps-apis service account has permission return nil, err } @@ -57,7 +63,7 @@ func FindAccessibleNamespaces(clusterTypedClientGetter clientgetter.TypedClientF } // Filter namespaces in which the user has permissions to write (secrets) only - if namespaceList, err := filterAllowedNamespaces(typedClient, maxWorkers, namespaces.Items); err != nil { + if namespaceList, err := filterAllowedNamespaces(userClient, maxWorkers, namespaces.Items); err != nil { return nil, err } else { return namespaceList, nil diff --git a/cmd/kubeapps-apis/plugins/resources/v1alpha1/namespaces_filtering.go b/cmd/kubeapps-apis/plugins/resources/v1alpha1/namespaces_filtering.go index bf77ff0ea95..33efb844b36 100644 --- a/cmd/kubeapps-apis/plugins/resources/v1alpha1/namespaces_filtering.go +++ b/cmd/kubeapps-apis/plugins/resources/v1alpha1/namespaces_filtering.go @@ -4,14 +4,16 @@ package main import ( "context" + "regexp" + "strings" + "github.com/vmware-tanzu/kubeapps/cmd/kubeapps-apis/plugins/pkg/resources" + "github.com/vmware-tanzu/kubeapps/pkg/kube" "google.golang.org/grpc/metadata" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" log "k8s.io/klog/v2" - "regexp" - "strings" ) func (s *Server) MaxWorkers() int { @@ -25,15 +27,29 @@ func (s *Server) GetAccessibleNamespaces(ctx context.Context, cluster string, tr if len(trustedNamespaces) > 0 { namespaceList = append(namespaceList, trustedNamespaces...) } else { - clusterTypedClientFunc := func() (kubernetes.Interface, error) { - return s.clusterServiceAccountClientGetter.Typed(ctx, cluster) + userTypedClientFunc := func() (kubernetes.Interface, error) { + return s.clientGetter.Typed(ctx, cluster) } - inClusterTypedClientFunc := func() (kubernetes.Interface, error) { - return s.localServiceAccountClientGetter.Typed(context.Background()) + + // The service account client returned for fetching namespaces depends on whether + // the target cluster is the same one Kubeapps is running on (in which case, + // we use the pod's token which will have been configured for access) or the + // token from the clusters config (if it exists). + var serviceAccountTypedClientFunc func() (kubernetes.Interface, error) + if kube.IsKubeappsClusterRef(cluster) { + serviceAccountTypedClientFunc = func() (kubernetes.Interface, error) { + // Not using ctx here so that we can't inadvertently send the user + // creds. + return s.localServiceAccountClientGetter.Typed(context.Background()) + } + } else { + serviceAccountTypedClientFunc = func() (kubernetes.Interface, error) { + return s.clusterServiceAccountClientGetter.Typed(ctx, cluster) + } } var err error - namespaceList, err = resources.FindAccessibleNamespaces(clusterTypedClientFunc, inClusterTypedClientFunc, s.MaxWorkers()) + namespaceList, err = resources.FindAccessibleNamespaces(userTypedClientFunc, serviceAccountTypedClientFunc, s.MaxWorkers()) if err != nil { return nil, err } diff --git a/cmd/kubeapps-apis/plugins/resources/v1alpha1/server.go b/cmd/kubeapps-apis/plugins/resources/v1alpha1/server.go index 7bb7c0fe52b..bc97026009e 100644 --- a/cmd/kubeapps-apis/plugins/resources/v1alpha1/server.go +++ b/cmd/kubeapps-apis/plugins/resources/v1alpha1/server.go @@ -7,11 +7,12 @@ import ( "context" "encoding/json" "fmt" + "os" + "sync" + "github.com/vmware-tanzu/kubeapps/cmd/kubeapps-apis/plugins/pkg/clientgetter" "github.com/vmware-tanzu/kubeapps/cmd/kubeapps-apis/plugins/resources/v1alpha1/common" "github.com/vmware-tanzu/kubeapps/pkg/kube" - "os" - "sync" "google.golang.org/grpc" "google.golang.org/grpc/codes" @@ -125,6 +126,9 @@ func NewServer(configGetter core.KubernetesConfigGetter, clientQPS float32, clie log.Fatalf("%s", err) } + // TODO(absoludity): The use of this service account should be audited. Ideally it would + // not be possible to use this outside of the request for namespaces, but the code now + // allows this to be used in other contexts, which could lead to a privilege escalation. clusterServiceAccountClientGetter, err := newClientGetter(configGetter, true, clustersConfig) if err != nil { log.Fatalf("%s", err) @@ -162,13 +166,26 @@ func NewServer(configGetter core.KubernetesConfigGetter, clientQPS float32, clie func newClientGetter(configGetter core.KubernetesConfigGetter, useServiceAccount bool, clustersConfig kube.ClustersConfig) (clientgetter.ClientProviderInterface, error) { customConfigGetter := func(ctx context.Context, cluster string) (*rest.Config, error) { + if useServiceAccount { + // If a service account client getter has been requested, the service account + // to use depends on which cluster is targeted. + restConfig, err := rest.InClusterConfig() + if err != nil { + return nil, status.Errorf(codes.FailedPrecondition, "unable to get config : %v", err.Error()) + } + err = setupRestConfigForCluster(restConfig, cluster, clustersConfig) + if err != nil { + return nil, status.Errorf(codes.FailedPrecondition, "unable to setup config for cluster : %v", err.Error()) + } + return restConfig, nil + } + // Rest config for a *user* created here - must already have token? So + // it is at this point where we should *not* pass the user credential / + // token if it is not needed? restConfig, err := configGetter(ctx, cluster) if err != nil { return nil, status.Errorf(codes.FailedPrecondition, "unable to get config : %v", err.Error()) } - if err := setupRestConfigForCluster(restConfig, cluster, useServiceAccount, clustersConfig); err != nil { - return nil, err - } return restConfig, nil } @@ -179,17 +196,19 @@ func newClientGetter(configGetter core.KubernetesConfigGetter, useServiceAccount return clientProvider, nil } -func setupRestConfigForCluster(restConfig *rest.Config, cluster string, useServiceAccount bool, clustersConfig kube.ClustersConfig) error { +func setupRestConfigForCluster(restConfig *rest.Config, cluster string, clustersConfig kube.ClustersConfig) error { // Override client config with the service token for additional cluster // Added from #5034 after deprecation of "kubeops" - if cluster != clustersConfig.KubeappsClusterName && useServiceAccount { + if cluster == clustersConfig.KubeappsClusterName { + log.Errorf("Kubeapps cluster, should already have correct token for service acc: %q\n%q", restConfig.BearerToken, restConfig.BearerTokenFile) + } else { additionalCluster, ok := clustersConfig.Clusters[cluster] if !ok { return status.Errorf(codes.Internal, "cluster %q has no configuration", cluster) } - if additionalCluster.ServiceToken != "" { - restConfig.BearerToken = additionalCluster.ServiceToken - } + // We *always* overwrite the token, even if it was configured empty. + restConfig.BearerToken = additionalCluster.ServiceToken + restConfig.BearerTokenFile = "" } return nil } diff --git a/cmd/kubeapps-apis/plugins/resources/v1alpha1/server_test.go b/cmd/kubeapps-apis/plugins/resources/v1alpha1/server_test.go index 12c40fa9615..90167e6f210 100644 --- a/cmd/kubeapps-apis/plugins/resources/v1alpha1/server_test.go +++ b/cmd/kubeapps-apis/plugins/resources/v1alpha1/server_test.go @@ -6,15 +6,16 @@ package main import ( "context" "errors" - "github.com/vmware-tanzu/kubeapps/cmd/kubeapps-apis/plugins/pkg/clientgetter" - "github.com/vmware-tanzu/kubeapps/pkg/kube" "io" - "k8s.io/client-go/rest" "net" "reflect" "testing" "time" + "github.com/vmware-tanzu/kubeapps/cmd/kubeapps-apis/plugins/pkg/clientgetter" + "github.com/vmware-tanzu/kubeapps/pkg/kube" + "k8s.io/client-go/rest" + "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "google.golang.org/grpc" @@ -534,28 +535,37 @@ func TestGetServiceAccountNames(t *testing.T) { func TestSetupConfigForCluster(t *testing.T) { testCases := []struct { name string - restConfig *rest.Config + inputConfig *rest.Config cluster string - useServiceAccount bool clustersConfig kube.ClustersConfig expectedRestConfig *rest.Config expectedErrorCode codes.Code }{ { - name: "config is not modified for kubeapps cluster", + name: "config is not modified and so includes the incluster token for kubeapps cluster", cluster: "default", + inputConfig: &rest.Config{ + BearerToken: "abc123", + BearerTokenFile: "/path/to/file", + }, clustersConfig: kube.ClustersConfig{ KubeappsClusterName: "default", Clusters: map[string]kube.ClusterConfig{ "default": {}, }, }, - restConfig: &rest.Config{}, - expectedRestConfig: &rest.Config{}, + expectedRestConfig: &rest.Config{ + BearerToken: "abc123", + BearerTokenFile: "/path/to/file", + }, }, { - name: "config is not modified for additional clusters and no service account", + name: "config creds are cleared for an additional clusters without a configured service account", cluster: "additional-1", + inputConfig: &rest.Config{ + BearerToken: "abc123", + BearerTokenFile: "/path/to/file", + }, clustersConfig: kube.ClustersConfig{ KubeappsClusterName: "default", Clusters: map[string]kube.ClusterConfig{ @@ -563,41 +573,28 @@ func TestSetupConfigForCluster(t *testing.T) { "additional-1": {}, }, }, - restConfig: &rest.Config{}, expectedRestConfig: &rest.Config{}, }, { - name: "config setup fails for additional clusters with no cluster config data", - cluster: "additional-1", - useServiceAccount: true, + name: "config setup fails for additional clusters with no cluster config data", + cluster: "additional-1", + inputConfig: &rest.Config{}, clustersConfig: kube.ClustersConfig{ KubeappsClusterName: "default", Clusters: map[string]kube.ClusterConfig{ "default": {}, }, }, - restConfig: &rest.Config{}, expectedRestConfig: &rest.Config{}, expectedErrorCode: codes.Internal, }, { - name: "config is not modified for additional clusters with no configured service token", - cluster: "additional-1", - useServiceAccount: true, - clustersConfig: kube.ClustersConfig{ - KubeappsClusterName: "default", - Clusters: map[string]kube.ClusterConfig{ - "default": {}, - "additional-1": {}, - }, + name: "config is modified for additional clusters when configured service token", + cluster: "additional-1", + inputConfig: &rest.Config{ + BearerToken: "abc123", + BearerTokenFile: "/path/to/file", }, - restConfig: &rest.Config{}, - expectedRestConfig: &rest.Config{}, - }, - { - name: "config is modified for additional clusters when configured service token", - cluster: "additional-1", - useServiceAccount: true, clustersConfig: kube.ClustersConfig{ KubeappsClusterName: "default", Clusters: map[string]kube.ClusterConfig{ @@ -607,7 +604,6 @@ func TestSetupConfigForCluster(t *testing.T) { }, }, }, - restConfig: &rest.Config{}, expectedRestConfig: &rest.Config{ BearerToken: "service-token-1", }, @@ -617,13 +613,13 @@ func TestSetupConfigForCluster(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - err := setupRestConfigForCluster(tc.restConfig, tc.cluster, tc.useServiceAccount, tc.clustersConfig) + err := setupRestConfigForCluster(tc.inputConfig, tc.cluster, tc.clustersConfig) if got, want := status.Code(err), tc.expectedErrorCode; !cmp.Equal(got, want, nil) { t.Errorf("mismatch (-want +got):\n%s", cmp.Diff(want, got, nil)) } - if got, want := tc.restConfig, tc.expectedRestConfig; !cmp.Equal(got, want, nil) { + if got, want := tc.inputConfig, tc.expectedRestConfig; !cmp.Equal(got, want, nil) { t.Errorf("mismatch (-want +got):\n%s", cmp.Diff(want, got, nil)) } }) diff --git a/pkg/kube/cluster_config.go b/pkg/kube/cluster_config.go index 8db604d769e..04ed2383e61 100644 --- a/pkg/kube/cluster_config.go +++ b/pkg/kube/cluster_config.go @@ -7,10 +7,11 @@ import ( "encoding/base64" "encoding/json" "fmt" - "k8s.io/client-go/rest" "net/http" "os" "path/filepath" + + "k8s.io/client-go/rest" ) const ( @@ -85,8 +86,8 @@ type ClustersConfig struct { Clusters map[string]ClusterConfig } -// NewClusterConfig returns a copy of an in-cluster config with a user token (leave blank for -// when configuring a service account). and/or custom cluster host +// NewClusterConfig returns a copy of an in-cluster config with a user token +// and/or custom cluster host func NewClusterConfig(inClusterConfig *rest.Config, userToken string, cluster string, clustersConfig ClustersConfig) (*rest.Config, error) { config := rest.CopyConfig(inClusterConfig) config.BearerToken = userToken diff --git a/site/content/docs/latest/reference/manifests/kubeapps-local-dev-users-rbac.yaml b/site/content/docs/latest/reference/manifests/kubeapps-local-dev-users-rbac.yaml index b4548d2ed5f..6fddc057305 100644 --- a/site/content/docs/latest/reference/manifests/kubeapps-local-dev-users-rbac.yaml +++ b/site/content/docs/latest/reference/manifests/kubeapps-local-dev-users-rbac.yaml @@ -51,6 +51,13 @@ subjects: - apiGroup: rbac.authorization.k8s.io kind: Group name: oidc:kubeapps-users + # pinniped doesn't use a prefix, unlike the cluster oidc config: + - apiGroup: rbac.authorization.k8s.io + kind: User + name: kubeapps-user@example.com + - apiGroup: rbac.authorization.k8s.io + kind: Group + name: kubeapps-users --- kind: RoleBinding apiVersion: rbac.authorization.k8s.io/v1 @@ -68,6 +75,13 @@ subjects: - apiGroup: rbac.authorization.k8s.io kind: Group name: oidc:kubeapps-users + # pinniped doesn't use a prefix, unlike the cluster oidc config: + - apiGroup: rbac.authorization.k8s.io + kind: User + name: kubeapps-user@example.com + - apiGroup: rbac.authorization.k8s.io + kind: Group + name: kubeapps-users --- # Currently unnecessary (when kubeapps operators are already cluster-admin) but # included to be explicit and plan to replace cluster-admin for kubeapps