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

Commit1d1070d

Browse files
authored
chore: ensure proper rbac permissions on 'Acquire' file in the cache (#18348)
The file cache was caching the `Unauthorized` errors if a user withoutthe right perms opened the file first. So all future opens would fail.Now the cache always opens with a subject that can read files. And authzis checked on the Acquire per user.
1 parentd83706b commit1d1070d

File tree

16 files changed

+218
-51
lines changed

16 files changed

+218
-51
lines changed

‎coderd/authorize.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ import (
1919
// objects that the user is authorized to perform the given action on.
2020
// This is faster than calling Authorize() on each object.
2121
funcAuthorizeFilter[O rbac.Objecter](h*HTTPAuthorizer,r*http.Request,action policy.Action,objects []O) ([]O,error) {
22-
roles:=httpmw.UserAuthorization(r)
22+
roles:=httpmw.UserAuthorization(r.Context())
2323
objects,err:=rbac.Filter(r.Context(),h.Authorizer,roles,action,objects)
2424
iferr!=nil {
2525
// Log the error as Filter should not be erroring.
@@ -65,7 +65,7 @@ func (api *API) Authorize(r *http.Request, action policy.Action, object rbac.Obj
6565
//return
6666
//}
6767
func (h*HTTPAuthorizer)Authorize(r*http.Request,action policy.Action,object rbac.Objecter)bool {
68-
roles:=httpmw.UserAuthorization(r)
68+
roles:=httpmw.UserAuthorization(r.Context())
6969
err:=h.Authorizer.Authorize(r.Context(),roles,action,object.RBACObject())
7070
iferr!=nil {
7171
// Log the errors for debugging
@@ -97,7 +97,7 @@ func (h *HTTPAuthorizer) Authorize(r *http.Request, action policy.Action, object
9797
// call 'Authorize()' on the returned objects.
9898
// Note the authorization is only for the given action and object type.
9999
func (h*HTTPAuthorizer)AuthorizeSQLFilter(r*http.Request,action policy.Action,objectTypestring) (rbac.PreparedAuthorized,error) {
100-
roles:=httpmw.UserAuthorization(r)
100+
roles:=httpmw.UserAuthorization(r.Context())
101101
prepared,err:=h.Authorizer.Prepare(r.Context(),roles,action,objectType)
102102
iferr!=nil {
103103
returnnil,xerrors.Errorf("prepare filter: %w",err)
@@ -120,7 +120,7 @@ func (h *HTTPAuthorizer) AuthorizeSQLFilter(r *http.Request, action policy.Actio
120120
// @Router /authcheck [post]
121121
func (api*API)checkAuthorization(rw http.ResponseWriter,r*http.Request) {
122122
ctx:=r.Context()
123-
auth:=httpmw.UserAuthorization(r)
123+
auth:=httpmw.UserAuthorization(r.Context())
124124

125125
varparams codersdk.AuthorizationRequest
126126
if!httpapi.Read(ctx,rw,r,&params) {

‎coderd/coderd.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -572,7 +572,7 @@ func New(options *Options) *API {
572572
TemplateScheduleStore:options.TemplateScheduleStore,
573573
UserQuietHoursScheduleStore:options.UserQuietHoursScheduleStore,
574574
AccessControlStore:options.AccessControlStore,
575-
FileCache:files.NewFromStore(options.Database,options.PrometheusRegistry),
575+
FileCache:files.NewFromStore(options.Database,options.PrometheusRegistry,options.Authorizer),
576576
Experiments:experiments,
577577
WebpushDispatcher:options.WebPushDispatcher,
578578
healthCheckGroup:&singleflight.Group[string,*healthsdk.HealthcheckReport]{},

‎coderd/coderdtest/authorize.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,10 @@ func (r *RecordingAuthorizer) AssertOutOfOrder(t *testing.T, actor rbac.Subject,
234234
// AssertActor asserts in order. If the order of authz calls does not match,
235235
// this will fail.
236236
func (r*RecordingAuthorizer)AssertActor(t*testing.T,actor rbac.Subject,did...ActionObjectPair) {
237+
r.AssertActorID(t,actor.ID,did...)
238+
}
239+
240+
func (r*RecordingAuthorizer)AssertActorID(t*testing.T,idstring,did...ActionObjectPair) {
237241
r.Lock()
238242
deferr.Unlock()
239243
ptr:=0
@@ -242,7 +246,7 @@ func (r *RecordingAuthorizer) AssertActor(t *testing.T, actor rbac.Subject, did
242246
// Finished all assertions
243247
return
244248
}
245-
ifcall.Actor.ID==actor.ID {
249+
ifcall.Actor.ID==id {
246250
action,object:=did[ptr].Action,did[ptr].Object
247251
assert.Equalf(t,action,call.Action,"assert action %d",ptr)
248252
assert.Equalf(t,object,call.Object,"assert object %d",ptr)

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -432,6 +432,25 @@ var (
432432
}),
433433
Scope:rbac.ScopeAll,
434434
}.WithCachedASTValue()
435+
436+
subjectFileReader= rbac.Subject{
437+
Type:rbac.SubjectTypeFileReader,
438+
FriendlyName:"Can Read All Files",
439+
// Arbitrary uuid to have a unique ID for this subject.
440+
ID:rbac.SubjectTypeFileReaderID,
441+
Roles:rbac.Roles([]rbac.Role{
442+
{
443+
Identifier: rbac.RoleIdentifier{Name:"file-reader"},
444+
DisplayName:"FileReader",
445+
Site:rbac.Permissions(map[string][]policy.Action{
446+
rbac.ResourceFile.Type: {policy.ActionRead},
447+
}),
448+
Org:map[string][]rbac.Permission{},
449+
User: []rbac.Permission{},
450+
},
451+
}),
452+
Scope:rbac.ScopeAll,
453+
}.WithCachedASTValue()
435454
)
436455

437456
// AsProvisionerd returns a context with an actor that has permissions required
@@ -498,6 +517,10 @@ func AsPrebuildsOrchestrator(ctx context.Context) context.Context {
498517
returnAs(ctx,subjectPrebuildsOrchestrator)
499518
}
500519

520+
funcAsFileReader(ctx context.Context) context.Context {
521+
returnAs(ctx,subjectFileReader)
522+
}
523+
501524
varAsRemoveActor= rbac.Subject{
502525
ID:"remove-actor",
503526
}

‎coderd/files/cache.go

Lines changed: 39 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -13,33 +13,41 @@ import (
1313

1414
archivefs"github.com/coder/coder/v2/archive/fs"
1515
"github.com/coder/coder/v2/coderd/database"
16+
"github.com/coder/coder/v2/coderd/database/dbauthz"
17+
"github.com/coder/coder/v2/coderd/rbac"
18+
"github.com/coder/coder/v2/coderd/rbac/policy"
1619
"github.com/coder/coder/v2/coderd/util/lazy"
1720
)
1821

1922
// NewFromStore returns a file cache that will fetch files from the provided
2023
// database.
21-
funcNewFromStore(store database.Store,registerer prometheus.Registerer)*Cache {
22-
fetch:=func(ctx context.Context,fileID uuid.UUID) (cacheEntryValue,error) {
23-
file,err:=store.GetFileByID(ctx,fileID)
24+
funcNewFromStore(store database.Store,registerer prometheus.Registerer,authz rbac.Authorizer)*Cache {
25+
fetch:=func(ctx context.Context,fileID uuid.UUID) (CacheEntryValue,error) {
26+
// Make sure the read does not fail due to authorization issues.
27+
// Authz is checked on the Acquire call, so this is safe.
28+
//nolint:gocritic
29+
file,err:=store.GetFileByID(dbauthz.AsFileReader(ctx),fileID)
2430
iferr!=nil {
25-
returncacheEntryValue{},xerrors.Errorf("failed to read file from database: %w",err)
31+
returnCacheEntryValue{},xerrors.Errorf("failed to read file from database: %w",err)
2632
}
2733

2834
content:=bytes.NewBuffer(file.Data)
29-
returncacheEntryValue{
30-
FS:archivefs.FromTarReader(content),
31-
size:int64(content.Len()),
35+
returnCacheEntryValue{
36+
Object:file.RBACObject(),
37+
FS:archivefs.FromTarReader(content),
38+
Size:int64(content.Len()),
3239
},nil
3340
}
3441

35-
returnNew(fetch,registerer)
42+
returnNew(fetch,registerer,authz)
3643
}
3744

38-
funcNew(fetchfetcher,registerer prometheus.Registerer)*Cache {
45+
funcNew(fetchfetcher,registerer prometheus.Registerer,authz rbac.Authorizer)*Cache {
3946
return (&Cache{
4047
lock: sync.Mutex{},
4148
data:make(map[uuid.UUID]*cacheEntry),
4249
fetcher:fetch,
50+
authz:authz,
4351
}).registerMetrics(registerer)
4452
}
4553

@@ -101,6 +109,7 @@ type Cache struct {
101109
lock sync.Mutex
102110
datamap[uuid.UUID]*cacheEntry
103111
fetcher
112+
authz rbac.Authorizer
104113

105114
// metrics
106115
cacheMetrics
@@ -117,18 +126,19 @@ type cacheMetrics struct {
117126
totalCacheSize prometheus.Counter
118127
}
119128

120-
typecacheEntryValuestruct {
129+
typeCacheEntryValuestruct {
121130
fs.FS
122-
sizeint64
131+
Object rbac.Object
132+
Sizeint64
123133
}
124134

125135
typecacheEntrystruct {
126136
// refCount must only be accessed while the Cache lock is held.
127137
refCountint
128-
value*lazy.ValueWithError[cacheEntryValue]
138+
value*lazy.ValueWithError[CacheEntryValue]
129139
}
130140

131-
typefetcherfunc(context.Context, uuid.UUID) (cacheEntryValue,error)
141+
typefetcherfunc(context.Context, uuid.UUID) (CacheEntryValue,error)
132142

133143
// Acquire will load the fs.FS for the given file. It guarantees that parallel
134144
// calls for the same fileID will only result in one fetch, and that parallel
@@ -146,22 +156,33 @@ func (c *Cache) Acquire(ctx context.Context, fileID uuid.UUID) (fs.FS, error) {
146156
c.Release(fileID)
147157
returnnil,err
148158
}
159+
160+
subject,ok:=dbauthz.ActorFromContext(ctx)
161+
if!ok {
162+
returnnil,dbauthz.ErrNoActor
163+
}
164+
// Always check the caller can actually read the file.
165+
iferr:=c.authz.Authorize(ctx,subject,policy.ActionRead,it.Object);err!=nil {
166+
c.Release(fileID)
167+
returnnil,err
168+
}
169+
149170
returnit.FS,err
150171
}
151172

152-
func (c*Cache)prepare(ctx context.Context,fileID uuid.UUID)*lazy.ValueWithError[cacheEntryValue] {
173+
func (c*Cache)prepare(ctx context.Context,fileID uuid.UUID)*lazy.ValueWithError[CacheEntryValue] {
153174
c.lock.Lock()
154175
deferc.lock.Unlock()
155176

156177
entry,ok:=c.data[fileID]
157178
if!ok {
158-
value:=lazy.NewWithError(func() (cacheEntryValue,error) {
179+
value:=lazy.NewWithError(func() (CacheEntryValue,error) {
159180
val,err:=c.fetcher(ctx,fileID)
160181

161182
// Always add to the cache size the bytes of the file loaded.
162183
iferr==nil {
163-
c.currentCacheSize.Add(float64(val.size))
164-
c.totalCacheSize.Add(float64(val.size))
184+
c.currentCacheSize.Add(float64(val.Size))
185+
c.totalCacheSize.Add(float64(val.Size))
165186
}
166187

167188
returnval,err
@@ -206,7 +227,7 @@ func (c *Cache) Release(fileID uuid.UUID) {
206227

207228
ev,err:=entry.value.Load()
208229
iferr==nil {
209-
c.currentCacheSize.Add(-1*float64(ev.size))
230+
c.currentCacheSize.Add(-1*float64(ev.Size))
210231
}
211232

212233
delete(c.data,fileID)

0 commit comments

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