From 32a8e7e8f51c8de2a136eb15d17be119203968c4 Mon Sep 17 00:00:00 2001 From: Rohith Jayawardene Date: Sun, 7 Jan 2018 19:19:06 +0000 Subject: [PATCH] Groups Claims (#301) The current implementation does not take into account the standard `groups` claim when matching on the resource. This PR add the ability to add an additional field `--groups=list,of,groups` to the resource. Note unlike the roles which are applied with an AND operation a user simply has to exist in one of the groups specified. --- CHANGELOG.md | 6 +++ README.md | 26 ++++++++++ doc.go | 31 +++++++----- middleware.go | 33 ++++++++----- middleware_test.go | 116 +++++++++++++++++++++++++++++++++++++++++++++ resource.go | 2 + resource_test.go | 8 ++++ server_test.go | 5 ++ user_context.go | 35 +++++++++----- utils.go | 26 +++++++--- utils_test.go | 93 +++++++++++++++++++++++++++++------- 11 files changed, 321 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index af09ab43..61c7efeb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ + +#### **2.1.1 (Unreleased)** + +FEATURES: +* Added the groups parameter to the resource, permitting users to use the `groups` claim in the token [#PR301](https://github.com/gambol99/keycloak-proxy/pull/301) + #### **2.1.0** FIXES: diff --git a/README.md b/README.md index 14ccae00..52260de3 100644 --- a/README.md +++ b/README.md @@ -211,6 +211,9 @@ resources: roles: - client:test1 - client:test2 + groups: + - admins + - users - uri: /backend* roles: - client:test1 @@ -357,6 +360,7 @@ On protected resources the upstream endpoint will receive a number of headers ad id := user.(*userContext) cx.Request().Header.Set("X-Auth-Email", id.email) cx.Request().Header.Set("X-Auth-ExpiresIn", id.expiresAt.String()) +cx.Request().Header.Set("X-Auth-Groups", strings.Join(id.groups, ",")) cx.Request().Header.Set("X-Auth-Roles", strings.Join(id.roles, ",")) cx.Request().Header.Set("X-Auth-Subject", id.id) cx.Request().Header.Set("X-Auth-Token", id.token.Encode()) @@ -433,6 +437,28 @@ match-claims: email: ^.*@example.com$ ``` +#### **Groups Claims** + +You can match on the group claims within a token via the `groups` parameter available within the resource. Note while roles are implicitly required i.e. `roles=admin,user` the user MUST have roles 'admin' AND 'user', groups are applied with an OR operation, so `groups=users,testers` requires the user MUST be within 'users' OR 'testers'. At present the claim name is hardcoded to `groups` i.e a JWT token would look like the below. + +```JSON +{ + "iss": "https://sso.example.com", + "sub": "", + "aud": "test", + "exp": 1515269245, + "iat": 1515182845, + "email": "gambol99@gmail.com", + "groups": [ + "group_one", + "group_two" + ], + "name": "Rohith" +} +``` + +Note: I'm also considering changing the way groups are implemented, exchanging for how match-claims are done, such as `--match=[]groups=(a|b|c)` but would mean adding matches to URI resource first. + #### **Custom Pages** By default the proxy will immediately redirect you for authentication and hand back 403 for access denied. Most users will probably want to present the user with a more friendly sign-in and access denied page. You can pass the command line options (or via config file) paths to the files i.e. --signin-page=PATH. The sign-in page will have a 'redirect' variable passed into the scope and holding the oauth redirection url. If you wish pass additional variables into the templates, perhaps title, sitename etc, you can use the --tags key=pair i.e. --tags title="This is my site"; the variable would be accessible from {{ .title }} diff --git a/doc.go b/doc.go index 9fd06fac..868ef70e 100644 --- a/doc.go +++ b/doc.go @@ -56,11 +56,12 @@ const ( tokenURL = "/token" debugURL = "/debug/pprof" - claimPreferredName = "preferred_username" claimAudience = "aud" - claimResourceAccess = "resource_access" + claimPreferredName = "preferred_username" claimRealmAccess = "realm_access" + claimResourceAccess = "resource_access" claimResourceRoles = "roles" + claimGroups = "groups" ) const ( @@ -99,6 +100,8 @@ type Resource struct { WhiteListed bool `json:"white-listed" yaml:"white-listed"` // Roles the roles required to access this url Roles []string `json:"roles" yaml:"roles"` + // Groups is a list of groups the user is in + Groups []string `json:"groups" yaml:"groups"` } // Config is the configuration for the proxy @@ -316,28 +319,30 @@ type reverseProxy interface { ServeHTTP(rw http.ResponseWriter, req *http.Request) } -// userContext represents a user +// userContext holds the information extracted the token type userContext struct { // the id of the user id string + // the audience for the token + audience string + // whether the context is from a session cookie or authorization header + bearerToken bool + // the claims associated to the token + claims jose.Claims // the email associated to the user email string + // the expiration of the access token + expiresAt time.Time + // groups is a collection of groups the user in in + groups []string // a name of the user name string - // the preferred name + // preferredName is the name of the user preferredName string - // the expiration of the access token - expiresAt time.Time - // a set of roles associated + // roles is a collection of roles the users holds roles []string - // the audience for the token - audience string // the access token itself token jose.JWT - // the claims associated to the token - claims jose.Claims - // whether the context is from a session cookie or authorization header - bearerToken bool } // tokenResponse diff --git a/middleware.go b/middleware.go index 58438e7b..8bb30058 100644 --- a/middleware.go +++ b/middleware.go @@ -245,18 +245,28 @@ func (r *oauthProxy) admissionMiddleware(resource *Resource) func(http.Handler) } user := scope.Identity - // step: we need to check the roles - if roles := len(resource.Roles); roles > 0 { - if !hasRoles(resource.Roles, user.roles) { - r.log.Warn("access denied, invalid roles", - zap.String("access", "denied"), - zap.String("email", user.email), - zap.String("resource", resource.URL), - zap.String("required", resource.getRoles())) + // @step: we need to check the roles + if !hasAccess(resource.Roles, user.roles, true) { + r.log.Warn("access denied, invalid roles", + zap.String("access", "denied"), + zap.String("email", user.email), + zap.String("resource", resource.URL), + zap.String("roles", resource.getRoles())) + + next.ServeHTTP(w, req.WithContext(r.accessForbidden(w, req))) + return + } - next.ServeHTTP(w, req.WithContext(r.accessForbidden(w, req))) - return - } + // @step: check if we have any groups, the groups are there + if !hasAccess(resource.Groups, user.groups, false) { + r.log.Warn("access denied, invalid roles", + zap.String("access", "denied"), + zap.String("email", user.email), + zap.String("resource", resource.URL), + zap.String("groups", strings.Join(resource.Groups, ","))) + + next.ServeHTTP(w, req.WithContext(r.accessForbidden(w, req))) + return } // step: if we have any claim matching, lets validate the tokens has the claims @@ -326,6 +336,7 @@ func (r *oauthProxy) headersMiddleware(custom []string) func(http.Handler) http. user := scope.Identity req.Header.Set("X-Auth-Email", user.email) req.Header.Set("X-Auth-ExpiresIn", user.expiresAt.String()) + req.Header.Set("X-Auth-Groups", strings.Join(user.groups, ",")) req.Header.Set("X-Auth-Roles", strings.Join(user.roles, ",")) req.Header.Set("X-Auth-Subject", user.id) req.Header.Set("X-Auth-Userid", user.name) diff --git a/middleware_test.go b/middleware_test.go index 2021b3a1..c8d96805 100644 --- a/middleware_test.go +++ b/middleware_test.go @@ -37,6 +37,7 @@ type fakeRequest struct { Cookies []*http.Cookie Expires time.Duration FormValues map[string]string + Groups []string HasCookieToken bool HasLogin bool HasToken bool @@ -177,6 +178,9 @@ func (f *fakeProxy) RunTests(t *testing.T, requests []fakeRequest) { if len(c.Roles) > 0 { token.addRealmRoles(c.Roles) } + if len(c.Groups) > 0 { + token.addGroups(c.Groups) + } if c.Expires > 0 || c.Expires < 0 { token.setExpiration(time.Now().Add(c.Expires)) } @@ -573,6 +577,118 @@ func TestWhiteListedRequests(t *testing.T) { newFakeProxy(cfg).RunTests(t, requests) } +func TestGroupPermissionsMiddleware(t *testing.T) { + cfg := newFakeKeycloakConfig() + cfg.Resources = []*Resource{ + { + URL: "/with_role_and_group*", + Methods: allHTTPMethods, + Groups: []string{"admin"}, + Roles: []string{"admin"}, + }, + { + URL: "/with_group*", + Methods: allHTTPMethods, + Groups: []string{"admin"}, + }, + { + URL: "/with_many_groups*", + Methods: allHTTPMethods, + Groups: []string{"admin", "user", "tester"}, + }, + { + URL: "/*", + Methods: allHTTPMethods, + Roles: []string{"user"}, + }, + } + requests := []fakeRequest{ + { + URI: "/", + ExpectedCode: http.StatusUnauthorized, + }, + { + URI: "/with_role_and_group/test", + HasToken: true, + Roles: []string{"admin"}, + ExpectedCode: http.StatusForbidden, + }, + { + URI: "/with_role_and_group/test", + HasToken: true, + Groups: []string{"admin"}, + ExpectedCode: http.StatusForbidden, + }, + { + URI: "/with_role_and_group/test", + HasToken: true, + Groups: []string{"admin"}, + Roles: []string{"admin"}, + ExpectedProxy: true, + ExpectedCode: http.StatusOK, + }, + { + URI: "/with_group/hello", + HasToken: true, + ExpectedCode: http.StatusForbidden, + }, + { + URI: "/with_groupdd", + HasToken: true, + ExpectedCode: http.StatusForbidden, + }, + { + URI: "/with_group/hello", + HasToken: true, + Groups: []string{"bad"}, + ExpectedCode: http.StatusForbidden, + }, + { + URI: "/with_group/hello", + HasToken: true, + Groups: []string{"admin"}, + ExpectedProxy: true, + ExpectedCode: http.StatusOK, + }, + { + URI: "/with_group/hello", + HasToken: true, + Groups: []string{"test", "admin"}, + ExpectedProxy: true, + ExpectedCode: http.StatusOK, + }, + { + URI: "/with_many_groups/test", + HasToken: true, + Groups: []string{"bad"}, + ExpectedCode: http.StatusForbidden, + }, + { + URI: "/with_many_groups/test", + HasToken: true, + Groups: []string{"user"}, + Roles: []string{"test"}, + ExpectedProxy: true, + ExpectedCode: http.StatusOK, + }, + { + URI: "/with_many_groups/test", + HasToken: true, + Groups: []string{"tester", "user"}, + ExpectedProxy: true, + ExpectedCode: http.StatusOK, + }, + { + URI: "/with_many_groups/test", + HasToken: true, + Groups: []string{"bad", "user"}, + ExpectedProxy: true, + ExpectedCode: http.StatusOK, + }, + } + newFakeProxy(cfg).RunTests(t, requests) +} + func TestRolePermissionsMiddleware(t *testing.T) { cfg := newFakeKeycloakConfig() cfg.Resources = []*Resource{ diff --git a/resource.go b/resource.go index 2a5eb800..ddcc1526 100644 --- a/resource.go +++ b/resource.go @@ -53,6 +53,8 @@ func (r *Resource) parse(resource string) (*Resource, error) { } case "roles": r.Roles = strings.Split(kp[1], ",") + case "groups": + r.Groups = strings.Split(kp[1], ",") case "white-listed": value, err := strconv.ParseBool(kp[1]) if err != nil { diff --git a/resource_test.go b/resource_test.go index 68825255..4c914697 100644 --- a/resource_test.go +++ b/resource_test.go @@ -72,6 +72,14 @@ func TestResourceParseOk(t *testing.T) { Option: "uri=/*|methods=any", Resource: &Resource{URL: "/*", Methods: allHTTPMethods}, }, + { + Option: "uri=/*|groups=admin,test", + Resource: &Resource{URL: "/*", Methods: allHTTPMethods, Groups: []string{"admin", "test"}}, + }, + { + Option: "uri=/*|groups=admin", + Resource: &Resource{URL: "/*", Methods: allHTTPMethods, Groups: []string{"admin"}}, + }, } for i, x := range cs { r, err := newResource().parse(x.Option) diff --git a/server_test.go b/server_test.go index 501bde7c..584df411 100644 --- a/server_test.go +++ b/server_test.go @@ -529,6 +529,11 @@ func (t *fakeToken) setExpiration(tm time.Time) { t.claims.Add("exp", float64(tm.Unix())) } +// addGroups adds groups to then token +func (t *fakeToken) addGroups(groups []string) { + t.claims.Add("groups", groups) +} + // addRealmRoles adds realms roles to token func (t *fakeToken) addRealmRoles(roles []string) { t.claims.Add("realm_access", map[string]interface{}{ diff --git a/user_context.go b/user_context.go index 875085f2..6ff75e08 100644 --- a/user_context.go +++ b/user_context.go @@ -34,7 +34,8 @@ func extractIdentity(token jose.JWT) (*userContext, error) { if err != nil { return nil, err } - // step: ensure we have and can extract the preferred name of the user, if not, we set to the ID + + // @step: ensure we have and can extract the preferred name of the user, if not, we set to the ID preferredName, found, err := claims.StringClaim(claimPreferredName) if err != nil || !found { preferredName = identity.Email @@ -43,38 +44,46 @@ func extractIdentity(token jose.JWT) (*userContext, error) { if err != nil || !found { return nil, ErrNoTokenAudience } - // step: extract the realm roles - var list []string + + // @step: extract the realm roles + var roleList []string if realmRoles, found := claims[claimRealmAccess].(map[string]interface{}); found { if roles, found := realmRoles[claimResourceRoles]; found { for _, r := range roles.([]interface{}) { - list = append(list, fmt.Sprintf("%s", r)) + roleList = append(roleList, fmt.Sprintf("%s", r)) } } } - // step: extract the client roles from the access token + // @step: extract the client roles from the access token if accesses, found := claims[claimResourceAccess].(map[string]interface{}); found { - for roleName, roleList := range accesses { - scopes := roleList.(map[string]interface{}) + for name, list := range accesses { + scopes := list.(map[string]interface{}) if roles, found := scopes[claimResourceRoles]; found { for _, r := range roles.([]interface{}) { - list = append(list, fmt.Sprintf("%s:%s", roleName, r)) + roleList = append(roleList, fmt.Sprintf("%s:%s", name, r)) } } } } + // @step: extract any group information from the tokens + groups, _, err := claims.StringsClaim(claimGroups) + if err != nil { + return nil, err + } + return &userContext{ - id: identity.ID, - name: preferredName, audience: audience, - preferredName: preferredName, + claims: claims, email: identity.Email, expiresAt: identity.ExpiresAt, - roles: list, + groups: groups, + id: identity.ID, + name: preferredName, + preferredName: preferredName, + roles: roleList, token: token, - claims: claims, }, nil } diff --git a/utils.go b/utils.go index 9d30d06c..7acc7a93 100644 --- a/utils.go +++ b/utils.go @@ -187,15 +187,29 @@ func fileExists(filename string) bool { return true } -// hasRoles checks the scopes are the same -func hasRoles(required, issued []string) bool { - for _, role := range required { - if !containedIn(role, issued) { - return false +// hasAccess checks we have all or any of the needed items in the list +func hasAccess(need, have []string, all bool) bool { + if len(need) == 0 { + return true + } + + var matched int + for _, x := range need { + found := containedIn(x, have) + switch found { + case true: + if !all { + return true + } + matched++ + default: + if all { + return false + } } } - return true + return matched > 0 } // containedIn checks if a value in a list of a strings diff --git a/utils_test.go b/utils_test.go index eabafb43..40318c53 100644 --- a/utils_test.go +++ b/utils_test.go @@ -225,32 +225,91 @@ func TestDecryptDataBlock(t *testing.T) { } -func TestHasRoles(t *testing.T) { - testCases := []struct { - Roles []string - Required []string - Ok bool +func TestHasAccessOK(t *testing.T) { + cs := []struct { + Have []string + Need []string + Required bool }{ + {}, + { + Have: []string{"a", "b"}, + }, + { + Have: []string{"a", "b", "c"}, + Need: []string{"a", "b"}, + Required: true, + }, + { + Have: []string{"a", "b", "c"}, + Need: []string{"a", "c"}, + }, { - Roles: []string{"a", "b", "c"}, - Required: []string{"a", "b"}, - Ok: true, + Have: []string{"a", "b", "c"}, + Need: []string{"c"}, }, { - Roles: []string{"a", "b"}, - Required: []string{"a", "b"}, - Ok: true, + Have: []string{"a", "b", "c"}, + Need: []string{"b"}, }, { - Roles: []string{"a", "b", "c"}, - Required: []string{"a", "d"}, + Have: []string{"a", "b", "c"}, + Need: []string{"b"}, + }, + { + Have: []string{"a", "b"}, + Need: []string{"a"}, + }, + { + Have: []string{"a", "b"}, + Need: []string{"a"}, + Required: true, + }, + { + Have: []string{"b", "a"}, + Need: []string{"a"}, + Required: true, }, } + for i, x := range cs { + assert.True(t, hasAccess(x.Need, x.Have, x.Required), "case: %d should be true, have: %v, need: %v, require: %t ", i, x.Have, x.Need, x.Required) + } +} - for i, test := range testCases { - if !hasRoles(test.Required, test.Roles) && test.Ok { - assert.Fail(t, "test case: %i should have ok, %s, %s", i, test.Roles, test.Required) - } +func TestHasAccessBad(t *testing.T) { + cs := []struct { + Have []string + Need []string + Required bool + }{ + { + Have: []string{"a", "b"}, + Need: []string{"c"}, + }, + { + Have: []string{"a", "b"}, + Need: []string{"c"}, + Required: true, + }, + { + Have: []string{"a", "c"}, + Need: []string{"a", "b"}, + Required: true, + }, + { + Have: []string{"a", "b", "c"}, + Need: []string{"b", "j"}, + Required: true, + }, + { + Have: []string{"a", "b", "c"}, + Need: []string{"a", "d"}, + Required: true, + }, + } + + for i, x := range cs { + assert.False(t, hasAccess(x.Need, x.Have, x.Required), "case: %d should be false, have: %v, need: %v, require: %t ", i, x.Have, x.Need, x.Required) } }