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

Commitec66090

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

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

1728+
func (q*querier)ExpirePrebuildsAPIKeys(ctx context.Context,now time.Time)error {
1729+
iferr:=q.authorizeContext(ctx,policy.ActionDelete,rbac.ResourceApiKey);err!=nil {
1730+
returnerr
1731+
}
1732+
returnq.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
returnq.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+
ifact,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
returninsert(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+
funcTestInsertAPIKey_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
returntemplate
157157
}
158158

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

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

0 commit comments

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