Auth Proxy: Include additional headers as part of the cache key (graf…

…ana#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.
xlson · Jul 31, 2019 · ed8aeb2 · ed8aeb2
1 parent 294eabd
commit ed8aeb2
Show file tree

Hide file tree

Showing 3 changed files with 108 additions and 57 deletions.
diff --git 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
-		}
+	}
 
-		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)
-			}
+	auth.headersIterator(func(field string, header string) {
+		if field == "Groups" {
+			extUser.Groups = util.SplitString(header)
+		} else {
+			reflect.ValueOf(extUser).Elem().FieldByName(field).SetString(header)
 		}
-	}
+	})
 
 	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
@@ -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)
+
+				key := fmt.Sprintf(CachePrefix, base32.StdEncoding.EncodeToString([]byte(name+"-"+group)))
+				store.Set(key, int64(33), 0)
 
-			So(err, ShouldBeNil)
-			So(id, ShouldEqual, 33)
+				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
@@ -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,19 +377,22 @@ 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 {
 					query.Result = &models.SignedInUser{OrgId: 4, UserId: query.UserId}
 					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() {