From 3369799853efcdd8a7fdb8e9cf3e8a7a2ba49f3b Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Sat, 12 Dec 2020 12:57:32 -0800 Subject: [PATCH 1/6] Migrate Keycloak to EnrichSession & support multiple groups --- pkg/apis/options/options.go | 4 +- pkg/validation/options.go | 5 +- providers/keycloak.go | 44 +++---- providers/keycloak_test.go | 232 ++++++++++++++++++++++++------------ 4 files changed, 183 insertions(+), 102 deletions(-) diff --git a/pkg/apis/options/options.go b/pkg/apis/options/options.go index 46cdcedb..c0f91422 100644 --- a/pkg/apis/options/options.go +++ b/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") diff --git a/pkg/validation/options.go b/pkg/validation/options.go index 4fc0b0a4..52b0fb69 100644 --- a/pkg/validation/options.go +++ b/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) diff --git a/providers/keycloak.go b/providers/keycloak.go index 60b3eaca..f6c74880 100644 --- a/providers/keycloak.go +++ b/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) @@ -59,41 +59,33 @@ func NewKeycloakProvider(p *ProviderData) *KeycloakProvider { return &KeycloakProvider{ProviderData: p} } -func (p *KeycloakProvider) SetGroup(group string) { - p.Group = group -} - -func (p *KeycloakProvider) GetEmailAddress(ctx context.Context, s *sessions.SessionState) (string, error) { +func (p *KeycloakProvider) EnrichSession(ctx context.Context, s *sessions.SessionState) error { json, err := requests.New(p.ValidateURL.String()). 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 { + logger.Errorf("Warning: unable to extract groups from userinfo endpoint: %v", err) + } else { + 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 } diff --git a/providers/keycloak_test.go b/providers/keycloak_test.go index 3f419f2e..d7ae4391 100644 --- a/providers/keycloak_test.go +++ b/providers/keycloak_test.go @@ -2,17 +2,33 @@ 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" + + // Userinfo Test Cases + tcUIStandard = "userinfo-standard" + tcUIFail = "userinfo-fail" + tcUISingleGroup = "userinfo-single-group" + tcUIMissingEmail = "userinfo-missing-email" + tcUIMissingGroups = "userinfo-missing-groups" +) + +func testKeycloakProvider(backend *httptest.Server) (*KeycloakProvider, error) { p := NewKeycloakProvider( &ProviderData{ ProviderName: "", @@ -22,38 +38,165 @@ 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" - +func testKeycloakBackend() *httptest.Server { return httptest.NewServer(http.HandlerFunc( func(w http.ResponseWriter, r *http.Request) { - url := r.URL - if url.Path != path { + if r.URL.Path != keycloakUserinfoPath { w.WriteHeader(404) - } else if !IsAuthorizedInHeader(r.Header) { - w.WriteHeader(403) - } else { + } + + var err error + switch r.URL.Query().Get("testcase") { + case tcUIStandard: w.WriteHeader(200) - w.Write([]byte(payload)) + _, err = w.Write([]byte(` + { + "email": "michael.bland@gsa.gov", + "groups": [ + "test-grp1", + "test-grp2" + ] + } + `)) + case tcUIFail: + w.WriteHeader(500) + case tcUISingleGroup: + w.WriteHeader(200) + _, err = w.Write([]byte(` + { + "email": "michael.bland@gsa.gov", + "groups": ["test-grp1"] + } + `)) + case tcUIMissingEmail: + w.WriteHeader(200) + _, err = w.Write([]byte(` + { + "groups": [ + "test-grp1", + "test-grp2" + ] + } + `)) + case tcUIMissingGroups: + w.WriteHeader(200) + _, err = w.Write([]byte(` + { + "email": "michael.bland@gsa.gov" + } + `)) + default: + w.WriteHeader(404) + } + if err != nil { + panic(err) } })) } +var _ = Describe("Keycloak Provider Tests", func() { + var p *KeycloakProvider + var b *httptest.Server + + BeforeEach(func() { + b = testKeycloakBackend() + + var err error + p, err = testKeycloakProvider(b) + Expect(err).To(BeNil()) + }) + + AfterEach(func() { + b.Close() + }) + + Context("EnrichSession", func() { + type enrichSessionTableInput struct { + testcase string + expectedError error + expectedEmail string + expectedGroups []string + } + + DescribeTable("should return expected results", + func(in enrichSessionTableInput) { + var err error + p.ValidateURL, err = url.Parse( + fmt.Sprintf("%s%s?testcase=%s", b.URL, keycloakUserinfoPath, in.testcase), + ) + Expect(err).To(BeNil()) + + session := &sessions.SessionState{AccessToken: keycloakAccessToken} + err = p.EnrichSession(context.Background(), session) + + if in.expectedError != nil { + Expect(err).To(Equal(in.expectedError)) + } else { + Expect(err).To(BeNil()) + } + + Expect(session.Email).To(Equal(in.expectedEmail)) + + if in.expectedGroups != nil { + Expect(session.Groups).To(Equal(in.expectedGroups)) + } else { + Expect(session.Groups).To(BeNil()) + } + }, + Entry("email and multiple groups", enrichSessionTableInput{ + testcase: tcUIStandard, + expectedError: nil, + expectedEmail: "michael.bland@gsa.gov", + expectedGroups: []string{"test-grp1", "test-grp2"}, + }), + Entry("email and single group", enrichSessionTableInput{ + testcase: tcUISingleGroup, + expectedError: nil, + expectedEmail: "michael.bland@gsa.gov", + expectedGroups: []string{"test-grp1"}, + }), + Entry("email and no groups", enrichSessionTableInput{ + testcase: tcUIMissingGroups, + expectedError: nil, + expectedEmail: "michael.bland@gsa.gov", + expectedGroups: nil, + }), + Entry("missing email", enrichSessionTableInput{ + testcase: tcUIMissingEmail, + 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{ + testcase: tcUIFail, + expectedError: errors.New(`unexpected status "500": `), + expectedEmail: "", + expectedGroups: nil, + }), + ) + }) +}) + func TestKeycloakProviderDefaults(t *testing.T) { - p := testKeycloakProvider("", "") + p, err := testKeycloakProvider(nil) + assert.NoError(t, err) assert.NotEqual(t, nil, p) assert.Equal(t, "Keycloak", p.Data().ProviderName) assert.Equal(t, "https://keycloak.org/oauth/authorize", @@ -104,60 +247,3 @@ func TestKeycloakProviderOverrides(t *testing.T) { p.Data().ValidateURL.String()) assert.Equal(t, "profile", p.Data().Scope) } - -func TestKeycloakProviderGetEmailAddress(t *testing.T) { - b := testKeycloakBackend("{\"email\": \"michael.bland@gsa.gov\"}") - defer b.Close() - - bURL, _ := url.Parse(b.URL) - p := testKeycloakProvider(bURL.Host, "") - - session := CreateAuthorizedSession() - email, err := p.GetEmailAddress(context.Background(), session) - assert.Equal(t, nil, err) - assert.Equal(t, "michael.bland@gsa.gov", email) -} - -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) -} From 0886f8035cdf877328cbf2bc0f05d50848922ab5 Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Sat, 12 Dec 2020 13:14:57 -0800 Subject: [PATCH 2/6] Move all Keycloak unit tests to Ginkgo --- providers/keycloak_test.go | 236 +++++++++++++++++-------------------- 1 file changed, 110 insertions(+), 126 deletions(-) diff --git a/providers/keycloak_test.go b/providers/keycloak_test.go index d7ae4391..b3920810 100644 --- a/providers/keycloak_test.go +++ b/providers/keycloak_test.go @@ -7,20 +7,18 @@ import ( "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" ) const ( keycloakAccessToken = "eyJKeycloak.eyJAccess.Token" keycloakUserinfoPath = "/api/v3/user" - // Userinfo Test Cases + // Userinfo Test Cases querystring toggles tcUIStandard = "userinfo-standard" tcUIFail = "userinfo-fail" tcUISingleGroup = "userinfo-single-group" @@ -111,139 +109,125 @@ func testKeycloakBackend() *httptest.Server { } var _ = Describe("Keycloak Provider Tests", func() { - var p *KeycloakProvider - var b *httptest.Server + 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")) + }) - BeforeEach(func() { - b = testKeycloakBackend() + 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"}, + ValidateURL: &url.URL{ + Scheme: "https", + Host: "example.com", + Path: "/api/v3/user"}, + Scope: "profile"}) + providerData := p.Data() - var err error - p, err = testKeycloakProvider(b) - Expect(err).To(BeNil()) + 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("")) + Expect(providerData.ValidateURL.String()).To(Equal("https://example.com/api/v3/user")) + Expect(providerData.Scope).To(Equal("profile")) + }) }) - AfterEach(func() { - b.Close() - }) + Context("With a test HTTP Server & Provider", func() { + var p *KeycloakProvider + var b *httptest.Server - Context("EnrichSession", func() { - type enrichSessionTableInput struct { - testcase string - expectedError error - expectedEmail string - expectedGroups []string - } + BeforeEach(func() { + b = testKeycloakBackend() - DescribeTable("should return expected results", - func(in enrichSessionTableInput) { - var err error - p.ValidateURL, err = url.Parse( - fmt.Sprintf("%s%s?testcase=%s", b.URL, keycloakUserinfoPath, in.testcase), - ) - Expect(err).To(BeNil()) + var err error + p, err = testKeycloakProvider(b) + Expect(err).To(BeNil()) + }) - session := &sessions.SessionState{AccessToken: keycloakAccessToken} - err = p.EnrichSession(context.Background(), session) + AfterEach(func() { + b.Close() + }) - if in.expectedError != nil { - Expect(err).To(Equal(in.expectedError)) - } else { + Context("EnrichSession", func() { + type enrichSessionTableInput struct { + testcase string + expectedError error + expectedEmail string + expectedGroups []string + } + + DescribeTable("should return expected results", + func(in enrichSessionTableInput) { + var err error + p.ValidateURL, err = url.Parse( + fmt.Sprintf("%s%s?testcase=%s", b.URL, keycloakUserinfoPath, in.testcase), + ) Expect(err).To(BeNil()) - } - Expect(session.Email).To(Equal(in.expectedEmail)) + session := &sessions.SessionState{AccessToken: keycloakAccessToken} + err = p.EnrichSession(context.Background(), session) - if in.expectedGroups != nil { - Expect(session.Groups).To(Equal(in.expectedGroups)) - } else { - Expect(session.Groups).To(BeNil()) - } - }, - Entry("email and multiple groups", enrichSessionTableInput{ - testcase: tcUIStandard, - expectedError: nil, - expectedEmail: "michael.bland@gsa.gov", - expectedGroups: []string{"test-grp1", "test-grp2"}, - }), - Entry("email and single group", enrichSessionTableInput{ - testcase: tcUISingleGroup, - expectedError: nil, - expectedEmail: "michael.bland@gsa.gov", - expectedGroups: []string{"test-grp1"}, - }), - Entry("email and no groups", enrichSessionTableInput{ - testcase: tcUIMissingGroups, - expectedError: nil, - expectedEmail: "michael.bland@gsa.gov", - expectedGroups: nil, - }), - Entry("missing email", enrichSessionTableInput{ - testcase: tcUIMissingEmail, - 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{ - testcase: tcUIFail, - expectedError: errors.New(`unexpected status "500": `), - expectedEmail: "", - expectedGroups: nil, - }), - ) + if in.expectedError != nil { + Expect(err).To(Equal(in.expectedError)) + } else { + Expect(err).To(BeNil()) + } + + Expect(session.Email).To(Equal(in.expectedEmail)) + + if in.expectedGroups != nil { + Expect(session.Groups).To(Equal(in.expectedGroups)) + } else { + Expect(session.Groups).To(BeNil()) + } + }, + Entry("email and multiple groups", enrichSessionTableInput{ + testcase: tcUIStandard, + expectedError: nil, + expectedEmail: "michael.bland@gsa.gov", + expectedGroups: []string{"test-grp1", "test-grp2"}, + }), + Entry("email and single group", enrichSessionTableInput{ + testcase: tcUISingleGroup, + expectedError: nil, + expectedEmail: "michael.bland@gsa.gov", + expectedGroups: []string{"test-grp1"}, + }), + Entry("email and no groups", enrichSessionTableInput{ + testcase: tcUIMissingGroups, + expectedError: nil, + expectedEmail: "michael.bland@gsa.gov", + expectedGroups: nil, + }), + Entry("missing email", enrichSessionTableInput{ + testcase: tcUIMissingEmail, + 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{ + testcase: tcUIFail, + expectedError: errors.New(`unexpected status "500": `), + expectedEmail: "", + expectedGroups: nil, + }), + ) + }) }) }) - -func TestKeycloakProviderDefaults(t *testing.T) { - p, err := testKeycloakProvider(nil) - assert.NoError(t, err) - 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) -} - -func TestNewKeycloakProvider(t *testing.T) { - g := NewWithT(t) - - // 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")) -} - -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) -} From 138a6b128a95f658da7920f523f840f7bd09c0c6 Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Sat, 12 Dec 2020 13:22:15 -0800 Subject: [PATCH 3/6] Use ProfileURL for userinfo EnrichSession calls in Keycloak --- providers/keycloak.go | 11 ++++++++++- providers/keycloak_test.go | 8 ++++++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/providers/keycloak.go b/providers/keycloak.go index f6c74880..66eda948 100644 --- a/providers/keycloak.go +++ b/providers/keycloak.go @@ -47,6 +47,7 @@ var ( } ) +// NewKeycloakProvider creates a KeyCloakProvider using the passed ProviderData func NewKeycloakProvider(p *ProviderData) *KeycloakProvider { p.setProviderDefaults(providerDefaults{ name: keycloakProviderName, @@ -59,8 +60,16 @@ func NewKeycloakProvider(p *ProviderData) *KeycloakProvider { return &KeycloakProvider{ProviderData: p} } +// 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 { - json, err := requests.New(p.ValidateURL.String()). + // Fallback to ValidateURL if ProfileURL not set for legacy compatibility + userinfoURL := p.ValidateURL.String() + if p.ProfileURL != nil { + userinfoURL = p.ProfileURL.String() + } + + json, err := requests.New(userinfoURL). WithContext(ctx). SetHeader("Authorization", "Bearer "+s.AccessToken). Do(). diff --git a/providers/keycloak_test.go b/providers/keycloak_test.go index b3920810..7c0b457b 100644 --- a/providers/keycloak_test.go +++ b/providers/keycloak_test.go @@ -131,6 +131,10 @@ var _ = Describe("Keycloak Provider Tests", func() { 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", @@ -141,7 +145,7 @@ var _ = Describe("Keycloak Provider Tests", func() { 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("")) + 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")) }) @@ -174,7 +178,7 @@ var _ = Describe("Keycloak Provider Tests", func() { DescribeTable("should return expected results", func(in enrichSessionTableInput) { var err error - p.ValidateURL, err = url.Parse( + p.ProfileURL, err = url.Parse( fmt.Sprintf("%s%s?testcase=%s", b.URL, keycloakUserinfoPath, in.testcase), ) Expect(err).To(BeNil()) From f07a5630f1138ce8364edda3de585a907c44d210 Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Sat, 12 Dec 2020 13:50:34 -0800 Subject: [PATCH 4/6] Update Keycloak documentation --- CHANGELOG.md | 5 +++++ docs/docs/configuration/auth.md | 26 ++++++++++++++++++-------- providers/keycloak.go | 4 +--- 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index edb631d6..6af45d6a 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/docs/docs/configuration/auth.md b/docs/docs/configuration/auth.md index 7c5bac39..f16f2e26 100644 --- a/docs/docs/configuration/auth.md +++ b/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-secret= - -login-url="http(s):///auth/realms//protocol/openid-connect/auth" - -redeem-url="http(s):///auth/realms//protocol/openid-connect/token" - -validate-url="http(s):///auth/realms//protocol/openid-connect/userinfo" - -keycloak-group= + --provider=keycloak + --client-id= + --client-secret= + --login-url="http(s):///auth/realms//protocol/openid-connect/auth" + --redeem-url="http(s):///auth/realms//protocol/openid-connect/token" + --profile-url="http(s):///auth/realms//protocol/openid-connect/userinfo" + --validate-url="http(s):///auth/realms//protocol/openid-connect/userinfo" + --keycloak-group= + --keycloak-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 diff --git a/providers/keycloak.go b/providers/keycloak.go index 66eda948..03d3194c 100644 --- a/providers/keycloak.go +++ b/providers/keycloak.go @@ -80,9 +80,7 @@ func (p *KeycloakProvider) EnrichSession(ctx context.Context, s *sessions.Sessio } groups, err := json.Get("groups").StringArray() - if err != nil { - logger.Errorf("Warning: unable to extract groups from userinfo endpoint: %v", err) - } else { + if err == nil { for _, group := range groups { if group != "" { s.Groups = append(s.Groups, group) From 816d9a4566de86608c11ab70ae1161b71a325361 Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Mon, 21 Dec 2020 13:46:54 -0800 Subject: [PATCH 5/6] Use a generic http.HandlerFunc in Keycloak tests --- providers/keycloak_test.go | 245 +++++++++++++++++-------------------- 1 file changed, 109 insertions(+), 136 deletions(-) diff --git a/providers/keycloak_test.go b/providers/keycloak_test.go index 7c0b457b..513be643 100644 --- a/providers/keycloak_test.go +++ b/providers/keycloak_test.go @@ -17,13 +17,6 @@ import ( const ( keycloakAccessToken = "eyJKeycloak.eyJAccess.Token" keycloakUserinfoPath = "/api/v3/user" - - // Userinfo Test Cases querystring toggles - tcUIStandard = "userinfo-standard" - tcUIFail = "userinfo-fail" - tcUISingleGroup = "userinfo-single-group" - tcUIMissingEmail = "userinfo-missing-email" - tcUIMissingGroups = "userinfo-missing-groups" ) func testKeycloakProvider(backend *httptest.Server) (*KeycloakProvider, error) { @@ -52,62 +45,6 @@ func testKeycloakProvider(backend *httptest.Server) (*KeycloakProvider, error) { return p, nil } -func testKeycloakBackend() *httptest.Server { - return httptest.NewServer(http.HandlerFunc( - func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path != keycloakUserinfoPath { - w.WriteHeader(404) - } - - var err error - switch r.URL.Query().Get("testcase") { - case tcUIStandard: - w.WriteHeader(200) - _, err = w.Write([]byte(` - { - "email": "michael.bland@gsa.gov", - "groups": [ - "test-grp1", - "test-grp2" - ] - } - `)) - case tcUIFail: - w.WriteHeader(500) - case tcUISingleGroup: - w.WriteHeader(200) - _, err = w.Write([]byte(` - { - "email": "michael.bland@gsa.gov", - "groups": ["test-grp1"] - } - `)) - case tcUIMissingEmail: - w.WriteHeader(200) - _, err = w.Write([]byte(` - { - "groups": [ - "test-grp1", - "test-grp2" - ] - } - `)) - case tcUIMissingGroups: - w.WriteHeader(200) - _, err = w.Write([]byte(` - { - "email": "michael.bland@gsa.gov" - } - `)) - default: - w.WriteHeader(404) - } - if err != nil { - panic(err) - } - })) -} - var _ = Describe("Keycloak Provider Tests", func() { Context("New Provider Init", func() { It("uses defaults", func() { @@ -151,87 +88,123 @@ var _ = Describe("Keycloak Provider Tests", func() { }) }) - Context("With a test HTTP Server & Provider", func() { - var p *KeycloakProvider - var b *httptest.Server + Context("EnrichSession", func() { + type enrichSessionTableInput struct { + backendHandler http.HandlerFunc + expectedError error + expectedEmail string + expectedGroups []string + } - BeforeEach(func() { - b = testKeycloakBackend() + DescribeTable("should return expected results", + func(in enrichSessionTableInput) { + backend := httptest.NewServer(in.backendHandler) + p, err := testKeycloakProvider(backend) + Expect(err).To(BeNil()) - var err error - p, err = testKeycloakProvider(b) - Expect(err).To(BeNil()) - }) + p.ProfileURL, err = url.Parse( + fmt.Sprintf("%s%s", backend.URL, keycloakUserinfoPath), + ) + Expect(err).To(BeNil()) - AfterEach(func() { - b.Close() - }) + session := &sessions.SessionState{AccessToken: keycloakAccessToken} + err = p.EnrichSession(context.Background(), session) - Context("EnrichSession", func() { - type enrichSessionTableInput struct { - testcase string - expectedError error - expectedEmail string - expectedGroups []string - } - - DescribeTable("should return expected results", - func(in enrichSessionTableInput) { - var err error - p.ProfileURL, err = url.Parse( - fmt.Sprintf("%s%s?testcase=%s", b.URL, keycloakUserinfoPath, in.testcase), - ) + if in.expectedError != nil { + Expect(err).To(Equal(in.expectedError)) + } else { Expect(err).To(BeNil()) + } - session := &sessions.SessionState{AccessToken: keycloakAccessToken} - err = p.EnrichSession(context.Background(), session) + Expect(session.Email).To(Equal(in.expectedEmail)) - if in.expectedError != nil { - Expect(err).To(Equal(in.expectedError)) - } else { - Expect(err).To(BeNil()) - } - - Expect(session.Email).To(Equal(in.expectedEmail)) - - if in.expectedGroups != nil { - Expect(session.Groups).To(Equal(in.expectedGroups)) - } else { - Expect(session.Groups).To(BeNil()) + 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) } }, - Entry("email and multiple groups", enrichSessionTableInput{ - testcase: tcUIStandard, - expectedError: nil, - expectedEmail: "michael.bland@gsa.gov", - expectedGroups: []string{"test-grp1", "test-grp2"}, - }), - Entry("email and single group", enrichSessionTableInput{ - testcase: tcUISingleGroup, - expectedError: nil, - expectedEmail: "michael.bland@gsa.gov", - expectedGroups: []string{"test-grp1"}, - }), - Entry("email and no groups", enrichSessionTableInput{ - testcase: tcUIMissingGroups, - expectedError: nil, - expectedEmail: "michael.bland@gsa.gov", - expectedGroups: nil, - }), - Entry("missing email", enrichSessionTableInput{ - testcase: tcUIMissingEmail, - 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{ - testcase: tcUIFail, - expectedError: errors.New(`unexpected status "500": `), - expectedEmail: "", - expectedGroups: nil, - }), - ) - }) + 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, + }), + ) }) }) From 4b28e6886cdba86ac03f5b74c88947c238c80600 Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Tue, 22 Dec 2020 21:34:15 -0800 Subject: [PATCH 6/6] Handle ValidateURL fallback for nil & empty struct cases --- providers/keycloak.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/providers/keycloak.go b/providers/keycloak.go index 03d3194c..a1e4f064 100644 --- a/providers/keycloak.go +++ b/providers/keycloak.go @@ -64,12 +64,12 @@ func NewKeycloakProvider(p *ProviderData) *KeycloakProvider { // email and groups. func (p *KeycloakProvider) EnrichSession(ctx context.Context, s *sessions.SessionState) error { // Fallback to ValidateURL if ProfileURL not set for legacy compatibility - userinfoURL := p.ValidateURL.String() - if p.ProfileURL != nil { - userinfoURL = p.ProfileURL.String() + profileURL := p.ValidateURL.String() + if p.ProfileURL.String() != "" { + profileURL = p.ProfileURL.String() } - json, err := requests.New(userinfoURL). + json, err := requests.New(profileURL). WithContext(ctx). SetHeader("Authorization", "Bearer "+s.AccessToken). Do().