From d29b7b26cd09a0414f7b947d2b8bb1c3f0c37c36 Mon Sep 17 00:00:00 2001 From: Milan Freml Date: Thu, 29 Sep 2022 14:13:16 +0200 Subject: [PATCH 1/3] [feat] Add access token scopes authorization based on graphql directives This only works on token based authorization, when using non-internal tokens. Existing functionality was kept backwards compatible on this POC, so if you have a token with "user:all" or "site-admin:sudo" everything works like before. Similarly session based auth still works the same. Added a directive `@authz(scopes: ["some_scope"])` which controls scopes that are required when calling the graphql API with a token based authentication. When this directive is present on a field, query, mutation or type, it is required that the token has scopes that are listed. The beauty of this approach is, that we can add different scopes to different queries/fields/mutations and single source of truth is our graphql schema. - unit tests - failing authorization on queries/mutations/fields without the directive - adding scopes to all needed graphql entities - UI changes to create tokens with more scopes - making backwards incompatible changes - authorizing internal API access Tested locally. To test locally, you need to modify `go.mod` to point to your own local fork of graph-gophers/graphql-go#446 It is also needed to apply the patch suggested in this comment: https://github.com/graph-gophers/graphql-go/pull/446/files#r914374506 To create a token with different scopes, create a normal token as you would usually by going to Settings -> Access tokens. You need to modify the scopes directly in the database (yikes!). Search the `schema.graphql` file for `@authz` directive scope definitions that are required with these changes. You then need to use the token directly with curl or similar. When using the token without proper scopes, you should see graphql errors instead of data. --- cmd/frontend/graphqlbackend/graphqlbackend.go | 31 ++++++++++++++++ cmd/frontend/graphqlbackend/schema.graphql | 13 ++++--- cmd/frontend/internal/httpapi/auth.go | 31 ++++++++++------ go.mod | 4 +++ internal/actor/actor.go | 5 +++ internal/database/access_tokens.go | 29 +++++++-------- internal/database/mocks_temp.go | 35 +++++++++---------- 7 files changed, 98 insertions(+), 50 deletions(-) diff --git a/cmd/frontend/graphqlbackend/graphqlbackend.go b/cmd/frontend/graphqlbackend/graphqlbackend.go index d48c27346424..e5cf5fd215e2 100644 --- a/cmd/frontend/graphqlbackend/graphqlbackend.go +++ b/cmd/frontend/graphqlbackend/graphqlbackend.go @@ -15,6 +15,7 @@ import ( "github.com/graph-gophers/graphql-go/introspection" "github.com/graph-gophers/graphql-go/relay" "github.com/graph-gophers/graphql-go/trace" + gqltypes "github.com/graph-gophers/graphql-go/types" "github.com/inconshreveable/log15" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promauto" @@ -490,6 +491,9 @@ func NewSchema( resolver, graphql.Tracer(&prometheusTracer{}), graphql.UseStringDescriptions(), + graphql.DirectiveVisitors(map[string]gqltypes.DirectiveVisitor{ + "authz": &authzDirectiveVisitor{}, + }), ) } @@ -848,3 +852,30 @@ func (r *schemaResolver) CodeHostSyncDue(ctx context.Context, args *struct { } return r.db.ExternalServices().SyncDue(ctx, ids, time.Duration(args.Seconds)*time.Second) } + +type authzDirectiveVisitor struct{} + +func (v *authzDirectiveVisitor) Before(ctx context.Context, directive *gqltypes.Directive, input interface{}) error { + if scopesAttr, ok := directive.Arguments.Get("scopes"); ok { + a := actor.FromContext(ctx) + // only care about token based auth and non-internal tokens for now + if a.FromToken && !a.Internal { + requiredScopes := scopesAttr.Deserialize(nil).([]interface{}) + if len(requiredScopes) < 1 { + return errors.Errorf("Authorization required, but no scopes are given in graphql schema") + } + // for now all scopes are required, but this can be changed in the future + // to be more flexible + for _, scope := range requiredScopes { + if ok := a.Scopes[scope.(string)]; !ok { + return errors.Errorf("Missing token scope %s", scope) + } + } + } + } + return nil +} + +func (v *authzDirectiveVisitor) After(ctx context.Context, directive *gqltypes.Directive, output interface{}) (interface{}, error) { + return output, nil +} diff --git a/cmd/frontend/graphqlbackend/schema.graphql b/cmd/frontend/graphqlbackend/schema.graphql index 66db48a2dc0f..6651d89c3fb1 100755 --- a/cmd/frontend/graphqlbackend/schema.graphql +++ b/cmd/frontend/graphqlbackend/schema.graphql @@ -1141,6 +1141,8 @@ input SurveySubmissionInput { better: String } +directive @authz(scopes: [String]!) on QUERY | FIELD_DEFINITION | OBJECT + """ Input for a happiness feedback submission. """ @@ -1217,7 +1219,7 @@ type Query { ): RepositoryRedirect """ Lists external services under given namespace. - If no namespace is given, it returns all external services. + If no namespace is given, it returns all external services """ externalServices( """ @@ -1233,7 +1235,8 @@ type Query { Opaque pagination cursor. """ after: String - ): ExternalServiceConnection! + ): ExternalServiceConnection! @authz(scopes: ["external_services:read"]) + """ List all repositories. """ @@ -1501,7 +1504,7 @@ type Query { """ Retrieve the list of defined feature flags """ - featureFlags: [FeatureFlag!]! + featureFlags: [FeatureFlag!]! @authz(scopes: ["feature_flags:read"]) """ Retrieve a feature flag @@ -2330,7 +2333,7 @@ type Highlight { """ A list of external services. """ -type ExternalServiceConnection { +type ExternalServiceConnection @authz(scopes: ["external_services:read"]) { """ A list of external services. """ @@ -2391,7 +2394,7 @@ type ExternalService implements Node { """ The JSON configuration of the external service. """ - config: JSONCString! + config: JSONCString! @authz(scopes: ["external_services:admin"]) """ When the external service was created. diff --git a/cmd/frontend/internal/httpapi/auth.go b/cmd/frontend/internal/httpapi/auth.go index 5622b14fb7cf..86a5a23c62b9 100644 --- a/cmd/frontend/internal/httpapi/auth.go +++ b/cmd/frontend/internal/httpapi/auth.go @@ -59,18 +59,29 @@ func AccessTokenAuthMiddleware(db database.DB, next http.Handler) http.Handler { return } + accessToken, err := db.AccessTokens().Lookup(r.Context(), token) + // convert scopes to a map for faster lookup + scopes := make(map[string]bool) + for _, scope := range accessToken.Scopes { + scopes[scope] = true + } // Validate access token. // // 🚨 SECURITY: It's important we check for the correct scopes to know what this token // is allowed to do. - var requiredScope string + var hasRequiredScope bool if sudoUser == "" { - requiredScope = authz.ScopeUserAll + ok := scopes[authz.ScopeUserAll] + // we require either user:all scope or any other scope to be present + hasRequiredScope = ok || len(accessToken.Scopes) > 0 } else { - requiredScope = authz.ScopeSiteAdminSudo + hasRequiredScope = scopes[authz.ScopeSiteAdminSudo] } - subjectUserID, err := db.AccessTokens().Lookup(r.Context(), token, requiredScope) - if err != nil { + if !hasRequiredScope { + err = database.ErrAccessTokenNotFound + } + + if err != nil || !hasRequiredScope { if err == database.ErrAccessTokenNotFound || errors.HasType(err, database.InvalidTokenError{}) { log15.Error("AccessTokenAuthMiddleware.invalidAccessToken", "token", token, "error", err) http.Error(w, "Invalid access token.", http.StatusUnauthorized) @@ -85,12 +96,12 @@ func AccessTokenAuthMiddleware(db database.DB, next http.Handler) http.Handler { // Determine the actor's user ID. var actorUserID int32 if sudoUser == "" { - actorUserID = subjectUserID + actorUserID = accessToken.SubjectUserID } else { // 🚨 SECURITY: Confirm that the sudo token's subject is still a site admin, to // prevent users from retaining site admin privileges after being demoted. - if err := auth.CheckUserIsSiteAdmin(r.Context(), db, subjectUserID); err != nil { - log15.Error("Sudo access token's subject is not a site admin.", "subjectUserID", subjectUserID, "err", err) + if err := auth.CheckUserIsSiteAdmin(r.Context(), db, accessToken.SubjectUserID); err != nil { + log15.Error("Sudo access token's subject is not a site admin.", "subjectUserID", accessToken.SubjectUserID, "err", err) http.Error(w, "The subject user of a sudo access token must be a site admin.", http.StatusForbidden) return } @@ -110,10 +121,10 @@ func AccessTokenAuthMiddleware(db database.DB, next http.Handler) http.Handler { return } actorUserID = user.ID - log15.Debug("HTTP request used sudo token.", "requestURI", r.URL.RequestURI(), "tokenSubjectUserID", subjectUserID, "actorUserID", actorUserID, "actorUsername", user.Username) + log15.Debug("HTTP request used sudo token.", "requestURI", r.URL.RequestURI(), "tokenSubjectUserID", accessToken.SubjectUserID, "actorUserID", actorUserID, "actorUsername", user.Username) } - r = r.WithContext(actor.WithActor(r.Context(), &actor.Actor{UID: actorUserID})) + r = r.WithContext(actor.WithActor(r.Context(), &actor.Actor{UID: actorUserID, Scopes: scopes, FromToken: true})) } next.ServeHTTP(w, r) diff --git a/go.mod b/go.mod index 85b13464ef8a..1e6f235a96e8 100644 --- a/go.mod +++ b/go.mod @@ -2,6 +2,10 @@ module github.com/sourcegraph/sourcegraph go 1.18 +// TODO: do not merge this local hack +// source is from this PR: https://github.com/graph-gophers/graphql-go/pull/446 +replace github.com/graph-gophers/graphql-go => /Users/milan/work/graphql-go + require ( cloud.google.com/go/kms v1.4.0 cloud.google.com/go/monitoring v1.2.0 diff --git a/internal/actor/actor.go b/internal/actor/actor.go index 6021a3888f02..bb9dbd098ac8 100644 --- a/internal/actor/actor.go +++ b/internal/actor/actor.go @@ -48,6 +48,11 @@ type Actor struct { // mockUser indicates this user was created in the context of a test. mockUser bool + + // true if actor was created from a token + FromToken bool + // list of scopes of a token in case token auth is used + Scopes map[string]bool } // FromUser returns an actor corresponding to the user with the given ID diff --git a/internal/database/access_tokens.go b/internal/database/access_tokens.go index c604a7ee0230..99adf4e13029 100644 --- a/internal/database/access_tokens.go +++ b/internal/database/access_tokens.go @@ -114,9 +114,9 @@ type AccessTokenStore interface { // // Calling Lookup also updates the access token's last-used-at date. // - // 🚨 SECURITY: This returns a user ID if and only if the tokenHexEncoded corresponds to a valid, + // 🚨 SECURITY: This returns an access token if and only if the tokenHexEncoded corresponds to a valid, // non-deleted access token. - Lookup(ctx context.Context, tokenHexEncoded, requiredScope string) (subjectUserID int32, err error) + Lookup(ctx context.Context, tokenHexEncoded string) (*AccessToken, error) Transact(context.Context) (AccessTokenStore, error) With(basestore.ShareableStore) AccessTokenStore @@ -187,16 +187,14 @@ INSERT INTO access_tokens(subject_user_id, scopes, value_sha256, note, creator_u return id, token, nil } -func (s *accessTokenStore) Lookup(ctx context.Context, tokenHexEncoded, requiredScope string) (subjectUserID int32, err error) { - if requiredScope == "" { - return 0, errors.New("no scope provided in access token lookup") - } - +func (s *accessTokenStore) Lookup(ctx context.Context, tokenHexEncoded string) (*AccessToken, error) { token, err := decodeToken(tokenHexEncoded) if err != nil { - return 0, errors.Wrap(err, "AccessTokens.Lookup") + return nil, errors.Wrap(err, "AccessTokens.Lookup") } + t := &AccessToken{} + if err := s.Handle().QueryRowContext(ctx, // Ensure that subject and creator users still exist. ` @@ -205,19 +203,18 @@ WHERE t.id IN ( SELECT t2.id FROM access_tokens t2 JOIN users subject_user ON t2.subject_user_id=subject_user.id AND subject_user.deleted_at IS NULL JOIN users creator_user ON t2.creator_user_id=creator_user.id AND creator_user.deleted_at IS NULL - WHERE t2.value_sha256=$1 AND t2.deleted_at IS NULL AND - $2 = ANY (t2.scopes) + WHERE t2.value_sha256=$1 AND t2.deleted_at IS NULL ) -RETURNING t.subject_user_id +RETURNING t.id, t.subject_user_id, t.scopes, t.creator_user_id, t.internal, t.created_at `, - toSHA256Bytes(token), requiredScope, - ).Scan(&subjectUserID); err != nil { + toSHA256Bytes(token), + ).Scan(&t.ID, &t.SubjectUserID, pq.Array(&t.Scopes), &t.CreatorUserID, &t.Internal, &t.CreatedAt); err != nil { if err == sql.ErrNoRows { - return 0, ErrAccessTokenNotFound + return nil, ErrAccessTokenNotFound } - return 0, err + return nil, err } - return subjectUserID, nil + return t, nil } func (s *accessTokenStore) GetByID(ctx context.Context, id int64) (*AccessToken, error) { diff --git a/internal/database/mocks_temp.go b/internal/database/mocks_temp.go index b75f66a9a6a2..ebd9d4f1cc38 100644 --- a/internal/database/mocks_temp.go +++ b/internal/database/mocks_temp.go @@ -130,7 +130,7 @@ func NewMockAccessTokenStore() *MockAccessTokenStore { }, }, LookupFunc: &AccessTokenStoreLookupFunc{ - defaultHook: func(context.Context, string, string) (r0 int32, r1 error) { + defaultHook: func(context.Context, string) (r0 *AccessToken, r1 error) { return }, }, @@ -202,7 +202,7 @@ func NewStrictMockAccessTokenStore() *MockAccessTokenStore { }, }, LookupFunc: &AccessTokenStoreLookupFunc{ - defaultHook: func(context.Context, string, string) (int32, error) { + defaultHook: func(context.Context, string) (*AccessToken, error) { panic("unexpected invocation of MockAccessTokenStore.Lookup") }, }, @@ -1361,24 +1361,24 @@ func (c AccessTokenStoreListFuncCall) Results() []interface{} { // AccessTokenStoreLookupFunc describes the behavior when the Lookup method // of the parent MockAccessTokenStore instance is invoked. type AccessTokenStoreLookupFunc struct { - defaultHook func(context.Context, string, string) (int32, error) - hooks []func(context.Context, string, string) (int32, error) + defaultHook func(context.Context, string) (*AccessToken, error) + hooks []func(context.Context, string) (*AccessToken, error) history []AccessTokenStoreLookupFuncCall mutex sync.Mutex } // Lookup delegates to the next hook function in the queue and stores the // parameter and result values of this invocation. -func (m *MockAccessTokenStore) Lookup(v0 context.Context, v1 string, v2 string) (int32, error) { - r0, r1 := m.LookupFunc.nextHook()(v0, v1, v2) - m.LookupFunc.appendCall(AccessTokenStoreLookupFuncCall{v0, v1, v2, r0, r1}) +func (m *MockAccessTokenStore) Lookup(v0 context.Context, v1 string) (*AccessToken, error) { + r0, r1 := m.LookupFunc.nextHook()(v0, v1) + m.LookupFunc.appendCall(AccessTokenStoreLookupFuncCall{v0, v1, r0, r1}) return r0, r1 } // SetDefaultHook sets function that is called when the Lookup method of the // parent MockAccessTokenStore instance is invoked and the hook queue is // empty. -func (f *AccessTokenStoreLookupFunc) SetDefaultHook(hook func(context.Context, string, string) (int32, error)) { +func (f *AccessTokenStoreLookupFunc) SetDefaultHook(hook func(context.Context, string) (*AccessToken, error)) { f.defaultHook = hook } @@ -1386,7 +1386,7 @@ func (f *AccessTokenStoreLookupFunc) SetDefaultHook(hook func(context.Context, s // Lookup method of the parent MockAccessTokenStore instance invokes the // hook at the front of the queue and discards it. After the queue is empty, // the default hook function is invoked for any future action. -func (f *AccessTokenStoreLookupFunc) PushHook(hook func(context.Context, string, string) (int32, error)) { +func (f *AccessTokenStoreLookupFunc) PushHook(hook func(context.Context, string) (*AccessToken, error)) { f.mutex.Lock() f.hooks = append(f.hooks, hook) f.mutex.Unlock() @@ -1394,20 +1394,20 @@ func (f *AccessTokenStoreLookupFunc) PushHook(hook func(context.Context, string, // SetDefaultReturn calls SetDefaultHook with a function that returns the // given values. -func (f *AccessTokenStoreLookupFunc) SetDefaultReturn(r0 int32, r1 error) { - f.SetDefaultHook(func(context.Context, string, string) (int32, error) { +func (f *AccessTokenStoreLookupFunc) SetDefaultReturn(r0 *AccessToken, r1 error) { + f.SetDefaultHook(func(context.Context, string) (*AccessToken, error) { return r0, r1 }) } // PushReturn calls PushHook with a function that returns the given values. -func (f *AccessTokenStoreLookupFunc) PushReturn(r0 int32, r1 error) { - f.PushHook(func(context.Context, string, string) (int32, error) { +func (f *AccessTokenStoreLookupFunc) PushReturn(r0 *AccessToken, r1 error) { + f.PushHook(func(context.Context, string) (*AccessToken, error) { return r0, r1 }) } -func (f *AccessTokenStoreLookupFunc) nextHook() func(context.Context, string, string) (int32, error) { +func (f *AccessTokenStoreLookupFunc) nextHook() func(context.Context, string) (*AccessToken, error) { f.mutex.Lock() defer f.mutex.Unlock() @@ -1446,12 +1446,9 @@ type AccessTokenStoreLookupFuncCall struct { // Arg1 is the value of the 2nd argument passed to this method // invocation. Arg1 string - // Arg2 is the value of the 3rd argument passed to this method - // invocation. - Arg2 string // Result0 is the value of the 1st result returned from this method // invocation. - Result0 int32 + Result0 *AccessToken // Result1 is the value of the 2nd result returned from this method // invocation. Result1 error @@ -1460,7 +1457,7 @@ type AccessTokenStoreLookupFuncCall struct { // Args returns an interface slice containing the arguments of this // invocation. func (c AccessTokenStoreLookupFuncCall) Args() []interface{} { - return []interface{}{c.Arg0, c.Arg1, c.Arg2} + return []interface{}{c.Arg0, c.Arg1} } // Results returns an interface slice containing the results of this From 8f01728fa56926624913b4b045dade15c61e068e Mon Sep 17 00:00:00 2001 From: Milan Freml Date: Fri, 30 Sep 2022 10:46:21 +0200 Subject: [PATCH 2/3] Make old tokens scopes backwards compatible --- cmd/frontend/graphqlbackend/graphqlbackend.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cmd/frontend/graphqlbackend/graphqlbackend.go b/cmd/frontend/graphqlbackend/graphqlbackend.go index e5cf5fd215e2..0d8406c07b60 100644 --- a/cmd/frontend/graphqlbackend/graphqlbackend.go +++ b/cmd/frontend/graphqlbackend/graphqlbackend.go @@ -25,6 +25,7 @@ import ( "github.com/sourcegraph/sourcegraph/internal/actor" "github.com/sourcegraph/sourcegraph/internal/api" "github.com/sourcegraph/sourcegraph/internal/auth" + "github.com/sourcegraph/sourcegraph/internal/authz" "github.com/sourcegraph/sourcegraph/internal/cloneurls" "github.com/sourcegraph/sourcegraph/internal/conf" "github.com/sourcegraph/sourcegraph/internal/database" @@ -859,7 +860,9 @@ func (v *authzDirectiveVisitor) Before(ctx context.Context, directive *gqltypes. if scopesAttr, ok := directive.Arguments.Get("scopes"); ok { a := actor.FromContext(ctx) // only care about token based auth and non-internal tokens for now - if a.FromToken && !a.Internal { + isUserAll := a.Scopes[authz.ScopeUserAll] + isSiteAdminSudo := a.Scopes[authz.ScopeSiteAdminSudo] + if a.FromToken && !a.Internal && !(isUserAll || isSiteAdminSudo) { requiredScopes := scopesAttr.Deserialize(nil).([]interface{}) if len(requiredScopes) < 1 { return errors.Errorf("Authorization required, but no scopes are given in graphql schema") From 62e366c6418554ed28c9e4489ed7876cea0c5ef7 Mon Sep 17 00:00:00 2001 From: Milan Freml Date: Wed, 12 Oct 2022 16:06:31 +0200 Subject: [PATCH 3/3] Add example on token scopes for REST API routes --- cmd/frontend/internal/httpapi/httpapi.go | 33 +++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/cmd/frontend/internal/httpapi/httpapi.go b/cmd/frontend/internal/httpapi/httpapi.go index c45e2a0ffea4..a8258771fae3 100644 --- a/cmd/frontend/internal/httpapi/httpapi.go +++ b/cmd/frontend/internal/httpapi/httpapi.go @@ -2,6 +2,7 @@ package httpapi import ( "context" + "fmt" "log" "net/http" "os" @@ -28,7 +29,9 @@ import ( frontendsearch "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/search" registry "github.com/sourcegraph/sourcegraph/cmd/frontend/registry/api" "github.com/sourcegraph/sourcegraph/cmd/frontend/webhooks" + "github.com/sourcegraph/sourcegraph/internal/actor" "github.com/sourcegraph/sourcegraph/internal/api" + "github.com/sourcegraph/sourcegraph/internal/authz" internalcodeintel "github.com/sourcegraph/sourcegraph/internal/codeintel" "github.com/sourcegraph/sourcegraph/internal/conf" "github.com/sourcegraph/sourcegraph/internal/database" @@ -116,14 +119,14 @@ func NewHandler( m.Get(apirouter.GraphQL).Handler(trace.Route(handler(serveGraphQL(logger, schema, rateLimiter, false)))) - m.Get(apirouter.SearchStream).Handler(trace.Route(frontendsearch.StreamHandler(db))) + m.Get(apirouter.SearchStream).Handler(trace.Route(restTokenScopesMiddleware([]string{"search-stream:read"}, frontendsearch.StreamHandler(db)))) // Return the minimum src-cli version that's compatible with this instance - m.Get(apirouter.SrcCli).Handler(trace.Route(newSrcCliVersionHandler(logger))) + m.Get(apirouter.SrcCli).Handler(trace.Route(restTokenScopesMiddleware([]string{"src-cli:read"}, newSrcCliVersionHandler(logger)))) // Set up the src-cli version cache handler (this will effectively be a // no-op anywhere other than dot-com). - m.Get(apirouter.SrcCliVersionCache).Handler(trace.Route(releasecache.NewHandler(logger))) + m.Get(apirouter.SrcCliVersionCache).Handler(trace.Route(restTokenScopesMiddleware([]string{"src-cli:read"}, releasecache.NewHandler(logger)))) m.Get(apirouter.Registry).Handler(trace.Route(handler(registry.HandleRegistry(db)))) @@ -269,3 +272,27 @@ func jsonMiddleware(errorHandler *errorHandler) func(func(http.ResponseWriter, * } } } + +func restTokenScopesMiddleware(scopes []string, next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if len(scopes) > 0 { + a := actor.FromContext(r.Context()) + // only care about token based auth and non-internal tokens for now + isUserAll := a.Scopes[authz.ScopeUserAll] + isSiteAdminSudo := a.Scopes[authz.ScopeSiteAdminSudo] + if a.FromToken && !a.Internal && !(isUserAll || isSiteAdminSudo) { + // for now all scopes are required, but this can be changed in the future + // to be more flexible + for _, scope := range scopes { + if ok := a.Scopes[scope]; !ok { + http.Error(w, fmt.Sprintf("forbidden, missing token scope: %s", scope), http.StatusForbidden) + return + } + } + } + } + + next.ServeHTTP(w, r) + return + }) +}