Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit20d67d7

Browse files
authored
fix: expire token for prebuilds user when regenerating session token (#19667) (#19669)
* provisionerdserver: Expires prebuild user token for workspace, if itexists, when regenerating session token.* dbauthz: disallow prebuilds user from creating api keys* dbpurge: added functionality to expire stale api keys owned by theprebuilds user(cherry picked from commit06cbb28)
1 parentdd62ec4 commit20d67d7

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+
ifuser.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
ifscope!="" {
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+
ifuser.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+
funcTestAPIKey_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
returnq.db.EnqueueNotificationMessage(ctx,arg)
16531653
}
16541654

1655+
func (q*querier)ExpirePrebuildsAPIKeys(ctx context.Context,now time.Time)error {
1656+
iferr:=q.authorizeContext(ctx,policy.ActionDelete,rbac.ResourceApiKey);err!=nil {
1657+
returnerr
1658+
}
1659+
returnq.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
returnq.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+
ifact,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
returninsert(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+
funcTestInsertAPIKey_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
returntemplate
109109
}
110110

111-
funcAPIKey(t testing.TB,db database.Store,seed database.APIKey) (key database.APIKey,tokenstring) {
111+
funcAPIKey(t testing.TB,db database.Store,seed database.APIKey,munge...func(*database.InsertAPIKeyParams)) (key database.APIKey,tokenstring) {
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:=rangemunge {
143+
fn(&params)
144+
}
145+
key,err:=db.InsertAPIKey(genCtx,params)
142146
require.NoError(t,err,"insert api key")
143147
returnkey,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
returnerr
25982598
}
25992599

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

‎coderd/database/dbmetrics/querymetrics.go‎

Lines changed: 7 additions & 0 deletions
Some generated files are not rendered by default. Learn more aboutcustomizing 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 aboutcustomizing 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
iferr:=tx.DeleteOldNotificationMessages(ctx);err!=nil {
6363
returnxerrors.Errorf("failed to delete old notification messages: %w",err)
6464
}
65+
iferr:=tx.ExpirePrebuildsAPIKeys(ctx,dbtime.Time(start));err!=nil {
66+
returnxerrors.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
funcTestPurge(t*testing.T) {
44+
if!dbtestutil.WillUsePostgres() {
45+
t.Skip("requires postgres")
46+
}
4347
ctx,cancel:=context.WithTimeout(context.Background(),testutil.WaitShort)
4448
defercancel()
4549

@@ -490,3 +494,71 @@ func containsProvisionerDaemon(daemons []database.ProvisionerDaemon, name string
490494
returnd.Name==name
491495
})
492496
}
497+
498+
funcTestExpireOldAPIKeys(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,namestring) 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+
returnk
525+
}
526+
assertKeyActive=func(kidstring) {
527+
k,err:=db.GetAPIKeyByID(ctx,kid)
528+
require.NoError(t,err)
529+
assert.True(t,k.ExpiresAt.After(now))
530+
}
531+
assertKeyExpired=func(kidstring) {
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)
[8]ページ先頭
©2009-2025 Movatter.jp