Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that the correct cluster is queried by authgate for catalog. #2038

Merged
merged 3 commits into from
Sep 16, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion chart/kubeapps/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion cmd/kubeops/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 3 additions & 1 deletion cmd/tiller-proxy/internal/handler/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -47,6 +48,7 @@ type TillerProxy struct {
ListLimit int
ChartClient chartUtils.Resolver
ProxyClient proxy.TillerClient
ClustersConfig kube.ClustersConfig
}

func (h *TillerProxy) logStatus(name string) {
Expand Down Expand Up @@ -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
}
Expand Down
3 changes: 2 additions & 1 deletion cmd/tiller-proxy/internal/handler/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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
Expand Down
17 changes: 14 additions & 3 deletions cmd/tiller-proxy/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -156,6 +165,7 @@ func main() {
ListLimit: listLimit,
ChartClient: chartClient,
ProxyClient: proxy,
ClustersConfig: clustersConfig,
}

// Routes
Expand All @@ -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)
}
Expand All @@ -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)),
Expand Down
10 changes: 6 additions & 4 deletions pkg/auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -91,14 +92,15 @@ 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
}
// 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
Expand Down
20 changes: 12 additions & 8 deletions pkg/auth/authgate.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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
Expand All @@ -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 {
Expand Down