From 84a415f44a5ef93cec9d8ad331d287d49afde4d0 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 20 May 2022 12:29:12 -0500 Subject: [PATCH 1/4] feat: Add RBAC to /files endpoints --- coderd/coderd.go | 1 + coderd/coderd_test.go | 7 ++++--- coderd/files.go | 15 ++++++++++++--- coderd/files_test.go | 2 +- 4 files changed, 18 insertions(+), 7 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 343da27bf55cf..492f72bdb3e7d 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -134,6 +134,7 @@ func newRouter(options *Options, a *api) chi.Router { r.Route("/files", func(r chi.Router) { r.Use( apiKeyMiddleware, + authRolesMiddleware, // This number is arbitrary, but reading/writing // file content is expensive so it should be small. httpmw.RateLimitPerMinute(12), diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index f6d555fb3d015..c1be86c800728 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -121,8 +121,6 @@ func TestAuthorizeAllEndpoints(t *testing.T) { "POST:/api/v2/users/{user}/organizations": {NoAuthorize: true}, - "POST:/api/v2/files": {NoAuthorize: true}, - "GET:/api/v2/files/{hash}": {NoAuthorize: true}, "GET:/api/v2/workspaces/{workspace}/watch": {NoAuthorize: true}, // These endpoints have more assertions. This is good, add more endpoints to assert if you can! @@ -184,6 +182,9 @@ func TestAuthorizeAllEndpoints(t *testing.T) { AssertObject: workspaceRBACObj, }, + "POST:/api/v2/files": {AssertAction: rbac.ActionCreate, AssertObject: rbac.ResourceFile}, + "GET:/api/v2/files/{hash}": {AssertAction: rbac.ActionRead, AssertObject: rbac.ResourceFile}, + // These endpoints need payloads to get to the auth part. Payloads will be required "PUT:/api/v2/users/{user}/roles": {StatusCode: http.StatusBadRequest, NoAuthorize: true}, "POST:/api/v2/workspaces/{workspace}/builds": {StatusCode: http.StatusBadRequest, NoAuthorize: true}, @@ -197,7 +198,7 @@ func TestAuthorizeAllEndpoints(t *testing.T) { } assertRoute[noTrailSlash] = v } - + c, _ := srv.Config.Handler.(*chi.Mux) err = chi.Walk(c, func(method string, route string, handler http.Handler, middlewares ...func(http.Handler) http.Handler) error { name := method + ":" + route diff --git a/coderd/files.go b/coderd/files.go index 5daa4255ada53..974aef8f4149a 100644 --- a/coderd/files.go +++ b/coderd/files.go @@ -14,11 +14,16 @@ import ( "github.com/coder/coder/coderd/database" "github.com/coder/coder/coderd/httpapi" "github.com/coder/coder/coderd/httpmw" + "github.com/coder/coder/coderd/rbac" "github.com/coder/coder/codersdk" ) func (api *api) postFile(rw http.ResponseWriter, r *http.Request) { apiKey := httpmw.APIKey(r) + if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceFile) { + return + } + contentType := r.Header.Get("Content-Type") switch contentType { @@ -77,9 +82,7 @@ func (api *api) fileByHash(rw http.ResponseWriter, r *http.Request) { } file, err := api.Database.GetFileByHash(r.Context(), hash) if errors.Is(err, sql.ErrNoRows) { - httpapi.Write(rw, http.StatusNotFound, httpapi.Response{ - Message: "no file exists with that hash", - }) + httpapi.Forbidden(rw) return } if err != nil { @@ -88,6 +91,12 @@ func (api *api) fileByHash(rw http.ResponseWriter, r *http.Request) { }) return } + + if !api.Authorize(rw, r, rbac.ActionRead, + rbac.ResourceFile.WithOwner(file.CreatedBy.String()).WithID(file.Hash)) { + return + } + rw.Header().Set("Content-Type", file.Mimetype) rw.WriteHeader(http.StatusOK) _, _ = rw.Write(file.Data) diff --git a/coderd/files_test.go b/coderd/files_test.go index 016774a030c88..19609c8611e95 100644 --- a/coderd/files_test.go +++ b/coderd/files_test.go @@ -50,7 +50,7 @@ func TestDownload(t *testing.T) { _, _, err := client.Download(context.Background(), "something") var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) - require.Equal(t, http.StatusNotFound, apiErr.StatusCode()) + require.Equal(t, http.StatusForbidden, apiErr.StatusCode()) }) t.Run("Insert", func(t *testing.T) { From bcff7826065e07198efd0c9712f55d04082ac8df Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 23 May 2022 09:21:04 -0500 Subject: [PATCH 2/4] feat: Protect /files endpoints with rbac --- coderd/files.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/coderd/files.go b/coderd/files.go index 974aef8f4149a..74b4b56f26f02 100644 --- a/coderd/files.go +++ b/coderd/files.go @@ -20,6 +20,8 @@ import ( func (api *api) postFile(rw http.ResponseWriter, r *http.Request) { apiKey := httpmw.APIKey(r) + // This requires the site wide action to create files. + // Once created, a user can read their own files uploaded if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceFile) { return } From 5379b950537990bea52256e574b18f7ce555b4d3 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 23 May 2022 09:31:58 -0500 Subject: [PATCH 3/4] better assert for rbac test --- coderd/coderd_test.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index c1be86c800728..c53bb07b45360 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -7,6 +7,8 @@ import ( "strings" "testing" + "github.com/coder/coder/codersdk" + "github.com/go-chi/chi/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -34,6 +36,7 @@ func TestBuildInfo(t *testing.T) { // TestAuthorizeAllEndpoints will check `authorize` is called on every endpoint registered. func TestAuthorizeAllEndpoints(t *testing.T) { t.Parallel() + ctx := context.Background() authorizer := &fakeAuthorizer{} srv, client, _ := coderdtest.NewWithServer(t, &coderdtest.Options{ @@ -50,6 +53,8 @@ func TestAuthorizeAllEndpoints(t *testing.T) { template := coderdtest.CreateTemplate(t, client, admin.OrganizationID, version.ID) workspace := coderdtest.CreateWorkspace(t, client, admin.OrganizationID, template.ID) coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID) + file, err := client.Upload(ctx, codersdk.ContentTypeTar, make([]byte, 1024)) + require.NoError(t, err, "upload file") // Always fail auth from this point forward authorizer.AlwaysReturn = rbac.ForbiddenWithInternal(xerrors.New("fake implementation"), nil, nil) @@ -182,8 +187,9 @@ func TestAuthorizeAllEndpoints(t *testing.T) { AssertObject: workspaceRBACObj, }, - "POST:/api/v2/files": {AssertAction: rbac.ActionCreate, AssertObject: rbac.ResourceFile}, - "GET:/api/v2/files/{hash}": {AssertAction: rbac.ActionRead, AssertObject: rbac.ResourceFile}, + "POST:/api/v2/files": {AssertAction: rbac.ActionCreate, AssertObject: rbac.ResourceFile}, + "GET:/api/v2/files/{fileHash}": {AssertAction: rbac.ActionRead, + AssertObject: rbac.ResourceFile.WithOwner(admin.UserID.String()).WithID(file.Hash)}, // These endpoints need payloads to get to the auth part. Payloads will be required "PUT:/api/v2/users/{user}/roles": {StatusCode: http.StatusBadRequest, NoAuthorize: true}, @@ -198,7 +204,7 @@ func TestAuthorizeAllEndpoints(t *testing.T) { } assertRoute[noTrailSlash] = v } - + c, _ := srv.Config.Handler.(*chi.Mux) err = chi.Walk(c, func(method string, route string, handler http.Handler, middlewares ...func(http.Handler) http.Handler) error { name := method + ":" + route @@ -221,6 +227,7 @@ func TestAuthorizeAllEndpoints(t *testing.T) { route = strings.ReplaceAll(route, "{workspacebuild}", workspace.LatestBuild.ID.String()) route = strings.ReplaceAll(route, "{workspacename}", workspace.Name) route = strings.ReplaceAll(route, "{workspacebuildname}", workspace.LatestBuild.Name) + route = strings.ReplaceAll(route, "{hash}", file.Hash) resp, err := client.Request(context.Background(), method, route, nil) require.NoError(t, err, "do req") From aa4efb88ea05d3192b3f6021e624ec398f552452 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 23 May 2022 09:46:23 -0500 Subject: [PATCH 4/4] Import order --- coderd/coderd_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index c53bb07b45360..01c6cb1145333 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -7,8 +7,6 @@ import ( "strings" "testing" - "github.com/coder/coder/codersdk" - "github.com/go-chi/chi/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -18,6 +16,7 @@ import ( "github.com/coder/coder/buildinfo" "github.com/coder/coder/coderd/coderdtest" "github.com/coder/coder/coderd/rbac" + "github.com/coder/coder/codersdk" ) func TestMain(m *testing.M) {