Ensure user client with service account backup when fetching namespac…

…es. (#5940)

<!--
Before you open the request please review the following guidelines and
tips to help it be more easily integrated:

 - Describe the scope of your change - i.e. what the change does.
 - Describe any known limitations with your change.
- Please run any tests or examples that can exercise your modified code.

 Thank you for contributing!
 -->

### Description of the change

<!-- Describe the scope of your change - i.e. what the change does. -->

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](#5755 (comment))

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
`[email protected]` is permitted to access, in addition to
`oidc:[email protected]`, since the former is what pinniped
uses).

### Benefits

<!-- What benefits will be realized by the code change? -->
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

<!-- Describe any known limitations with your change -->

### Applicable issues

<!-- Enter any applicable Issues here (You can reference an issue using
#) -->

- fixes #5755 

### Additional information

<!-- If there's anything else that's important and relevant to your pull
request, mention that information here.-->

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 <[email protected]>

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,27 +29,32 @@ 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)
 	}
 
 	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

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
 		}

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
 }

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,70 +535,66 @@ 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{
 					"default":      {},
 					"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))
 			}
 		})

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

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: [email protected]
+  - 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: [email protected]
+  - 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