Skip to content

Commit 1d1070d

Browse files
EmyrkEmyrk
authored
chore: ensure proper rbac permissions on 'Acquire' file in the cache (#18348)
The file cache was caching the `Unauthorized` errors if a user without the 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 authz is checked on the Acquire per user.
1 parent d83706b commit 1d1070d

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
func AuthorizeFilter[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
if err != 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
if err != 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, objectType string) (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
if err != nil {
103103
return nil, 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
var params 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, id string, did ...ActionObjectPair) {
237241
r.Lock()
238242
defer r.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-
if call.Actor.ID == actor.ID {
249+
if call.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
return As(ctx, subjectPrebuildsOrchestrator)
499518
}
500519

520+
func AsFileReader(ctx context.Context) context.Context {
521+
return As(ctx, subjectFileReader)
522+
}
523+
501524
var AsRemoveActor = 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-
func NewFromStore(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+
func NewFromStore(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
if err != nil {
25-
return cacheEntryValue{}, xerrors.Errorf("failed to read file from database: %w", err)
31+
return CacheEntryValue{}, xerrors.Errorf("failed to read file from database: %w", err)
2632
}
2733

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

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

38-
func New(fetch fetcher, registerer prometheus.Registerer) *Cache {
45+
func New(fetch fetcher, 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
data map[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-
type cacheEntryValue struct {
129+
type CacheEntryValue struct {
121130
fs.FS
122-
size int64
131+
Object rbac.Object
132+
Size int64
123133
}
124134

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

131-
type fetcher func(context.Context, uuid.UUID) (cacheEntryValue, error)
141+
type fetcher func(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
return nil, err
148158
}
159+
160+
subject, ok := dbauthz.ActorFromContext(ctx)
161+
if !ok {
162+
return nil, dbauthz.ErrNoActor
163+
}
164+
// Always check the caller can actually read the file.
165+
if err := c.authz.Authorize(ctx, subject, policy.ActionRead, it.Object); err != nil {
166+
c.Release(fileID)
167+
return nil, err
168+
}
169+
149170
return it.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
defer c.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
if err == 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
return val, err
@@ -206,7 +227,7 @@ func (c *Cache) Release(fileID uuid.UUID) {
206227

207228
ev, err := entry.value.Load()
208229
if err == 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)