From ed8aeb29993b3566cdb2686ad4ab512f05fdb154 Mon Sep 17 00:00:00 2001 From: gotjosh Date: Wed, 31 Jul 2019 11:23:00 +0100 Subject: [PATCH] Auth Proxy: Include additional headers as part of the cache key (#18298) * Auth Proxy: Include additional headers as part of the cache key Auth proxy has support to send additional user attributes as part of the authentication flow. These attributes (e.g. Groups) need to be monitored as part of the process in case of change. This commit changes the way we compute the cache key to include all of the attributes sent as part of the authentication request. That way, if we change any user attributes we'll upsert the user information. --- pkg/middleware/auth_proxy/auth_proxy.go | 62 +++++++++---- pkg/middleware/auth_proxy/auth_proxy_test.go | 92 ++++++++++++-------- pkg/middleware/middleware_test.go | 11 ++- 3 files changed, 108 insertions(+), 57 deletions(-) diff --git a/pkg/middleware/auth_proxy/auth_proxy.go b/pkg/middleware/auth_proxy/auth_proxy.go index ff19627ecbb..869e8a702bd 100644 --- a/pkg/middleware/auth_proxy/auth_proxy.go +++ b/pkg/middleware/auth_proxy/auth_proxy.go @@ -1,6 +1,7 @@ package authproxy import ( + "encoding/base32" "fmt" "net" "net/mail" @@ -32,6 +33,9 @@ var isLDAPEnabled = ldap.IsEnabled // newLDAP creates multiple LDAP instance var newLDAP = multildap.New +// supportedHeaders states the supported headers configuration fields +var supportedHeaderFields = []string{"Name", "Email", "Login", "Groups"} + // AuthProxy struct type AuthProxy struct { store *remotecache.RemoteCache @@ -142,9 +146,18 @@ func (auth *AuthProxy) IsAllowedIP() (bool, *Error) { return false, newError("Proxy authentication required", err) } -// getKey forms a key for the cache +// getKey forms a key for the cache based on the headers received as part of the authentication flow. +// Our configuration supports multiple headers. The main header contains the email or username. +// And the additional ones that allow us to specify extra attributes: Name, Email or Groups. func (auth *AuthProxy) getKey() string { - return fmt.Sprintf(CachePrefix, auth.header) + key := strings.TrimSpace(auth.header) // start the key with the main header + + auth.headersIterator(func(_, header string) { + key = strings.Join([]string{key, header}, "-") // compose the key with any additional headers + }) + + hashedKey := base32.StdEncoding.EncodeToString([]byte(key)) + return fmt.Sprintf(CachePrefix, hashedKey) } // Login logs in user id with whatever means possible @@ -232,40 +245,36 @@ func (auth *AuthProxy) LoginViaHeader() (int64, error) { AuthId: auth.header, } - if auth.headerType == "username" { + switch auth.headerType { + case "username": extUser.Login = auth.header - // only set Email if it can be parsed as an email address - emailAddr, emailErr := mail.ParseAddress(auth.header) + emailAddr, emailErr := mail.ParseAddress(auth.header) // only set Email if it can be parsed as an email address if emailErr == nil { extUser.Email = emailAddr.Address } - } else if auth.headerType == "email" { + case "email": extUser.Email = auth.header extUser.Login = auth.header - } else { + default: return 0, newError("Auth proxy header property invalid", nil) + } - for _, field := range []string{"Name", "Email", "Login", "Groups"} { - if auth.headers[field] == "" { - continue + auth.headersIterator(func(field string, header string) { + if field == "Groups" { + extUser.Groups = util.SplitString(header) + } else { + reflect.ValueOf(extUser).Elem().FieldByName(field).SetString(header) } - - if val := auth.ctx.Req.Header.Get(auth.headers[field]); val != "" { - if field == "Groups" { - extUser.Groups = util.SplitString(val) - } else { - reflect.ValueOf(extUser).Elem().FieldByName(field).SetString(val) - } - } - } + }) upsert := &models.UpsertUserCommand{ ReqContext: auth.ctx, SignupAllowed: setting.AuthProxyAutoSignUp, ExternalUser: extUser, } + err := bus.Dispatch(upsert) if err != nil { return 0, err @@ -274,6 +283,21 @@ func (auth *AuthProxy) LoginViaHeader() (int64, error) { return upsert.Result.Id, nil } +// headersIterator iterates over all non-empty supported additional headers +func (auth *AuthProxy) headersIterator(fn func(field string, header string)) { + for _, field := range supportedHeaderFields { + h := auth.headers[field] + + if h == "" { + continue + } + + if value := auth.ctx.Req.Header.Get(h); value != "" { + fn(field, strings.TrimSpace(value)) + } + } +} + // GetSignedUser get full signed user info func (auth *AuthProxy) GetSignedUser(userID int64) (*models.SignedInUser, *Error) { query := &models.GetSignedInUserQuery{ diff --git a/pkg/middleware/auth_proxy/auth_proxy_test.go b/pkg/middleware/auth_proxy/auth_proxy_test.go index 4a1edc66ba5..bd749393834 100644 --- a/pkg/middleware/auth_proxy/auth_proxy_test.go +++ b/pkg/middleware/auth_proxy/auth_proxy_test.go @@ -1,20 +1,20 @@ package authproxy import ( + "encoding/base32" "errors" "fmt" "net/http" "testing" - . "github.com/smartystreets/goconvey/convey" - "gopkg.in/macaron.v1" - "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/infra/remotecache" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/ldap" "github.com/grafana/grafana/pkg/services/multildap" "github.com/grafana/grafana/pkg/setting" + . "github.com/smartystreets/goconvey/convey" + "gopkg.in/macaron.v1" ) type TestMultiLDAP struct { @@ -45,37 +45,70 @@ func (stub *TestMultiLDAP) User(login string) ( return result, nil } +func prepareMiddleware(t *testing.T, req *http.Request, store *remotecache.RemoteCache) *AuthProxy { + t.Helper() + + ctx := &models.ReqContext{ + Context: &macaron.Context{ + Req: macaron.Request{ + Request: req, + }, + }, + } + + auth := New(&Options{ + Store: store, + Ctx: ctx, + OrgID: 4, + }) + + return auth +} + func TestMiddlewareContext(t *testing.T) { Convey("auth_proxy helper", t, func() { req, _ := http.NewRequest("POST", "http://example.com", nil) setting.AuthProxyHeaderName = "X-Killa" - name := "markelog" + store := remotecache.NewFakeStore(t) + name := "markelog" req.Header.Add(setting.AuthProxyHeaderName, name) - ctx := &models.ReqContext{ - Context: &macaron.Context{ - Req: macaron.Request{ - Request: req, - }, - }, - } + Convey("when the cache only contains the main header", func() { - Convey("logs in user from the cache", func() { - store := remotecache.NewFakeStore(t) - key := fmt.Sprintf(CachePrefix, name) - store.Set(key, int64(33), 0) + Convey("with a simple cache key", func() { + // Set cache key + key := fmt.Sprintf(CachePrefix, base32.StdEncoding.EncodeToString([]byte(name))) + store.Set(key, int64(33), 0) - auth := New(&Options{ - Store: store, - Ctx: ctx, - OrgID: 4, + // Set up the middleware + auth := prepareMiddleware(t, req, store) + id, err := auth.Login() + + So(auth.getKey(), ShouldEqual, "auth-proxy-sync-ttl:NVQXE23FNRXWO===") + So(err, ShouldBeNil) + So(id, ShouldEqual, 33) }) - id, err := auth.Login() + Convey("when the cache key contains additional headers", func() { + setting.AuthProxyHeaders = map[string]string{"Groups": "X-WEBAUTH-GROUPS"} + group := "grafana-core-team" + req.Header.Add("X-WEBAUTH-GROUPS", group) - So(err, ShouldBeNil) - So(id, ShouldEqual, 33) + key := fmt.Sprintf(CachePrefix, base32.StdEncoding.EncodeToString([]byte(name+"-"+group))) + store.Set(key, int64(33), 0) + + auth := prepareMiddleware(t, req, store) + + id, err := auth.Login() + + So(auth.getKey(), ShouldEqual, "auth-proxy-sync-ttl:NVQXE23FNRXWOLLHOJQWMYLOMEWWG33SMUWXIZLBNU======") + So(err, ShouldBeNil) + So(id, ShouldEqual, 33) + }) + + Convey("when the does not exist", func() { + }) }) Convey("LDAP", func() { @@ -119,13 +152,9 @@ func TestMiddlewareContext(t *testing.T) { store := remotecache.NewFakeStore(t) - server := New(&Options{ - Store: store, - Ctx: ctx, - OrgID: 4, - }) + auth := prepareMiddleware(t, req, store) - id, err := server.Login() + id, err := auth.Login() So(err, ShouldBeNil) So(id, ShouldEqual, 42) @@ -149,11 +178,7 @@ func TestMiddlewareContext(t *testing.T) { store := remotecache.NewFakeStore(t) - auth := New(&Options{ - Store: store, - Ctx: ctx, - OrgID: 4, - }) + auth := prepareMiddleware(t, req, store) stub := &TestMultiLDAP{ ID: 42, @@ -170,7 +195,6 @@ func TestMiddlewareContext(t *testing.T) { So(id, ShouldNotEqual, 42) So(stub.loginCalled, ShouldEqual, false) }) - }) }) } diff --git a/pkg/middleware/middleware_test.go b/pkg/middleware/middleware_test.go index 590818c15ce..95ab3a778e8 100644 --- a/pkg/middleware/middleware_test.go +++ b/pkg/middleware/middleware_test.go @@ -2,6 +2,7 @@ package middleware import ( "context" + "encoding/base32" "encoding/json" "fmt" "net/http" @@ -10,9 +11,6 @@ import ( "testing" "time" - . "github.com/smartystreets/goconvey/convey" - "gopkg.in/macaron.v1" - "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/infra/remotecache" @@ -21,7 +19,9 @@ import ( "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" + . "github.com/smartystreets/goconvey/convey" "github.com/stretchr/testify/assert" + "gopkg.in/macaron.v1" ) const errorTemplate = "error-template" @@ -377,7 +377,9 @@ func TestMiddlewareContext(t *testing.T) { setting.LDAPEnabled = true setting.AuthProxyHeaderName = "X-WEBAUTH-USER" setting.AuthProxyHeaderProperty = "username" + setting.AuthProxyHeaders = map[string]string{"Groups": "X-WEBAUTH-GROUPS"} name := "markelog" + group := "grafana-core-team" middlewareScenario(t, "should not sync the user if it's in the cache", func(sc *scenarioContext) { bus.AddHandler("test", func(query *models.GetSignedInUserQuery) error { @@ -385,11 +387,12 @@ func TestMiddlewareContext(t *testing.T) { return nil }) - key := fmt.Sprintf(cachePrefix, name) + key := fmt.Sprintf(cachePrefix, base32.StdEncoding.EncodeToString([]byte(name+"-"+group))) sc.remoteCacheService.Set(key, int64(33), 0) sc.fakeReq("GET", "/") sc.req.Header.Add(setting.AuthProxyHeaderName, name) + sc.req.Header.Add("X-WEBAUTH-GROUPS", group) sc.exec() Convey("Should init user via cache", func() {