mirror of
https://github.com/coder/coder.git
synced 2026-10-16 14:48:56 +08:00
fix: enforce template ACL in workspace app proxy
The workspace app proxy bypassed template ACL checks, allowing users to access workspace apps (e.g., code-server) even after their template access was revoked. The dashboard UI and terminal correctly returned 404, but the app proxy continued serving the app. This happened because the proxy used a system context for all database queries and only checks workspace-level RBAC. The fix widens ScopeApplicationConnect to include template read, and checks template ACL when verifying the user has access to a workspace app. When the workspace owner's template access has been revoked, app access is now blocked. Note: revocation takes up to 1 minute to take effect. The app proxy caches authorization decisions in signed JWT tokens. While a cached token is valid, requests bypass the authorization path. Once the token expires, the next request triggers a fresh authorization check which enforces the updated template ACL.
This commit is contained in:
@@ -592,9 +592,9 @@ func TestAuthorizeDomain(t *testing.T) {
|
||||
|
||||
{resource: ResourceWorkspace.WithOwner("not-me")},
|
||||
}),
|
||||
// Other Objects
|
||||
// Other Objects: non-read actions still blocked by scope
|
||||
cases(func(c authTestCase) authTestCase {
|
||||
c.actions = []policy.Action{policy.ActionCreate, policy.ActionRead, policy.ActionUpdate, policy.ActionDelete}
|
||||
c.actions = []policy.Action{policy.ActionCreate, policy.ActionUpdate, policy.ActionDelete}
|
||||
c.allow = false
|
||||
return c
|
||||
}, []authTestCase{
|
||||
@@ -621,6 +621,37 @@ func TestAuthorizeDomain(t *testing.T) {
|
||||
|
||||
{resource: ResourceTemplate.WithOwner("not-me")},
|
||||
}),
|
||||
// Template read: allowed by scope, checked against roles.
|
||||
// The member role grants template:* on own resources via user
|
||||
// permissions, so owned templates pass.
|
||||
cases(func(c authTestCase) authTestCase {
|
||||
c.actions = []policy.Action{policy.ActionRead}
|
||||
return c
|
||||
}, []authTestCase{
|
||||
// Org + me (member role allows template:* on own resources)
|
||||
{resource: ResourceTemplate.InOrg(defOrg).WithOwner(user.ID), allow: true},
|
||||
{resource: ResourceTemplate.InOrg(defOrg), allow: false},
|
||||
|
||||
// No org + me (member user-level perms allow template:*)
|
||||
{resource: ResourceTemplate.WithOwner(user.ID), allow: true},
|
||||
|
||||
{resource: ResourceTemplate.All(), allow: false},
|
||||
|
||||
// Other org + me
|
||||
{resource: ResourceTemplate.InOrg(unusedID).WithOwner(user.ID), allow: false},
|
||||
{resource: ResourceTemplate.InOrg(unusedID), allow: false},
|
||||
|
||||
// Other org + other user
|
||||
{resource: ResourceTemplate.InOrg(defOrg).WithOwner("not-me"), allow: false},
|
||||
|
||||
{resource: ResourceTemplate.WithOwner("not-me"), allow: false},
|
||||
|
||||
// Other org + other use
|
||||
{resource: ResourceTemplate.InOrg(unusedID).WithOwner("not-me"), allow: false},
|
||||
{resource: ResourceTemplate.InOrg(unusedID), allow: false},
|
||||
|
||||
{resource: ResourceTemplate.WithOwner("not-me"), allow: false},
|
||||
}),
|
||||
)
|
||||
|
||||
// In practice this is a token scope on a regular subject
|
||||
|
||||
@@ -99,6 +99,7 @@ var builtinScopes = map[ScopeName]Scope{
|
||||
DisplayName: "Ability to connect to applications",
|
||||
Site: Permissions(map[string][]policy.Action{
|
||||
ResourceWorkspace.Type: {policy.ActionApplicationConnect},
|
||||
ResourceTemplate.Type: {policy.ActionRead},
|
||||
}),
|
||||
User: []Permission{},
|
||||
ByOrgID: map[string]OrgPermissions{},
|
||||
|
||||
@@ -92,8 +92,8 @@ func (p *DBTokenProvider) FromRequest(r *http.Request) (*SignedToken, bool) {
|
||||
func (p *DBTokenProvider) Issue(ctx context.Context, rw http.ResponseWriter, r *http.Request, issueReq IssueTokenRequest) (*SignedToken, string, bool) {
|
||||
// nolint:gocritic // We need to make a number of database calls. Setting a system context here
|
||||
// // is simpler than calling dbauthz.AsSystemRestricted on every call.
|
||||
// // dangerousSystemCtx is only used for database calls. The actual authentication
|
||||
// // logic is handled in Provider.authorizeWorkspaceApp which directly checks the actor's
|
||||
// // dangerousSystemCtx is only used for database calls. The actual authorization
|
||||
// // logic is handled in authorizeRequest, which directly checks the actor's
|
||||
// // permissions.
|
||||
dangerousSystemCtx := dbauthz.AsSystemRestricted(ctx)
|
||||
|
||||
@@ -299,6 +299,16 @@ func (p *DBTokenProvider) authorizeRequest(ctx context.Context, roles *rbac.Subj
|
||||
return sharingLevel == database.AppSharingLevelPublic, warnings, nil
|
||||
}
|
||||
|
||||
// Enforce template ACLs for workspace owners. If the owner's access
|
||||
// to the template has been revoked, they should not be able to access
|
||||
// workspace apps. Non-owners skip this check because they may
|
||||
// legitimately lack direct template access (e.g. shared app users).
|
||||
if dbReq.Workspace.OwnerID.String() == roles.ID {
|
||||
if err := p.Authorizer.Authorize(ctx, *roles, policy.ActionRead, dbReq.Template.RBACObject()); err != nil {
|
||||
return false, warnings, nil
|
||||
}
|
||||
}
|
||||
|
||||
// Block anyone from accessing workspaces they don't own in path-based apps
|
||||
// unless the admin disables this security feature. This blocks site-owners
|
||||
// from accessing any apps from any user's workspaces.
|
||||
|
||||
@@ -203,6 +203,9 @@ type databaseRequest struct {
|
||||
// AppSharingLevel is the sharing level of the app. This is forced to be set
|
||||
// to AppSharingLevelOwner if the access method is terminal.
|
||||
AppSharingLevel database.AppSharingLevel
|
||||
// Template is the template that the workspace is using. This is used
|
||||
// for template ACL checks.
|
||||
Template database.Template
|
||||
// CorsBehavior is set at the template level for all apps/ports in a workspace, and can
|
||||
// either be the current CORS middleware 'simple' or bypass the cors middleware with 'passthru'.
|
||||
CorsBehavior database.CorsBehavior
|
||||
@@ -424,6 +427,7 @@ func (r Request) getDatabase(ctx context.Context, db database.Store) (*databaseR
|
||||
Workspace: workspace.WorkspaceTable(),
|
||||
Agent: agent,
|
||||
App: app,
|
||||
Template: tmpl,
|
||||
AppURL: appURLParsed,
|
||||
AppSharingLevel: appSharingLevel,
|
||||
CorsBehavior: corsBehavior,
|
||||
@@ -448,11 +452,17 @@ func (r Request) getDatabaseTerminal(ctx context.Context, db database.Store) (*d
|
||||
return nil, xerrors.Errorf("get workspace agent %q with workspace: %w", agentID, err)
|
||||
}
|
||||
|
||||
tmpl, err := db.GetTemplateByID(ctx, aw.WorkspaceTable.TemplateID)
|
||||
if err != nil {
|
||||
return nil, xerrors.Errorf("get template %q: %w", aw.WorkspaceTable.TemplateID, err)
|
||||
}
|
||||
|
||||
return &databaseRequest{
|
||||
Request: r,
|
||||
UserID: aw.WorkspaceTable.OwnerID,
|
||||
Workspace: aw.WorkspaceTable,
|
||||
Agent: aw.WorkspaceAgent,
|
||||
Template: tmpl,
|
||||
AppURL: nil,
|
||||
AppSharingLevel: database.AppSharingLevelOwner,
|
||||
}, nil
|
||||
|
||||
@@ -4,6 +4,7 @@ import (
|
||||
"bytes"
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"net/http"
|
||||
"slices"
|
||||
"testing"
|
||||
@@ -15,6 +16,7 @@ import (
|
||||
|
||||
"cdr.dev/slog/v3"
|
||||
"cdr.dev/slog/v3/sloggers/slogtest"
|
||||
"github.com/coder/coder/v2/agent/agenttest"
|
||||
"github.com/coder/coder/v2/coderd/audit"
|
||||
"github.com/coder/coder/v2/coderd/coderdtest"
|
||||
"github.com/coder/coder/v2/coderd/database"
|
||||
@@ -2326,3 +2328,104 @@ func TestInvalidateTemplatePrebuilds_LicenseFeatureDisabled(t *testing.T) {
|
||||
require.ErrorAs(t, err, &sdkError)
|
||||
require.Equal(t, http.StatusForbidden, sdkError.StatusCode())
|
||||
}
|
||||
|
||||
func TestTemplateACLBlocksWorkspaceAppAccess(t *testing.T) {
|
||||
// Verify that revoking a user's template ACL access also blocks access
|
||||
// to workspace apps served via the app proxy.
|
||||
t.Parallel()
|
||||
|
||||
ownerClient, owner := coderdenttest.New(t, &coderdenttest.Options{
|
||||
Options: &coderdtest.Options{
|
||||
IncludeProvisionerDaemon: true,
|
||||
},
|
||||
LicenseOptions: &coderdenttest.LicenseOptions{
|
||||
Features: license.Features{
|
||||
codersdk.FeatureTemplateRBAC: 1,
|
||||
},
|
||||
},
|
||||
})
|
||||
|
||||
// Create a member user who will own the workspace.
|
||||
memberClient, _ := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID, rbac.RoleMember())
|
||||
|
||||
ctx := testutil.Context(t, testutil.WaitLong)
|
||||
|
||||
// Create a template with a workspace app.
|
||||
agentAuthToken := uuid.NewString()
|
||||
version := coderdtest.CreateTemplateVersion(t, ownerClient, owner.OrganizationID, &echo.Responses{
|
||||
Parse: echo.ParseComplete,
|
||||
ProvisionPlan: echo.PlanComplete,
|
||||
ProvisionGraph: []*proto.Response{{
|
||||
Type: &proto.Response_Graph{
|
||||
Graph: &proto.GraphComplete{
|
||||
Resources: []*proto.Resource{{
|
||||
Name: "example",
|
||||
Type: "aws_instance",
|
||||
Agents: []*proto.Agent{{
|
||||
Id: uuid.NewString(),
|
||||
Name: "agent",
|
||||
Auth: &proto.Agent_Token{
|
||||
Token: agentAuthToken,
|
||||
},
|
||||
Apps: []*proto.App{{
|
||||
Slug: "test-app",
|
||||
DisplayName: "Test App",
|
||||
SharingLevel: proto.AppSharingLevel_OWNER,
|
||||
Url: "http://localhost:8080",
|
||||
}},
|
||||
}},
|
||||
}},
|
||||
},
|
||||
},
|
||||
}},
|
||||
})
|
||||
coderdtest.AwaitTemplateVersionJobCompleted(t, ownerClient, version.ID)
|
||||
template := coderdtest.CreateTemplate(t, ownerClient, owner.OrganizationID, version.ID)
|
||||
|
||||
// Member creates a workspace from the template.
|
||||
workspace := coderdtest.CreateWorkspace(t, memberClient, template.ID)
|
||||
coderdtest.AwaitWorkspaceBuildJobCompleted(t, memberClient, workspace.LatestBuild.ID)
|
||||
|
||||
// Start the agent so the workspace app can be resolved.
|
||||
_ = agenttest.New(t, ownerClient.URL, agentAuthToken)
|
||||
_ = coderdtest.NewWorkspaceAgentWaiter(t, memberClient, workspace.ID).AgentNames([]string{"agent"}).Wait()
|
||||
|
||||
me, err := memberClient.User(ctx, codersdk.Me)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Verify the member can access the workspace app before ACL change.
|
||||
// A non-404 status means the auth check passed.
|
||||
appURL := fmt.Sprintf("/@%s/%s/apps/test-app/", me.Username, workspace.Name)
|
||||
resp, err := memberClient.Request(ctx, http.MethodGet, appURL, nil)
|
||||
require.NoError(t, err)
|
||||
_ = resp.Body.Close()
|
||||
require.NotEqual(t, http.StatusNotFound, resp.StatusCode,
|
||||
"expected app to be accessible before template ACL revocation")
|
||||
|
||||
// Revoke the member's template access by removing the everyone group.
|
||||
//nolint:gocritic // Using ownerClient is required to manage template ACLs.
|
||||
acl, err := ownerClient.TemplateACL(ctx, template.ID)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, acl.Groups, 1, "expected exactly one group")
|
||||
require.Equal(t, acl.Groups[0].IsEveryone(), true, "expected everyone group")
|
||||
|
||||
//nolint:gocritic // Using ownerClient is required to manage template ACLs.
|
||||
err = ownerClient.UpdateTemplateACL(ctx, template.ID, codersdk.UpdateTemplateACL{
|
||||
GroupPerms: map[string]codersdk.TemplateRole{
|
||||
acl.Groups[0].ID.String(): codersdk.TemplateRoleDeleted,
|
||||
},
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
// Verify the member can no longer see the template.
|
||||
_, err = memberClient.Template(ctx, template.ID)
|
||||
require.Error(t, err, "member should not be able to see template after ACL revocation")
|
||||
|
||||
// Verify the member can no longer access the workspace app. The app
|
||||
// proxy should check template ACL and block access.
|
||||
resp, err = memberClient.Request(ctx, http.MethodGet, appURL, nil)
|
||||
require.NoError(t, err)
|
||||
_ = resp.Body.Close()
|
||||
require.Equal(t, http.StatusNotFound, resp.StatusCode,
|
||||
"workspace app should return 404 after template ACL revocation")
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user