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/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..a1e4f064 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) @@ -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 } diff --git a/providers/keycloak_test.go b/providers/keycloak_test.go index 3f419f2e..513be643 100644 --- a/providers/keycloak_test.go +++ b/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, + }), + ) + }) +})