[tiller-proxy] Improve forbidden response (#378)

* Improve forbidden response

* Remove dev dockerfile

* Review

* Test minor review

* Add Icon to AppOverview

.travis.yml

@@ -86,3 +86,5 @@ jobs:
     env: IMAGE=kubeapps/apprepository-controller
   - <<: *imageBuild
     env: IMAGE=kubeapps/dashboard
+  - <<: *imageBuild
+    env: IMAGE=kubeapps/tiller-proxy

Makefile

@@ -35,8 +35,9 @@ kubeapps/%:
 kubeapps/dashboard:
 	docker build -t kubeapps/dashboard:$(VERSION) -f dashboard/Dockerfile dashboard/
 
-tiller-proxy:
-	docker build -t kubeapps/tiller-proxy -f ./cmd/tiller-proxy/Dockerfile .
+kubeapps/tiller-proxy:
+	CGO_ENABLED=0 GOOS=linux go build -installsuffix cgo -o ./cmd/tiller-proxy/proxy-static ./cmd/tiller-proxy
+	docker build -t kubeapps/tiller-proxy:$(VERSION) -f cmd/tiller-proxy/Dockerfile cmd/tiller-proxy
 
 test: $(EMBEDDED_STATIC)
 	$(GO) test $(GO_PACKAGES)

cmd/tiller-proxy/Dockerfile

@@ -1,9 +1,4 @@
-FROM quay.io/deis/go-dev:v1.8.2 as builder
-COPY . /go/src/github.com/kubeapps/kubeapps
-WORKDIR /go/src/github.com/kubeapps/kubeapps
-RUN CGO_ENABLED=0 go build -a -installsuffix cgo ./cmd/tiller-proxy
-
-FROM scratch
-COPY --from=builder /go/src/github.com/kubeapps/kubeapps/tiller-proxy /proxy
-EXPOSE 8080
+FROM alpine:3.6
+RUN apk --no-cache add ca-certificates
+COPY ./proxy-static /proxy
 CMD ["/proxy"]

cmd/tiller-proxy/handler.go

@@ -18,6 +18,7 @@ package main
 
 import (
 	"context"
+	"encoding/json"
 	"io/ioutil"
 	"net/http"
 	"strings"
@@ -138,6 +139,16 @@ func logStatus(name string) {
 	}
 }
 
+func returnForbiddenActions(forbiddenActions []auth.Action, w http.ResponseWriter) {
+	w.Header().Set("Content-Type", "application/json")
+	body, err := json.Marshal(forbiddenActions)
+	if err != nil {
+		response.NewErrorResponse(errorCode(err), err.Error()).Write(w)
+		return
+	}
+	response.NewErrorResponse(http.StatusForbidden, string(body)).Write(w)
+}
+
 func createRelease(w http.ResponseWriter, req *http.Request, params Params) {
 	log.Printf("Creating Helm Release")
 	chartDetails, ch, err := getChart(req)
@@ -152,11 +163,15 @@ func createRelease(w http.ResponseWriter, req *http.Request, params Params) {
 			return
 		}
 		userAuth := req.Context().Value(userKey).(auth.UserAuth)
-		err = userAuth.CanI(params["namespace"], "create", manifest)
+		forbiddenActions, err := userAuth.GetForbiddenActions(params["namespace"], "create", manifest)
 		if err != nil {
 			response.NewErrorResponse(errorCode(err), err.Error()).Write(w)
 			return
 		}
+		if len(forbiddenActions) > 0 {
+			returnForbiddenActions(forbiddenActions, w)
+			return
+		}
 	}
 	rel, err := proxy.CreateRelease(chartDetails.ReleaseName, params["namespace"], chartDetails.Values, ch)
 	if err != nil {
@@ -182,11 +197,15 @@ func upgradeRelease(w http.ResponseWriter, req *http.Request, params Params) {
 			return
 		}
 		userAuth := req.Context().Value(userKey).(auth.UserAuth)
-		err = userAuth.CanI(params["namespace"], "create", manifest)
+		forbiddenActions, err := userAuth.GetForbiddenActions(params["namespace"], "upgrade", manifest)
 		if err != nil {
 			response.NewErrorResponse(errorCode(err), err.Error()).Write(w)
 			return
 		}
+		if len(forbiddenActions) > 0 {
+			returnForbiddenActions(forbiddenActions, w)
+			return
+		}
 	}
 	rel, err := proxy.UpdateRelease(params["releaseName"], params["namespace"], chartDetails.Values, ch)
 	if err != nil {
@@ -229,7 +248,15 @@ func getRelease(w http.ResponseWriter, req *http.Request, params Params) {
 			return
 		}
 		userAuth := req.Context().Value(userKey).(auth.UserAuth)
-		err = userAuth.CanI(params["namespace"], "get", manifest)
+		forbiddenActions, err := userAuth.GetForbiddenActions(params["namespace"], "get", manifest)
+		if err != nil {
+			response.NewErrorResponse(errorCode(err), err.Error()).Write(w)
+			return
+		}
+		if len(forbiddenActions) > 0 {
+			returnForbiddenActions(forbiddenActions, w)
+			return
+		}
 	}
 	response.NewDataResponse(*rel).Write(w)
 }
@@ -247,11 +274,15 @@ func deleteRelease(w http.ResponseWriter, req *http.Request, params Params) {
 			return
 		}
 		userAuth := req.Context().Value(userKey).(auth.UserAuth)
-		err = userAuth.CanI(params["namespace"], "delete", manifest)
+		forbiddenActions, err := userAuth.GetForbiddenActions(params["namespace"], "delete", manifest)
 		if err != nil {
 			response.NewErrorResponse(errorCode(err), err.Error()).Write(w)
 			return
 		}
+		if len(forbiddenActions) > 0 {
+			returnForbiddenActions(forbiddenActions, w)
+			return
+		}
 	}
 	err := proxy.DeleteRelease(params["releaseName"], params["namespace"])
 	if err != nil {

pkg/auth/auth.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 
 	authorizationapi "k8s.io/api/authorization/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	discovery "k8s.io/client-go/discovery"
 	"k8s.io/client-go/kubernetes"
 	authorizationv1 "k8s.io/client-go/kubernetes/typed/authorization/v1"
@@ -28,18 +29,66 @@ import (
 	yamlUtils "github.com/kubeapps/kubeapps/pkg/yaml"
 )
 
-// UserAuth contains information to check user permissions
-type UserAuth struct {
-	authCli      authorizationv1.AuthorizationV1Interface
-	discoveryCli discovery.DiscoveryInterface
-}
-
 type resource struct {
 	APIVersion string
 	Kind       string
 	Namespace  string
 }
 
+type k8sAuthInterface interface {
+	Validate() error
+	GetResourceList(groupVersion string) (*metav1.APIResourceList, error)
+	CanI(verb, group, resource, namespace string) (bool, error)
+}
+
+type k8sAuth struct {
+	AuthCli      authorizationv1.AuthorizationV1Interface
+	DiscoveryCli discovery.DiscoveryInterface
+}
+
+func (u k8sAuth) Validate() error {
+	_, err := u.AuthCli.SelfSubjectRulesReviews().Create(&authorizationapi.SelfSubjectRulesReview{
+		Spec: authorizationapi.SelfSubjectRulesReviewSpec{
+			Namespace: "default",
+		},
+	})
+	return err
+}
+
+func (u k8sAuth) GetResourceList(groupVersion string) (*metav1.APIResourceList, error) {
+	return u.DiscoveryCli.ServerResourcesForGroupVersion(groupVersion)
+}
+
+func (u k8sAuth) CanI(verb, group, resource, namespace string) (bool, error) {
+	res, err := u.AuthCli.SelfSubjectAccessReviews().Create(&authorizationapi.SelfSubjectAccessReview{
+		Spec: authorizationapi.SelfSubjectAccessReviewSpec{
+			ResourceAttributes: &authorizationapi.ResourceAttributes{
+				Group:     group,
+				Resource:  resource,
+				Verb:      verb,
+				Namespace: namespace,
+			},
+		},
+	})
+	if err != nil {
+		return false, err
+	}
+	return res.Status.Allowed, nil
+}
+
+// UserAuth contains information to check user permissions
+type UserAuth struct {
+	k8sAuth k8sAuthInterface
+}
+
+// Action represents a specific set of verbs against a resource
+type Action struct {
+	APIVersion string   `json:"apiGroup"`
+	Resource   string   `json:"resource"`
+	Namespace  string   `json:"namespace"`
+	Verbs      []string `json:"verbs"`
+}
+
 // NewAuth creates an auth agent
 func NewAuth(token string) (*UserAuth, error) {
 	config, err := rest.InClusterConfig()
@@ -51,22 +100,21 @@ func NewAuth(token string) (*UserAuth, error) {
 	kubeClient, err := kubernetes.NewForConfig(config)
 	authCli := kubeClient.AuthorizationV1()
 	discoveryCli := kubeClient.Discovery()
+	k8sAuthCli := k8sAuth{
+		AuthCli:      authCli,
+		DiscoveryCli: discoveryCli,
+	}
 
-	return &UserAuth{authCli, discoveryCli}, nil
+	return &UserAuth{k8sAuthCli}, nil
 }
 
 // Validate checks if the given token is valid
 func (u *UserAuth) Validate() error {
-	_, err := u.authCli.SelfSubjectRulesReviews().Create(&authorizationapi.SelfSubjectRulesReview{
-		Spec: authorizationapi.SelfSubjectRulesReviewSpec{
-			Namespace: "default",
-		},
-	})
-	return err
+	return u.k8sAuth.Validate()
 }
 
-func resolve(discoveryCli discovery.DiscoveryInterface, groupVersion, kind string) (string, error) {
-	resourceList, err := discoveryCli.ServerResourcesForGroupVersion(groupVersion)
+func (u *UserAuth) resolve(groupVersion, kind string) (string, error) {
+	resourceList, err := u.k8sAuth.GetResourceList(groupVersion)
 	if err != nil {
 		return "", nil
 	}
@@ -78,23 +126,6 @@ func resolve(discoveryCli discovery.DiscoveryInterface, groupVersion, kind strin
 	return "", fmt.Errorf("Unable to find the kind %s in the resource group %s", kind, groupVersion)
 }
 
-func (u *UserAuth) canPerform(verb, group, resource, namespace string) (bool, error) {
-	res, err := u.authCli.SelfSubjectAccessReviews().Create(&authorizationapi.SelfSubjectAccessReview{
-		Spec: authorizationapi.SelfSubjectAccessReviewSpec{
-			ResourceAttributes: &authorizationapi.ResourceAttributes{
-				Group:     group,
-				Resource:  resource,
-				Verb:      verb,
-				Namespace: namespace,
-			},
-		},
-	})
-	if err != nil {
-		return false, err
-	}
-	return res.Status.Allowed, nil
-}
-
 func (u *UserAuth) getResourcesToCheck(namespace, manifest string) ([]resource, error) {
 	objs, err := yamlUtils.ParseObjects(manifest)
 	if err != nil {
@@ -118,45 +149,84 @@ func (u *UserAuth) getResourcesToCheck(namespace, manifest string) ([]resource,
 	return result, nil
 }
 
-func (u *UserAuth) isAllowed(verb string, itemsToCheck []resource) error {
+func (u *UserAuth) isAllowed(verb string, itemsToCheck []resource) ([]Action, error) {
+	rejectedActions := []Action{}
 	for _, i := range itemsToCheck {
-		resource, err := resolve(u.discoveryCli, i.APIVersion, i.Kind)
+		resource, err := u.resolve(i.APIVersion, i.Kind)
 		if err != nil {
-			return err
+			return []Action{}, err
 		}
 		group := i.APIVersion
 		if group == "v1" {
 			// The group should be empty for the core API group
 			group = ""
 		}
-		allowed, _ := u.canPerform(verb, group, resource, i.Namespace)
+		allowed, err := u.k8sAuth.CanI(verb, group, resource, i.Namespace)
+		if err != nil {
+			return []Action{}, err
+		}
 		if !allowed {
-			return fmt.Errorf("Unauthorized to %s %s/%s in the %s namespace", verb, i.APIVersion, resource, i.Namespace)
+			rejectedActions = append(rejectedActions, Action{
+				APIVersion: i.APIVersion,
+				Resource:   resource,
+				Namespace:  i.Namespace,
+				Verbs:      []string{verb},
+			})
+		}
+	}
+	return rejectedActions, nil
+}
+
+func reduceActionsByVerb(actions []Action) []Action {
+	resMap := map[string]Action{}
+	res := []Action{}
+	for _, action := range actions {
+		req := fmt.Sprintf("%s/%s/%s", action.Namespace, action.APIVersion, action.Resource)
+		if _, ok := resMap[req]; ok {
+			// Element already exists
+			verbs := append(resMap[req].Verbs, action.Verbs...)
+			resMap[req] = Action{
+				APIVersion: action.APIVersion,
+				Resource:   action.Resource,
+				Namespace:  action.Namespace,
+				Verbs:      verbs,
+			}
+		} else {
+			resMap[req] = action
 		}
 	}
-	return nil
+	for _, a := range resMap {
+		res = append(res, a)
+	}
+	return res
 }
 
-// CanI returns if the user can perform the given action with the given chart and parameters
-func (u *UserAuth) CanI(namespace, action, manifest string) error {
+// GetForbiddenActions parses a K8s manifest and checks if the current user can do the action given
+// over all the elements of the manifest. It return the list of forbidden Actions if any.
+func (u *UserAuth) GetForbiddenActions(namespace, action, manifest string) ([]Action, error) {
 	resources, err := u.getResourcesToCheck(namespace, manifest)
+	forbiddenActions := []Action{}
 	if err != nil {
-		return err
+		return []Action{}, err
 	}
 	switch action {
 	case "upgrade":
 		// For upgrading a chart the user should be able to create, update and delete resources
 		for _, v := range []string{"create", "update", "delete"} {
-			err = u.isAllowed(v, resources)
+			actions, err := u.isAllowed(v, resources)
 			if err != nil {
-				return err
+				return []Action{}, err
 			}
+			forbiddenActions = append(forbiddenActions, actions...)
+		}
+		if len(forbiddenActions) > 0 {
+			forbiddenActions = reduceActionsByVerb(forbiddenActions)
 		}
 	default:
-		err := u.isAllowed(action, resources)
+		forbiddenActions, err = u.isAllowed(action, resources)
 		if err != nil {
-			return err
+			return []Action{}, err
 		}
 	}
-	return nil
+	return forbiddenActions, nil
 }