Skip to content

Commit 20d67d7

Browse files
johnstcnjohnstcn
authored
fix: expire token for prebuilds user when regenerating session token (#19667) (#19669)
* provisionerdserver: Expires prebuild user token for workspace, if it exists, when regenerating session token. * dbauthz: disallow prebuilds user from creating api keys * dbpurge: added functionality to expire stale api keys owned by the prebuilds user (cherry picked from commit 06cbb28)
1 parent dd62ec4 commit 20d67d7

File tree

15 files changed

+360
-14
lines changed

15 files changed

+360
-14
lines changed

‎coderd/apikey.go‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ import (
1212
"github.com/moby/moby/pkg/namesgenerator"
1313
"golang.org/x/xerrors"
1414

15+
"cdr.dev/slog"
16+
1517
"github.com/coder/coder/v2/coderd/apikey"
1618
"github.com/coder/coder/v2/coderd/audit"
1719
"github.com/coder/coder/v2/coderd/database"
@@ -56,6 +58,14 @@ func (api *API) postToken(rw http.ResponseWriter, r *http.Request) {
5658
return
5759
}
5860

61+
// TODO(Cian): System users technically just have the 'member' role
62+
// and we don't want to disallow all members from creating API keys.
63+
if user.IsSystem {
64+
api.Logger.Warn(ctx, "disallowed creating api key for system user", slog.F("user_id", user.ID))
65+
httpapi.Forbidden(rw)
66+
return
67+
}
68+
5969
scope := database.APIKeyScopeAll
6070
if scope != "" {
6171
scope = database.APIKeyScope(createToken.Scope)
@@ -124,6 +134,14 @@ func (api *API) postAPIKey(rw http.ResponseWriter, r *http.Request) {
124134
ctx := r.Context()
125135
user := httpmw.UserParam(r)
126136

137+
// TODO(Cian): System users technically just have the 'member' role
138+
// and we don't want to disallow all members from creating API keys.
139+
if user.IsSystem {
140+
api.Logger.Warn(ctx, "disallowed creating api key for system user", slog.F("user_id", user.ID))
141+
httpapi.Forbidden(rw)
142+
return
143+
}
144+
127145
cookie, _, err := api.createAPIKey(ctx, apikey.CreateParams{
128146
UserID: user.ID,
129147
DefaultLifetime: api.DeploymentValues.Sessions.DefaultTokenDuration.Value(),

‎coderd/apikey_test.go‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,10 @@ import (
1313
"github.com/coder/coder/v2/coderd/audit"
1414
"github.com/coder/coder/v2/coderd/coderdtest"
1515
"github.com/coder/coder/v2/coderd/database"
16+
"github.com/coder/coder/v2/coderd/database/dbgen"
1617
"github.com/coder/coder/v2/coderd/database/dbtestutil"
1718
"github.com/coder/coder/v2/coderd/database/dbtime"
19+
"github.com/coder/coder/v2/coderd/httpapi"
1820
"github.com/coder/coder/v2/codersdk"
1921
"github.com/coder/coder/v2/testutil"
2022
"github.com/coder/serpent"
@@ -351,3 +353,34 @@ func TestAPIKey_SetDefault(t *testing.T) {
351353
require.NoError(t, err)
352354
require.EqualValues(t, dc.Sessions.DefaultTokenDuration.Value().Seconds(), apiKey1.LifetimeSeconds)
353355
}
356+
357+
func TestAPIKey_PrebuildsNotAllowed(t *testing.T) {
358+
t.Parallel()
359+
360+
db, pubsub := dbtestutil.NewDB(t)
361+
dc := coderdtest.DeploymentValues(t)
362+
dc.Sessions.DefaultTokenDuration = serpent.Duration(time.Hour * 12)
363+
client := coderdtest.New(t, &coderdtest.Options{
364+
Database: db,
365+
Pubsub: pubsub,
366+
DeploymentValues: dc,
367+
})
368+
369+
ctx := testutil.Context(t, testutil.WaitLong)
370+
371+
// Given: an existing api token for the prebuilds user
372+
_, prebuildsToken := dbgen.APIKey(t, db, database.APIKey{
373+
UserID: database.PrebuildsSystemUserID,
374+
})
375+
client.SetSessionToken(prebuildsToken)
376+
377+
// When: the prebuilds user tries to create an API key
378+
_, err := client.CreateAPIKey(ctx, database.PrebuildsSystemUserID.String())
379+
// Then: denied.
380+
require.ErrorContains(t, err, httpapi.ResourceForbiddenResponse.Message)
381+
382+
// When: the prebuilds user tries to create a token
383+
_, err = client.CreateToken(ctx, database.PrebuildsSystemUserID.String(), codersdk.CreateTokenRequest{})
384+
// Then: also denied.
385+
require.ErrorContains(t, err, httpapi.ResourceForbiddenResponse.Message)
386+
}

‎coderd/database/dbauthz/dbauthz.go‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1652,6 +1652,13 @@ func (q *querier) EnqueueNotificationMessage(ctx context.Context, arg database.E
16521652
return q.db.EnqueueNotificationMessage(ctx, arg)
16531653
}
16541654

1655+
func (q *querier) ExpirePrebuildsAPIKeys(ctx context.Context, now time.Time) error {
1656+
if err := q.authorizeContext(ctx, policy.ActionDelete, rbac.ResourceApiKey); err != nil {
1657+
return err
1658+
}
1659+
return q.db.ExpirePrebuildsAPIKeys(ctx, now)
1660+
}
1661+
16551662
func (q *querier) FavoriteWorkspace(ctx context.Context, id uuid.UUID) error {
16561663
fetch := func(ctx context.Context, id uuid.UUID) (database.Workspace, error) {
16571664
return q.db.GetWorkspaceByID(ctx, id)
@@ -3507,6 +3514,14 @@ func (q *querier) HasTemplateVersionsWithAITask(ctx context.Context) (bool, erro
35073514
}
35083515

35093516
func (q *querier) InsertAPIKey(ctx context.Context, arg database.InsertAPIKeyParams) (database.APIKey, error) {
3517+
// TODO(Cian): ideally this would be encoded in the policy, but system users are just members and we
3518+
// don't currently have a capability to conditionally deny creating resources by owner ID in a role.
3519+
// We also need to enrich rbac.Actor with IsSystem so that we can distinguish all system users.
3520+
// For now, there is only one system user (prebuilds).
3521+
if act, ok := ActorFromContext(ctx); ok && act.ID == database.PrebuildsSystemUserID.String() {
3522+
return database.APIKey{}, logNotAuthorizedError(ctx, q.log, NotAuthorizedError{Err: xerrors.Errorf("prebuild user may not create api keys")})
3523+
}
3524+
35103525
return insert(q.log, q.auth,
35113526
rbac.ResourceApiKey.WithOwner(arg.UserID.String()),
35123527
q.db.InsertAPIKey)(ctx, arg)

‎coderd/database/dbauthz/dbauthz_test.go‎

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,24 +14,26 @@ import (
1414
"github.com/google/uuid"
1515
"github.com/sqlc-dev/pqtype"
1616
"github.com/stretchr/testify/require"
17+
"go.uber.org/mock/gomock"
1718
"golang.org/x/xerrors"
1819

1920
"cdr.dev/slog"
20-
21-
"github.com/coder/coder/v2/coderd/database/db2sdk"
22-
"github.com/coder/coder/v2/coderd/database/dbmem"
23-
"github.com/coder/coder/v2/coderd/notifications"
24-
"github.com/coder/coder/v2/coderd/rbac/policy"
25-
"github.com/coder/coder/v2/codersdk"
21+
"cdr.dev/slog/sloggers/slogtest"
2622

2723
"github.com/coder/coder/v2/coderd/coderdtest"
2824
"github.com/coder/coder/v2/coderd/database"
25+
"github.com/coder/coder/v2/coderd/database/db2sdk"
2926
"github.com/coder/coder/v2/coderd/database/dbauthz"
3027
"github.com/coder/coder/v2/coderd/database/dbgen"
28+
"github.com/coder/coder/v2/coderd/database/dbmem"
29+
"github.com/coder/coder/v2/coderd/database/dbmock"
3130
"github.com/coder/coder/v2/coderd/database/dbtestutil"
3231
"github.com/coder/coder/v2/coderd/database/dbtime"
32+
"github.com/coder/coder/v2/coderd/notifications"
3333
"github.com/coder/coder/v2/coderd/rbac"
34+
"github.com/coder/coder/v2/coderd/rbac/policy"
3435
"github.com/coder/coder/v2/coderd/util/slice"
36+
"github.com/coder/coder/v2/codersdk"
3537
"github.com/coder/coder/v2/provisionersdk"
3638
"github.com/coder/coder/v2/testutil"
3739
)
@@ -1573,6 +1575,9 @@ func (s *MethodTestSuite) TestUser() {
15731575
UserID: u.ID,
15741576
OrganizationID: uuid.New(),
15751577
}).Asserts(u, policy.ActionRead).Returns(int64(0))
1578+
s.Run("ExpirePrebuildsAPIKeys", s.Subtest(func(db database.Store, check *expects) {
1579+
check.Args(dbtime.Now()).Asserts(rbac.ResourceApiKey, policy.ActionDelete).Returns()
1580+
}))
15761581
}))
15771582
s.Run("GetQuotaConsumedForUser", s.Subtest(func(db database.Store, check *expects) {
15781583
u := dbgen.User(s.T(), db, database.User{})
@@ -5637,3 +5642,18 @@ func (s *MethodTestSuite) TestAuthorizePrebuiltWorkspace() {
56375642
}).Asserts(w, policy.ActionUpdate, w.AsPrebuild(), policy.ActionUpdate)
56385643
}))
56395644
}
5645+
5646+
// Ensures that the prebuilds actor may never insert an api key.
5647+
func TestInsertAPIKey_AsPrebuildsUser(t *testing.T) {
5648+
t.Parallel()
5649+
prebuildsSubj := rbac.Subject{
5650+
ID: database.PrebuildsSystemUserID.String(),
5651+
}
5652+
ctx := dbauthz.As(testutil.Context(t, testutil.WaitShort), prebuildsSubj)
5653+
mDB := dbmock.NewMockStore(gomock.NewController(t))
5654+
log := slogtest.Make(t, nil)
5655+
mDB.EXPECT().Wrappers().Times(1).Return([]string{})
5656+
dbz := dbauthz.New(mDB, nil, log, nil)
5657+
_, err := dbz.InsertAPIKey(ctx, database.InsertAPIKeyParams{})
5658+
require.True(t, dbauthz.IsNotAuthorizedError(err))
5659+
}

‎coderd/database/dbgen/dbgen.go‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ func Template(t testing.TB, db database.Store, seed database.Template) database.
108108
return template
109109
}
110110

111-
func APIKey(t testing.TB, db database.Store, seed database.APIKey) (key database.APIKey, token string) {
111+
func APIKey(t testing.TB, db database.Store, seed database.APIKey, munge ...func(*database.InsertAPIKeyParams)) (key database.APIKey, token string) {
112112
id, _ := cryptorand.String(10)
113113
secret, _ := cryptorand.String(22)
114114
hashed := sha256.Sum256([]byte(secret))
@@ -124,7 +124,7 @@ func APIKey(t testing.TB, db database.Store, seed database.APIKey) (key database
124124
}
125125
}
126126

127-
key, err := db.InsertAPIKey(genCtx, database.InsertAPIKeyParams{
127+
params := database.InsertAPIKeyParams{
128128
ID: takeFirst(seed.ID, id),
129129
// 0 defaults to 86400 at the db layer
130130
LifetimeSeconds: takeFirst(seed.LifetimeSeconds, 0),
@@ -138,7 +138,11 @@ func APIKey(t testing.TB, db database.Store, seed database.APIKey) (key database
138138
LoginType: takeFirst(seed.LoginType, database.LoginTypePassword),
139139
Scope: takeFirst(seed.Scope, database.APIKeyScopeAll),
140140
TokenName: takeFirst(seed.TokenName),
141-
})
141+
}
142+
for _, fn := range munge {
143+
fn(&params)
144+
}
145+
key, err := db.InsertAPIKey(genCtx, params)
142146
require.NoError(t, err, "insert api key")
143147
return key, fmt.Sprintf("%s-%s", key.ID, secret)
144148
}

‎coderd/database/dbmem/dbmem.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2597,6 +2597,11 @@ func (q *FakeQuerier) EnqueueNotificationMessage(_ context.Context, arg database
25972597
return err
25982598
}
25992599

2600+
func (*FakeQuerier) ExpirePrebuildsAPIKeys(_ context.Context, _ time.Time) error {
2601+
// Implemented in postgres.
2602+
return nil
2603+
}
2604+
26002605
func (q *FakeQuerier) FavoriteWorkspace(_ context.Context, arg uuid.UUID) error {
26012606
err := validateDatabaseType(arg)
26022607
if err != nil {

‎coderd/database/dbmetrics/querymetrics.go‎

Lines changed: 7 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/dbmock/dbmock.go‎

Lines changed: 14 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/dbpurge/dbpurge.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,9 @@ func New(ctx context.Context, logger slog.Logger, db database.Store, clk quartz.
6262
if err := tx.DeleteOldNotificationMessages(ctx); err != nil {
6363
return xerrors.Errorf("failed to delete old notification messages: %w", err)
6464
}
65+
if err := tx.ExpirePrebuildsAPIKeys(ctx, dbtime.Time(start)); err != nil {
66+
return xerrors.Errorf("failed to expire prebuilds user api keys: %w", err)
67+
}
6568

6669
logger.Debug(ctx, "purged old database entries", slog.F("duration", clk.Since(start)))
6770

‎coderd/database/dbpurge/dbpurge_test.go‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525
"github.com/coder/coder/v2/coderd/database/dbrollup"
2626
"github.com/coder/coder/v2/coderd/database/dbtestutil"
2727
"github.com/coder/coder/v2/coderd/database/dbtime"
28+
"github.com/coder/coder/v2/coderd/provisionerdserver"
2829
"github.com/coder/coder/v2/codersdk"
2930
"github.com/coder/coder/v2/provisionerd/proto"
3031
"github.com/coder/coder/v2/provisionersdk"
@@ -40,6 +41,9 @@ func TestMain(m *testing.M) {
4041
//
4142
//nolint:paralleltest // It uses LockIDDBPurge.
4243
func TestPurge(t *testing.T) {
44+
if !dbtestutil.WillUsePostgres() {
45+
t.Skip("requires postgres")
46+
}
4347
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
4448
defer cancel()
4549

@@ -490,3 +494,71 @@ func containsProvisionerDaemon(daemons []database.ProvisionerDaemon, name string
490494
return d.Name == name
491495
})
492496
}
497+
498+
func TestExpireOldAPIKeys(t *testing.T) {
499+
t.Parallel()
500+
if !dbtestutil.WillUsePostgres() {
501+
t.Skip("only implemented in postgres")
502+
}
503+
504+
// Given: a number of workspaces and API keys owned by a regular user and the prebuilds system user.
505+
var (
506+
ctx = testutil.Context(t, testutil.WaitShort)
507+
now = dbtime.Now()
508+
db, _ = dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure())
509+
org = dbgen.Organization(t, db, database.Organization{})
510+
user = dbgen.User(t, db, database.User{})
511+
tpl = dbgen.Template(t, db, database.Template{OrganizationID: org.ID, CreatedBy: user.ID})
512+
userWs = dbgen.Workspace(t, db, database.WorkspaceTable{
513+
OwnerID: user.ID,
514+
TemplateID: tpl.ID,
515+
})
516+
prebuildsWs = dbgen.Workspace(t, db, database.WorkspaceTable{
517+
OwnerID: database.PrebuildsSystemUserID,
518+
TemplateID: tpl.ID,
519+
})
520+
createAPIKey = func(userID uuid.UUID, name string) database.APIKey {
521+
k, _ := dbgen.APIKey(t, db, database.APIKey{UserID: userID, TokenName: name, ExpiresAt: now.Add(time.Hour)}, func(iap *database.InsertAPIKeyParams) {
522+
iap.TokenName = name
523+
})
524+
return k
525+
}
526+
assertKeyActive = func(kid string) {
527+
k, err := db.GetAPIKeyByID(ctx, kid)
528+
require.NoError(t, err)
529+
assert.True(t, k.ExpiresAt.After(now))
530+
}
531+
assertKeyExpired = func(kid string) {
532+
k, err := db.GetAPIKeyByID(ctx, kid)
533+
require.NoError(t, err)
534+
assert.True(t, k.ExpiresAt.Equal(now))
535+
}
536+
unnamedUserAPIKey = createAPIKey(user.ID, "")
537+
unnamedPrebuildsAPIKey = createAPIKey(database.PrebuildsSystemUserID, "")
538+
namedUserAPIKey = createAPIKey(user.ID, "my-token")
539+
namedPrebuildsAPIKey = createAPIKey(database.PrebuildsSystemUserID, "also-my-token")
540+
userWorkspaceAPIKey1 = createAPIKey(user.ID, provisionerdserver.WorkspaceSessionTokenName(user.ID, userWs.ID))
541+
userWorkspaceAPIKey2 = createAPIKey(user.ID, provisionerdserver.WorkspaceSessionTokenName(user.ID, prebuildsWs.ID))
542+
prebuildsWorkspaceAPIKey1 = createAPIKey(database.PrebuildsSystemUserID, provisionerdserver.WorkspaceSessionTokenName(database.PrebuildsSystemUserID, prebuildsWs.ID))
543+
prebuildsWorkspaceAPIKey2 = createAPIKey(database.PrebuildsSystemUserID, provisionerdserver.WorkspaceSessionTokenName(database.PrebuildsSystemUserID, userWs.ID))
544+
)
545+
546+
// When: we call ExpirePrebuildsAPIKeys
547+
err := db.ExpirePrebuildsAPIKeys(ctx, now)
548+
// Then: no errors is reported.
549+
require.NoError(t, err)
550+
551+
// We do not touch user API keys.
552+
assertKeyActive(unnamedUserAPIKey.ID)
553+
assertKeyActive(namedUserAPIKey.ID)
554+
assertKeyActive(userWorkspaceAPIKey1.ID)
555+
assertKeyActive(userWorkspaceAPIKey2.ID)
556+
// Unnamed prebuilds API keys get expired.
557+
assertKeyExpired(unnamedPrebuildsAPIKey.ID)
558+
// API keys for workspaces still owned by prebuilds user remain active until claimed.
559+
assertKeyActive(prebuildsWorkspaceAPIKey1.ID)
560+
// API keys for workspaces no longer owned by prebuilds user get expired.
561+
assertKeyExpired(prebuildsWorkspaceAPIKey2.ID)
562+
// Out of an abundance of caution, we do not expire explicitly named prebuilds API keys.
563+
assertKeyActive(namedPrebuildsAPIKey.ID)
564+
}

0 commit comments

Comments
 (0)