Merge pull request #953 from grnhse/keycloak-refactor-provider-methods

Refactor Keycloak Provider Methods

CHANGELOG.md

@@ -4,6 +4,8 @@
 
 ## Important Notes
 
+- [#953](https://github.com/oauth2-proxy/oauth2-proxy/pull/953) Keycloak will now use `--profile-url` if set for the userinfo endpoint
+  instead of `--validate-url`. `--validate-url` will still work for backwards compatibility.
 - [#936](https://github.com/oauth2-proxy/oauth2-proxy/pull/936) `--user-id-claim` option is deprecated and replaced by `--oidc-email-claim`
 - [#630](https://github.com/oauth2-proxy/oauth2-proxy/pull/630) Gitlab projects needs a Gitlab application with the extra `read_api` enabled
 - [#849](https://github.com/oauth2-proxy/oauth2-proxy/pull/849) `/oauth2/auth` `allowed_groups` querystring parameter can be paired with the `allowed-groups` configuration option.
@@ -33,6 +35,8 @@
 
 ## Breaking Changes
 
+- [#953](https://github.com/oauth2-proxy/oauth2-proxy/pull/953) In config files & envvar configs, `keycloak_group` is now the plural `keycloak_groups`.
+  Flag configs are still `--keycloak-group` but it can be passed multiple times.
 - [#911](https://github.com/oauth2-proxy/oauth2-proxy/pull/911) Specifying a non-existent provider will cause OAuth2-Proxy to fail on startup instead of defaulting to "google".
 - [#797](https://github.com/oauth2-proxy/oauth2-proxy/pull/797) Security changes to Google provider group authorization flow
   - If you change the list of allowed groups, existing sessions that now don't have a valid group will be logged out immediately.
@@ -54,6 +58,7 @@
 
 ## Changes since v6.1.1
 
+- [#953](https://github.com/oauth2-proxy/oauth2-proxy/pull/953) Migrate Keycloak to EnrichSession & support multiple groups for authorization (@NickMeves)
 - [#630](https://github.com/oauth2-proxy/oauth2-proxy/pull/630) Add support for Gitlab project based authentication (@factorysh)
 - [#907](https://github.com/oauth2-proxy/oauth2-proxy/pull/907) Introduce alpha configuration option to enable testing of structured configuration (@JoelSpeed)
 - [#938](https://github.com/oauth2-proxy/oauth2-proxy/pull/938) Cleanup missed provider renaming refactor methods (@NickMeves)

docs/docs/configuration/auth.md

@@ -135,15 +135,25 @@ If you are using GitHub enterprise, make sure you set the following to the appro
 
 Make sure you set the following to the appropriate url:
 
-    -provider=keycloak
-    -client-id=<client you have created>
-    -client-secret=<your client's secret>
-    -login-url="http(s)://<keycloak host>/auth/realms/<your realm>/protocol/openid-connect/auth"
-    -redeem-url="http(s)://<keycloak host>/auth/realms/<your realm>/protocol/openid-connect/token"
-    -validate-url="http(s)://<keycloak host>/auth/realms/<your realm>/protocol/openid-connect/userinfo"
-    -keycloak-group=<user_group>
+    --provider=keycloak
+    --client-id=<client you have created>
+    --client-secret=<your client's secret>
+    --login-url="http(s)://<keycloak host>/auth/realms/<your realm>/protocol/openid-connect/auth"
+    --redeem-url="http(s)://<keycloak host>/auth/realms/<your realm>/protocol/openid-connect/token"
+    --profile-url="http(s)://<keycloak host>/auth/realms/<your realm>/protocol/openid-connect/userinfo"
+    --validate-url="http(s)://<keycloak host>/auth/realms/<your realm>/protocol/openid-connect/userinfo"
+    --keycloak-group=<first_allowed_user_group>
+    --keycloak-group=<second_allowed_user_group>
+    
+For group based authorization, the optional `--keycloak-group` (legacy) or `--allowed-group` (global standard)
+flags can be used to specify which groups to limit access to.
 
-The group management in keycloak is using a tree. If you create a group named admin in keycloak you should define the 'keycloak-group' value to /admin.
+If these are unset but a `groups` mapper is set up above in step (3), the provider will still
+populate the `X-Forwarded-Groups` header to your upstream server with the `groups` data in the
+Keycloak userinfo endpoint response.
+
+The group management in keycloak is using a tree. If you create a group named admin in keycloak
+you should define the 'keycloak-group' value to /admin.
 
 ### GitLab Auth Provider
 

pkg/apis/options/options.go

@@ -36,7 +36,7 @@ type Options struct {
 	TLSKeyFile         string   `flag:"tls-key-file" cfg:"tls_key_file"`
 
 	AuthenticatedEmailsFile  string   `flag:"authenticated-emails-file" cfg:"authenticated_emails_file"`
-	KeycloakGroup            string   `flag:"keycloak-group" cfg:"keycloak_group"`
+	KeycloakGroups           []string `flag:"keycloak-group" cfg:"keycloak_groups"`
 	AzureTenant              string   `flag:"azure-tenant" cfg:"azure_tenant"`
 	BitbucketTeam            string   `flag:"bitbucket-team" cfg:"bitbucket_team"`
 	BitbucketRepository      string   `flag:"bitbucket-repository" cfg:"bitbucket_repository"`
@@ -181,7 +181,7 @@ func NewFlagSet() *pflag.FlagSet {
 
 	flagSet.StringSlice("email-domain", []string{}, "authenticate emails with the specified domain (may be given multiple times). Use * to authenticate any email")
 	flagSet.StringSlice("whitelist-domain", []string{}, "allowed domains for redirection after authentication. Prefix domain with a . to allow subdomains (eg .example.com)")
-	flagSet.String("keycloak-group", "", "restrict login to members of this group.")
+	flagSet.StringSlice("keycloak-group", []string{}, "restrict logins to members of these groups (may be given multiple times)")
 	flagSet.String("azure-tenant", "common", "go to a tenant-specific or common (tenant-independent) endpoint.")
 	flagSet.String("bitbucket-team", "", "restrict logins to members of this team")
 	flagSet.String("bitbucket-repository", "", "restrict logins to user with access to this repository")

pkg/validation/options.go

@@ -263,7 +263,10 @@ func parseProviderInfo(o *options.Options, msgs []string) []string {
 		p.SetRepo(o.GitHubRepo, o.GitHubToken)
 		p.SetUsers(o.GitHubUsers)
 	case *providers.KeycloakProvider:
-		p.SetGroup(o.KeycloakGroup)
+		// Backwards compatibility with `--keycloak-group` option
+		if len(o.KeycloakGroups) > 0 {
+			p.SetAllowedGroups(o.KeycloakGroups)
+		}
 	case *providers.GoogleProvider:
 		if o.GoogleServiceAccountJSON != "" {
 			file, err := os.Open(o.GoogleServiceAccountJSON)

providers/keycloak.go

@@ -2,6 +2,7 @@ package providers
 
 import (
 	"context"
+	"fmt"
 	"net/url"
 
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions"
@@ -11,7 +12,6 @@ import (
 
 type KeycloakProvider struct {
 	*ProviderData
-	Group string
 }
 
 var _ Provider = (*KeycloakProvider)(nil)
@@ -47,6 +47,7 @@ var (
 	}
 )
 
+// NewKeycloakProvider creates a KeyCloakProvider using the passed ProviderData
 func NewKeycloakProvider(p *ProviderData) *KeycloakProvider {
 	p.setProviderDefaults(providerDefaults{
 		name:        keycloakProviderName,
@@ -59,41 +60,39 @@ func NewKeycloakProvider(p *ProviderData) *KeycloakProvider {
 	return &KeycloakProvider{ProviderData: p}
 }
 
-func (p *KeycloakProvider) SetGroup(group string) {
-	p.Group = group
-}
+// EnrichSession uses the Keycloak userinfo endpoint to populate the session's
+// email and groups.
+func (p *KeycloakProvider) EnrichSession(ctx context.Context, s *sessions.SessionState) error {
+	// Fallback to ValidateURL if ProfileURL not set for legacy compatibility
+	profileURL := p.ValidateURL.String()
+	if p.ProfileURL.String() != "" {
+		profileURL = p.ProfileURL.String()
+	}
 
-func (p *KeycloakProvider) GetEmailAddress(ctx context.Context, s *sessions.SessionState) (string, error) {
-	json, err := requests.New(p.ValidateURL.String()).
+	json, err := requests.New(profileURL).
 		WithContext(ctx).
 		SetHeader("Authorization", "Bearer "+s.AccessToken).
 		Do().
 		UnmarshalJSON()
 	if err != nil {
-		logger.Errorf("failed making request %s", err)
-		return "", err
+		logger.Errorf("failed making request %v", err)
+		return err
 	}
 
-	if p.Group != "" {
-		var groups, err = json.Get("groups").Array()
-		if err != nil {
-			logger.Printf("groups not found %s", err)
-			return "", err
-		}
-
-		var found = false
-		for i := range groups {
-			if groups[i].(string) == p.Group {
-				found = true
-				break
+	groups, err := json.Get("groups").StringArray()
+	if err == nil {
+		for _, group := range groups {
+			if group != "" {
+				s.Groups = append(s.Groups, group)
 			}
 		}
-
-		if !found {
-			logger.Printf("group not found, access denied")
-			return "", nil
-		}
 	}
 
-	return json.Get("email").String()
+	email, err := json.Get("email").String()
+	if err != nil {
+		return fmt.Errorf("unable to extract email from userinfo endpoint: %v", err)
+	}
+	s.Email = email
+
+	return nil
 }

providers/keycloak_test.go

@@ -2,17 +2,24 @@ package providers
 
 import (
 	"context"
+	"errors"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
-	"testing"
 
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions"
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/ginkgo/extensions/table"
 	. "github.com/onsi/gomega"
-	"github.com/stretchr/testify/assert"
 )
 
-func testKeycloakProvider(hostname, group string) *KeycloakProvider {
+const (
+	keycloakAccessToken  = "eyJKeycloak.eyJAccess.Token"
+	keycloakUserinfoPath = "/api/v3/user"
+)
+
+func testKeycloakProvider(backend *httptest.Server) (*KeycloakProvider, error) {
 	p := NewKeycloakProvider(
 		&ProviderData{
 			ProviderName: "",
@@ -22,142 +29,182 @@ func testKeycloakProvider(hostname, group string) *KeycloakProvider {
 			ValidateURL:  &url.URL{},
 			Scope:        ""})
 
-	if group != "" {
-		p.SetGroup(group)
-	}
+	if backend != nil {
+		bURL, err := url.Parse(backend.URL)
+		if err != nil {
+			return nil, err
+		}
+		hostname := bURL.Host
 
-	if hostname != "" {
 		updateURL(p.Data().LoginURL, hostname)
 		updateURL(p.Data().RedeemURL, hostname)
 		updateURL(p.Data().ProfileURL, hostname)
 		updateURL(p.Data().ValidateURL, hostname)
 	}
-	return p
+
+	return p, nil
 }
 
-func testKeycloakBackend(payload string) *httptest.Server {
-	path := "/api/v3/user"
+var _ = Describe("Keycloak Provider Tests", func() {
+	Context("New Provider Init", func() {
+		It("uses defaults", func() {
+			providerData := NewKeycloakProvider(&ProviderData{}).Data()
+			Expect(providerData.ProviderName).To(Equal("Keycloak"))
+			Expect(providerData.LoginURL.String()).To(Equal("https://keycloak.org/oauth/authorize"))
+			Expect(providerData.RedeemURL.String()).To(Equal("https://keycloak.org/oauth/token"))
+			Expect(providerData.ProfileURL.String()).To(Equal(""))
+			Expect(providerData.ValidateURL.String()).To(Equal("https://keycloak.org/api/v3/user"))
+			Expect(providerData.Scope).To(Equal("api"))
+		})
 
-	return httptest.NewServer(http.HandlerFunc(
-		func(w http.ResponseWriter, r *http.Request) {
-			url := r.URL
-			if url.Path != path {
-				w.WriteHeader(404)
-			} else if !IsAuthorizedInHeader(r.Header) {
-				w.WriteHeader(403)
-			} else {
-				w.WriteHeader(200)
-				w.Write([]byte(payload))
-			}
-		}))
-}
+		It("overrides defaults", func() {
+			p := NewKeycloakProvider(
+				&ProviderData{
+					LoginURL: &url.URL{
+						Scheme: "https",
+						Host:   "example.com",
+						Path:   "/oauth/auth"},
+					RedeemURL: &url.URL{
+						Scheme: "https",
+						Host:   "example.com",
+						Path:   "/oauth/token"},
+					ProfileURL: &url.URL{
+						Scheme: "https",
+						Host:   "example.com",
+						Path:   "/api/v3/user"},
+					ValidateURL: &url.URL{
+						Scheme: "https",
+						Host:   "example.com",
+						Path:   "/api/v3/user"},
+					Scope: "profile"})
+			providerData := p.Data()
 
-func TestKeycloakProviderDefaults(t *testing.T) {
-	p := testKeycloakProvider("", "")
-	assert.NotEqual(t, nil, p)
-	assert.Equal(t, "Keycloak", p.Data().ProviderName)
-	assert.Equal(t, "https://keycloak.org/oauth/authorize",
-		p.Data().LoginURL.String())
-	assert.Equal(t, "https://keycloak.org/oauth/token",
-		p.Data().RedeemURL.String())
-	assert.Equal(t, "https://keycloak.org/api/v3/user",
-		p.Data().ValidateURL.String())
-	assert.Equal(t, "api", p.Data().Scope)
-}
+			Expect(providerData.ProviderName).To(Equal("Keycloak"))
+			Expect(providerData.LoginURL.String()).To(Equal("https://example.com/oauth/auth"))
+			Expect(providerData.RedeemURL.String()).To(Equal("https://example.com/oauth/token"))
+			Expect(providerData.ProfileURL.String()).To(Equal("https://example.com/api/v3/user"))
+			Expect(providerData.ValidateURL.String()).To(Equal("https://example.com/api/v3/user"))
+			Expect(providerData.Scope).To(Equal("profile"))
+		})
+	})
 
-func TestNewKeycloakProvider(t *testing.T) {
-	g := NewWithT(t)
+	Context("EnrichSession", func() {
+		type enrichSessionTableInput struct {
+			backendHandler http.HandlerFunc
+			expectedError  error
+			expectedEmail  string
+			expectedGroups []string
+		}
 
-	// Test that defaults are set when calling for a new provider with nothing set
-	providerData := NewKeycloakProvider(&ProviderData{}).Data()
-	g.Expect(providerData.ProviderName).To(Equal("Keycloak"))
-	g.Expect(providerData.LoginURL.String()).To(Equal("https://keycloak.org/oauth/authorize"))
-	g.Expect(providerData.RedeemURL.String()).To(Equal("https://keycloak.org/oauth/token"))
-	g.Expect(providerData.ProfileURL.String()).To(Equal(""))
-	g.Expect(providerData.ValidateURL.String()).To(Equal("https://keycloak.org/api/v3/user"))
-	g.Expect(providerData.Scope).To(Equal("api"))
-}
+		DescribeTable("should return expected results",
+			func(in enrichSessionTableInput) {
+				backend := httptest.NewServer(in.backendHandler)
+				p, err := testKeycloakProvider(backend)
+				Expect(err).To(BeNil())
 
-func TestKeycloakProviderOverrides(t *testing.T) {
-	p := NewKeycloakProvider(
-		&ProviderData{
-			LoginURL: &url.URL{
-				Scheme: "https",
-				Host:   "example.com",
-				Path:   "/oauth/auth"},
-			RedeemURL: &url.URL{
-				Scheme: "https",
-				Host:   "example.com",
-				Path:   "/oauth/token"},
-			ValidateURL: &url.URL{
-				Scheme: "https",
-				Host:   "example.com",
-				Path:   "/api/v3/user"},
-			Scope: "profile"})
-	assert.NotEqual(t, nil, p)
-	assert.Equal(t, "Keycloak", p.Data().ProviderName)
-	assert.Equal(t, "https://example.com/oauth/auth",
-		p.Data().LoginURL.String())
-	assert.Equal(t, "https://example.com/oauth/token",
-		p.Data().RedeemURL.String())
-	assert.Equal(t, "https://example.com/api/v3/user",
-		p.Data().ValidateURL.String())
-	assert.Equal(t, "profile", p.Data().Scope)
-}
+				p.ProfileURL, err = url.Parse(
+					fmt.Sprintf("%s%s", backend.URL, keycloakUserinfoPath),
+				)
+				Expect(err).To(BeNil())
 
-func TestKeycloakProviderGetEmailAddress(t *testing.T) {
-	b := testKeycloakBackend("{\"email\": \"michael.bland@gsa.gov\"}")
-	defer b.Close()
+				session := &sessions.SessionState{AccessToken: keycloakAccessToken}
+				err = p.EnrichSession(context.Background(), session)
 
-	bURL, _ := url.Parse(b.URL)
-	p := testKeycloakProvider(bURL.Host, "")
+				if in.expectedError != nil {
+					Expect(err).To(Equal(in.expectedError))
+				} else {
+					Expect(err).To(BeNil())
+				}
 
-	session := CreateAuthorizedSession()
-	email, err := p.GetEmailAddress(context.Background(), session)
-	assert.Equal(t, nil, err)
-	assert.Equal(t, "michael.bland@gsa.gov", email)
-}
+				Expect(session.Email).To(Equal(in.expectedEmail))
 
-func TestKeycloakProviderGetEmailAddressAndGroup(t *testing.T) {
-	b := testKeycloakBackend("{\"email\": \"michael.bland@gsa.gov\", \"groups\": [\"test-grp1\", \"test-grp2\"]}")
-	defer b.Close()
-
-	bURL, _ := url.Parse(b.URL)
-	p := testKeycloakProvider(bURL.Host, "test-grp1")
-
-	session := CreateAuthorizedSession()
-	email, err := p.GetEmailAddress(context.Background(), session)
-	assert.Equal(t, nil, err)
-	assert.Equal(t, "michael.bland@gsa.gov", email)
-}
-
-// Note that trying to trigger the "failed building request" case is not
-// practical, since the only way it can fail is if the URL fails to parse.
-func TestKeycloakProviderGetEmailAddressFailedRequest(t *testing.T) {
-	b := testKeycloakBackend("unused payload")
-	defer b.Close()
-
-	bURL, _ := url.Parse(b.URL)
-	p := testKeycloakProvider(bURL.Host, "")
-
-	// We'll trigger a request failure by using an unexpected access
-	// token. Alternatively, we could allow the parsing of the payload as
-	// JSON to fail.
-	session := &sessions.SessionState{AccessToken: "unexpected_access_token"}
-	email, err := p.GetEmailAddress(context.Background(), session)
-	assert.NotEqual(t, nil, err)
-	assert.Equal(t, "", email)
-}
-
-func TestKeycloakProviderGetEmailAddressEmailNotPresentInPayload(t *testing.T) {
-	b := testKeycloakBackend("{\"foo\": \"bar\"}")
-	defer b.Close()
-
-	bURL, _ := url.Parse(b.URL)
-	p := testKeycloakProvider(bURL.Host, "")
-
-	session := CreateAuthorizedSession()
-	email, err := p.GetEmailAddress(context.Background(), session)
-	assert.NotEqual(t, nil, err)
-	assert.Equal(t, "", email)
-}
+				if in.expectedGroups != nil {
+					Expect(session.Groups).To(Equal(in.expectedGroups))
+				} else {
+					Expect(session.Groups).To(BeNil())
+				}
+			},
+			Entry("email and multiple groups", enrichSessionTableInput{
+				backendHandler: func(w http.ResponseWriter, _ *http.Request) {
+					w.WriteHeader(200)
+					_, err := w.Write([]byte(`
+						{
+							"email": "michael.bland@gsa.gov",
+							"groups": [
+								"test-grp1",
+								"test-grp2"
+							]
+						}
+					`))
+					if err != nil {
+						panic(err)
+					}
+				},
+				expectedError:  nil,
+				expectedEmail:  "michael.bland@gsa.gov",
+				expectedGroups: []string{"test-grp1", "test-grp2"},
+			}),
+			Entry("email and single group", enrichSessionTableInput{
+				backendHandler: func(w http.ResponseWriter, _ *http.Request) {
+					w.WriteHeader(200)
+					_, err := w.Write([]byte(`
+						{
+							"email": "michael.bland@gsa.gov",
+							"groups": ["test-grp1"]
+						}
+					`))
+					if err != nil {
+						panic(err)
+					}
+				},
+				expectedError:  nil,
+				expectedEmail:  "michael.bland@gsa.gov",
+				expectedGroups: []string{"test-grp1"},
+			}),
+			Entry("email and no groups", enrichSessionTableInput{
+				backendHandler: func(w http.ResponseWriter, _ *http.Request) {
+					w.WriteHeader(200)
+					_, err := w.Write([]byte(`
+						{
+							"email": "michael.bland@gsa.gov"
+						}
+					`))
+					if err != nil {
+						panic(err)
+					}
+				},
+				expectedError:  nil,
+				expectedEmail:  "michael.bland@gsa.gov",
+				expectedGroups: nil,
+			}),
+			Entry("missing email", enrichSessionTableInput{
+				backendHandler: func(w http.ResponseWriter, _ *http.Request) {
+					w.WriteHeader(200)
+					_, err := w.Write([]byte(`
+						{
+							"groups": [
+								"test-grp1",
+								"test-grp2"
+							]
+						}
+					`))
+					if err != nil {
+						panic(err)
+					}
+				},
+				expectedError: errors.New(
+					"unable to extract email from userinfo endpoint: type assertion to string failed"),
+				expectedEmail:  "",
+				expectedGroups: []string{"test-grp1", "test-grp2"},
+			}),
+			Entry("request failure", enrichSessionTableInput{
+				backendHandler: func(w http.ResponseWriter, _ *http.Request) {
+					w.WriteHeader(500)
+				},
+				expectedError:  errors.New(`unexpected status "500": `),
+				expectedEmail:  "",
+				expectedGroups: nil,
+			}),
+		)
+	})
+})