diff --git a/coderd/apikey.go b/coderd/apikey.go index 895be440ef930..12646d627a212 100644 --- a/coderd/apikey.go +++ b/coderd/apikey.go @@ -12,6 +12,8 @@ import ( "github.com/moby/moby/pkg/namesgenerator" "golang.org/x/xerrors" + "cdr.dev/slog" + "github.com/coder/coder/v2/coderd/apikey" "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" @@ -56,6 +58,14 @@ func (api *API) postToken(rw http.ResponseWriter, r *http.Request) { return } + // TODO(Cian): System users technically just have the 'member' role + // and we don't want to disallow all members from creating API keys. + if user.IsSystem { + api.Logger.Warn(ctx, "disallowed creating api key for system user", slog.F("user_id", user.ID)) + httpapi.Forbidden(rw) + return + } + scope := database.APIKeyScopeAll if scope != "" { scope = database.APIKeyScope(createToken.Scope) @@ -124,6 +134,14 @@ func (api *API) postAPIKey(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() user := httpmw.UserParam(r) + // TODO(Cian): System users technically just have the 'member' role + // and we don't want to disallow all members from creating API keys. + if user.IsSystem { + api.Logger.Warn(ctx, "disallowed creating api key for system user", slog.F("user_id", user.ID)) + httpapi.Forbidden(rw) + return + } + cookie, _, err := api.createAPIKey(ctx, apikey.CreateParams{ UserID: user.ID, DefaultLifetime: api.DeploymentValues.Sessions.DefaultTokenDuration.Value(), diff --git a/coderd/apikey_test.go b/coderd/apikey_test.go index dbf5a3520a6f0..1509aa2e2f402 100644 --- a/coderd/apikey_test.go +++ b/coderd/apikey_test.go @@ -13,8 +13,10 @@ import ( "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbgen" "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/coderd/database/dbtime" + "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/testutil" "github.com/coder/serpent" @@ -351,3 +353,34 @@ func TestAPIKey_SetDefault(t *testing.T) { require.NoError(t, err) require.EqualValues(t, dc.Sessions.DefaultTokenDuration.Value().Seconds(), apiKey1.LifetimeSeconds) } + +func TestAPIKey_PrebuildsNotAllowed(t *testing.T) { + t.Parallel() + + db, pubsub := dbtestutil.NewDB(t) + dc := coderdtest.DeploymentValues(t) + dc.Sessions.DefaultTokenDuration = serpent.Duration(time.Hour * 12) + client := coderdtest.New(t, &coderdtest.Options{ + Database: db, + Pubsub: pubsub, + DeploymentValues: dc, + }) + + ctx := testutil.Context(t, testutil.WaitLong) + + // Given: an existing api token for the prebuilds user + _, prebuildsToken := dbgen.APIKey(t, db, database.APIKey{ + UserID: database.PrebuildsSystemUserID, + }) + client.SetSessionToken(prebuildsToken) + + // When: the prebuilds user tries to create an API key + _, err := client.CreateAPIKey(ctx, database.PrebuildsSystemUserID.String()) + // Then: denied. + require.ErrorContains(t, err, httpapi.ResourceForbiddenResponse.Message) + + // When: the prebuilds user tries to create a token + _, err = client.CreateToken(ctx, database.PrebuildsSystemUserID.String(), codersdk.CreateTokenRequest{}) + // Then: also denied. + require.ErrorContains(t, err, httpapi.ResourceForbiddenResponse.Message) +} diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index a2c3b1d5705da..bacfca08bc163 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1652,6 +1652,13 @@ func (q *querier) EnqueueNotificationMessage(ctx context.Context, arg database.E return q.db.EnqueueNotificationMessage(ctx, arg) } +func (q *querier) ExpirePrebuildsAPIKeys(ctx context.Context, now time.Time) error { + if err := q.authorizeContext(ctx, policy.ActionDelete, rbac.ResourceApiKey); err != nil { + return err + } + return q.db.ExpirePrebuildsAPIKeys(ctx, now) +} + func (q *querier) FavoriteWorkspace(ctx context.Context, id uuid.UUID) error { fetch := func(ctx context.Context, id uuid.UUID) (database.Workspace, error) { return q.db.GetWorkspaceByID(ctx, id) @@ -3507,6 +3514,14 @@ func (q *querier) HasTemplateVersionsWithAITask(ctx context.Context) (bool, erro } func (q *querier) InsertAPIKey(ctx context.Context, arg database.InsertAPIKeyParams) (database.APIKey, error) { + // TODO(Cian): ideally this would be encoded in the policy, but system users are just members and we + // don't currently have a capability to conditionally deny creating resources by owner ID in a role. + // We also need to enrich rbac.Actor with IsSystem so that we can distinguish all system users. + // For now, there is only one system user (prebuilds). + if act, ok := ActorFromContext(ctx); ok && act.ID == database.PrebuildsSystemUserID.String() { + return database.APIKey{}, logNotAuthorizedError(ctx, q.log, NotAuthorizedError{Err: xerrors.Errorf("prebuild user may not create api keys")}) + } + return insert(q.log, q.auth, rbac.ResourceApiKey.WithOwner(arg.UserID.String()), q.db.InsertAPIKey)(ctx, arg) diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index e83b2bd4710c5..0c20bc4214ddd 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -14,24 +14,26 @@ import ( "github.com/google/uuid" "github.com/sqlc-dev/pqtype" "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" "golang.org/x/xerrors" "cdr.dev/slog" - - "github.com/coder/coder/v2/coderd/database/db2sdk" - "github.com/coder/coder/v2/coderd/database/dbmem" - "github.com/coder/coder/v2/coderd/notifications" - "github.com/coder/coder/v2/coderd/rbac/policy" - "github.com/coder/coder/v2/codersdk" + "cdr.dev/slog/sloggers/slogtest" "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/db2sdk" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbgen" + "github.com/coder/coder/v2/coderd/database/dbmem" + "github.com/coder/coder/v2/coderd/database/dbmock" "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/coderd/database/dbtime" + "github.com/coder/coder/v2/coderd/notifications" "github.com/coder/coder/v2/coderd/rbac" + "github.com/coder/coder/v2/coderd/rbac/policy" "github.com/coder/coder/v2/coderd/util/slice" + "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/provisionersdk" "github.com/coder/coder/v2/testutil" ) @@ -1573,6 +1575,9 @@ func (s *MethodTestSuite) TestUser() { UserID: u.ID, OrganizationID: uuid.New(), }).Asserts(u, policy.ActionRead).Returns(int64(0)) + s.Run("ExpirePrebuildsAPIKeys", s.Subtest(func(db database.Store, check *expects) { + check.Args(dbtime.Now()).Asserts(rbac.ResourceApiKey, policy.ActionDelete).Returns() + })) })) s.Run("GetQuotaConsumedForUser", s.Subtest(func(db database.Store, check *expects) { u := dbgen.User(s.T(), db, database.User{}) @@ -5637,3 +5642,18 @@ func (s *MethodTestSuite) TestAuthorizePrebuiltWorkspace() { }).Asserts(w, policy.ActionUpdate, w.AsPrebuild(), policy.ActionUpdate) })) } + +// Ensures that the prebuilds actor may never insert an api key. +func TestInsertAPIKey_AsPrebuildsUser(t *testing.T) { + t.Parallel() + prebuildsSubj := rbac.Subject{ + ID: database.PrebuildsSystemUserID.String(), + } + ctx := dbauthz.As(testutil.Context(t, testutil.WaitShort), prebuildsSubj) + mDB := dbmock.NewMockStore(gomock.NewController(t)) + log := slogtest.Make(t, nil) + mDB.EXPECT().Wrappers().Times(1).Return([]string{}) + dbz := dbauthz.New(mDB, nil, log, nil) + _, err := dbz.InsertAPIKey(ctx, database.InsertAPIKeyParams{}) + require.True(t, dbauthz.IsNotAuthorizedError(err)) +} diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go index fd842b73348c5..30976cac5c5f6 100644 --- a/coderd/database/dbgen/dbgen.go +++ b/coderd/database/dbgen/dbgen.go @@ -108,7 +108,7 @@ func Template(t testing.TB, db database.Store, seed database.Template) database. return template } -func APIKey(t testing.TB, db database.Store, seed database.APIKey) (key database.APIKey, token string) { +func APIKey(t testing.TB, db database.Store, seed database.APIKey, munge ...func(*database.InsertAPIKeyParams)) (key database.APIKey, token string) { id, _ := cryptorand.String(10) secret, _ := cryptorand.String(22) hashed := sha256.Sum256([]byte(secret)) @@ -124,7 +124,7 @@ func APIKey(t testing.TB, db database.Store, seed database.APIKey) (key database } } - key, err := db.InsertAPIKey(genCtx, database.InsertAPIKeyParams{ + params := database.InsertAPIKeyParams{ ID: takeFirst(seed.ID, id), // 0 defaults to 86400 at the db layer LifetimeSeconds: takeFirst(seed.LifetimeSeconds, 0), @@ -138,7 +138,11 @@ func APIKey(t testing.TB, db database.Store, seed database.APIKey) (key database LoginType: takeFirst(seed.LoginType, database.LoginTypePassword), Scope: takeFirst(seed.Scope, database.APIKeyScopeAll), TokenName: takeFirst(seed.TokenName), - }) + } + for _, fn := range munge { + fn(¶ms) + } + key, err := db.InsertAPIKey(genCtx, params) require.NoError(t, err, "insert api key") return key, fmt.Sprintf("%s-%s", key.ID, secret) } diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 42d61244d9098..c3d9e51af5e2c 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -2597,6 +2597,11 @@ func (q *FakeQuerier) EnqueueNotificationMessage(_ context.Context, arg database return err } +func (*FakeQuerier) ExpirePrebuildsAPIKeys(_ context.Context, _ time.Time) error { + // Implemented in postgres. + return nil +} + func (q *FakeQuerier) FavoriteWorkspace(_ context.Context, arg uuid.UUID) error { err := validateDatabaseType(arg) if err != nil { diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index ca2b0c2ce7fa5..c1df170bdcf35 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -487,6 +487,13 @@ func (m queryMetricsStore) EnqueueNotificationMessage(ctx context.Context, arg d return r0 } +func (m queryMetricsStore) ExpirePrebuildsAPIKeys(ctx context.Context, now time.Time) error { + start := time.Now() + r0 := m.s.ExpirePrebuildsAPIKeys(ctx, now) + m.queryLatencies.WithLabelValues("ExpirePrebuildsAPIKeys").Observe(time.Since(start).Seconds()) + return r0 +} + func (m queryMetricsStore) FavoriteWorkspace(ctx context.Context, arg uuid.UUID) error { start := time.Now() r0 := m.s.FavoriteWorkspace(ctx, arg) diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index 9d7d6c74cb0ce..6a699ae09a1e4 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -874,6 +874,20 @@ func (mr *MockStoreMockRecorder) EnqueueNotificationMessage(ctx, arg any) *gomoc return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "EnqueueNotificationMessage", reflect.TypeOf((*MockStore)(nil).EnqueueNotificationMessage), ctx, arg) } +// ExpirePrebuildsAPIKeys mocks base method. +func (m *MockStore) ExpirePrebuildsAPIKeys(ctx context.Context, now time.Time) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ExpirePrebuildsAPIKeys", ctx, now) + ret0, _ := ret[0].(error) + return ret0 +} + +// ExpirePrebuildsAPIKeys indicates an expected call of ExpirePrebuildsAPIKeys. +func (mr *MockStoreMockRecorder) ExpirePrebuildsAPIKeys(ctx, now any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ExpirePrebuildsAPIKeys", reflect.TypeOf((*MockStore)(nil).ExpirePrebuildsAPIKeys), ctx, now) +} + // FavoriteWorkspace mocks base method. func (m *MockStore) FavoriteWorkspace(ctx context.Context, id uuid.UUID) error { m.ctrl.T.Helper() diff --git a/coderd/database/dbpurge/dbpurge.go b/coderd/database/dbpurge/dbpurge.go index b7a308cfd6a06..6fcf4b0e46ef7 100644 --- a/coderd/database/dbpurge/dbpurge.go +++ b/coderd/database/dbpurge/dbpurge.go @@ -62,6 +62,9 @@ func New(ctx context.Context, logger slog.Logger, db database.Store, clk quartz. if err := tx.DeleteOldNotificationMessages(ctx); err != nil { return xerrors.Errorf("failed to delete old notification messages: %w", err) } + if err := tx.ExpirePrebuildsAPIKeys(ctx, dbtime.Time(start)); err != nil { + return xerrors.Errorf("failed to expire prebuilds user api keys: %w", err) + } logger.Debug(ctx, "purged old database entries", slog.F("duration", clk.Since(start))) diff --git a/coderd/database/dbpurge/dbpurge_test.go b/coderd/database/dbpurge/dbpurge_test.go index 4e81868ac73fb..148a7f053fa70 100644 --- a/coderd/database/dbpurge/dbpurge_test.go +++ b/coderd/database/dbpurge/dbpurge_test.go @@ -25,6 +25,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbrollup" "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/coderd/database/dbtime" + "github.com/coder/coder/v2/coderd/provisionerdserver" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/provisionerd/proto" "github.com/coder/coder/v2/provisionersdk" @@ -40,6 +41,9 @@ func TestMain(m *testing.M) { // //nolint:paralleltest // It uses LockIDDBPurge. func TestPurge(t *testing.T) { + if !dbtestutil.WillUsePostgres() { + t.Skip("requires postgres") + } ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) defer cancel() @@ -490,3 +494,71 @@ func containsProvisionerDaemon(daemons []database.ProvisionerDaemon, name string return d.Name == name }) } + +func TestExpireOldAPIKeys(t *testing.T) { + t.Parallel() + if !dbtestutil.WillUsePostgres() { + t.Skip("only implemented in postgres") + } + + // Given: a number of workspaces and API keys owned by a regular user and the prebuilds system user. + var ( + ctx = testutil.Context(t, testutil.WaitShort) + now = dbtime.Now() + db, _ = dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure()) + org = dbgen.Organization(t, db, database.Organization{}) + user = dbgen.User(t, db, database.User{}) + tpl = dbgen.Template(t, db, database.Template{OrganizationID: org.ID, CreatedBy: user.ID}) + userWs = dbgen.Workspace(t, db, database.WorkspaceTable{ + OwnerID: user.ID, + TemplateID: tpl.ID, + }) + prebuildsWs = dbgen.Workspace(t, db, database.WorkspaceTable{ + OwnerID: database.PrebuildsSystemUserID, + TemplateID: tpl.ID, + }) + createAPIKey = func(userID uuid.UUID, name string) database.APIKey { + k, _ := dbgen.APIKey(t, db, database.APIKey{UserID: userID, TokenName: name, ExpiresAt: now.Add(time.Hour)}, func(iap *database.InsertAPIKeyParams) { + iap.TokenName = name + }) + return k + } + assertKeyActive = func(kid string) { + k, err := db.GetAPIKeyByID(ctx, kid) + require.NoError(t, err) + assert.True(t, k.ExpiresAt.After(now)) + } + assertKeyExpired = func(kid string) { + k, err := db.GetAPIKeyByID(ctx, kid) + require.NoError(t, err) + assert.True(t, k.ExpiresAt.Equal(now)) + } + unnamedUserAPIKey = createAPIKey(user.ID, "") + unnamedPrebuildsAPIKey = createAPIKey(database.PrebuildsSystemUserID, "") + namedUserAPIKey = createAPIKey(user.ID, "my-token") + namedPrebuildsAPIKey = createAPIKey(database.PrebuildsSystemUserID, "also-my-token") + userWorkspaceAPIKey1 = createAPIKey(user.ID, provisionerdserver.WorkspaceSessionTokenName(user.ID, userWs.ID)) + userWorkspaceAPIKey2 = createAPIKey(user.ID, provisionerdserver.WorkspaceSessionTokenName(user.ID, prebuildsWs.ID)) + prebuildsWorkspaceAPIKey1 = createAPIKey(database.PrebuildsSystemUserID, provisionerdserver.WorkspaceSessionTokenName(database.PrebuildsSystemUserID, prebuildsWs.ID)) + prebuildsWorkspaceAPIKey2 = createAPIKey(database.PrebuildsSystemUserID, provisionerdserver.WorkspaceSessionTokenName(database.PrebuildsSystemUserID, userWs.ID)) + ) + + // When: we call ExpirePrebuildsAPIKeys + err := db.ExpirePrebuildsAPIKeys(ctx, now) + // Then: no errors is reported. + require.NoError(t, err) + + // We do not touch user API keys. + assertKeyActive(unnamedUserAPIKey.ID) + assertKeyActive(namedUserAPIKey.ID) + assertKeyActive(userWorkspaceAPIKey1.ID) + assertKeyActive(userWorkspaceAPIKey2.ID) + // Unnamed prebuilds API keys get expired. + assertKeyExpired(unnamedPrebuildsAPIKey.ID) + // API keys for workspaces still owned by prebuilds user remain active until claimed. + assertKeyActive(prebuildsWorkspaceAPIKey1.ID) + // API keys for workspaces no longer owned by prebuilds user get expired. + assertKeyExpired(prebuildsWorkspaceAPIKey2.ID) + // Out of an abundance of caution, we do not expire explicitly named prebuilds API keys. + assertKeyActive(namedPrebuildsAPIKey.ID) +} diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 4d5052b42aadc..d785d4d9d6947 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -124,6 +124,11 @@ type sqlcQuerier interface { // of the test-only in-memory database. Do not use this in new code. DisableForeignKeysAndTriggers(ctx context.Context) error EnqueueNotificationMessage(ctx context.Context, arg EnqueueNotificationMessageParams) error + // Firstly, collect api_keys owned by the prebuilds user that correlate + // to workspaces no longer owned by the prebuilds user. + // Next, collect api_keys that belong to the prebuilds user but have no token name. + // These were most likely created via 'coder login' as the prebuilds user. + ExpirePrebuildsAPIKeys(ctx context.Context, now time.Time) error FavoriteWorkspace(ctx context.Context, id uuid.UUID) error FetchMemoryResourceMonitorsByAgentID(ctx context.Context, agentID uuid.UUID) (WorkspaceAgentMemoryResourceMonitor, error) FetchMemoryResourceMonitorsUpdatedAfter(ctx context.Context, updatedAt time.Time) ([]WorkspaceAgentMemoryResourceMonitor, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index a24de735bf539..8d4e95f912985 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -144,6 +144,46 @@ func (q *sqlQuerier) DeleteApplicationConnectAPIKeysByUserID(ctx context.Context return err } +const expirePrebuildsAPIKeys = `-- name: ExpirePrebuildsAPIKeys :exec +WITH unexpired_prebuilds_workspace_session_tokens AS ( + SELECT id, SUBSTRING(token_name FROM 38 FOR 36)::uuid AS workspace_id + FROM api_keys + WHERE user_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid + AND expires_at > $1::timestamptz + AND token_name SIMILAR TO 'c42fdf75-3097-471c-8c33-fb52454d81c0_[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}_session_token' +), +stale_prebuilds_workspace_session_tokens AS ( + SELECT upwst.id + FROM unexpired_prebuilds_workspace_session_tokens upwst + LEFT JOIN workspaces w + ON w.id = upwst.workspace_id + WHERE w.owner_id <> 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid +), +unnamed_prebuilds_api_keys AS ( + SELECT id + FROM api_keys + WHERE user_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid + AND token_name = '' + AND expires_at > $1::timestamptz +) +UPDATE api_keys +SET expires_at = $1::timestamptz +WHERE id IN ( + SELECT id FROM stale_prebuilds_workspace_session_tokens + UNION + SELECT id FROM unnamed_prebuilds_api_keys +) +` + +// Firstly, collect api_keys owned by the prebuilds user that correlate +// to workspaces no longer owned by the prebuilds user. +// Next, collect api_keys that belong to the prebuilds user but have no token name. +// These were most likely created via 'coder login' as the prebuilds user. +func (q *sqlQuerier) ExpirePrebuildsAPIKeys(ctx context.Context, now time.Time) error { + _, err := q.db.ExecContext(ctx, expirePrebuildsAPIKeys, now) + return err +} + const getAPIKeyByID = `-- name: GetAPIKeyByID :one SELECT id, hashed_secret, user_id, last_used, expires_at, created_at, updated_at, login_type, lifetime_seconds, ip_address, scope, token_name diff --git a/coderd/database/queries/apikeys.sql b/coderd/database/queries/apikeys.sql index 4ff77cb469cd5..98be411ca65ea 100644 --- a/coderd/database/queries/apikeys.sql +++ b/coderd/database/queries/apikeys.sql @@ -83,3 +83,37 @@ DELETE FROM api_keys WHERE user_id = $1; + +-- name: ExpirePrebuildsAPIKeys :exec +-- Firstly, collect api_keys owned by the prebuilds user that correlate +-- to workspaces no longer owned by the prebuilds user. +WITH unexpired_prebuilds_workspace_session_tokens AS ( + SELECT id, SUBSTRING(token_name FROM 38 FOR 36)::uuid AS workspace_id + FROM api_keys + WHERE user_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid + AND expires_at > @now::timestamptz + AND token_name SIMILAR TO 'c42fdf75-3097-471c-8c33-fb52454d81c0_[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}_session_token' +), +stale_prebuilds_workspace_session_tokens AS ( + SELECT upwst.id + FROM unexpired_prebuilds_workspace_session_tokens upwst + LEFT JOIN workspaces w + ON w.id = upwst.workspace_id + WHERE w.owner_id <> 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid +), +-- Next, collect api_keys that belong to the prebuilds user but have no token name. +-- These were most likely created via 'coder login' as the prebuilds user. +unnamed_prebuilds_api_keys AS ( + SELECT id + FROM api_keys + WHERE user_id = 'c42fdf75-3097-471c-8c33-fb52454d81c0'::uuid + AND token_name = '' + AND expires_at > @now::timestamptz +) +UPDATE api_keys +SET expires_at = @now::timestamptz +WHERE id IN ( + SELECT id FROM stale_prebuilds_workspace_session_tokens + UNION + SELECT id FROM unnamed_prebuilds_api_keys +); diff --git a/coderd/provisionerdserver/provisionerdserver.go b/coderd/provisionerdserver/provisionerdserver.go index f545169c93b31..b3ab8547c1013 100644 --- a/coderd/provisionerdserver/provisionerdserver.go +++ b/coderd/provisionerdserver/provisionerdserver.go @@ -2708,15 +2708,23 @@ func InsertWorkspaceResource(ctx context.Context, db database.Store, jobID uuid. return nil } -func workspaceSessionTokenName(workspace database.Workspace) string { - return fmt.Sprintf("%s_%s_session_token", workspace.OwnerID, workspace.ID) +func WorkspaceSessionTokenName(ownerID, workspaceID uuid.UUID) string { + return fmt.Sprintf("%s_%s_session_token", ownerID, workspaceID) } func (s *server) regenerateSessionToken(ctx context.Context, user database.User, workspace database.Workspace) (string, error) { + // NOTE(Cian): Once a workspace is claimed, there's no reason for the session token to be valid any longer. + // Not generating any session token at all for a system user may unintentionally break existing templates, + // which we want to avoid. If there's no session token for the workspace belonging to the prebuilds user, + // then there's nothing for us to worry about here. + // TODO(Cian): Update this to handle _all_ system users. At the time of writing, only one system user exists. + if err := deleteSessionTokenForUserAndWorkspace(ctx, s.Database, database.PrebuildsSystemUserID, workspace.ID); err != nil && !errors.Is(err, sql.ErrNoRows) { + s.Logger.Error(ctx, "failed to delete prebuilds session token", slog.Error(err), slog.F("workspace_id", workspace.ID)) + } newkey, sessionToken, err := apikey.Generate(apikey.CreateParams{ UserID: user.ID, LoginType: user.LoginType, - TokenName: workspaceSessionTokenName(workspace), + TokenName: WorkspaceSessionTokenName(workspace.OwnerID, workspace.ID), DefaultLifetime: s.DeploymentValues.Sessions.DefaultTokenDuration.Value(), LifetimeSeconds: int64(s.DeploymentValues.Sessions.MaximumTokenDuration.Value().Seconds()), }) @@ -2744,10 +2752,14 @@ func (s *server) regenerateSessionToken(ctx context.Context, user database.User, } func deleteSessionToken(ctx context.Context, db database.Store, workspace database.Workspace) error { + return deleteSessionTokenForUserAndWorkspace(ctx, db, workspace.OwnerID, workspace.ID) +} + +func deleteSessionTokenForUserAndWorkspace(ctx context.Context, db database.Store, userID, workspaceID uuid.UUID) error { err := db.InTx(func(tx database.Store) error { key, err := tx.GetAPIKeyByName(ctx, database.GetAPIKeyByNameParams{ - UserID: workspace.OwnerID, - TokenName: workspaceSessionTokenName(workspace), + UserID: userID, + TokenName: WorkspaceSessionTokenName(userID, workspaceID), }) if err == nil { err = tx.DeleteAPIKeyByID(ctx, key.ID) diff --git a/coderd/provisionerdserver/provisionerdserver_test.go b/coderd/provisionerdserver/provisionerdserver_test.go index 66684835650a8..dbec8b4052acd 100644 --- a/coderd/provisionerdserver/provisionerdserver_test.go +++ b/coderd/provisionerdserver/provisionerdserver_test.go @@ -3576,6 +3576,70 @@ func TestNotifications(t *testing.T) { }) } +func TestServer_ExpirePrebuildsSessionToken(t *testing.T) { + t.Parallel() + + // Given: a prebuilt workspace where an API key was previously created for the prebuilds user. + var ( + ctx = testutil.Context(t, testutil.WaitShort) + srv, db, ps, pd = setup(t, false, nil) + user = dbgen.User(t, db, database.User{}) + template = dbgen.Template(t, db, database.Template{ + OrganizationID: pd.OrganizationID, + CreatedBy: user.ID, + }) + version = dbgen.TemplateVersion(t, db, database.TemplateVersion{ + TemplateID: uuid.NullUUID{UUID: template.ID, Valid: true}, + OrganizationID: pd.OrganizationID, + CreatedBy: user.ID, + }) + workspace = dbgen.Workspace(t, db, database.WorkspaceTable{ + OrganizationID: pd.OrganizationID, + TemplateID: template.ID, + OwnerID: database.PrebuildsSystemUserID, + }) + workspaceBuildID = uuid.New() + buildJob = dbgen.ProvisionerJob(t, db, ps, database.ProvisionerJob{ + OrganizationID: pd.OrganizationID, + FileID: dbgen.File(t, db, database.File{CreatedBy: user.ID}).ID, + Type: database.ProvisionerJobTypeWorkspaceBuild, + Input: must(json.Marshal(provisionerdserver.WorkspaceProvisionJob{ + WorkspaceBuildID: workspaceBuildID, + })), + InitiatorID: database.PrebuildsSystemUserID, + Tags: pd.Tags, + }) + _ = dbgen.WorkspaceBuild(t, db, database.WorkspaceBuild{ + ID: workspaceBuildID, + WorkspaceID: workspace.ID, + TemplateVersionID: version.ID, + JobID: buildJob.ID, + Transition: database.WorkspaceTransitionStart, + InitiatorID: database.PrebuildsSystemUserID, + }) + existingKey, _ = dbgen.APIKey(t, db, database.APIKey{ + UserID: database.PrebuildsSystemUserID, + TokenName: provisionerdserver.WorkspaceSessionTokenName(database.PrebuildsSystemUserID, workspace.ID), + }) + ) + + // When: the prebuild claim job is acquired + fs := newFakeStream(ctx) + err := srv.AcquireJobWithCancel(fs) + require.NoError(t, err) + job, err := fs.waitForJob() + require.NoError(t, err) + require.NotNil(t, job) + workspaceBuildJob := job.Type.(*proto.AcquiredJob_WorkspaceBuild_).WorkspaceBuild + require.NotNil(t, workspaceBuildJob.Metadata) + + // Assert test invariant: we acquired the expected build job + require.Equal(t, workspaceBuildID.String(), workspaceBuildJob.WorkspaceBuildId) + // Then: The session token should be deleted + _, err = db.GetAPIKeyByID(ctx, existingKey.ID) + require.ErrorIs(t, err, sql.ErrNoRows, "api key for prebuilds user should be deleted") +} + type overrides struct { ctx context.Context deploymentValues *codersdk.DeploymentValues