mirror of
https://github.com/coder/coder.git
synced 2026-09-22 05:05:20 +08:00
fix(coderd): prevent cross-tenant workspace app rebinding (#26103)
This commit is contained in:
@@ -259,8 +259,9 @@ func (a *SubAgentAPI) CreateSubAgent(ctx context.Context, req *agentproto.Create
|
||||
slugHashEnc := base32.HexEncoding.WithPadding(base32.NoPadding).EncodeToString(slugHash[:])
|
||||
computedSlug := strings.ToLower(slugHashEnc[:8]) + "-" + app.Slug
|
||||
|
||||
appID := uuid.New()
|
||||
_, err := a.Database.UpsertWorkspaceApp(ctx, database.UpsertWorkspaceAppParams{
|
||||
ID: uuid.New(), // NOTE: we may need to maintain the app's ID here for stability, but for now we'll leave this as-is.
|
||||
ID: appID, // NOTE: we may need to maintain the app's ID here for stability, but for now we'll leave this as-is.
|
||||
CreatedAt: createdAt,
|
||||
AgentID: subAgent.ID,
|
||||
Slug: computedSlug,
|
||||
@@ -291,6 +292,12 @@ func (a *SubAgentAPI) CreateSubAgent(ctx context.Context, req *agentproto.Create
|
||||
Tooltip: "", // tooltips are not currently supported in subagent workspaces, default to empty string
|
||||
})
|
||||
if err != nil {
|
||||
if errors.Is(err, sql.ErrNoRows) {
|
||||
// The upsert's ON CONFLICT guard refused to rebind an
|
||||
// existing workspace-owned app to an agent outside that
|
||||
// workspace, including agents that resolve to no workspace.
|
||||
return xerrors.Errorf("workspace app slug %q with ID %q is already bound to a workspace-owned agent and cannot be rebound to an agent in another workspace or to an agent without a workspace; refusing to rebind to agent ID %q", computedSlug, appID, subAgent.ID)
|
||||
}
|
||||
return xerrors.Errorf("insert workspace app: %w", err)
|
||||
}
|
||||
|
||||
|
||||
@@ -12,6 +12,7 @@ import (
|
||||
"github.com/prometheus/client_golang/prometheus"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
"go.uber.org/mock/gomock"
|
||||
|
||||
"cdr.dev/slog/v3"
|
||||
"github.com/coder/coder/v2/agent/proto"
|
||||
@@ -19,6 +20,7 @@ import (
|
||||
"github.com/coder/coder/v2/coderd/database"
|
||||
"github.com/coder/coder/v2/coderd/database/dbauthz"
|
||||
"github.com/coder/coder/v2/coderd/database/dbgen"
|
||||
"github.com/coder/coder/v2/coderd/database/dbmock"
|
||||
"github.com/coder/coder/v2/coderd/database/dbtestutil"
|
||||
"github.com/coder/coder/v2/coderd/rbac"
|
||||
"github.com/coder/coder/v2/coderd/util/ptr"
|
||||
@@ -804,6 +806,81 @@ func TestSubAgentAPI(t *testing.T) {
|
||||
})
|
||||
})
|
||||
|
||||
t.Run("CreateSubAgentWithAppRebindRejected", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
clock := quartz.NewMock(t)
|
||||
createdAt := clock.Now()
|
||||
parentAgent := database.WorkspaceAgent{
|
||||
ID: uuid.New(),
|
||||
ResourceID: uuid.New(),
|
||||
ConnectionTimeoutSeconds: 30,
|
||||
TroubleshootingURL: "https://example.com/troubleshoot",
|
||||
APIKeyScope: database.AgentKeyScopeEnumAll,
|
||||
}
|
||||
workspace := database.Workspace{
|
||||
ID: uuid.New(),
|
||||
TemplateID: uuid.New(),
|
||||
}
|
||||
template := database.Template{
|
||||
ID: workspace.TemplateID,
|
||||
MaxPortSharingLevel: database.AppSharingLevelPublic,
|
||||
}
|
||||
insertedSubAgent := database.WorkspaceAgent{
|
||||
ID: uuid.New(),
|
||||
ParentID: uuid.NullUUID{UUID: parentAgent.ID, Valid: true},
|
||||
ResourceID: parentAgent.ResourceID,
|
||||
Name: "child-agent",
|
||||
AuthToken: uuid.New(),
|
||||
}
|
||||
|
||||
dbM := dbmock.NewMockStore(gomock.NewController(t))
|
||||
dbM.EXPECT().GetWorkspaceByAgentID(gomock.Any(), parentAgent.ID).Return(workspace, nil)
|
||||
dbM.EXPECT().GetTemplateByID(gomock.Any(), workspace.TemplateID).Return(template, nil)
|
||||
dbM.EXPECT().InsertWorkspaceAgent(gomock.Any(), gomock.Cond(func(params database.InsertWorkspaceAgentParams) bool {
|
||||
return params.ParentID.Valid && params.ParentID.UUID == parentAgent.ID &&
|
||||
params.ResourceID == parentAgent.ResourceID &&
|
||||
params.Name == insertedSubAgent.Name
|
||||
})).Return(insertedSubAgent, nil)
|
||||
dbM.EXPECT().UpsertWorkspaceApp(gomock.Any(), gomock.Cond(func(params database.UpsertWorkspaceAppParams) bool {
|
||||
return params.ID != uuid.Nil &&
|
||||
params.AgentID == insertedSubAgent.ID &&
|
||||
params.CreatedAt.Equal(createdAt) &&
|
||||
params.Slug == "fdqf0lpd-code-server" &&
|
||||
params.DisplayName == "VS Code"
|
||||
})).Return(database.WorkspaceApp{}, sql.ErrNoRows)
|
||||
|
||||
api := &agentapi.SubAgentAPI{
|
||||
OwnerID: uuid.New(),
|
||||
OrganizationID: uuid.New(),
|
||||
AgentFn: func(context.Context) (database.WorkspaceAgent, error) { return parentAgent, nil },
|
||||
Clock: clock,
|
||||
Database: dbM,
|
||||
Log: testutil.Logger(t),
|
||||
}
|
||||
|
||||
createResp, err := api.CreateSubAgent(context.Background(), &proto.CreateSubAgentRequest{
|
||||
Name: insertedSubAgent.Name,
|
||||
Directory: "/workspaces/coder",
|
||||
Architecture: "amd64",
|
||||
OperatingSystem: "linux",
|
||||
Apps: []*proto.CreateSubAgentRequest_App{
|
||||
{
|
||||
Slug: "code-server",
|
||||
DisplayName: ptr.Ref("VS Code"),
|
||||
},
|
||||
},
|
||||
})
|
||||
require.NoError(t, err)
|
||||
require.Len(t, createResp.AppCreationErrors, 1)
|
||||
require.Equal(t, int32(0), createResp.AppCreationErrors[0].Index)
|
||||
require.Nil(t, createResp.AppCreationErrors[0].Field)
|
||||
require.Contains(t, createResp.AppCreationErrors[0].Error, "workspace app slug \"fdqf0lpd-code-server\"")
|
||||
require.Contains(t, createResp.AppCreationErrors[0].Error, "already bound to a workspace-owned agent")
|
||||
require.Contains(t, createResp.AppCreationErrors[0].Error, "cannot be rebound to an agent in another workspace or to an agent without a workspace")
|
||||
require.NotContains(t, createResp.AppCreationErrors[0].Error, "sql: no rows in result set")
|
||||
})
|
||||
|
||||
t.Run("DeleteSubAgent", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
|
||||
@@ -7558,6 +7558,178 @@ func TestWorkspaceAgentNameUniqueTrigger(t *testing.T) {
|
||||
})
|
||||
}
|
||||
|
||||
func TestUpsertWorkspaceAppCannotRebindAcrossWorkspaces(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
db, _ := dbtestutil.NewDB(t)
|
||||
org := dbgen.Organization(t, db, database.Organization{})
|
||||
ctx := testutil.Context(t, testutil.WaitShort)
|
||||
|
||||
// createWorkspace builds the owner -> template -> version -> workspace chain
|
||||
// and returns the workspace plus its template version so callers can create
|
||||
// additional builds (and thus agents) within the same workspace.
|
||||
createWorkspace := func(t *testing.T) (database.WorkspaceTable, uuid.UUID) {
|
||||
t.Helper()
|
||||
user := dbgen.User(t, db, database.User{})
|
||||
template := dbgen.Template(t, db, database.Template{
|
||||
OrganizationID: org.ID,
|
||||
CreatedBy: user.ID,
|
||||
})
|
||||
version := dbgen.TemplateVersion(t, db, database.TemplateVersion{
|
||||
TemplateID: uuid.NullUUID{Valid: true, UUID: template.ID},
|
||||
OrganizationID: org.ID,
|
||||
CreatedBy: user.ID,
|
||||
})
|
||||
workspace := dbgen.Workspace(t, db, database.WorkspaceTable{
|
||||
OrganizationID: org.ID,
|
||||
TemplateID: template.ID,
|
||||
OwnerID: user.ID,
|
||||
})
|
||||
return workspace, version.ID
|
||||
}
|
||||
|
||||
// addAgent creates a build, resource, and agent for the workspace. The
|
||||
// build's JobID matches the resource's JobID so the upsert's
|
||||
// agent -> resource -> workspace_builds(job_id) -> workspace_id traversal
|
||||
// resolves to the workspace.
|
||||
addAgent := func(t *testing.T, workspace database.WorkspaceTable, versionID uuid.UUID, buildNumber int32) database.WorkspaceAgent {
|
||||
t.Helper()
|
||||
job := dbgen.ProvisionerJob(t, db, nil, database.ProvisionerJob{
|
||||
Type: database.ProvisionerJobTypeWorkspaceBuild,
|
||||
OrganizationID: org.ID,
|
||||
})
|
||||
dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{
|
||||
BuildNumber: buildNumber,
|
||||
JobID: job.ID,
|
||||
WorkspaceID: workspace.ID,
|
||||
TemplateVersionID: versionID,
|
||||
})
|
||||
resource := dbgen.WorkspaceResource(t, db, database.WorkspaceResource{
|
||||
JobID: job.ID,
|
||||
})
|
||||
return dbgen.WorkspaceAgent(t, db, database.WorkspaceAgent{
|
||||
ResourceID: resource.ID,
|
||||
})
|
||||
}
|
||||
|
||||
upsertApp := func(appID, agentID uuid.UUID, slug string) (database.WorkspaceApp, error) {
|
||||
return db.UpsertWorkspaceApp(ctx, database.UpsertWorkspaceAppParams{
|
||||
ID: appID,
|
||||
CreatedAt: dbtime.Now(),
|
||||
AgentID: agentID,
|
||||
Slug: slug,
|
||||
DisplayName: "Code Server",
|
||||
Icon: "/icon.png",
|
||||
SharingLevel: database.AppSharingLevelOwner,
|
||||
Health: database.WorkspaceAppHealthDisabled,
|
||||
OpenIn: database.WorkspaceAppOpenInSlimWindow,
|
||||
})
|
||||
}
|
||||
|
||||
// Given: two independent workspaces, each with an agent that resolves to its
|
||||
// own workspace.
|
||||
workspaceA, versionA := createWorkspace(t)
|
||||
workspaceB, versionB := createWorkspace(t)
|
||||
agentA := addAgent(t, workspaceA, versionA, 1)
|
||||
agentB := addAgent(t, workspaceB, versionB, 1)
|
||||
|
||||
gotA, err := db.GetWorkspaceByAgentID(ctx, agentA.ID)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, workspaceA.ID, gotA.ID)
|
||||
gotB, err := db.GetWorkspaceByAgentID(ctx, agentB.ID)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, workspaceB.ID, gotB.ID)
|
||||
|
||||
appID := uuid.New()
|
||||
const originalSlug = "code-server"
|
||||
|
||||
// Initial insert under workspace A's agent succeeds (no conflict).
|
||||
app, err := upsertApp(appID, agentA.ID, originalSlug)
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, appID, app.ID)
|
||||
require.Equal(t, agentA.ID, app.AgentID)
|
||||
require.Equal(t, originalSlug, app.Slug)
|
||||
|
||||
// Upserting the same app id onto workspace B's agent is rejected because the
|
||||
// existing row and the incoming agent resolve to different workspaces. The
|
||||
// guard updates zero rows, so the :one query returns sql.ErrNoRows.
|
||||
_, err = upsertApp(appID, agentB.ID, "hijacked")
|
||||
require.ErrorIs(t, err, sql.ErrNoRows)
|
||||
|
||||
// The app remains bound to workspace A's agent, unchanged.
|
||||
appsA, err := db.GetWorkspaceAppsByAgentID(ctx, agentA.ID)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, appsA, 1)
|
||||
require.Equal(t, appID, appsA[0].ID)
|
||||
require.Equal(t, agentA.ID, appsA[0].AgentID)
|
||||
require.Equal(t, originalSlug, appsA[0].Slug)
|
||||
|
||||
// Workspace B's agent has no app.
|
||||
appsB, err := db.GetWorkspaceAppsByAgentID(ctx, agentB.ID)
|
||||
require.NoError(t, err)
|
||||
require.Empty(t, appsB)
|
||||
|
||||
// A legitimate rebuild of workspace A produces a new agent (agent IDs are
|
||||
// regenerated every build). Rebinding the persistent app to it succeeds
|
||||
// because both agents resolve to workspace A.
|
||||
agentA2 := addAgent(t, workspaceA, versionA, 2)
|
||||
app, err = upsertApp(appID, agentA2.ID, "code-server-v2")
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, agentA2.ID, app.AgentID)
|
||||
require.Equal(t, "code-server-v2", app.Slug)
|
||||
|
||||
appsA2, err := db.GetWorkspaceAppsByAgentID(ctx, agentA2.ID)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, appsA2, 1)
|
||||
require.Equal(t, appID, appsA2[0].ID)
|
||||
|
||||
// Set up a template-import agent. It is intentionally not associated with
|
||||
// a workspace build, so it resolves to no workspace.
|
||||
importJob := dbgen.ProvisionerJob(t, db, nil, database.ProvisionerJob{
|
||||
Type: database.ProvisionerJobTypeTemplateVersionImport,
|
||||
OrganizationID: org.ID,
|
||||
})
|
||||
importResource := dbgen.WorkspaceResource(t, db, database.WorkspaceResource{
|
||||
JobID: importJob.ID,
|
||||
})
|
||||
importAgent := dbgen.WorkspaceAgent(t, db, database.WorkspaceAgent{
|
||||
ResourceID: importResource.ID,
|
||||
})
|
||||
_, err = db.GetWorkspaceByAgentID(ctx, importAgent.ID)
|
||||
require.ErrorIs(t, err, sql.ErrNoRows, "import agent must not resolve to a workspace")
|
||||
|
||||
// An app that already belongs to a workspace cannot be rebound to a
|
||||
// template-import agent. Otherwise a second update could move it from
|
||||
// the import agent to a different workspace.
|
||||
_, err = upsertApp(appID, importAgent.ID, "hijacked-by-import")
|
||||
require.ErrorIs(t, err, sql.ErrNoRows)
|
||||
|
||||
appsA2, err = db.GetWorkspaceAppsByAgentID(ctx, agentA2.ID)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, appsA2, 1)
|
||||
require.Equal(t, appID, appsA2[0].ID)
|
||||
require.Equal(t, agentA2.ID, appsA2[0].AgentID)
|
||||
require.Equal(t, "code-server-v2", appsA2[0].Slug)
|
||||
|
||||
appsImport, err := db.GetWorkspaceAppsByAgentID(ctx, importAgent.ID)
|
||||
require.NoError(t, err)
|
||||
require.Empty(t, appsImport)
|
||||
|
||||
_, err = upsertApp(appID, agentB.ID, "hijacked-after-import")
|
||||
require.ErrorIs(t, err, sql.ErrNoRows)
|
||||
|
||||
unownedAppID := uuid.New()
|
||||
_, err = upsertApp(unownedAppID, importAgent.ID, "import-app")
|
||||
require.NoError(t, err)
|
||||
|
||||
// An app whose existing agent belongs to a template-import job resolves to
|
||||
// no workspace, so rebinding it is permitted. It is not a cross-tenant
|
||||
// victim.
|
||||
rebound, err := upsertApp(unownedAppID, agentA.ID, "import-app")
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, agentA.ID, rebound.AgentID)
|
||||
}
|
||||
|
||||
func TestGetWorkspaceAgentsByParentID(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
|
||||
Generated
+36
@@ -33272,6 +33272,42 @@ ON CONFLICT (id) DO UPDATE SET
|
||||
agent_id = EXCLUDED.agent_id,
|
||||
slug = EXCLUDED.slug,
|
||||
tooltip = EXCLUDED.tooltip
|
||||
WHERE
|
||||
-- Prevent cross-tenant/cross-workspace agent rebinding (SEC-91).
|
||||
-- App IDs persist across builds of the same workspace, but agent IDs are
|
||||
-- regenerated every build, so compare by the workspace that owns the agent
|
||||
-- rather than by agent_id. Permit unowned apps to be claimed and permit
|
||||
-- same-workspace rebuilds. If an existing app belongs to a workspace, block
|
||||
-- moves to both different workspaces and template import or dry-run agents
|
||||
-- that resolve to no workspace. The conflicting row is then left untouched,
|
||||
-- and the :one query returns no row, which the caller treats as a
|
||||
-- rejection.
|
||||
NOT EXISTS (
|
||||
SELECT 1
|
||||
FROM workspace_agents AS existing_agent
|
||||
INNER JOIN workspace_resources AS existing_resource
|
||||
ON existing_agent.resource_id = existing_resource.id
|
||||
INNER JOIN workspace_builds AS existing_build
|
||||
ON existing_resource.job_id = existing_build.job_id
|
||||
WHERE existing_agent.id = workspace_apps.agent_id
|
||||
)
|
||||
OR EXISTS (
|
||||
SELECT 1
|
||||
FROM workspace_agents AS existing_agent
|
||||
INNER JOIN workspace_resources AS existing_resource
|
||||
ON existing_agent.resource_id = existing_resource.id
|
||||
INNER JOIN workspace_builds AS existing_build
|
||||
ON existing_resource.job_id = existing_build.job_id
|
||||
INNER JOIN workspace_agents AS incoming_agent
|
||||
ON incoming_agent.id = EXCLUDED.agent_id
|
||||
INNER JOIN workspace_resources AS incoming_resource
|
||||
ON incoming_agent.resource_id = incoming_resource.id
|
||||
INNER JOIN workspace_builds AS incoming_build
|
||||
ON incoming_resource.job_id = incoming_build.job_id
|
||||
WHERE
|
||||
existing_agent.id = workspace_apps.agent_id
|
||||
AND existing_build.workspace_id = incoming_build.workspace_id
|
||||
)
|
||||
RETURNING id, created_at, agent_id, display_name, icon, command, url, healthcheck_url, healthcheck_interval, healthcheck_threshold, health, subdomain, sharing_level, slug, external, display_order, hidden, open_in, display_group, tooltip
|
||||
`
|
||||
|
||||
|
||||
@@ -55,6 +55,42 @@ ON CONFLICT (id) DO UPDATE SET
|
||||
agent_id = EXCLUDED.agent_id,
|
||||
slug = EXCLUDED.slug,
|
||||
tooltip = EXCLUDED.tooltip
|
||||
WHERE
|
||||
-- Prevent cross-tenant/cross-workspace agent rebinding (SEC-91).
|
||||
-- App IDs persist across builds of the same workspace, but agent IDs are
|
||||
-- regenerated every build, so compare by the workspace that owns the agent
|
||||
-- rather than by agent_id. Permit unowned apps to be claimed and permit
|
||||
-- same-workspace rebuilds. If an existing app belongs to a workspace, block
|
||||
-- moves to both different workspaces and template import or dry-run agents
|
||||
-- that resolve to no workspace. The conflicting row is then left untouched,
|
||||
-- and the :one query returns no row, which the caller treats as a
|
||||
-- rejection.
|
||||
NOT EXISTS (
|
||||
SELECT 1
|
||||
FROM workspace_agents AS existing_agent
|
||||
INNER JOIN workspace_resources AS existing_resource
|
||||
ON existing_agent.resource_id = existing_resource.id
|
||||
INNER JOIN workspace_builds AS existing_build
|
||||
ON existing_resource.job_id = existing_build.job_id
|
||||
WHERE existing_agent.id = workspace_apps.agent_id
|
||||
)
|
||||
OR EXISTS (
|
||||
SELECT 1
|
||||
FROM workspace_agents AS existing_agent
|
||||
INNER JOIN workspace_resources AS existing_resource
|
||||
ON existing_agent.resource_id = existing_resource.id
|
||||
INNER JOIN workspace_builds AS existing_build
|
||||
ON existing_resource.job_id = existing_build.job_id
|
||||
INNER JOIN workspace_agents AS incoming_agent
|
||||
ON incoming_agent.id = EXCLUDED.agent_id
|
||||
INNER JOIN workspace_resources AS incoming_resource
|
||||
ON incoming_agent.resource_id = incoming_resource.id
|
||||
INNER JOIN workspace_builds AS incoming_build
|
||||
ON incoming_resource.job_id = incoming_build.job_id
|
||||
WHERE
|
||||
existing_agent.id = workspace_apps.agent_id
|
||||
AND existing_build.workspace_id = incoming_build.workspace_id
|
||||
)
|
||||
RETURNING *;
|
||||
|
||||
-- name: UpdateWorkspaceAppHealthByID :exec
|
||||
|
||||
@@ -1704,6 +1704,7 @@ func (s *server) completeTemplateImportJob(ctx context.Context, job database.Pro
|
||||
slog.F("transition", transition))
|
||||
|
||||
if err := InsertWorkspaceResource(ctx, db, jobID, transition, resource, telemetrySnapshot); err != nil {
|
||||
s.warnWorkspaceAppRebindRejected(ctx, jobID, err)
|
||||
return xerrors.Errorf("insert resource: %w", err)
|
||||
}
|
||||
}
|
||||
@@ -2122,6 +2123,7 @@ func (s *server) completeWorkspaceBuildJob(ctx context.Context, job database.Pro
|
||||
InsertWorkspaceResourceWithAgentIDsFromProto(),
|
||||
)
|
||||
if err != nil {
|
||||
s.warnWorkspaceAppRebindRejected(ctx, jobID, err)
|
||||
return xerrors.Errorf("insert provisioner job: %w", err)
|
||||
}
|
||||
}
|
||||
@@ -2590,6 +2592,7 @@ func (s *server) completeTemplateDryRunJob(ctx context.Context, job database.Pro
|
||||
|
||||
err := InsertWorkspaceResource(ctx, db, jobID, database.WorkspaceTransitionStart, resource, telemetrySnapshot)
|
||||
if err != nil {
|
||||
s.warnWorkspaceAppRebindRejected(ctx, jobID, err)
|
||||
return xerrors.Errorf("insert resource: %w", err)
|
||||
}
|
||||
}
|
||||
@@ -3614,6 +3617,32 @@ func insertAgentScriptsAndLogSources(ctx context.Context, db database.Store, age
|
||||
return nil
|
||||
}
|
||||
|
||||
type workspaceAppRebindError struct {
|
||||
slug string
|
||||
appID uuid.UUID
|
||||
agentID uuid.UUID
|
||||
}
|
||||
|
||||
func (e *workspaceAppRebindError) Error() string {
|
||||
return fmt.Sprintf("workspace app slug %q with ID %q is already bound to a workspace-owned agent and cannot be rebound to an agent in another workspace or to an agent without a workspace; refusing to rebind to agent ID %q", e.slug, e.appID, e.agentID)
|
||||
}
|
||||
|
||||
func (s *server) warnWorkspaceAppRebindRejected(ctx context.Context, jobID uuid.UUID, err error) {
|
||||
slog.Helper()
|
||||
|
||||
var rebindErr *workspaceAppRebindError
|
||||
if !errors.As(err, &rebindErr) {
|
||||
return
|
||||
}
|
||||
|
||||
s.Logger.Warn(ctx, "workspace app rebind rejected by SQL guard",
|
||||
slog.F("job_id", jobID.String()),
|
||||
slog.F("app_id", rebindErr.appID.String()),
|
||||
slog.F("agent_id", rebindErr.agentID.String()),
|
||||
slog.F("app_slug", rebindErr.slug),
|
||||
)
|
||||
}
|
||||
|
||||
func insertAgentApp(ctx context.Context, db database.Store, agentID uuid.UUID, app *sdkproto.App, appSlugs map[string]struct{}, snapshot *telemetry.Snapshot) error {
|
||||
// Similar logic is duplicated in terraform/resources.go.
|
||||
slug := app.Slug
|
||||
@@ -3702,6 +3731,17 @@ func insertAgentApp(ctx context.Context, db database.Store, agentID uuid.UUID, a
|
||||
Tooltip: app.Tooltip,
|
||||
})
|
||||
if err != nil {
|
||||
if errors.Is(err, sql.ErrNoRows) {
|
||||
// The upsert's ON CONFLICT guard refused to rebind an app
|
||||
// owned by a workspace to an agent outside that workspace,
|
||||
// including agents from import or dry-run jobs that resolve
|
||||
// to no workspace (SEC-91).
|
||||
return &workspaceAppRebindError{
|
||||
slug: slug,
|
||||
appID: id,
|
||||
agentID: agentID,
|
||||
}
|
||||
}
|
||||
return xerrors.Errorf("upsert app: %w", err)
|
||||
}
|
||||
|
||||
|
||||
@@ -26,6 +26,7 @@ import (
|
||||
"google.golang.org/protobuf/types/known/timestamppb"
|
||||
"storj.io/drpc"
|
||||
|
||||
"cdr.dev/slog/v3"
|
||||
"cdr.dev/slog/v3/sloggers/slogtest"
|
||||
"github.com/coder/coder/v2/buildinfo"
|
||||
"github.com/coder/coder/v2/coderd"
|
||||
@@ -2348,6 +2349,109 @@ func TestCompleteJob(t *testing.T) {
|
||||
})
|
||||
}
|
||||
})
|
||||
t.Run("WorkspaceBuild_CrossWorkspaceAppRebindRejected", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
logSink := &recordingSlogSink{}
|
||||
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).AppendSinks(logSink)
|
||||
srv, db, _, pd := setup(t, false, &overrides{provisionerdLogger: &logger})
|
||||
|
||||
// Given: a victim workspace whose agent owns an app with a known UUID.
|
||||
victimAppID, victimAgentID, victimSlug := setupWorkspaceAppRebindVictim(
|
||||
t, db, pd.OrganizationID,
|
||||
)
|
||||
|
||||
// Given: an attacker workspace with a running build job acquired by the
|
||||
// provisioner daemon.
|
||||
attackerUser := dbgen.User(t, db, database.User{})
|
||||
attackerTemplate := dbgen.Template(t, db, database.Template{
|
||||
CreatedBy: attackerUser.ID,
|
||||
OrganizationID: pd.OrganizationID,
|
||||
})
|
||||
attackerVersion := dbgen.TemplateVersion(t, db, database.TemplateVersion{
|
||||
CreatedBy: attackerUser.ID,
|
||||
OrganizationID: pd.OrganizationID,
|
||||
TemplateID: uuid.NullUUID{UUID: attackerTemplate.ID, Valid: true},
|
||||
JobID: uuid.New(),
|
||||
})
|
||||
attackerWorkspace := dbgen.Workspace(t, db, database.WorkspaceTable{
|
||||
TemplateID: attackerTemplate.ID,
|
||||
OwnerID: attackerUser.ID,
|
||||
OrganizationID: pd.OrganizationID,
|
||||
})
|
||||
attackerBuildID := uuid.New()
|
||||
attackerJob := dbgen.ProvisionerJob(t, db, nil, database.ProvisionerJob{
|
||||
InitiatorID: attackerUser.ID,
|
||||
Type: database.ProvisionerJobTypeWorkspaceBuild,
|
||||
Input: must(json.Marshal(provisionerdserver.WorkspaceProvisionJob{
|
||||
WorkspaceBuildID: attackerBuildID,
|
||||
})),
|
||||
OrganizationID: pd.OrganizationID,
|
||||
})
|
||||
dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{
|
||||
ID: attackerBuildID,
|
||||
JobID: attackerJob.ID,
|
||||
WorkspaceID: attackerWorkspace.ID,
|
||||
TemplateVersionID: attackerVersion.ID,
|
||||
InitiatorID: attackerUser.ID,
|
||||
Transition: database.WorkspaceTransitionStart,
|
||||
Reason: database.BuildReasonInitiator,
|
||||
})
|
||||
_, err := db.AcquireProvisionerJob(ctx, database.AcquireProvisionerJobParams{
|
||||
OrganizationID: pd.OrganizationID,
|
||||
WorkerID: uuid.NullUUID{UUID: pd.ID, Valid: true},
|
||||
Types: []database.ProvisionerType{database.ProvisionerTypeEcho},
|
||||
StartedAt: sql.NullTime{Time: time.Now(), Valid: true},
|
||||
ProvisionerTags: must(json.Marshal(attackerJob.Tags)),
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
// When: the attacker's build completes with an app that reuses the
|
||||
// victim's app UUID but points at the attacker's (new) agent.
|
||||
attackerAgent := &sdkproto.Agent{
|
||||
Id: uuid.NewString(),
|
||||
Name: "dev",
|
||||
Auth: &sdkproto.Agent_Token{Token: uuid.NewString()},
|
||||
Apps: []*sdkproto.App{{
|
||||
Id: victimAppID.String(),
|
||||
Slug: "attacker-app",
|
||||
}},
|
||||
}
|
||||
_, err = srv.CompleteJob(ctx, &proto.CompletedJob{
|
||||
JobId: attackerJob.ID.String(),
|
||||
Type: &proto.CompletedJob_WorkspaceBuild_{
|
||||
WorkspaceBuild: &proto.CompletedJob_WorkspaceBuild{
|
||||
State: []byte{},
|
||||
Resources: []*sdkproto.Resource{{
|
||||
Name: "example",
|
||||
Type: "aws_instance",
|
||||
Agents: []*sdkproto.Agent{attackerAgent},
|
||||
}},
|
||||
},
|
||||
},
|
||||
})
|
||||
// Then: the build is rejected with the cross-tenant rebind error.
|
||||
require.Error(t, err)
|
||||
require.ErrorContains(t, err, "already bound to a workspace-owned agent")
|
||||
assertWorkspaceAppRebindWarning(
|
||||
t,
|
||||
logSink,
|
||||
workspaceAppRebindWarning{
|
||||
jobID: attackerJob.ID,
|
||||
appID: victimAppID,
|
||||
slug: "attacker-app",
|
||||
agentID: attackerAgent.Id,
|
||||
},
|
||||
)
|
||||
|
||||
// And: the victim's app remains bound to the victim agent, unchanged.
|
||||
victimApps, err := db.GetWorkspaceAppsByAgentID(ctx, victimAgentID)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, victimApps, 1)
|
||||
require.Equal(t, victimAppID, victimApps[0].ID)
|
||||
require.Equal(t, victimAgentID, victimApps[0].AgentID)
|
||||
require.Equal(t, victimSlug, victimApps[0].Slug)
|
||||
})
|
||||
t.Run("TemplateDryRun", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
srv, db, _, pd := setup(t, false, &overrides{})
|
||||
@@ -2398,6 +2502,161 @@ func TestCompleteJob(t *testing.T) {
|
||||
require.NoError(t, err)
|
||||
})
|
||||
|
||||
t.Run("TemplateDryRun_CrossWorkspaceAppRebindRejected", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
logSink := &recordingSlogSink{}
|
||||
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).AppendSinks(logSink)
|
||||
srv, db, _, pd := setup(t, false, &overrides{provisionerdLogger: &logger})
|
||||
|
||||
victimAppID, victimAgentID, victimSlug := setupWorkspaceAppRebindVictim(
|
||||
t, db, pd.OrganizationID,
|
||||
)
|
||||
|
||||
user := dbgen.User(t, db, database.User{})
|
||||
version := dbgen.TemplateVersion(t, db, database.TemplateVersion{
|
||||
CreatedBy: user.ID,
|
||||
OrganizationID: pd.OrganizationID,
|
||||
JobID: uuid.New(),
|
||||
})
|
||||
job, err := db.InsertProvisionerJob(ctx, database.InsertProvisionerJobParams{
|
||||
ID: version.JobID,
|
||||
Provisioner: database.ProvisionerTypeEcho,
|
||||
Type: database.ProvisionerJobTypeTemplateVersionDryRun,
|
||||
StorageMethod: database.ProvisionerStorageMethodFile,
|
||||
Input: must(json.Marshal(provisionerdserver.TemplateVersionDryRunJob{
|
||||
TemplateVersionID: version.ID,
|
||||
})),
|
||||
OrganizationID: pd.OrganizationID,
|
||||
Tags: pd.Tags,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
_, err = db.AcquireProvisionerJob(ctx, database.AcquireProvisionerJobParams{
|
||||
WorkerID: uuid.NullUUID{UUID: pd.ID, Valid: true},
|
||||
Types: []database.ProvisionerType{database.ProvisionerTypeEcho},
|
||||
StartedAt: sql.NullTime{Time: dbtime.Now(), Valid: true},
|
||||
OrganizationID: pd.OrganizationID,
|
||||
ProvisionerTags: must(json.Marshal(job.Tags)),
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
dryRunAgent := &sdkproto.Agent{
|
||||
Name: "dev",
|
||||
Auth: &sdkproto.Agent_Token{Token: uuid.NewString()},
|
||||
Apps: []*sdkproto.App{{
|
||||
Id: victimAppID.String(),
|
||||
Slug: "dry-run-app",
|
||||
}},
|
||||
}
|
||||
_, err = srv.CompleteJob(ctx, &proto.CompletedJob{
|
||||
JobId: job.ID.String(),
|
||||
Type: &proto.CompletedJob_TemplateDryRun_{
|
||||
TemplateDryRun: &proto.CompletedJob_TemplateDryRun{
|
||||
Resources: []*sdkproto.Resource{{
|
||||
Name: "something",
|
||||
Type: "aws_instance",
|
||||
Agents: []*sdkproto.Agent{dryRunAgent},
|
||||
}},
|
||||
},
|
||||
},
|
||||
})
|
||||
require.Error(t, err)
|
||||
require.ErrorContains(t, err, "already bound to a workspace-owned agent")
|
||||
assertWorkspaceAppRebindWarning(
|
||||
t,
|
||||
logSink,
|
||||
workspaceAppRebindWarning{
|
||||
jobID: job.ID,
|
||||
appID: victimAppID,
|
||||
slug: "dry-run-app",
|
||||
},
|
||||
)
|
||||
|
||||
victimApps, err := db.GetWorkspaceAppsByAgentID(ctx, victimAgentID)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, victimApps, 1)
|
||||
require.Equal(t, victimAppID, victimApps[0].ID)
|
||||
require.Equal(t, victimAgentID, victimApps[0].AgentID)
|
||||
require.Equal(t, victimSlug, victimApps[0].Slug)
|
||||
})
|
||||
|
||||
t.Run("TemplateImport_CrossWorkspaceAppRebindRejected", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
logSink := &recordingSlogSink{}
|
||||
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).AppendSinks(logSink)
|
||||
srv, db, _, pd := setup(t, false, &overrides{provisionerdLogger: &logger})
|
||||
|
||||
victimAppID, victimAgentID, victimSlug := setupWorkspaceAppRebindVictim(
|
||||
t, db, pd.OrganizationID,
|
||||
)
|
||||
|
||||
user := dbgen.User(t, db, database.User{})
|
||||
version := dbgen.TemplateVersion(t, db, database.TemplateVersion{
|
||||
CreatedBy: user.ID,
|
||||
OrganizationID: pd.OrganizationID,
|
||||
JobID: uuid.New(),
|
||||
})
|
||||
job, err := db.InsertProvisionerJob(ctx, database.InsertProvisionerJobParams{
|
||||
ID: version.JobID,
|
||||
Provisioner: database.ProvisionerTypeEcho,
|
||||
Type: database.ProvisionerJobTypeTemplateVersionImport,
|
||||
StorageMethod: database.ProvisionerStorageMethodFile,
|
||||
Input: must(json.Marshal(provisionerdserver.TemplateVersionImportJob{
|
||||
TemplateVersionID: version.ID,
|
||||
})),
|
||||
OrganizationID: pd.OrganizationID,
|
||||
Tags: pd.Tags,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
_, err = db.AcquireProvisionerJob(ctx, database.AcquireProvisionerJobParams{
|
||||
WorkerID: uuid.NullUUID{UUID: pd.ID, Valid: true},
|
||||
Types: []database.ProvisionerType{database.ProvisionerTypeEcho},
|
||||
StartedAt: sql.NullTime{Time: dbtime.Now(), Valid: true},
|
||||
OrganizationID: pd.OrganizationID,
|
||||
ProvisionerTags: must(json.Marshal(job.Tags)),
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
importAgent := &sdkproto.Agent{
|
||||
Name: "dev",
|
||||
Auth: &sdkproto.Agent_Token{Token: uuid.NewString()},
|
||||
Apps: []*sdkproto.App{{
|
||||
Id: victimAppID.String(),
|
||||
Slug: "import-app",
|
||||
}},
|
||||
}
|
||||
_, err = srv.CompleteJob(ctx, &proto.CompletedJob{
|
||||
JobId: job.ID.String(),
|
||||
Type: &proto.CompletedJob_TemplateImport_{
|
||||
TemplateImport: &proto.CompletedJob_TemplateImport{
|
||||
StartResources: []*sdkproto.Resource{{
|
||||
Name: "something",
|
||||
Type: "aws_instance",
|
||||
Agents: []*sdkproto.Agent{importAgent},
|
||||
}},
|
||||
Plan: []byte("{}"),
|
||||
},
|
||||
},
|
||||
})
|
||||
require.Error(t, err)
|
||||
require.ErrorContains(t, err, "already bound to a workspace-owned agent")
|
||||
assertWorkspaceAppRebindWarning(
|
||||
t,
|
||||
logSink,
|
||||
workspaceAppRebindWarning{
|
||||
jobID: job.ID,
|
||||
appID: victimAppID,
|
||||
slug: "import-app",
|
||||
},
|
||||
)
|
||||
|
||||
victimApps, err := db.GetWorkspaceAppsByAgentID(ctx, victimAgentID)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, victimApps, 1)
|
||||
require.Equal(t, victimAppID, victimApps[0].ID)
|
||||
require.Equal(t, victimAgentID, victimApps[0].AgentID)
|
||||
require.Equal(t, victimSlug, victimApps[0].Slug)
|
||||
})
|
||||
|
||||
t.Run("Modules", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
@@ -3383,6 +3642,59 @@ func TestCompleteJob(t *testing.T) {
|
||||
})
|
||||
}
|
||||
|
||||
func setupWorkspaceAppRebindVictim(
|
||||
t *testing.T,
|
||||
db database.Store,
|
||||
organizationID uuid.UUID,
|
||||
) (appID uuid.UUID, agentID uuid.UUID, slug string) {
|
||||
t.Helper()
|
||||
|
||||
victimUser := dbgen.User(t, db, database.User{})
|
||||
victimTemplate := dbgen.Template(t, db, database.Template{
|
||||
CreatedBy: victimUser.ID,
|
||||
OrganizationID: organizationID,
|
||||
})
|
||||
victimVersion := dbgen.TemplateVersion(t, db, database.TemplateVersion{
|
||||
CreatedBy: victimUser.ID,
|
||||
OrganizationID: organizationID,
|
||||
TemplateID: uuid.NullUUID{UUID: victimTemplate.ID, Valid: true},
|
||||
})
|
||||
victimWorkspace := dbgen.Workspace(t, db, database.WorkspaceTable{
|
||||
TemplateID: victimTemplate.ID,
|
||||
OwnerID: victimUser.ID,
|
||||
OrganizationID: organizationID,
|
||||
})
|
||||
victimJob := dbgen.ProvisionerJob(t, db, nil, database.ProvisionerJob{
|
||||
Type: database.ProvisionerJobTypeWorkspaceBuild,
|
||||
OrganizationID: organizationID,
|
||||
StartedAt: sql.NullTime{Time: time.Now().Add(-time.Hour), Valid: true},
|
||||
CompletedAt: sql.NullTime{Time: time.Now().Add(-time.Hour), Valid: true},
|
||||
})
|
||||
dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{
|
||||
JobID: victimJob.ID,
|
||||
WorkspaceID: victimWorkspace.ID,
|
||||
TemplateVersionID: victimVersion.ID,
|
||||
InitiatorID: victimUser.ID,
|
||||
Transition: database.WorkspaceTransitionStart,
|
||||
Reason: database.BuildReasonInitiator,
|
||||
})
|
||||
victimResource := dbgen.WorkspaceResource(t, db, database.WorkspaceResource{
|
||||
JobID: victimJob.ID,
|
||||
})
|
||||
victimAgent := dbgen.WorkspaceAgent(t, db, database.WorkspaceAgent{
|
||||
ResourceID: victimResource.ID,
|
||||
})
|
||||
victimAppID := uuid.New()
|
||||
const victimSlug = "code-server"
|
||||
dbgen.WorkspaceApp(t, db, database.WorkspaceApp{
|
||||
ID: victimAppID,
|
||||
AgentID: victimAgent.ID,
|
||||
Slug: victimSlug,
|
||||
})
|
||||
|
||||
return victimAppID, victimAgent.ID, victimSlug
|
||||
}
|
||||
|
||||
type mockPrebuildsOrchestrator struct {
|
||||
agplprebuilds.ReconciliationOrchestrator
|
||||
|
||||
@@ -4781,6 +5093,70 @@ func TestServer_ExpirePrebuildsSessionToken(t *testing.T) {
|
||||
require.ErrorIs(t, err, sql.ErrNoRows, "api key for prebuilds user should be deleted")
|
||||
}
|
||||
|
||||
type workspaceAppRebindWarning struct {
|
||||
jobID uuid.UUID
|
||||
appID uuid.UUID
|
||||
slug string
|
||||
agentID string
|
||||
}
|
||||
|
||||
func assertWorkspaceAppRebindWarning(t *testing.T, logSink *recordingSlogSink, want workspaceAppRebindWarning) {
|
||||
t.Helper()
|
||||
|
||||
for _, entry := range logSink.Entries() {
|
||||
if entry.Message != "workspace app rebind rejected by SQL guard" {
|
||||
continue
|
||||
}
|
||||
|
||||
require.Equal(t, slog.LevelWarn, entry.Level)
|
||||
require.Contains(t, entry.File, "coderd/provisionerdserver/provisionerdserver.go")
|
||||
require.NotContains(t, entry.Func, "warnWorkspaceAppRebindRejected")
|
||||
fields := slogFieldsByName(entry.Fields)
|
||||
require.Equal(t, want.jobID.String(), fields["job_id"])
|
||||
require.Equal(t, want.appID.String(), fields["app_id"])
|
||||
require.Equal(t, want.slug, fields["app_slug"])
|
||||
agentID, ok := fields["agent_id"].(string)
|
||||
require.True(t, ok)
|
||||
require.NotEqual(t, uuid.Nil.String(), agentID)
|
||||
if want.agentID != "" {
|
||||
require.Equal(t, want.agentID, agentID)
|
||||
} else {
|
||||
_, err := uuid.Parse(agentID)
|
||||
require.NoError(t, err)
|
||||
}
|
||||
return
|
||||
}
|
||||
|
||||
require.Fail(t, "expected workspace app rebind warning")
|
||||
}
|
||||
|
||||
type recordingSlogSink struct {
|
||||
mu sync.Mutex
|
||||
entries []slog.SinkEntry
|
||||
}
|
||||
|
||||
func (s *recordingSlogSink) LogEntry(_ context.Context, entry slog.SinkEntry) {
|
||||
s.mu.Lock()
|
||||
defer s.mu.Unlock()
|
||||
s.entries = append(s.entries, entry)
|
||||
}
|
||||
|
||||
func (*recordingSlogSink) Sync() {}
|
||||
|
||||
func (s *recordingSlogSink) Entries() []slog.SinkEntry {
|
||||
s.mu.Lock()
|
||||
defer s.mu.Unlock()
|
||||
return append([]slog.SinkEntry(nil), s.entries...)
|
||||
}
|
||||
|
||||
func slogFieldsByName(fields []slog.Field) map[string]any {
|
||||
byName := make(map[string]any, len(fields))
|
||||
for _, field := range fields {
|
||||
byName[field.Name] = field.Value
|
||||
}
|
||||
return byName
|
||||
}
|
||||
|
||||
type overrides struct {
|
||||
ctx context.Context
|
||||
deploymentValues *codersdk.DeploymentValues
|
||||
@@ -4795,6 +5171,7 @@ type overrides struct {
|
||||
auditor audit.Auditor
|
||||
notificationEnqueuer notifications.Enqueuer
|
||||
prebuildsOrchestrator agplprebuilds.ReconciliationOrchestrator
|
||||
provisionerdLogger *slog.Logger
|
||||
}
|
||||
|
||||
func setup(t *testing.T, ignoreLogErrors bool, ov *overrides) (proto.DRPCProvisionerDaemonServer, database.Store, pubsub.Pubsub, database.ProvisionerDaemon) {
|
||||
@@ -4871,6 +5248,10 @@ func setup(t *testing.T, ignoreLogErrors bool, ov *overrides) (proto.DRPCProvisi
|
||||
} else {
|
||||
notifEnq = notifications.NewNoopEnqueuer()
|
||||
}
|
||||
provisionerdLogger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: ignoreLogErrors})
|
||||
if ov.provisionerdLogger != nil {
|
||||
provisionerdLogger = *ov.provisionerdLogger
|
||||
}
|
||||
|
||||
daemon, err := db.UpsertProvisionerDaemon(ov.ctx, database.UpsertProvisionerDaemonParams{
|
||||
Name: "test",
|
||||
@@ -4902,7 +5283,7 @@ func setup(t *testing.T, ignoreLogErrors bool, ov *overrides) (proto.DRPCProvisi
|
||||
&url.URL{},
|
||||
daemon.ID,
|
||||
defOrg.ID,
|
||||
slogtest.Make(t, &slogtest.Options{IgnoreErrors: ignoreLogErrors}),
|
||||
provisionerdLogger,
|
||||
[]database.ProvisionerType{database.ProvisionerTypeEcho},
|
||||
provisionerdserver.Tags(daemon.Tags),
|
||||
serverDB,
|
||||
|
||||
Reference in New Issue
Block a user