Skip to content

Commit 496a096

Browse files
EmyrkEmyrk
committed
chore: check proper rbac perms on open file in cache
1 parent 5944b1c commit 496a096

File tree

6 files changed

+62
-14
lines changed

6 files changed

+62
-14
lines changed

‎coderd/coderd.go

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -572,10 +572,13 @@ 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),
576-
Experiments: experiments,
577-
WebpushDispatcher: options.WebPushDispatcher,
578-
healthCheckGroup: &singleflight.Group[string, *healthsdk.HealthcheckReport]{},
575+
FileCache: files.NewFromStore(options.Database, options.PrometheusRegistry, func(ctx context.Context, action policy.Action, object rbac.Object) error {
576+
subject := httpmw.UserAuthorizationCtx(ctx)
577+
return options.Authorizer.Authorize(ctx, subject, action, object)
578+
}),
579+
Experiments: experiments,
580+
WebpushDispatcher: options.WebPushDispatcher,
581+
healthCheckGroup: &singleflight.Group[string, *healthsdk.HealthcheckReport]{},
579582
Acquirer: provisionerdserver.NewAcquirer(
580583
ctx,
581584
options.Logger.Named("acquirer"),

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -432,6 +432,24 @@ var (
432432
}),
433433
Scope: rbac.ScopeAll,
434434
}.WithCachedASTValue()
435+
436+
subjectFileReader = rbac.Subject{
437+
Type: rbac.SubjectTypeFileReader,
438+
FriendlyName: "Can Read All Files",
439+
ID: uuid.Nil.String(),
440+
Roles: rbac.Roles([]rbac.Role{
441+
{
442+
Identifier: rbac.RoleIdentifier{Name: "file-reader"},
443+
DisplayName: "FileReader",
444+
Site: rbac.Permissions(map[string][]policy.Action{
445+
rbac.ResourceFile.Type: {policy.ActionRead},
446+
}),
447+
Org: map[string][]rbac.Permission{},
448+
User: []rbac.Permission{},
449+
},
450+
}),
451+
Scope: rbac.ScopeAll,
452+
}.WithCachedASTValue()
435453
)
436454

437455
// AsProvisionerd returns a context with an actor that has permissions required
@@ -498,6 +516,10 @@ func AsPrebuildsOrchestrator(ctx context.Context) context.Context {
498516
return As(ctx, subjectPrebuildsOrchestrator)
499517
}
500518

519+
func AsFileReader(ctx context.Context) context.Context {
520+
return As(ctx, subjectFileReader)
521+
}
522+
501523
var AsRemoveActor = rbac.Subject{
502524
ID: "remove-actor",
503525
}

‎coderd/files/cache.go

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,33 +13,42 @@ 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

22+
type AuthorizeFile func(ctx context.Context, action policy.Action, object rbac.Object) error
23+
1924
// NewFromStore returns a file cache that will fetch files from the provided
2025
// database.
21-
func NewFromStore(store database.Store, registerer prometheus.Registerer) *Cache {
26+
func NewFromStore(store database.Store, registerer prometheus.Registerer, authz AuthorizeFile) *Cache {
2227
fetch := func(ctx context.Context, fileID uuid.UUID) (cacheEntryValue, error) {
23-
file, err := store.GetFileByID(ctx, fileID)
28+
// Make sure the read does not fail due to authorization issues.
29+
// Authz is checked on the Acquire call, so this is safe.
30+
file, err := store.GetFileByID(dbauthz.AsFileReader(ctx), fileID)
2431
if err != nil {
2532
return cacheEntryValue{}, xerrors.Errorf("failed to read file from database: %w", err)
2633
}
2734

2835
content := bytes.NewBuffer(file.Data)
2936
return cacheEntryValue{
30-
FS: archivefs.FromTarReader(content),
31-
size: int64(content.Len()),
37+
object: rbac.ResourceFile.WithID(file.ID).WithOwner(file.CreatedBy.String()),
38+
FS: archivefs.FromTarReader(content),
39+
size: int64(content.Len()),
3240
}, nil
3341
}
3442

35-
return New(fetch, registerer)
43+
return New(fetch, registerer, authz)
3644
}
3745

38-
func New(fetch fetcher, registerer prometheus.Registerer) *Cache {
46+
func New(fetch fetcher, registerer prometheus.Registerer, authz AuthorizeFile) *Cache {
3947
return (&Cache{
4048
lock: sync.Mutex{},
4149
data: make(map[uuid.UUID]*cacheEntry),
4250
fetcher: fetch,
51+
authz: authz,
4352
}).registerMetrics(registerer)
4453
}
4554

@@ -101,6 +110,7 @@ type Cache struct {
101110
lock sync.Mutex
102111
data map[uuid.UUID]*cacheEntry
103112
fetcher
113+
authz AuthorizeFile
104114

105115
// metrics
106116
cacheMetrics
@@ -118,6 +128,7 @@ type cacheMetrics struct {
118128
}
119129

120130
type cacheEntryValue struct {
131+
object rbac.Object
121132
fs.FS
122133
size int64
123134
}
@@ -146,6 +157,13 @@ func (c *Cache) Acquire(ctx context.Context, fileID uuid.UUID) (fs.FS, error) {
146157
c.Release(fileID)
147158
return nil, err
148159
}
160+
161+
// Always check the caller can actually read the file.
162+
if c.authz(ctx, policy.ActionRead, it.object) != nil {
163+
c.Release(fileID)
164+
return nil, err
165+
}
166+
149167
return it.FS, err
150168
}
151169

‎coderd/httpmw/apikey.go

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,14 +47,18 @@ func APIKey(r *http.Request) database.APIKey {
4747

4848
// UserAuthorizationOptional may return the roles and scope used for
4949
// authorization. Depends on the ExtractAPIKey handler.
50-
func UserAuthorizationOptional(r *http.Request) (rbac.Subject, bool) {
51-
return dbauthz.ActorFromContext(r.Context())
50+
func UserAuthorizationOptional(ctx context.Context) (rbac.Subject, bool) {
51+
return dbauthz.ActorFromContext(ctx)
5252
}
5353

5454
// UserAuthorization returns the roles and scope used for authorization. Depends
5555
// on the ExtractAPIKey handler.
5656
func UserAuthorization(r *http.Request) rbac.Subject {
57-
auth, ok := UserAuthorizationOptional(r)
57+
return UserAuthorizationCtx(r.Context())
58+
}
59+
60+
func UserAuthorizationCtx(ctx context.Context) rbac.Subject {
61+
auth, ok := UserAuthorizationOptional(ctx)
5862
if !ok {
5963
panic("developer error: ExtractAPIKey middleware not provided")
6064
}

‎coderd/httpmw/apikey_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ func TestAPIKey(t *testing.T) {
5858
assert.NoError(t, err, "actor rego ok")
5959
}
6060

61-
auth, ok := httpmw.UserAuthorizationOptional(r)
61+
auth, ok := httpmw.UserAuthorizationOptional(r.Context())
6262
assert.True(t, ok, "httpmw auth ok")
6363
if ok {
6464
_, err := auth.Roles.Expand()

‎coderd/rbac/authz.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ const (
7474
SubjectTypeSystemRestricted SubjectType = "system_restricted"
7575
SubjectTypeNotifier SubjectType = "notifier"
7676
SubjectTypeSubAgentAPI SubjectType = "sub_agent_api"
77+
SubjectTypeFileReader SubjectType = "file_reader"
7778
)
7879

7980
// Subject is a struct that contains all the elements of a subject in an rbac

0 commit comments

Comments
 (0)