From f3c7c23623f36ebecd27efff7993bc6b57d16523 Mon Sep 17 00:00:00 2001 From: dylanhuff-at-coder Date: Thu, 11 Jun 2026 15:02:41 -0400 Subject: [PATCH] fix(coderd): prevent cross-tenant workspace app rebinding (#26103) --- coderd/agentapi/subagent.go | 9 +- coderd/agentapi/subagent_test.go | 77 ++++ coderd/database/querier_test.go | 172 ++++++++ coderd/database/queries.sql.go | 36 ++ coderd/database/queries/workspaceapps.sql | 36 ++ .../provisionerdserver/provisionerdserver.go | 40 ++ .../provisionerdserver_test.go | 383 +++++++++++++++++- 7 files changed, 751 insertions(+), 2 deletions(-) diff --git a/coderd/agentapi/subagent.go b/coderd/agentapi/subagent.go index bfb951544c..ec509bc98e 100644 --- a/coderd/agentapi/subagent.go +++ b/coderd/agentapi/subagent.go @@ -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) } diff --git a/coderd/agentapi/subagent_test.go b/coderd/agentapi/subagent_test.go index a7217cc513..81c98091b1 100644 --- a/coderd/agentapi/subagent_test.go +++ b/coderd/agentapi/subagent_test.go @@ -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() diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index b7c3490c98..c48088314b 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -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() diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 10c6fbfb2f..1b30d96415 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -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 ` diff --git a/coderd/database/queries/workspaceapps.sql b/coderd/database/queries/workspaceapps.sql index 5f826d2985..d297241a68 100644 --- a/coderd/database/queries/workspaceapps.sql +++ b/coderd/database/queries/workspaceapps.sql @@ -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 diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index 1c66333aef..d233cb41dd 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -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) } diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go index e6ad6f74eb..6c1bec1570 100644 --- a/coderd/provisionerdserver/provisionerdserver_test.go +++ b/coderd/provisionerdserver/provisionerdserver_test.go @@ -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,