diff --git a/cmd/kubeops/internal/handler/handler.go b/cmd/kubeops/internal/handler/handler.go index 03ecc6c9492..d2b45dedbb1 100644 --- a/cmd/kubeops/internal/handler/handler.go +++ b/cmd/kubeops/internal/handler/handler.go @@ -1,6 +1,7 @@ package handler import ( + "encoding/json" "net/http" "strconv" @@ -131,11 +132,37 @@ func AddRouteWith( } } +func returnForbiddenActions(forbiddenActions []auth.Action, w http.ResponseWriter) { + w.Header().Set("Content-Type", "application/json") + body, err := json.Marshal(forbiddenActions) + if err != nil { + returnErrMessage(err, w) + return + } + response.NewErrorResponse(http.StatusForbidden, string(body)).Write(w) +} + +func returnErrMessage(err error, w http.ResponseWriter) { + code := handlerutil.ErrorCode(err) + errMessage := err.Error() + if code == http.StatusForbidden { + forbiddenActions := auth.ParseForbiddenActions(errMessage) + if len(forbiddenActions) > 0 { + returnForbiddenActions(forbiddenActions, w) + } else { + // Unable to parse forbidden actions, return the raw message + response.NewErrorResponse(code, errMessage).Write(w) + } + } else { + response.NewErrorResponse(code, errMessage).Write(w) + } +} + // ListReleases list existing releases. func ListReleases(cfg Config, w http.ResponseWriter, req *http.Request, params handlerutil.Params) { apps, err := agent.ListReleases(cfg.ActionConfig, params[namespaceParam], cfg.Options.ListLimit, req.URL.Query().Get("statuses")) if err != nil { - response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) + returnErrMessage(err, w) return } response.NewDataResponse(apps).Write(w) @@ -150,7 +177,7 @@ func ListAllReleases(cfg Config, w http.ResponseWriter, req *http.Request, _ han func CreateRelease(cfg Config, w http.ResponseWriter, req *http.Request, params handlerutil.Params) { chartDetails, chartMulti, err := handlerutil.ParseAndGetChart(req, cfg.ChartClient, isV1SupportRequired) if err != nil { - response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) + returnErrMessage(err, w) return } ch := chartMulti.Helm3Chart @@ -159,7 +186,7 @@ func CreateRelease(cfg Config, w http.ResponseWriter, req *http.Request, params valuesString := chartDetails.Values release, err := agent.CreateRelease(cfg.ActionConfig, releaseName, namespace, valuesString, ch) if err != nil { - response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) + returnErrMessage(err, w) return } response.NewDataResponse(release).Write(w) @@ -183,18 +210,18 @@ func upgradeRelease(cfg Config, w http.ResponseWriter, req *http.Request, params releaseName := params[nameParam] chartDetails, chartMulti, err := handlerutil.ParseAndGetChart(req, cfg.ChartClient, isV1SupportRequired) if err != nil { - response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) + returnErrMessage(err, w) return } ch := chartMulti.Helm3Chart rel, err := agent.UpgradeRelease(cfg.ActionConfig, releaseName, chartDetails.Values, ch) if err != nil { - response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) + returnErrMessage(err, w) return } compatRelease, err := helm3to2.Convert(*rel) if err != nil { - response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) + returnErrMessage(err, w) return } response.NewDataResponse(compatRelease).Write(w) @@ -209,17 +236,17 @@ func rollbackRelease(cfg Config, w http.ResponseWriter, req *http.Request, param } revisionInt, err := strconv.ParseInt(revision, 10, 32) if err != nil { - response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) + returnErrMessage(err, w) return } rel, err := agent.RollbackRelease(cfg.ActionConfig, releaseName, int(revisionInt)) if err != nil { - response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) + returnErrMessage(err, w) return } compatRelease, err := helm3to2.Convert(*rel) if err != nil { - response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) + returnErrMessage(err, w) return } response.NewDataResponse(compatRelease).Write(w) @@ -231,12 +258,12 @@ func GetRelease(cfg Config, w http.ResponseWriter, req *http.Request, params han releaseName := params[nameParam] release, err := agent.GetRelease(cfg.ActionConfig, releaseName) if err != nil { - response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) + returnErrMessage(err, w) return } compatRelease, err := helm3to2.Convert(*release) if err != nil { - response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) + returnErrMessage(err, w) return } response.NewDataResponse(compatRelease).Write(w) @@ -251,7 +278,7 @@ func DeleteRelease(cfg Config, w http.ResponseWriter, req *http.Request, params keepHistory := !purge err := agent.DeleteRelease(cfg.ActionConfig, releaseName, keepHistory) if err != nil { - response.NewErrorResponse(handlerutil.ErrorCode(err), err.Error()).Write(w) + returnErrMessage(err, w) return } w.Header().Set("Status-Code", "200") diff --git a/cmd/kubeops/internal/handler/handler_test.go b/cmd/kubeops/internal/handler/handler_test.go index 045db24c0d9..99c61cb27a4 100644 --- a/cmd/kubeops/internal/handler/handler_test.go +++ b/cmd/kubeops/internal/handler/handler_test.go @@ -1,6 +1,7 @@ package handler import ( + "errors" "fmt" "io/ioutil" "net/http" @@ -31,13 +32,13 @@ var ( // newConfigFixture returns a Config with fake clients // and memory storage. -func newConfigFixture(t *testing.T) *Config { +func newConfigFixture(t *testing.T, k *kubefake.FailingKubeClient) *Config { t.Helper() return &Config{ ActionConfig: &action.Configuration{ Releases: storage.Init(driver.NewMemory()), - KubeClient: &kubefake.FailingKubeClient{PrintingKubeClient: kubefake.PrintingKubeClient{Out: ioutil.Discard}}, + KubeClient: k, Capabilities: chartutil.DefaultCapabilities, Log: func(format string, v ...interface{}) { t.Helper() @@ -81,6 +82,7 @@ func TestActions(t *testing.T) { Description string ExistingReleases []*release.Release Skip bool //TODO: Remove this when the memory bug is fixed + KubeError error // Request params RequestBody string RequestQuery string @@ -244,6 +246,23 @@ func TestActions(t *testing.T) { RemainingReleases: nil, ResponseBody: "", }, + { + // Scenario params + Description: "Creates a release with missing permissions", + ExistingReleases: []*release.Release{}, + KubeError: errors.New(`Failed to create: secrets is forbidden: User "foo" cannot create resource "secrets" in API group "" in the namespace "default"`), + // Request params + RequestBody: `{"chartName": "foo", "releaseName": "foobar", "version": "1.0.0"}`, + RequestQuery: "", + Action: "create", + Params: map[string]string{"namespace": "default"}, + // Expected result + StatusCode: 403, + RemainingReleases: []*release.Release{ + createRelease("foo", "foobar", "default", 1, release.StatusFailed), + }, + ResponseBody: `{"code":403,"message":"[{\"apiGroup\":\"\",\"resource\":\"secrets\",\"namespace\":\"default\",\"clusterWide\":false,\"verbs\":[\"create\"]}]"}`, + }, } for _, test := range tests { @@ -256,7 +275,13 @@ func TestActions(t *testing.T) { // Initialize environment for test req := httptest.NewRequest("GET", fmt.Sprintf("http://foo.bar%s", test.RequestQuery), strings.NewReader(test.RequestBody)) response := httptest.NewRecorder() - cfg := newConfigFixture(t) + k := &kubefake.FailingKubeClient{PrintingKubeClient: kubefake.PrintingKubeClient{Out: ioutil.Discard}} + if test.KubeError != nil { + k.CreateError = test.KubeError + k.UpdateError = test.KubeError + k.DeleteError = test.KubeError + } + cfg := newConfigFixture(t, k) createExistingReleases(t, cfg, test.ExistingReleases) // Perform request @@ -403,7 +428,8 @@ func TestRollbackAction(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - cfg := newConfigFixture(t) + k := &kubefake.FailingKubeClient{PrintingKubeClient: kubefake.PrintingKubeClient{Out: ioutil.Discard}} + cfg := newConfigFixture(t, k) createExistingReleases(t, cfg, tc.existingReleases) req := httptest.NewRequest("PUT", fmt.Sprintf("https://example.com/whatever?%s", tc.queryString), strings.NewReader("")) response := httptest.NewRecorder() @@ -472,7 +498,8 @@ func TestUpgradeAction(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - cfg := newConfigFixture(t) + k := &kubefake.FailingKubeClient{PrintingKubeClient: kubefake.PrintingKubeClient{Out: ioutil.Discard}} + cfg := newConfigFixture(t, k) createExistingReleases(t, cfg, tc.existingReleases) req := httptest.NewRequest("PUT", fmt.Sprintf("https://example.com/whatever?%s", tc.queryString), strings.NewReader(tc.requestBody)) response := httptest.NewRecorder() diff --git a/go.sum b/go.sum index bc448d2fec5..0c6beaa3a98 100644 --- a/go.sum +++ b/go.sum @@ -222,6 +222,7 @@ github.com/google/go-cmp v0.2.0/go.mod h1:oXzfMopK8JAjlY9xF4vHSVASa0yLyX7SntLO5a github.com/google/go-cmp v0.3.0/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMywk6iLU= github.com/google/go-cmp v0.3.1 h1:Xye71clBPdm5HgqGwUkwhbynsUJZhDbS20FvLhQ2izg= github.com/google/go-cmp v0.3.1/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMywk6iLU= +github.com/google/go-cmp v0.4.0 h1:xsAVV57WRhGj6kEIi8ReJzQlHHqcBYCElAvkovg3B/4= github.com/google/gofuzz v0.0.0-20161122191042-44d81051d367/go.mod h1:HP5RmnzzSNb993RKQDq4+1A4ia9nllfqcQFTQJedwGI= github.com/google/gofuzz v1.0.0 h1:A8PeW59pxE9IoFRqBp37U+mSNaQoZ46F1f0f863XSXw= github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= @@ -551,6 +552,7 @@ gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v2 v2.2.4 h1:/eiJrUcujPVeJ3xlSWaiNi3uSVmDGBK1pDHUHAnao1I= gopkg.in/yaml.v2 v2.2.4/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gotest.tools v2.2.0+incompatible/go.mod h1:DsYFclhRJ6vuDpmuTbkuFWG+y2sxOXAzmJt81HFBacw= +helm.sh/helm v2.16.1+incompatible h1:np11uYeEtlYcFIFRya8Xs5ZweV1z6MvaWQqJAW+1SZQ= helm.sh/helm/v3 v3.0.0 h1:or/9cs1GgfcTQeEnR2CVJNw893/rmqIG1KsNHmUiSFw= helm.sh/helm/v3 v3.0.0/go.mod h1:sI7B9yfvMgxtTPMWdk1jSKJ2aa59UyP9qhPydqW6mgo= helm.sh/helm/v3 v3.0.2 h1:BggvLisIMrAc+Is5oAHVrlVxgwOOrMN8nddfQbm5gKo= diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index e2e9989bfd5..9cfe521035f 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -18,6 +18,7 @@ package auth import ( "fmt" + "regexp" "strings" yamlUtils "github.com/kubeapps/kubeapps/pkg/yaml" @@ -214,24 +215,40 @@ func (u *UserAuth) isAllowed(verb string, itemsToCheck []resource) ([]Action, er return rejectedActions, nil } +func uniqVerbs(current []string, new []string) []string { + resMap := map[string]bool{} + for _, v := range current { + if !resMap[v] { + resMap[v] = true + } + } + res := append([]string{}, current...) + for _, v := range new { + if !resMap[v] { + resMap[v] = true + res = append(res, v) + } + } + return res +} + 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, + Verbs: uniqVerbs(resMap[req].Verbs, action.Verbs), } } else { resMap[req] = action } } + res := []Action{} for _, a := range resMap { res = append(res, a) } @@ -267,3 +284,24 @@ func (u *UserAuth) GetForbiddenActions(namespace, action, manifest string) ([]Ac } return forbiddenActions, nil } + +// ParseForbiddenActions parses a forbidden error returned by the Kubernetes API and return the list of forbidden actions +func ParseForbiddenActions(message string) []Action { + // TODO(andresmgot): Helm may not return all the required permissions in the same message. At the moment of writing this + // the only supported format is an error string so we can only parse the message with a regex + // More info: https://github.com/helm/helm/issues/7453 + re := regexp.MustCompile(`User "(.*?)" cannot (.*?) resource "(.*?)" in API group "(.*?)"(?: in the namespace "(.*?)")?`) + match := re.FindAllStringSubmatch(message, -1) + forbiddenActions := []Action{} + for _, role := range match { + forbiddenActions = append(forbiddenActions, Action{ + // TODO(andresmgot): Return the user/serviceaccount trying to perform the action + Verbs: []string{role[2]}, + Resource: role[3], + APIVersion: role[4], + Namespace: role[5], + ClusterWide: role[5] == "", + }) + } + return reduceActionsByVerb(forbiddenActions) +} diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index 78a5954243e..f48d3df31be 100644 --- a/pkg/auth/auth_test.go +++ b/pkg/auth/auth_test.go @@ -17,6 +17,8 @@ limitations under the License. package auth import ( + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "reflect" "strings" "testing" @@ -211,3 +213,54 @@ kind: FooBar } } } + +func TestParseForbiddenActions(t *testing.T) { + testSuite := []struct { + Description string + Error string + ExpectedActions []Action + }{ + { + "parses an error with a single resource", + `User "foo" cannot create resource "secrets" in API group "" in the namespace "default"`, + []Action{ + {APIVersion: "", Resource: "secrets", Namespace: "default", Verbs: []string{"create"}}, + }, + }, + { + "parses an error with a cluster-wide resource", + `User "foo" cannot create resource "clusterroles" in API group "v1"`, + []Action{ + {APIVersion: "v1", Resource: "clusterroles", Namespace: "", Verbs: []string{"create"}, ClusterWide: true}, + }, + }, + { + "parses several resources", + `User "foo" cannot create resource "secrets" in API group "" in the namespace "default"; + User "foo" cannot create resource "pods" in API group "" in the namespace "default"`, + []Action{ + {APIVersion: "", Resource: "secrets", Namespace: "default", Verbs: []string{"create"}}, + {APIVersion: "", Resource: "pods", Namespace: "default", Verbs: []string{"create"}}, + }, + }, + { + "includes different verbs and remove duplicates", + `User "foo" cannot create resource "secrets" in API group "" in the namespace "default"; + User "foo" cannot create resource "secrets" in API group "" in the namespace "default"; + User "foo" cannot delete resource "secrets" in API group "" in the namespace "default"`, + []Action{ + {APIVersion: "", Resource: "secrets", Namespace: "default", Verbs: []string{"create", "delete"}}, + }, + }, + } + for _, test := range testSuite { + t.Run(test.Description, func(t *testing.T) { + actions := ParseForbiddenActions(test.Error) + // order actions by resource + less := func(x, y Action) bool { return strings.Compare(x.Resource, y.Resource) < 0 } + if !cmp.Equal(actions, test.ExpectedActions, cmpopts.SortSlices(less)) { + t.Errorf("Unexpected forbidden actions: %v", cmp.Diff(actions, test.ExpectedActions)) + } + }) + } +} diff --git a/pkg/handlerutil/handlerutil.go b/pkg/handlerutil/handlerutil.go index 2d152575fe9..c21a452d218 100644 --- a/pkg/handlerutil/handlerutil.go +++ b/pkg/handlerutil/handlerutil.go @@ -47,24 +47,27 @@ func isUnprocessable(err error) bool { return re.MatchString(err.Error()) } +// ErrorCode returns the int representing an error. func ErrorCode(err error) int { return ErrorCodeWithDefault(err, http.StatusInternalServerError) } +// ErrorCodeWithDefault returns the int representing an error with a default value. func ErrorCodeWithDefault(err error, defaultCode int) int { errCode := defaultCode if isAlreadyExists(err) { errCode = http.StatusConflict - } else if isNotFound(err) { - errCode = http.StatusNotFound } else if isForbidden(err) { errCode = http.StatusForbidden + } else if isNotFound(err) { + errCode = http.StatusNotFound } else if isUnprocessable(err) { errCode = http.StatusUnprocessableEntity } return errCode } +// ParseAndGetChart request and parse a chart. func ParseAndGetChart(req *http.Request, cu chartUtils.Resolver, requireV1Support bool) (*chartUtils.Details, *chartUtils.ChartMultiVersion, error) { defer req.Body.Close() body, err := ioutil.ReadAll(req.Body)