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

Commit06cbb28

Browse files
authored
fix: expire token for prebuilds user when regenerating session token (#19667)
* 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
1 parenta2a758d commit06cbb28

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

1790+
func (q*querier)ExpirePrebuildsAPIKeys(ctx context.Context,now time.Time)error {
1791+
iferr:=q.authorizeContext(ctx,policy.ActionDelete,rbac.ResourceApiKey);err!=nil {
1792+
returnerr
1793+
}
1794+
returnq.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
returnq.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+
ifact,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
returninsert(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+
funcTestInsertAPIKey_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
returntemplate
158158
}
159159

160-
funcAPIKey(t testing.TB,db database.Store,seed database.APIKey) (key database.APIKey,tokenstring) {
160+
funcAPIKey(t testing.TB,db database.Store,seed database.APIKey,munge...func(*database.InsertAPIKeyParams)) (key database.APIKey,tokenstring) {
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:=rangemunge {
192+
fn(&params)
193+
}
194+
key,err:=db.InsertAPIKey(genCtx,params)
191195
require.NoError(t,err,"insert api key")
192196
returnkey,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 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
@@ -68,6 +68,9 @@ func New(ctx context.Context, logger slog.Logger, db database.Store, clk quartz.
6868
iferr:=tx.DeleteOldNotificationMessages(ctx);err!=nil {
6969
returnxerrors.Errorf("failed to delete old notification messages: %w",err)
7070
}
71+
iferr:=tx.ExpirePrebuildsAPIKeys(ctx,dbtime.Time(start));err!=nil {
72+
returnxerrors.Errorf("failed to expire prebuilds user api keys: %w",err)
73+
}
7174

7275
deleteOldAuditLogConnectionEventsBefore:=start.Add(-maxAuditLogConnectionEventAge)
7376
iferr:=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+
funcTestExpireOldAPIKeys(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,namestring) 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+
returnk
667+
}
668+
assertKeyActive=func(kidstring) {
669+
k,err:=db.GetAPIKeyByID(ctx,kid)
670+
require.NoError(t,err)
671+
assert.True(t,k.ExpiresAt.After(now))
672+
}
673+
assertKeyExpired=func(kidstring) {
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 aboutcustomizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp