Skip to content

Commit 06cbb28

Browse files
johnstcnjohnstcn
authored
fix: expire token for prebuilds user when regenerating session token (#19667)
* 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
1 parent a2a758d commit 06cbb28

File tree

14 files changed

+344
-8
lines changed

14 files changed

+344
-8
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
@@ -1787,6 +1787,13 @@ func (q *querier) EnqueueNotificationMessage(ctx context.Context, arg database.E
17871787
return q.db.EnqueueNotificationMessage(ctx, arg)
17881788
}
17891789

1790+
func (q *querier) ExpirePrebuildsAPIKeys(ctx context.Context, now time.Time) error {
1791+
if err := q.authorizeContext(ctx, policy.ActionDelete, rbac.ResourceApiKey); err != nil {
1792+
return err
1793+
}
1794+
return q.db.ExpirePrebuildsAPIKeys(ctx, now)
1795+
}
1796+
17901797
func (q *querier) FavoriteWorkspace(ctx context.Context, id uuid.UUID) error {
17911798
fetch := func(ctx context.Context, id uuid.UUID) (database.Workspace, error) {
17921799
return q.db.GetWorkspaceByID(ctx, id)
@@ -3727,6 +3734,14 @@ func (q *querier) GetWorkspacesEligibleForTransition(ctx context.Context, now ti
37273734
}
37283735

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

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"golang.org/x/xerrors"
1919

2020
"cdr.dev/slog"
21+
"cdr.dev/slog/sloggers/slogtest"
2122
"github.com/coder/coder/v2/coderd/coderdtest"
2223
"github.com/coder/coder/v2/coderd/database"
2324
"github.com/coder/coder/v2/coderd/database/db2sdk"
@@ -1297,6 +1298,10 @@ func (s *MethodTestSuite) TestUser() {
12971298
dbm.EXPECT().DeleteAPIKeysByUserID(gomock.Any(), key.UserID).Return(nil).AnyTimes()
12981299
check.Args(key.UserID).Asserts(rbac.ResourceApiKey.WithOwner(key.UserID.String()), policy.ActionDelete).Returns()
12991300
}))
1301+
s.Run("ExpirePrebuildsAPIKeys", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) {
1302+
dbm.EXPECT().ExpirePrebuildsAPIKeys(gomock.Any(), gomock.Any()).Times(1).Return(nil)
1303+
check.Args(dbtime.Now()).Asserts(rbac.ResourceApiKey, policy.ActionDelete).Returns()
1304+
}))
13001305
s.Run("GetQuotaAllowanceForUser", s.Mocked(func(dbm *dbmock.MockStore, faker *gofakeit.Faker, check *expects) {
13011306
u := testutil.Fake(s.T(), faker, database.User{})
13021307
arg := database.GetQuotaAllowanceForUserParams{UserID: u.ID, OrganizationID: uuid.New()}
@@ -4287,3 +4292,19 @@ func (s *MethodTestSuite) TestUsageEvents() {
42874292
}).Asserts(rbac.ResourceUsageEvent, policy.ActionRead)
42884293
}))
42894294
}
4295+
4296+
// Ensures that the prebuilds actor may never insert an api key.
4297+
func TestInsertAPIKey_AsPrebuildsUser(t *testing.T) {
4298+
t.Parallel()
4299+
prebuildsSubj := rbac.Subject{
4300+
ID: database.PrebuildsSystemUserID.String(),
4301+
}
4302+
ctx := dbauthz.As(testutil.Context(t, testutil.WaitShort), prebuildsSubj)
4303+
mDB := dbmock.NewMockStore(gomock.NewController(t))
4304+
log := slogtest.Make(t, nil)
4305+
mDB.EXPECT().Wrappers().Times(1).Return([]string{})
4306+
dbz := dbauthz.New(mDB, nil, log, nil)
4307+
faker := gofakeit.New(0)
4308+
_, err := dbz.InsertAPIKey(ctx, testutil.Fake(t, faker, database.InsertAPIKeyParams{}))
4309+
require.True(t, dbauthz.IsNotAuthorizedError(err))
4310+
}

‎coderd/database/dbgen/dbgen.go

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

160-
func APIKey(t testing.TB, db database.Store, seed database.APIKey) (key database.APIKey, token string) {
160+
func APIKey(t testing.TB, db database.Store, seed database.APIKey, munge ...func(*database.InsertAPIKeyParams)) (key database.APIKey, token string) {
161161
id, _ := cryptorand.String(10)
162162
secret, _ := cryptorand.String(22)
163163
hashed := sha256.Sum256([]byte(secret))
@@ -173,7 +173,7 @@ func APIKey(t testing.TB, db database.Store, seed database.APIKey) (key database
173173
}
174174
}
175175

176-
key, err := db.InsertAPIKey(genCtx, database.InsertAPIKeyParams{
176+
params := database.InsertAPIKeyParams{
177177
ID: takeFirst(seed.ID, id),
178178
// 0 defaults to 86400 at the db layer
179179
LifetimeSeconds: takeFirst(seed.LifetimeSeconds, 0),
@@ -187,7 +187,11 @@ func APIKey(t testing.TB, db database.Store, seed database.APIKey) (key database
187187
LoginType: takeFirst(seed.LoginType, database.LoginTypePassword),
188188
Scope: takeFirst(seed.Scope, database.APIKeyScopeAll),
189189
TokenName: takeFirst(seed.TokenName),
190-
})
190+
}
191+
for _, fn := range munge {
192+
fn(&params)
193+
}
194+
key, err := db.InsertAPIKey(genCtx, params)
191195
require.NoError(t, err, "insert api key")
192196
return key, fmt.Sprintf("%s-%s", key.ID, secret)
193197
}

‎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
@@ -68,6 +68,9 @@ func New(ctx context.Context, logger slog.Logger, db database.Store, clk quartz.
6868
if err := tx.DeleteOldNotificationMessages(ctx); err != nil {
6969
return xerrors.Errorf("failed to delete old notification messages: %w", err)
7070
}
71+
if err := tx.ExpirePrebuildsAPIKeys(ctx, dbtime.Time(start)); err != nil {
72+
return xerrors.Errorf("failed to expire prebuilds user api keys: %w", err)
73+
}
7174

7275
deleteOldAuditLogConnectionEventsBefore := start.Add(-maxAuditLogConnectionEventAge)
7376
if err := tx.DeleteOldAuditLogConnectionEvents(ctx, database.DeleteOldAuditLogConnectionEventsParams{

‎coderd/database/dbpurge/dbpurge_test.go

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import (
2727
"github.com/coder/coder/v2/coderd/database/dbrollup"
2828
"github.com/coder/coder/v2/coderd/database/dbtestutil"
2929
"github.com/coder/coder/v2/coderd/database/dbtime"
30+
"github.com/coder/coder/v2/coderd/provisionerdserver"
3031
"github.com/coder/coder/v2/codersdk"
3132
"github.com/coder/coder/v2/provisionerd/proto"
3233
"github.com/coder/coder/v2/provisionersdk"
@@ -638,3 +639,68 @@ func TestDeleteOldAuditLogConnectionEventsLimit(t *testing.T) {
638639

639640
require.Len(t, logs, 0)
640641
}
642+
643+
func TestExpireOldAPIKeys(t *testing.T) {
644+
t.Parallel()
645+
646+
// Given: a number of workspaces and API keys owned by a regular user and the prebuilds system user.
647+
var (
648+
ctx = testutil.Context(t, testutil.WaitShort)
649+
now = dbtime.Now()
650+
db, _ = dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure())
651+
org = dbgen.Organization(t, db, database.Organization{})
652+
user = dbgen.User(t, db, database.User{})
653+
tpl = dbgen.Template(t, db, database.Template{OrganizationID: org.ID, CreatedBy: user.ID})
654+
userWs = dbgen.Workspace(t, db, database.WorkspaceTable{
655+
OwnerID: user.ID,
656+
TemplateID: tpl.ID,
657+
})
658+
prebuildsWs = dbgen.Workspace(t, db, database.WorkspaceTable{
659+
OwnerID: database.PrebuildsSystemUserID,
660+
TemplateID: tpl.ID,
661+
})
662+
createAPIKey = func(userID uuid.UUID, name string) database.APIKey {
663+
k, _ := dbgen.APIKey(t, db, database.APIKey{UserID: userID, TokenName: name, ExpiresAt: now.Add(time.Hour)}, func(iap *database.InsertAPIKeyParams) {
664+
iap.TokenName = name
665+
})
666+
return k
667+
}
668+
assertKeyActive = func(kid string) {
669+
k, err := db.GetAPIKeyByID(ctx, kid)
670+
require.NoError(t, err)
671+
assert.True(t, k.ExpiresAt.After(now))
672+
}
673+
assertKeyExpired = func(kid string) {
674+
k, err := db.GetAPIKeyByID(ctx, kid)
675+
require.NoError(t, err)
676+
assert.True(t, k.ExpiresAt.Equal(now))
677+
}
678+
unnamedUserAPIKey = createAPIKey(user.ID, "")
679+
unnamedPrebuildsAPIKey = createAPIKey(database.PrebuildsSystemUserID, "")
680+
namedUserAPIKey = createAPIKey(user.ID, "my-token")
681+
namedPrebuildsAPIKey = createAPIKey(database.PrebuildsSystemUserID, "also-my-token")
682+
userWorkspaceAPIKey1 = createAPIKey(user.ID, provisionerdserver.WorkspaceSessionTokenName(user.ID, userWs.ID))
683+
userWorkspaceAPIKey2 = createAPIKey(user.ID, provisionerdserver.WorkspaceSessionTokenName(user.ID, prebuildsWs.ID))
684+
prebuildsWorkspaceAPIKey1 = createAPIKey(database.PrebuildsSystemUserID, provisionerdserver.WorkspaceSessionTokenName(database.PrebuildsSystemUserID, prebuildsWs.ID))
685+
prebuildsWorkspaceAPIKey2 = createAPIKey(database.PrebuildsSystemUserID, provisionerdserver.WorkspaceSessionTokenName(database.PrebuildsSystemUserID, userWs.ID))
686+
)
687+
688+
// When: we call ExpirePrebuildsAPIKeys
689+
err := db.ExpirePrebuildsAPIKeys(ctx, now)
690+
// Then: no errors is reported.
691+
require.NoError(t, err)
692+
693+
// We do not touch user API keys.
694+
assertKeyActive(unnamedUserAPIKey.ID)
695+
assertKeyActive(namedUserAPIKey.ID)
696+
assertKeyActive(userWorkspaceAPIKey1.ID)
697+
assertKeyActive(userWorkspaceAPIKey2.ID)
698+
// Unnamed prebuilds API keys get expired.
699+
assertKeyExpired(unnamedPrebuildsAPIKey.ID)
700+
// API keys for workspaces still owned by prebuilds user remain active until claimed.
701+
assertKeyActive(prebuildsWorkspaceAPIKey1.ID)
702+
// API keys for workspaces no longer owned by prebuilds user get expired.
703+
assertKeyExpired(prebuildsWorkspaceAPIKey2.ID)
704+
// Out of an abundance of caution, we do not expire explicitly named prebuilds API keys.
705+
assertKeyActive(namedPrebuildsAPIKey.ID)
706+
}

‎coderd/database/querier.go

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

0 commit comments

Comments
 (0)