From a196aad28f277f6262c544f1624cbbb88b75b45e Mon Sep 17 00:00:00 2001 From: Michael Nelson Date: Wed, 5 Aug 2020 17:44:32 +1000 Subject: [PATCH] Enable setting a service token for namespace discovery in other clusters. (#1921) --- chart/kubeapps/values.yaml | 1 + cmd/kubeops/main_test.go | 3 +- ...pps-local-dev-additional-kind-cluster.yaml | 4 + ...ps-local-dev-namespace-discovery-rbac.yaml | 43 +++++++ pkg/kube/kube_handler.go | 60 ++++++++-- pkg/kube/kube_handler_test.go | 113 +++++++++++++----- script/cluster-kind.mk | 1 + 7 files changed, 181 insertions(+), 44 deletions(-) create mode 100644 docs/user/manifests/kubeapps-local-dev-namespace-discovery-rbac.yaml diff --git a/chart/kubeapps/values.yaml b/chart/kubeapps/values.yaml index 1755b7af58c..18eafcf17d5 100644 --- a/chart/kubeapps/values.yaml +++ b/chart/kubeapps/values.yaml @@ -708,5 +708,6 @@ featureFlags: # - name: second-cluster # apiServiceURL: https://second-cluster:6443 # certificateAuthorityData: LS0tLS1CRUdJ... + # serviceToken: ... # ui is a WIP feature for the migration to Clarity design system. ui: hex diff --git a/cmd/kubeops/main_test.go b/cmd/kubeops/main_test.go index 998d04e20a3..71e7da1f126 100644 --- a/cmd/kubeops/main_test.go +++ b/cmd/kubeops/main_test.go @@ -19,12 +19,13 @@ func TestParseAdditionalClusterConfig(t *testing.T) { }{ { name: "parses a single additional cluster", - configJSON: `[{"name": "cluster-2", "apiServiceURL": "https://example.com", "certificateAuthorityData": "Y2EtY2VydC1kYXRhCg=="}]`, + configJSON: `[{"name": "cluster-2", "apiServiceURL": "https://example.com", "certificateAuthorityData": "Y2EtY2VydC1kYXRhCg==", "serviceToken": "abcd"}]`, expectedConfig: kube.AdditionalClustersConfig{ "cluster-2": { Name: "cluster-2", APIServiceURL: "https://example.com", CertificateAuthorityData: "ca-cert-data\n", + ServiceToken: "abcd", }, }, }, diff --git a/docs/user/manifests/kubeapps-local-dev-additional-kind-cluster.yaml b/docs/user/manifests/kubeapps-local-dev-additional-kind-cluster.yaml index e7ec1960420..54e9162c9be 100644 --- a/docs/user/manifests/kubeapps-local-dev-additional-kind-cluster.yaml +++ b/docs/user/manifests/kubeapps-local-dev-additional-kind-cluster.yaml @@ -5,3 +5,7 @@ featureFlags: # insecure is set to true only for local dev cluster testing. Always specify the # certificateAuthorityData as documented in teh values.yaml. insecure: true + # serviceToken is only required if users viewing the additional cluster should be able + # to see the namespaces to which they have access in the namespace selector (and should + # grant only the required RBAC) + serviceToken: ZXlKaGJHY2lPaUpTVXpJMU5pSXNJbXRwWkNJNklsbHpiSEp5TlZwM1QwaG9WSE5PYkhVdE5GQkRablY2TW0wd05rUmtMVmxFWVV4MlZEazNaeTEyUmxFaWZRLmV5SnBjM01pT2lKcmRXSmxjbTVsZEdWekwzTmxjblpwWTJWaFkyTnZkVzUwSWl3aWEzVmlaWEp1WlhSbGN5NXBieTl6WlhKMmFXTmxZV05qYjNWdWRDOXVZVzFsYzNCaFkyVWlPaUprWldaaGRXeDBJaXdpYTNWaVpYSnVaWFJsY3k1cGJ5OXpaWEoyYVdObFlXTmpiM1Z1ZEM5elpXTnlaWFF1Ym1GdFpTSTZJbXQxWW1WaGNIQnpMVzVoYldWemNHRmpaUzFrYVhOamIzWmxjbmt0ZEc5clpXNHRjV295Ym1naUxDSnJkV0psY201bGRHVnpMbWx2TDNObGNuWnBZMlZoWTJOdmRXNTBMM05sY25acFkyVXRZV05qYjNWdWRDNXVZVzFsSWpvaWEzVmlaV0Z3Y0hNdGJtRnRaWE53WVdObExXUnBjMk52ZG1WeWVTSXNJbXQxWW1WeWJtVjBaWE11YVc4dmMyVnlkbWxqWldGalkyOTFiblF2YzJWeWRtbGpaUzFoWTJOdmRXNTBMblZwWkNJNkltVXhaakE1WmpSakxUTTRNemt0TkRJME15MWhZbUptTFRKaU5HWm1OREZrWW1RMllTSXNJbk4xWWlJNkluTjVjM1JsYlRwelpYSjJhV05sWVdOamIzVnVkRHBrWldaaGRXeDBPbXQxWW1WaGNIQnpMVzVoYldWemNHRmpaUzFrYVhOamIzWmxjbmtpZlEuTnh6V2dsUGlrVWpROVQ1NkpWM2xJN1VWTUVSR3J2bklPSHJENkh4dUVwR0luLWFUUzV5Q0pDa3Z0cTF6S3Z3b05sc2MyX0YxaTdFOUxWRGFwbC1UQlhleUN5Rl92S1B1TDF4dTdqZFBMZ1dKT1pQX3JMcXppaDV4ZlkxalFoOHNhdTRZclFJLUtqb3U1UkRRZ0tOQS1BaS1lRlFOZVh2bmlUNlBKYWVkc184V0t3dHRMMC1wdHpYRnBnOFl5dkx6N0U1UWdTR2tjNWpDVXlsS0RvZVRUaVRSOEc2RHFHYkFQQUYwREt0b3MybU9Geno4SlJYNHhoQmdvaUcxVTVmR1g4Z3hnTU1SV0VHRE9kaGMyeXRvcFdRUkRpYmhvaldNS3VDZlNua09zMDRGYTBkYmEwQ0NTbld2a29LZ3Z4QVR5aVVrWm9wV3VpZ1JJNFd5dDkzbXhR diff --git a/docs/user/manifests/kubeapps-local-dev-namespace-discovery-rbac.yaml b/docs/user/manifests/kubeapps-local-dev-namespace-discovery-rbac.yaml new file mode 100644 index 00000000000..214141aaf47 --- /dev/null +++ b/docs/user/manifests/kubeapps-local-dev-namespace-discovery-rbac.yaml @@ -0,0 +1,43 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: kubeapps-namespace-discovery +rules: + - apiGroups: + - "" + resources: + - namespaces + verbs: + - list +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: kubeapps-namespace-discovery +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: "kubeapps:kubeops-ns-discovery" +subjects: + - kind: ServiceAccount + name: kubeapps-namespace-discovery + namespace: default +--- +apiVersion: v1 +kind: ServiceAccount +metadata: + name: kubeapps-namespace-discovery + namespace: default +secrets: +- name: kubeapps-namespace-discovery +--- +apiVersion: v1 +kind: Secret +metadata: + name: kubeapps-namespace-discovery + namespace: default + annotations: + "kubernetes.io/service-account.name": kubeapps-namespace-discovery +type: "kubernetes.io/service-account-token" +data: + token: ZXlKaGJHY2lPaUpTVXpJMU5pSXNJbXRwWkNJNklsbHpiSEp5TlZwM1QwaG9WSE5PYkhVdE5GQkRablY2TW0wd05rUmtMVmxFWVV4MlZEazNaeTEyUmxFaWZRLmV5SnBjM01pT2lKcmRXSmxjbTVsZEdWekwzTmxjblpwWTJWaFkyTnZkVzUwSWl3aWEzVmlaWEp1WlhSbGN5NXBieTl6WlhKMmFXTmxZV05qYjNWdWRDOXVZVzFsYzNCaFkyVWlPaUprWldaaGRXeDBJaXdpYTNWaVpYSnVaWFJsY3k1cGJ5OXpaWEoyYVdObFlXTmpiM1Z1ZEM5elpXTnlaWFF1Ym1GdFpTSTZJbXQxWW1WaGNIQnpMVzVoYldWemNHRmpaUzFrYVhOamIzWmxjbmt0ZEc5clpXNHRjV295Ym1naUxDSnJkV0psY201bGRHVnpMbWx2TDNObGNuWnBZMlZoWTJOdmRXNTBMM05sY25acFkyVXRZV05qYjNWdWRDNXVZVzFsSWpvaWEzVmlaV0Z3Y0hNdGJtRnRaWE53WVdObExXUnBjMk52ZG1WeWVTSXNJbXQxWW1WeWJtVjBaWE11YVc4dmMyVnlkbWxqWldGalkyOTFiblF2YzJWeWRtbGpaUzFoWTJOdmRXNTBMblZwWkNJNkltVXhaakE1WmpSakxUTTRNemt0TkRJME15MWhZbUptTFRKaU5HWm1OREZrWW1RMllTSXNJbk4xWWlJNkluTjVjM1JsYlRwelpYSjJhV05sWVdOamIzVnVkRHBrWldaaGRXeDBPbXQxWW1WaGNIQnpMVzVoYldWemNHRmpaUzFrYVhOamIzWmxjbmtpZlEuTnh6V2dsUGlrVWpROVQ1NkpWM2xJN1VWTUVSR3J2bklPSHJENkh4dUVwR0luLWFUUzV5Q0pDa3Z0cTF6S3Z3b05sc2MyX0YxaTdFOUxWRGFwbC1UQlhleUN5Rl92S1B1TDF4dTdqZFBMZ1dKT1pQX3JMcXppaDV4ZlkxalFoOHNhdTRZclFJLUtqb3U1UkRRZ0tOQS1BaS1lRlFOZVh2bmlUNlBKYWVkc184V0t3dHRMMC1wdHpYRnBnOFl5dkx6N0U1UWdTR2tjNWpDVXlsS0RvZVRUaVRSOEc2RHFHYkFQQUYwREt0b3MybU9Geno4SlJYNHhoQmdvaUcxVTVmR1g4Z3hnTU1SV0VHRE9kaGMyeXRvcFdRUkRpYmhvaldNS3VDZlNua09zMDRGYTBkYmEwQ0NTbld2a29LZ3Z4QVR5aVVrWm9wV3VpZ1JJNFd5dDkzbXhR diff --git a/pkg/kube/kube_handler.go b/pkg/kube/kube_handler.go index ade88106b5b..f06d6db81ac 100644 --- a/pkg/kube/kube_handler.go +++ b/pkg/kube/kube_handler.go @@ -57,7 +57,13 @@ type AdditionalClusterConfig struct { // Embedding genericclioptions.ConfigFlags in a struct which includes the actual rest.Config // and returning that for ToRESTConfig() isn't enough, so we each configured cert out and // include a CAFile field in the config. - CAFile string + CAFile string + // ServiceToken can be configured so that the Kubeapps application itself + // has access to get all namespaces on additional clusters, for example. It + // should *not* be for reading secrets or similar, but limited to the + // required functionality. + ServiceToken string + Insecure bool `json:"insecure"` } @@ -186,9 +192,33 @@ func (a *kubeHandler) AsUser(token, cluster string) (handler, error) { log.Errorf("unable to create clientset: %v", err) return nil, err } + + // Just use the service clientset if we're on the default cluster, but otherwise + // create a new clientset using a configured service token for a specific cluster. + // This is used when requesting the namespaces for a cluster (to populate the selector) + // iff the users own credential does not suffice. If a service token is not configured + // for the cluster, the namespace selector remains unpopulated. + var svcClientset combinedClientsetInterface + if cluster == DefaultClusterName { + svcClientset = a.svcClientset + } else { + additionalCluster, ok := a.additionalClustersConfig[cluster] + if !ok { + return nil, fmt.Errorf("cluster %q has no configuration", cluster) + } + svcConfig := *config + svcConfig.BearerToken = additionalCluster.ServiceToken + + svcClientset, err = a.clientsetForConfig(config) + if err != nil { + log.Errorf("unable to create clientset: %v", err) + return nil, err + } + } + return &userHandler{ kubeappsNamespace: a.kubeappsNamespace, - svcClientset: a.svcClientset, + svcClientset: svcClientset, clientset: clientset, }, nil } @@ -552,7 +582,7 @@ func KubeappsSecretNameForRepo(repoName, namespace string) string { return fmt.Sprintf("%s-%s", namespace, secretNameForRepo(repoName)) } -func filterAllowedNamespaces(userClientset combinedClientsetInterface, namespaces *corev1.NamespaceList) ([]corev1.Namespace, error) { +func filterAllowedNamespaces(userClientset combinedClientsetInterface, namespaces *corev1.NamespaceList) (*corev1.NamespaceList, error) { allowedNamespaces := []corev1.Namespace{} for _, namespace := range namespaces.Items { res, err := userClientset.AuthorizationV1().SelfSubjectAccessReviews().Create(context.TODO(), &authorizationapi.SelfSubjectAccessReview{ @@ -572,7 +602,8 @@ func filterAllowedNamespaces(userClientset combinedClientsetInterface, namespace allowedNamespaces = append(allowedNamespaces, namespace) } } - return allowedNamespaces, nil + namespaces.Items = allowedNamespaces + return namespaces, nil } // GetNamespaces return the list of namespaces that the user has permission to access @@ -584,18 +615,23 @@ func (a *userHandler) GetNamespaces() ([]corev1.Namespace, error) { if k8sErrors.IsForbidden(err) { // The user doesn't have permissions to list namespaces, use the current serviceaccount namespaces, err = a.svcClientset.CoreV1().Namespaces().List(context.TODO(), metav1.ListOptions{}) - } - if err != nil { + if err != nil && k8sErrors.IsForbidden(err) { + // If the configured svcclient doesn't have permission, just return an empty list. + return []corev1.Namespace{}, nil + } + + // Only if we obtained the namespaces from the svc client do we filter it using + // the user clientset. + namespaces, err = filterAllowedNamespaces(a.clientset, namespaces) + if err != nil { + return nil, err + } + } else { return nil, err } } - allowedNamespaces, err := filterAllowedNamespaces(a.clientset, namespaces) - if err != nil { - return nil, err - } - - return allowedNamespaces, nil + return namespaces.Items, nil } // GetSecret return the a secret from a namespace using a token if given diff --git a/pkg/kube/kube_handler_test.go b/pkg/kube/kube_handler_test.go index 999687b5a72..d947ef82b06 100644 --- a/pkg/kube/kube_handler_test.go +++ b/pkg/kube/kube_handler_test.go @@ -33,6 +33,7 @@ import ( k8sErrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/discovery" fakecoreclientset "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/rest" @@ -730,47 +731,72 @@ func TestSecretForRequest(t *testing.T) { func TestGetNamespaces(t *testing.T) { testCases := []struct { - name string - existingNS []string - expectedResponse []corev1.Namespace - allowed bool + name string + existingNamespaces []string + allowed bool + userClientErr error + svcClientErr error + expectedNamespaces []string }{ { - name: "it list namespaces", - existingNS: []string{"foo"}, - expectedResponse: []corev1.Namespace{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "foo", - }, - }, - }, - allowed: true, + name: "it lists namespaces if the user client returns the namespaces", + existingNamespaces: []string{"foo", "bar", "zed"}, + expectedNamespaces: []string{"foo", "bar", "zed"}, + allowed: true, + }, + { + name: "it does not filter the namespaces if returned by the user client", + existingNamespaces: []string{"foo", "bar", "zed"}, + expectedNamespaces: []string{"foo", "bar", "zed"}, + allowed: false, }, { - name: "it returns an empty list if not allowed", - existingNS: []string{"foo"}, - expectedResponse: []corev1.Namespace{}, - allowed: false, + name: "it lists namespaces if the userclient fails but the service client succeeds", + existingNamespaces: []string{"foo"}, + userClientErr: k8sErrors.NewForbidden(schema.GroupResource{}, "bang", fmt.Errorf("Bang")), + expectedNamespaces: []string{"foo"}, + allowed: true, + }, + { + name: "it filters the namespaces if the userclient fails but the service client succeeds", + existingNamespaces: []string{"foo"}, + userClientErr: k8sErrors.NewForbidden(schema.GroupResource{}, "bang", fmt.Errorf("Bang")), + expectedNamespaces: []string{}, + allowed: false, + }, + { + name: "it returns an empty list if both the user and service account forbidden", + existingNamespaces: []string{"foo"}, + userClientErr: k8sErrors.NewForbidden(schema.GroupResource{}, "bang", fmt.Errorf("Bang")), + svcClientErr: k8sErrors.NewForbidden(schema.GroupResource{}, "bang", fmt.Errorf("Bang")), + expectedNamespaces: []string{}, + allowed: true, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - cs := fakeCombinedClientset{ + userClientSet := fakeCombinedClientset{ fakeapprepoclientset.NewSimpleClientset(), fakecoreclientset.NewSimpleClientset(), &fakeRest.RESTClient{}, } - for _, ns := range tc.existingNS { - cs.Clientset.CoreV1().Namespaces().Create(context.TODO(), &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: ns, - }, - }, metav1.CreateOptions{}) + svcClientSet := fakeCombinedClientset{ + fakeapprepoclientset.NewSimpleClientset(), + fakecoreclientset.NewSimpleClientset(), + &fakeRest.RESTClient{}, } - cs.Clientset.Fake.PrependReactor( + setClientsetData(userClientSet, tc.existingNamespaces, tc.userClientErr) + setClientsetData(svcClientSet, tc.existingNamespaces, tc.svcClientErr) + + // Set whether the userClientSet is allowed to create self subject access reviews. + // The handler for the reactor has only the action as input, which does not convey + // enough info to be able to decide whether an individual namespace is allowed + // (as action.GetNamespace() is always empty as self subject access reviews are + // *not* themselves namespaced - the spec includes the namespace but is not contained + // in the action). As a result, we can only filter everything or nothing in tests. + userClientSet.Clientset.Fake.PrependReactor( "create", "selfsubjectaccessreviews", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { @@ -785,23 +811,48 @@ func TestGetNamespaces(t *testing.T) { ) handler := kubeHandler{ - clientsetForConfig: func(*rest.Config) (combinedClientsetInterface, error) { return cs, nil }, + clientsetForConfig: func(*rest.Config) (combinedClientsetInterface, error) { return userClientSet, nil }, kubeappsNamespace: "kubeapps", - svcClientset: cs, + svcClientset: svcClientSet, } - namespaces, err := handler.AsSVC().GetNamespaces() + userHandler, err := handler.AsUser("token", "default") + if err != nil { + t.Errorf("Unexpected error %v", err) + } + namespaces, err := userHandler.GetNamespaces() if err != nil { t.Errorf("Unexpected error %v", err) } - if !cmp.Equal(namespaces, tc.expectedResponse) { - t.Errorf("Unexpected response: %s", cmp.Diff(namespaces, tc.expectedResponse)) + namespaceNames := []string{} + for _, ns := range namespaces { + namespaceNames = append(namespaceNames, ns.ObjectMeta.Name) + } + if !cmp.Equal(namespaceNames, tc.expectedNamespaces) { + t.Errorf("Unexpected response: %s", cmp.Diff(namespaces, tc.expectedNamespaces)) } }) } } +// setClientsetData configures the fake clientset with the return and error. +func setClientsetData(cs fakeCombinedClientset, namespaceNames []string, err error) { + namespaces := []corev1.Namespace{} + for _, ns := range namespaceNames { + namespaces = append(namespaces, corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: ns}, + }) + } + cs.Clientset.Fake.PrependReactor( + "list", + "namespaces", + func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, &corev1.NamespaceList{Items: namespaces}, err + }, + ) +} + func TestValidateAppRepository(t *testing.T) { const kubeappsNamespace = "kubeapps" getValidationCliAndReqTests := []struct { diff --git a/script/cluster-kind.mk b/script/cluster-kind.mk index f160292b00e..6e8f68c5142 100644 --- a/script/cluster-kind.mk +++ b/script/cluster-kind.mk @@ -37,6 +37,7 @@ ${ADDITIONAL_CLUSTER_CONFIG}: devel/dex.crt --retain \ --wait 10s kubectl apply --kubeconfig=${ADDITIONAL_CLUSTER_CONFIG} -f ./docs/user/manifests/kubeapps-local-dev-users-rbac.yaml + kubectl apply --kubeconfig=${ADDITIONAL_CLUSTER_CONFIG} -f ./docs/user/manifests/kubeapps-local-dev-namespace-discovery-rbac.yaml additional-cluster-kind: ${ADDITIONAL_CLUSTER_CONFIG}