From 7eb47e3035a8e1a520da0ab429671d77980b9def Mon Sep 17 00:00:00 2001 From: Michael Nelson Date: Wed, 16 Sep 2020 11:53:49 +1000 Subject: [PATCH 1/3] Ensure that the correct cluster is queried by authgate for catalog. --- chart/kubeapps/values.yaml | 2 +- cmd/kubeops/main.go | 2 +- cmd/tiller-proxy/internal/handler/handler.go | 4 +++- .../internal/handler/handler_test.go | 3 ++- cmd/tiller-proxy/main.go | 17 +++++++++++++--- pkg/auth/auth.go | 4 +++- pkg/auth/authgate.go | 20 +++++++++++-------- 7 files changed, 36 insertions(+), 16 deletions(-) diff --git a/chart/kubeapps/values.yaml b/chart/kubeapps/values.yaml index 0cff295ee1d..aed3a82c974 100644 --- a/chart/kubeapps/values.yaml +++ b/chart/kubeapps/values.yaml @@ -27,7 +27,7 @@ enableIPv6: false ## the name you are assigning to the cluster on which Kubeapps is itself installed. Specifying more than ## one cluster without an apiServiceURL will cause the chart display an error. ## The base64-encoded certificateAuthorityData can be obtained from the additional cluster's kube config -## file, for example: +## file, for example, to get the ca data for the 0th cluster in your config (adjust the index 0 as necessary): ## kubectl --kubeconfig ~/.kube/kind-config-kubeapps-additional config view --raw -o jsonpath='{.clusters[0].cluster.certificate-authority-data}' # # clusters: diff --git a/cmd/kubeops/main.go b/cmd/kubeops/main.go index a3867e1f77a..e14f3062d9f 100644 --- a/cmd/kubeops/main.go +++ b/cmd/kubeops/main.go @@ -129,7 +129,7 @@ func main() { // TODO(mnelson) remove this reverse proxy once the haproxy frontend // proxies requests directly to the assetsvc. Move the authz to the // assetsvc itself. - authGate := auth.AuthGate(kubeappsNamespace) + authGate := auth.AuthGate(clustersConfig, kubeappsNamespace) parsedAssetsvcURL, err := url.Parse(assetsvcURL) if err != nil { log.Fatalf("Unable to parse the assetsvc URL: %v", err) diff --git a/cmd/tiller-proxy/internal/handler/handler.go b/cmd/tiller-proxy/internal/handler/handler.go index 298681936b2..abd2226a362 100644 --- a/cmd/tiller-proxy/internal/handler/handler.go +++ b/cmd/tiller-proxy/internal/handler/handler.go @@ -25,6 +25,7 @@ import ( "github.com/kubeapps/kubeapps/pkg/auth" chartUtils "github.com/kubeapps/kubeapps/pkg/chart" "github.com/kubeapps/kubeapps/pkg/handlerutil" + "github.com/kubeapps/kubeapps/pkg/kube" proxy "github.com/kubeapps/kubeapps/pkg/proxy" log "github.com/sirupsen/logrus" ) @@ -47,6 +48,7 @@ type TillerProxy struct { ListLimit int ChartClient chartUtils.Resolver ProxyClient proxy.TillerClient + ClustersConfig kube.ClustersConfig } func (h *TillerProxy) logStatus(name string) { @@ -272,7 +274,7 @@ func (h *TillerProxy) forbiddenActionsForRelease(req *http.Request, namespace, v } func (h *TillerProxy) forbiddenActionsForManifest(req *http.Request, namespace, verb, manifest string) ([]auth.Action, error) { - userAuth, err := h.CheckerForRequest(req) + userAuth, err := h.CheckerForRequest(h.ClustersConfig, req) if err != nil { return nil, err } diff --git a/cmd/tiller-proxy/internal/handler/handler_test.go b/cmd/tiller-proxy/internal/handler/handler_test.go index c14d261914a..699fbbba8d9 100644 --- a/cmd/tiller-proxy/internal/handler/handler_test.go +++ b/cmd/tiller-proxy/internal/handler/handler_test.go @@ -30,6 +30,7 @@ import ( "github.com/kubeapps/kubeapps/pkg/auth" authFake "github.com/kubeapps/kubeapps/pkg/auth/fake" chartFake "github.com/kubeapps/kubeapps/pkg/chart/fake" + "github.com/kubeapps/kubeapps/pkg/kube" proxyFake "github.com/kubeapps/kubeapps/pkg/proxy/fake" ) @@ -412,7 +413,7 @@ func TestActions(t *testing.T) { ProxyClient: proxy, } req := httptest.NewRequest("GET", fmt.Sprintf("http://foo.bar%s", test.RequestQuery), strings.NewReader(test.RequestBody)) - handler.CheckerForRequest = func(req *http.Request) (auth.Checker, error) { + handler.CheckerForRequest = func(clustersConfig kube.ClustersConfig, req *http.Request) (auth.Checker, error) { return &authFake.FakeAuth{ ForbiddenActions: test.ForbiddenActions, }, nil diff --git a/cmd/tiller-proxy/main.go b/cmd/tiller-proxy/main.go index 288bba0e787..22b3dfcd7cf 100644 --- a/cmd/tiller-proxy/main.go +++ b/cmd/tiller-proxy/main.go @@ -136,7 +136,16 @@ func main() { log.Fatalf("POD_NAMESPACE should be defined") } - kubeHandler, err := kube.NewHandler(kubeappsNamespace, kube.ClustersConfig{KubeappsClusterName: kubeappsClusterName}) + // Tiller proxy does not support multi-cluster. + clustersConfig := kube.ClustersConfig{ + KubeappsClusterName: kubeappsClusterName, + Clusters: map[string]kube.ClusterConfig{ + kubeappsClusterName: { + Name: kubeappsClusterName, + }, + }, + } + kubeHandler, err := kube.NewHandler(kubeappsNamespace, clustersConfig) if err != nil { log.Fatalf("Failed to create handler: %v", err) } @@ -156,6 +165,7 @@ func main() { ListLimit: listLimit, ChartClient: chartClient, ProxyClient: proxy, + ClustersConfig: clustersConfig, } // Routes @@ -175,7 +185,7 @@ func main() { apiv1.Methods("DELETE").Path("/clusters/{cluster}/namespaces/{namespace}/releases/{releaseName}").Handler(handlerutil.WithParams(h.DeleteRelease)) // Backend routes unrelated to tiller-proxy functionality. - err = backendHandlers.SetupDefaultRoutes(r.PathPrefix("/backend/v1").Subrouter(), kube.ClustersConfig{KubeappsClusterName: kubeappsClusterName}) + err = backendHandlers.SetupDefaultRoutes(r.PathPrefix("/backend/v1").Subrouter(), clustersConfig) if err != nil { log.Fatalf("Unable to setup backend routes: %+v", err) } @@ -194,7 +204,8 @@ func main() { // Logos don't require authentication so bypass that step. Nor are they cluster-aware as they're // embedded as links in the stored chart data. assetsvcRouter.Methods("GET").Path("/v1/ns/{ns}/assets/{repo}/{id}/logo").Handler(http.StripPrefix(assetsvcPrefix, assetsvcProxy)) - authGate := auth.AuthGate(kubeappsNamespace) + + authGate := auth.AuthGate(clustersConfig, kubeappsNamespace) assetsvcRouter.PathPrefix("/v1/clusters/{cluster}/namespaces/{namespace}/").Handler(negroni.New( authGate, negroni.Wrap(http.StripPrefix(assetsvcPrefix, assetsvcProxy)), diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 69ec1177180..5fae3c86dac 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -22,6 +22,7 @@ import ( "regexp" "strings" + "github.com/kubeapps/kubeapps/pkg/kube" yamlUtils "github.com/kubeapps/kubeapps/pkg/yaml" authorizationapi "k8s.io/api/authorization/v1" k8sErrors "k8s.io/apimachinery/pkg/api/errors" @@ -91,11 +92,12 @@ type Checker interface { } // NewAuth creates an auth agent -func NewAuth(token string) (*UserAuth, error) { +func NewAuth(token, clusterName string, clustersConfig kube.ClustersConfig) (*UserAuth, error) { config, err := rest.InClusterConfig() if err != nil { return nil, err } + // config, err = kube.NewClusterConfig(config, token, clusterName, clustersConfig) // Overwrite default token config.BearerToken = token config.BearerTokenFile = "" // https://github.com/kubeapps/kubeapps/pull/1359#issuecomment-564077326 diff --git a/pkg/auth/authgate.go b/pkg/auth/authgate.go index 60a282c6f3a..1fdc95ce9d6 100644 --- a/pkg/auth/authgate.go +++ b/pkg/auth/authgate.go @@ -8,6 +8,7 @@ import ( "github.com/gorilla/mux" "github.com/kubeapps/common/response" "github.com/kubeapps/kubeapps/pkg/dbutils" + "github.com/kubeapps/kubeapps/pkg/kube" "github.com/urfave/negroni" ) @@ -16,26 +17,27 @@ const tokenPrefix = "Bearer " // CheckerForRequest defines a function type so we can also inject a fake for tests // rather than setting a context value. -type CheckerForRequest func(req *http.Request) (Checker, error) +type CheckerForRequest func(clustersConfig kube.ClustersConfig, req *http.Request) (Checker, error) -func AuthCheckerForRequest(req *http.Request) (Checker, error) { +func AuthCheckerForRequest(clustersConfig kube.ClustersConfig, req *http.Request) (Checker, error) { token := ExtractToken(req.Header.Get("Authorization")) if token == "" { return nil, fmt.Errorf("Authorization token missing") } - return NewAuth(token) + clusterName := mux.Vars(req)["cluster"] + return NewAuth(token, clusterName, clustersConfig) } -// AuthGate implements middleware to check if the user has access to read from +// AuthGate implements middleware to check if the user has access to charts from // the specific namespace before continuing. // * If the path being handled by the // AuthGate middleware does not include the 'namespace' mux var, or the value // is _all, then the check is for cluster-wide access. // * If the namespace is the global chart namespace (ie. kubeappsNamespace) then // we allow read access regardless. -func AuthGate(kubeappsNamespace string) negroni.HandlerFunc { +func AuthGate(clustersConfig kube.ClustersConfig, kubeappsNamespace string) negroni.HandlerFunc { return func(w http.ResponseWriter, req *http.Request, next http.HandlerFunc) { - userAuth, err := AuthCheckerForRequest(req) + userAuth, err := AuthCheckerForRequest(clustersConfig, req) if err != nil { response.NewErrorResponse(http.StatusUnauthorized, err.Error()).Write(w) return @@ -45,9 +47,11 @@ func AuthGate(kubeappsNamespace string) negroni.HandlerFunc { namespace = "" } - // If the request is for the global public charts (ie. kubeappsNamespace) - // we do not check authz. + // The auth-gate is used only for access to the asset-svc and the functionality should be + // moved to the assetsvc itself if and when the assetsvc is updated to be cluster aware. authz := false + // TODO(absoludity): Update to allow access to assets from the global kubeapps namespace + // on the kubeapps cluster only. See #2037. if namespace == kubeappsNamespace { authz = true } else { From 45a4c0d2428a79087109ce10e66f4a1ea50fdc43 Mon Sep 17 00:00:00 2001 From: Michael Nelson Date: Wed, 16 Sep 2020 13:23:25 +1000 Subject: [PATCH 2/3] Use the kube helper to create the cluster-aware config. --- pkg/auth/auth.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 5fae3c86dac..d29c797cacc 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -97,10 +97,10 @@ func NewAuth(token, clusterName string, clustersConfig kube.ClustersConfig) (*Us if err != nil { return nil, err } - // config, err = kube.NewClusterConfig(config, token, clusterName, clustersConfig) - // Overwrite default token - config.BearerToken = token - config.BearerTokenFile = "" // https://github.com/kubeapps/kubeapps/pull/1359#issuecomment-564077326 + config, err = kube.NewClusterConfig(config, token, clusterName, clustersConfig) + if err != nil { + return nil, err + } kubeClient, err := kubernetes.NewForConfig(config) if err != nil { return nil, err @@ -118,6 +118,7 @@ func NewAuth(token, clusterName string, clustersConfig kube.ClustersConfig) (*Us // ValidateForNamespace checks if the user can access secrets in the given // namespace, as a check of whether they can view the namespace. func (u *UserAuth) ValidateForNamespace(namespace string) (bool, error) { + return u.k8sAuth.CanI("get", "", "secrets", namespace) } From a48595929e5fe776a6dde0922921b2d69365cf4e Mon Sep 17 00:00:00 2001 From: Michael Nelson Date: Thu, 17 Sep 2020 09:00:38 +1000 Subject: [PATCH 3/3] Lint --- pkg/auth/auth.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index d29c797cacc..93c26d410fb 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -118,7 +118,6 @@ func NewAuth(token, clusterName string, clustersConfig kube.ClustersConfig) (*Us // ValidateForNamespace checks if the user can access secrets in the given // namespace, as a check of whether they can view the namespace. func (u *UserAuth) ValidateForNamespace(namespace string) (bool, error) { - return u.k8sAuth.CanI("get", "", "secrets", namespace) }