From 038ad91c0f41908c27b6c1b21ff9e2d7d65243ee Mon Sep 17 00:00:00 2001 From: McKayla Washburn Date: Wed, 9 Apr 2025 21:04:53 +0000 Subject: [PATCH 1/6] add x-authz-checks header --- coderd/coderd.go | 11 +++- coderd/httpapi/httpapi.go | 21 ++++++++ coderd/httpmw/authz.go | 13 +++++ coderd/rbac/authz.go | 104 +++++++++++++++++++++++++++++++++++-- coderd/util/syncmap/map.go | 11 +++- 5 files changed, 155 insertions(+), 5 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index ff566ed369a15..536911ad1ad08 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -315,6 +315,9 @@ func New(options *Options) *API { if options.Authorizer == nil { options.Authorizer = rbac.NewCachingAuthorizer(options.PrometheusRegistry) } + if buildinfo.IsDev() { + options.Authorizer = rbac.Recorder(options.Authorizer) + } if options.AccessControlStore == nil { options.AccessControlStore = &atomic.Pointer[dbauthz.AccessControlStore]{} @@ -456,8 +459,14 @@ func New(options *Options) *API { options.NotificationsEnqueuer = notifications.NewNoopEnqueuer() } - ctx, cancel := context.WithCancel(context.Background()) r := chi.NewRouter() + // We add this middleware early, to make sure that authorization checks made + // by other middleware get recorded. + if buildinfo.IsDev() { + r.Use(httpmw.RecordAuthzChecks) + } + + ctx, cancel := context.WithCancel(context.Background()) // nolint:gocritic // Load deployment ID. This never changes depID, err := options.Database.GetDeploymentID(dbauthz.AsSystemRestricted(ctx)) diff --git a/coderd/httpapi/httpapi.go b/coderd/httpapi/httpapi.go index c70290ffe56b0..297a0c2153870 100644 --- a/coderd/httpapi/httpapi.go +++ b/coderd/httpapi/httpapi.go @@ -20,6 +20,7 @@ import ( "github.com/coder/websocket/wsjson" "github.com/coder/coder/v2/coderd/httpapi/httpapiconstraints" + "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/tracing" "github.com/coder/coder/v2/codersdk" ) @@ -198,6 +199,22 @@ func Write(ctx context.Context, rw http.ResponseWriter, status int, response int _, span := tracing.StartSpan(ctx) defer span.End() + if rec, ok := rbac.GetAuthzCheckRecorder(ctx); ok { + // If you're here because you saw this header in a response, and you're + // trying to investigate the code, here are a couple of notable things + // for you to know: + // - If any of the checks are `false`, they might not represent the whole + // picture. There could be additional checks that weren't performed, + // because processing stopped after the failure. + // - The checks are recorded by the `authzRecorder` type, which is + // configured on server startup for development and testing builds. + // - If this header is missing from a response, make sure the response is + // being written by calling `httpapi.Write`! + // - An empty x-authz-checks header can be valid! Some requests don't + // require authorization. + rw.Header().Set("x-authz-checks", rec.String()) + } + rw.Header().Set("Content-Type", "application/json; charset=utf-8") rw.WriteHeader(status) @@ -213,6 +230,10 @@ func WriteIndent(ctx context.Context, rw http.ResponseWriter, status int, respon _, span := tracing.StartSpan(ctx) defer span.End() + if rec, ok := rbac.GetAuthzCheckRecorder(ctx); ok { + rw.Header().Set("x-dbauthz-checks", rec.String()) + } + rw.Header().Set("Content-Type", "application/json; charset=utf-8") rw.WriteHeader(status) diff --git a/coderd/httpmw/authz.go b/coderd/httpmw/authz.go index 4c94ce362be2a..53aadb6cb7a57 100644 --- a/coderd/httpmw/authz.go +++ b/coderd/httpmw/authz.go @@ -6,6 +6,7 @@ import ( "github.com/go-chi/chi/v5" "github.com/coder/coder/v2/coderd/database/dbauthz" + "github.com/coder/coder/v2/coderd/rbac" ) // AsAuthzSystem is a chained handler that temporarily sets the dbauthz context @@ -35,3 +36,15 @@ func AsAuthzSystem(mws ...func(http.Handler) http.Handler) func(http.Handler) ht }) } } + +// RecordAuthzChecks enables recording all of the authorization checks that +// occurred in the processing of a request. This is mostly helpful for debugging +// and understanding what permissions are required for a given action. +// +// Requires using a Recorder Authorizer. +func RecordAuthzChecks(next http.Handler) http.Handler { + return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + r = r.WithContext(rbac.WithAuthzCheckRecorder(r.Context())) + next.ServeHTTP(rw, r) + }) +} diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index aaba7d6eae3af..417b3b04a0f5a 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -6,6 +6,7 @@ import ( _ "embed" "encoding/json" "errors" + "fmt" "strings" "sync" "time" @@ -23,7 +24,9 @@ import ( "github.com/coder/coder/v2/coderd/rbac/regosql" "github.com/coder/coder/v2/coderd/rbac/regosql/sqltypes" "github.com/coder/coder/v2/coderd/tracing" + "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/coderd/util/slice" + "github.com/coder/coder/v2/coderd/util/syncmap" ) type AuthCall struct { @@ -362,11 +365,11 @@ func (a RegoAuthorizer) Authorize(ctx context.Context, subject Subject, action p defer span.End() err := a.authorize(ctx, subject, action, object) - - span.SetAttributes(attribute.Bool("authorized", err == nil)) + authorized := err == nil + span.SetAttributes(attribute.Bool("authorized", authorized)) dur := time.Since(start) - if err != nil { + if !authorized { a.authorizeHist.WithLabelValues("false").Observe(dur.Seconds()) return err } @@ -741,3 +744,98 @@ func rbacTraceAttributes(actor Subject, action policy.Action, objectType string, attribute.String("object_type", objectType), )...) } + +type authRecorder struct { + authz Authorizer +} + +// Recorder returns an Authorizer that records any authorization checks made +// on the Context provided for the authorization check. +// +// Requires using the RecordAuthzChecks middleware. +func Recorder(authz Authorizer) Authorizer { + return &authRecorder{authz: authz} +} + +func (c *authRecorder) Authorize(ctx context.Context, subject Subject, action policy.Action, object Object) error { + err := c.authz.Authorize(ctx, subject, action, object) + authorized := err == nil + recordAuthzCheck(ctx, action, object, authorized) + return err +} + +func (c *authRecorder) Prepare(ctx context.Context, subject Subject, action policy.Action, objectType string) (PreparedAuthorized, error) { + return c.authz.Prepare(ctx, subject, action, objectType) +} + +type authzCheckRecorderKey struct{} + +func WithAuthzCheckRecorder(ctx context.Context) context.Context { + return context.WithValue(ctx, authzCheckRecorderKey{}, ptr.Ref(AuthzCheckRecorder{ + checks: syncmap.Map[string, bool]{}, + })) +} + +type AuthzCheckRecorder struct { + // Checks is a map from preformatted authz check IDs to their authorization + // status (true => authorized, false => not authorized) + checks syncmap.Map[string, bool] +} + +func recordAuthzCheck(ctx context.Context, action policy.Action, object Object, authorized bool) { + r, ok := ctx.Value(authzCheckRecorderKey{}).(*AuthzCheckRecorder) + if !ok { + return + } + + // We serialize the check using the following syntax + var b strings.Builder + if object.OrgID != "" { + _, err := fmt.Fprintf(&b, "organization:%v::", object.OrgID) + if err != nil { + return + } + } + if object.AnyOrgOwner { + _, err := fmt.Fprint(&b, "organization:any::") + if err != nil { + return + } + } + if object.Owner != "" { + _, err := fmt.Fprintf(&b, "owner:%v::", object.Owner) + if err != nil { + return + } + } + if object.ID != "" { + _, err := fmt.Fprintf(&b, "id:%v::", object.ID) + if err != nil { + return + } + } + _, err := fmt.Fprintf(&b, "%v.%v", object.RBACObject().Type, action) + if err != nil { + return + } + + r.checks.Store(b.String(), authorized) +} + +func GetAuthzCheckRecorder(ctx context.Context) (*AuthzCheckRecorder, bool) { + checks, ok := ctx.Value(authzCheckRecorderKey{}).(*AuthzCheckRecorder) + if !ok { + return nil, false + } + + return checks, true +} + +// String serializes all of the checks recorded, using the following syntax: +func (r *AuthzCheckRecorder) String() string { + checks := make([]string, 0) + for check, result := range r.checks.Seq() { + checks = append(checks, fmt.Sprintf("%v=%v", check, result)) + } + return strings.Join(checks, "; ") +} diff --git a/coderd/util/syncmap/map.go b/coderd/util/syncmap/map.go index 178aa3e4f6fd0..df7102fdf2afa 100644 --- a/coderd/util/syncmap/map.go +++ b/coderd/util/syncmap/map.go @@ -1,6 +1,9 @@ package syncmap -import "sync" +import ( + "iter" + "sync" +) // Map is a type safe sync.Map type Map[K, V any] struct { @@ -75,3 +78,9 @@ func (m *Map[K, V]) Range(f func(key K, value V) bool) { return f(key.(K), value.(V)) }) } + +func (m *Map[K, V]) Seq() iter.Seq2[K, V] { + return func(yield func(K, V) bool) { + m.Range(yield) + } +} From 132ed27fc36d0329ee2b80118b24ccba794b9993 Mon Sep 17 00:00:00 2001 From: McKayla Washburn Date: Wed, 9 Apr 2025 21:10:33 +0000 Subject: [PATCH 2/6] fix /users endpoint --- coderd/users.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/coderd/users.go b/coderd/users.go index 9b6407156cfa1..d97abc82b2fd1 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -9,7 +9,6 @@ import ( "slices" "github.com/go-chi/chi/v5" - "github.com/go-chi/render" "github.com/google/uuid" "golang.org/x/xerrors" @@ -273,8 +272,7 @@ func (api *API) users(rw http.ResponseWriter, r *http.Request) { organizationIDsByUserID[organizationIDsByMemberIDsRow.UserID] = organizationIDsByMemberIDsRow.OrganizationIDs } - render.Status(r, http.StatusOK) - render.JSON(rw, r, codersdk.GetUsersResponse{ + httpapi.Write(ctx, rw, http.StatusOK, codersdk.GetUsersResponse{ Users: convertUsers(users, organizationIDsByUserID), Count: int(userCount), }) From 3d16842ecdd5fadbb7faf1befce4f7fdac7dae4e Mon Sep 17 00:00:00 2001 From: McKayla Washburn Date: Wed, 9 Apr 2025 21:40:57 +0000 Subject: [PATCH 3/6] update go.mod --- go.mod | 1 - go.sum | 2 -- 2 files changed, 3 deletions(-) diff --git a/go.mod b/go.mod index 7421d224d7c5d..56fdd053f407e 100644 --- a/go.mod +++ b/go.mod @@ -116,7 +116,6 @@ require ( github.com/go-chi/chi/v5 v5.1.0 github.com/go-chi/cors v1.2.1 github.com/go-chi/httprate v0.15.0 - github.com/go-chi/render v1.0.1 github.com/go-jose/go-jose/v4 v4.0.5 github.com/go-logr/logr v1.4.2 github.com/go-playground/validator/v10 v10.26.0 diff --git a/go.sum b/go.sum index 197ae825a2c5f..ca3e4d2caedf3 100644 --- a/go.sum +++ b/go.sum @@ -367,8 +367,6 @@ github.com/go-chi/hostrouter v0.2.0 h1:GwC7TZz8+SlJN/tV/aeJgx4F+mI5+sp+5H1PelQUj github.com/go-chi/hostrouter v0.2.0/go.mod h1:pJ49vWVmtsKRKZivQx0YMYv4h0aX+Gcn6V23Np9Wf1s= github.com/go-chi/httprate v0.15.0 h1:j54xcWV9KGmPf/X4H32/aTH+wBlrvxL7P+SdnRqxh5g= github.com/go-chi/httprate v0.15.0/go.mod h1:rzGHhVrsBn3IMLYDOZQsSU4fJNWcjui4fWKJcCId1R4= -github.com/go-chi/render v1.0.1 h1:4/5tis2cKaNdnv9zFLfXzcquC9HbeZgCnxGnKrltBS8= -github.com/go-chi/render v1.0.1/go.mod h1:pq4Rr7HbnsdaeHagklXub+p6Wd16Af5l9koip1OvJns= github.com/go-ini/ini v1.67.0 h1:z6ZrTEZqSWOTyH2FlglNbNgARyHG8oLW9gMELqKr06A= github.com/go-ini/ini v1.67.0/go.mod h1:ByCAeIL28uOIIG0E3PJtZPDL8WnHpFKFOtgjp+3Ies8= github.com/go-jose/go-jose/v4 v4.0.5 h1:M6T8+mKZl/+fNNuFHvGIzDz7BTLQPIounk/b9dw3AaE= From d59ecbe8592b950f33ac101fe87974236b54c71c Mon Sep 17 00:00:00 2001 From: McKayla Washburn Date: Wed, 9 Apr 2025 22:11:03 +0000 Subject: [PATCH 4/6] fixem --- coderd/coderd.go | 6 +++--- enterprise/coderd/coderd.go | 3 +++ 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 536911ad1ad08..0434b9d9a17c4 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -314,9 +314,9 @@ func New(options *Options) *API { if options.Authorizer == nil { options.Authorizer = rbac.NewCachingAuthorizer(options.PrometheusRegistry) - } - if buildinfo.IsDev() { - options.Authorizer = rbac.Recorder(options.Authorizer) + if buildinfo.IsDev() { + options.Authorizer = rbac.Recorder(options.Authorizer) + } } if options.AccessControlStore == nil { diff --git a/enterprise/coderd/coderd.go b/enterprise/coderd/coderd.go index cb2a342fb1c8a..c451e71fc445e 100644 --- a/enterprise/coderd/coderd.go +++ b/enterprise/coderd/coderd.go @@ -71,6 +71,9 @@ func New(ctx context.Context, options *Options) (_ *API, err error) { } if options.Options.Authorizer == nil { options.Options.Authorizer = rbac.NewCachingAuthorizer(options.PrometheusRegistry) + if buildinfo.IsDev() { + options.Authorizer = rbac.Recorder(options.Authorizer) + } } if options.ReplicaErrorGracePeriod == 0 { // This will prevent the error from being shown for a minute From fe0e466befa9feebc351ef95a5d62fe946aca72a Mon Sep 17 00:00:00 2001 From: McKayla Washburn Date: Thu, 10 Apr 2025 16:49:57 +0000 Subject: [PATCH 5/6] =?UTF-8?q?=F0=9F=A7=B9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- coderd/httpapi/httpapi.go | 4 +--- coderd/rbac/authz.go | 38 +++++++++++++++++++++++++------------- coderd/util/syncmap/map.go | 7 ------- 3 files changed, 26 insertions(+), 23 deletions(-) diff --git a/coderd/httpapi/httpapi.go b/coderd/httpapi/httpapi.go index 297a0c2153870..5c5c623474a47 100644 --- a/coderd/httpapi/httpapi.go +++ b/coderd/httpapi/httpapi.go @@ -210,8 +210,6 @@ func Write(ctx context.Context, rw http.ResponseWriter, status int, response int // configured on server startup for development and testing builds. // - If this header is missing from a response, make sure the response is // being written by calling `httpapi.Write`! - // - An empty x-authz-checks header can be valid! Some requests don't - // require authorization. rw.Header().Set("x-authz-checks", rec.String()) } @@ -231,7 +229,7 @@ func WriteIndent(ctx context.Context, rw http.ResponseWriter, status int, respon defer span.End() if rec, ok := rbac.GetAuthzCheckRecorder(ctx); ok { - rw.Header().Set("x-dbauthz-checks", rec.String()) + rw.Header().Set("x-authz-checks", rec.String()) } rw.Header().Set("Content-Type", "application/json; charset=utf-8") diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index 417b3b04a0f5a..f3abf884e9f48 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -24,9 +24,7 @@ import ( "github.com/coder/coder/v2/coderd/rbac/regosql" "github.com/coder/coder/v2/coderd/rbac/regosql/sqltypes" "github.com/coder/coder/v2/coderd/tracing" - "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/coderd/util/slice" - "github.com/coder/coder/v2/coderd/util/syncmap" ) type AuthCall struct { @@ -770,16 +768,21 @@ func (c *authRecorder) Prepare(ctx context.Context, subject Subject, action poli type authzCheckRecorderKey struct{} -func WithAuthzCheckRecorder(ctx context.Context) context.Context { - return context.WithValue(ctx, authzCheckRecorderKey{}, ptr.Ref(AuthzCheckRecorder{ - checks: syncmap.Map[string, bool]{}, - })) +type AuthzCheckRecorder struct { + // lock guards checks + lock sync.Mutex + // checks is a list preformatted authz check IDs and their result + checks []recordedCheck } -type AuthzCheckRecorder struct { - // Checks is a map from preformatted authz check IDs to their authorization - // status (true => authorized, false => not authorized) - checks syncmap.Map[string, bool] +type recordedCheck struct { + name string + // true => authorized, false => not authorized + result bool +} + +func WithAuthzCheckRecorder(ctx context.Context) context.Context { + return context.WithValue(ctx, authzCheckRecorderKey{}, &AuthzCheckRecorder{}) } func recordAuthzCheck(ctx context.Context, action policy.Action, object Object, authorized bool) { @@ -819,7 +822,9 @@ func recordAuthzCheck(ctx context.Context, action policy.Action, object Object, return } - r.checks.Store(b.String(), authorized) + r.lock.Lock() + defer r.lock.Unlock() + r.checks = append(r.checks, recordedCheck{name: b.String(), result: authorized}) } func GetAuthzCheckRecorder(ctx context.Context) (*AuthzCheckRecorder, bool) { @@ -833,8 +838,15 @@ func GetAuthzCheckRecorder(ctx context.Context) (*AuthzCheckRecorder, bool) { // String serializes all of the checks recorded, using the following syntax: func (r *AuthzCheckRecorder) String() string { - checks := make([]string, 0) - for check, result := range r.checks.Seq() { + r.lock.Lock() + defer r.lock.Unlock() + + if len(r.checks) == 0 { + return "nil" + } + + checks := make([]string, 0, len(r.checks)) + for check, result := range r.checks { checks = append(checks, fmt.Sprintf("%v=%v", check, result)) } return strings.Join(checks, "; ") diff --git a/coderd/util/syncmap/map.go b/coderd/util/syncmap/map.go index df7102fdf2afa..f35973ea42690 100644 --- a/coderd/util/syncmap/map.go +++ b/coderd/util/syncmap/map.go @@ -1,7 +1,6 @@ package syncmap import ( - "iter" "sync" ) @@ -78,9 +77,3 @@ func (m *Map[K, V]) Range(f func(key K, value V) bool) { return f(key.(K), value.(V)) }) } - -func (m *Map[K, V]) Seq() iter.Seq2[K, V] { - return func(yield func(K, V) bool) { - m.Range(yield) - } -} From f2682275b500a2dc88b51b8c348a32bce3e79581 Mon Sep 17 00:00:00 2001 From: McKayla Washburn Date: Thu, 10 Apr 2025 16:57:46 +0000 Subject: [PATCH 6/6] fix formatting --- coderd/rbac/authz.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index f3abf884e9f48..3239ea3c42dc5 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -846,8 +846,8 @@ func (r *AuthzCheckRecorder) String() string { } checks := make([]string, 0, len(r.checks)) - for check, result := range r.checks { - checks = append(checks, fmt.Sprintf("%v=%v", check, result)) + for _, check := range r.checks { + checks = append(checks, fmt.Sprintf("%v=%v", check.name, check.result)) } return strings.Join(checks, "; ") }