Add comments, implement workpace agent scope

Emyrk · Emyrk · commit f3ff52e62cb1 · 2023-01-27T09:05:28.000-06:00
diff --git a/coderd/gitsshkey.go b/coderd/gitsshkey.go
@@ -4,7 +4,6 @@ import (
 	"net/http"
 
 	"github.com/coder/coder/coderd/audit"
-	"github.com/coder/coder/coderd/authzquery"
 	"github.com/coder/coder/coderd/database"
 	"github.com/coder/coder/coderd/gitsshkey"
 	"github.com/coder/coder/coderd/httpapi"
@@ -127,7 +126,6 @@ func (api *API) gitSSHKey(rw http.ResponseWriter, r *http.Request) {
 func (api *API) agentGitSSHKey(rw http.ResponseWriter, r *http.Request) {
 	ctx := r.Context()
 	agent := httpmw.WorkspaceAgent(r)
-	agentCtx := authzquery.WithWorkspaceAgentTokenContext(ctx, agent.ResourceID, agent.ID, rbac.RoleNames([]string{}), []string{})
 	resource, err := api.Database.GetWorkspaceResourceByID(agentCtx, agent.ResourceID)
 	if err != nil {
 		httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
diff --git a/coderd/httpmw/workspaceagent.go b/coderd/httpmw/workspaceagent.go
@@ -92,11 +92,16 @@ func ExtractWorkspaceAgent(db database.Store) func(http.Handler) http.Handler {
 				return
 			}
 
+			// A user that creates a workspace can use this agent auth token and
+			// impersonate the workspace. So to prevent privledge escalation, the
+			// subject inherits the roles of the user that owns the workspace.
+			// We then add a workspace-agent scope to limit the permissions
+			// to only what the workspace agent needs.
 			subject := rbac.Subject{
 				ID:     user.ID.String(),
 				Roles:  rbac.RoleNames(roles.Roles),
 				Groups: roles.Groups,
-				Scope:  rbac.ScopeAll, // TODO: ScopeWorkspaceAgent
+				Scope:  rbac.WorkspaceAgentScope(workspace.ID),
 			}
 
 			ctx = context.WithValue(ctx, workspaceAgentContextKey{}, agent)
diff --git a/coderd/rbac/scopes.go b/coderd/rbac/scopes.go
@@ -3,6 +3,8 @@ package rbac
 import (
 	"fmt"
 
+	"github.com/google/uuid"
+
 	"golang.org/x/xerrors"
 )
 
@@ -41,6 +43,24 @@ func (s Scope) Name() string {
 	return s.Role.Name
 }
 
+func WorkspaceAgentScope(workspaceID uuid.UUID) Scope {
+	allScope, err := ScopeAll.Expand()
+	if err != nil {
+		panic("failed to expand scope all, this should never happen")
+	}
+	return Scope{
+		// TODO: We want to limit the role too to be extra safe.
+		// Even though the allowlist blocks anything else, it is still good
+		// incase we change the behavior of the allowlist. The allowlist is new
+		// and evolving.
+		Role: allScope.Role,
+		// This prevents the agent from being able to access any other resource.
+		AllowIDList: []string{
+			workspaceID.String(),
+		},
+	}
+}
+
 const (
 	ScopeAll                ScopeName = "all"
 	ScopeApplicationConnect ScopeName = "application_connect"
diff --git a/coderd/templates.go b/coderd/templates.go
@@ -99,6 +99,10 @@ func (api *API) deleteTemplate(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// TODO: This just returns the workspaces a user can view. We should use
+	// a system function to get all workspaces that use this template.
+	// This data should never be exposed to the user aside from a non-zero count.
+	// Or we move this into a postgres constraint.
 	workspaces, err := api.Database.GetWorkspaces(ctx, database.GetWorkspacesParams{
 		TemplateIds: []uuid.UUID{template.ID},
 	})
diff --git a/coderd/users.go b/coderd/users.go
@@ -72,6 +72,7 @@ func (api *API) firstUser(rw http.ResponseWriter, r *http.Request) {
 // @Success 201 {object} codersdk.CreateFirstUserResponse
 // @Router /users/first [post]
 func (api *API) postFirstUser(rw http.ResponseWriter, r *http.Request) {
+	// TODO: Should this admin system context be in a middleware?
 	ctx := authzquery.WithAuthorizeSystemContext(r.Context(), rbac.RolesAdminSystem())
 	var createUser codersdk.CreateFirstUserRequest
 	if !httpapi.Read(ctx, rw, r, &createUser) {
diff --git a/coderd/workspaces.go b/coderd/workspaces.go
@@ -359,6 +359,9 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
 		return
 	}
 
+	// TODO: This should be a system call as the actor might not be able to
+	// read other workspaces. Ideally we check the error on create and look for
+	// a postgres conflict error.
 	workspace, err := api.Database.GetWorkspaceByOwnerIDAndName(ctx, database.GetWorkspaceByOwnerIDAndNameParams{
 		OwnerID: user.ID,
 		Name:    createWorkspace.Name,

Original file line number	Diff line number	Diff line change
`@@ -359,6 +359,9 @@ func (api API) postWorkspacesByOrganization(rw http.ResponseWriter, r http.Req`
`359`	`359`	`return`
`360`	`360`	`}`
`361`	`361`
	`362`	`+ // TODO: This should be a system call as the actor might not be able to`
	`363`	`+ // read other workspaces. Ideally we check the error on create and look for`
	`364`	`+ // a postgres conflict error.`
`362`	`365`	`workspace, err := api.Database.GetWorkspaceByOwnerIDAndName(ctx, database.GetWorkspaceByOwnerIDAndNameParams{`
`363`	`366`	`OwnerID: user.ID,`
`364`	`367`	`Name: createWorkspace.Name,`