Skip to content

Commit ec66090

Browse files
johnstcnjohnstcn
authored
fix: expire token for prebuilds user when regenerating session token (#19667) (#19668)
* 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 ee80509 commit ec66090

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
@@ -1725,6 +1725,13 @@ func (q *querier) EnqueueNotificationMessage(ctx context.Context, arg database.E
17251725
return q.db.EnqueueNotificationMessage(ctx, arg)
17261726
}
17271727

1728+
func (q *querier) ExpirePrebuildsAPIKeys(ctx context.Context, now time.Time) error {
1729+
if err := q.authorizeContext(ctx, policy.ActionDelete, rbac.ResourceApiKey); err != nil {
1730+
return err
1731+
}
1732+
return q.db.ExpirePrebuildsAPIKeys(ctx, now)
1733+
}
1734+
17281735
func (q *querier) FavoriteWorkspace(ctx context.Context, id uuid.UUID) error {
17291736
fetch := func(ctx context.Context, id uuid.UUID) (database.Workspace, error) {
17301737
return q.db.GetWorkspaceByID(ctx, id)
@@ -3623,6 +3630,14 @@ func (q *querier) HasTemplateVersionsWithAITask(ctx context.Context) (bool, erro
36233630
}
36243631

36253632
func (q *querier) InsertAPIKey(ctx context.Context, arg database.InsertAPIKeyParams) (database.APIKey, error) {
3633+
// TODO(Cian): ideally this would be encoded in the policy, but system users are just members and we
3634+
// don't currently have a capability to conditionally deny creating resources by owner ID in a role.
3635+
// We also need to enrich rbac.Actor with IsSystem so that we can distinguish all system users.
3636+
// For now, there is only one system user (prebuilds).
3637+
if act, ok := ActorFromContext(ctx); ok && act.ID == database.PrebuildsSystemUserID.String() {
3638+
return database.APIKey{}, logNotAuthorizedError(ctx, q.log, NotAuthorizedError{Err: xerrors.Errorf("prebuild user may not create api keys")})
3639+
}
3640+
36263641
return insert(q.log, q.auth,
36273642
rbac.ResourceApiKey.WithOwner(arg.UserID.String()),
36283643
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
@@ -14,14 +14,17 @@ 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"
21+
"cdr.dev/slog/sloggers/slogtest"
2022
"github.com/coder/coder/v2/coderd/coderdtest"
2123
"github.com/coder/coder/v2/coderd/database"
2224
"github.com/coder/coder/v2/coderd/database/db2sdk"
2325
"github.com/coder/coder/v2/coderd/database/dbauthz"
2426
"github.com/coder/coder/v2/coderd/database/dbgen"
27+
"github.com/coder/coder/v2/coderd/database/dbmock"
2528
"github.com/coder/coder/v2/coderd/database/dbtestutil"
2629
"github.com/coder/coder/v2/coderd/database/dbtime"
2730
"github.com/coder/coder/v2/coderd/notifications"
@@ -1681,6 +1684,9 @@ func (s *MethodTestSuite) TestUser() {
16811684
u := dbgen.User(s.T(), db, database.User{})
16821685
check.Args(u.ID).Asserts(rbac.ResourceApiKey.WithOwner(u.ID.String()), policy.ActionDelete).Returns()
16831686
}))
1687+
s.Run("ExpirePrebuildsAPIKeys", s.Subtest(func(db database.Store, check *expects) {
1688+
check.Args(dbtime.Now()).Asserts(rbac.ResourceApiKey, policy.ActionDelete).Returns()
1689+
}))
16841690
s.Run("GetQuotaAllowanceForUser", s.Subtest(func(db database.Store, check *expects) {
16851691
u := dbgen.User(s.T(), db, database.User{})
16861692
check.Args(database.GetQuotaAllowanceForUserParams{
@@ -5845,3 +5851,18 @@ func (s *MethodTestSuite) TestAuthorizePrebuiltWorkspace() {
58455851
}).Asserts(w, policy.ActionUpdate, w.AsPrebuild(), policy.ActionUpdate)
58465852
}))
58475853
}
5854+
5855+
// Ensures that the prebuilds actor may never insert an api key.
5856+
func TestInsertAPIKey_AsPrebuildsUser(t *testing.T) {
5857+
t.Parallel()
5858+
prebuildsSubj := rbac.Subject{
5859+
ID: database.PrebuildsSystemUserID.String(),
5860+
}
5861+
ctx := dbauthz.As(testutil.Context(t, testutil.WaitShort), prebuildsSubj)
5862+
mDB := dbmock.NewMockStore(gomock.NewController(t))
5863+
log := slogtest.Make(t, nil)
5864+
mDB.EXPECT().Wrappers().Times(1).Return([]string{})
5865+
dbz := dbauthz.New(mDB, nil, log, nil)
5866+
_, err := dbz.InsertAPIKey(ctx, database.InsertAPIKeyParams{})
5867+
require.True(t, dbauthz.IsNotAuthorizedError(err))
5868+
}

‎coderd/database/dbgen/dbgen.go‎

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

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

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

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

7174
deleteOldAuditLogConnectionEventsBefore := start.Add(-maxAuditLogConnectionEventAge)
7275
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
@@ -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"
@@ -635,3 +636,68 @@ func TestDeleteOldAuditLogConnectionEventsLimit(t *testing.T) {
635636

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

‎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)