From 012c155d2ad4d2911b264a5cda9b0c3c507afff3 Mon Sep 17 00:00:00 2001 From: SamTV12345 Date: Wed, 20 Nov 2024 08:46:14 +0100 Subject: [PATCH 1/4] Added validation of configured acr values (cherry picked from commit 0a5d74a7bb13554a41eaba3b7505d508fb53970a) --- pkg/apis/middleware/session.go | 2 ++ pkg/apis/options/legacy_options.go | 1 + pkg/apis/options/providers.go | 1 + pkg/apis/sessions/session_state.go | 3 +++ providers/keycloak.go | 1 + providers/keycloak_oidc.go | 1 + providers/provider_data.go | 10 ++++++++++ providers/provider_default.go | 11 +++++++++++ 8 files changed, 30 insertions(+) diff --git a/pkg/apis/middleware/session.go b/pkg/apis/middleware/session.go index 9fcd974b..4ef54b41 100644 --- a/pkg/apis/middleware/session.go +++ b/pkg/apis/middleware/session.go @@ -25,6 +25,7 @@ func CreateTokenToSessionFunc(verify VerifyFunc) TokenToSessionFunc { Verified *bool `json:"email_verified"` PreferredUsername string `json:"preferred_username"` Groups []string `json:"groups"` + Acr string `json:"acr"` } idToken, err := verify(ctx, token) @@ -49,6 +50,7 @@ func CreateTokenToSessionFunc(verify VerifyFunc) TokenToSessionFunc { User: claims.Subject, Groups: claims.Groups, PreferredUsername: claims.PreferredUsername, + Acr: claims.Acr, AccessToken: token, IDToken: token, RefreshToken: "", diff --git a/pkg/apis/options/legacy_options.go b/pkg/apis/options/legacy_options.go index 163aaa2c..8f539898 100644 --- a/pkg/apis/options/legacy_options.go +++ b/pkg/apis/options/legacy_options.go @@ -727,6 +727,7 @@ func (l *LegacyProvider) convert() (Providers, error) { provider.KeycloakConfig = KeycloakOptions{ Groups: l.KeycloakGroups, Roles: l.AllowedRoles, + ACRs: l.AcrValues, } case "keycloak": provider.KeycloakConfig = KeycloakOptions{ diff --git a/pkg/apis/options/providers.go b/pkg/apis/options/providers.go index 94c23ce1..11bbc2db 100644 --- a/pkg/apis/options/providers.go +++ b/pkg/apis/options/providers.go @@ -149,6 +149,7 @@ type KeycloakOptions struct { // Role enables to restrict login to users with role (only available when using the keycloak-oidc provider) Roles []string `json:"roles,omitempty"` + ACRs string `json:"acr,omitempty"` } type AzureOptions struct { diff --git a/pkg/apis/sessions/session_state.go b/pkg/apis/sessions/session_state.go index b5e4fc83..75c24a07 100644 --- a/pkg/apis/sessions/session_state.go +++ b/pkg/apis/sessions/session_state.go @@ -28,6 +28,7 @@ type SessionState struct { User string `msgpack:"u,omitempty"` Groups []string `msgpack:"g,omitempty"` PreferredUsername string `msgpack:"pu,omitempty"` + Acr string `msgpack:"acr,omitempty"` // Internal helpers, not serialized Clock clock.Clock `msgpack:"-"` @@ -148,6 +149,8 @@ func (s *SessionState) GetClaim(claim string) []string { return groups case "preferred_username": return []string{s.PreferredUsername} + case "acr": + return []string{s.Acr} default: return []string{} } diff --git a/providers/keycloak.go b/providers/keycloak.go index e36068c4..cc2f1bc9 100644 --- a/providers/keycloak.go +++ b/providers/keycloak.go @@ -61,6 +61,7 @@ func NewKeycloakProvider(p *ProviderData, opts options.KeycloakOptions) *Keycloa provider := &KeycloakProvider{ProviderData: p} provider.setAllowedGroups(opts.Groups) + provider.setAllowedACR(opts.ACRs) return provider } diff --git a/providers/keycloak_oidc.go b/providers/keycloak_oidc.go index ab613752..353d0748 100644 --- a/providers/keycloak_oidc.go +++ b/providers/keycloak_oidc.go @@ -26,6 +26,7 @@ func NewKeycloakOIDCProvider(p *ProviderData, opts options.Provider) *KeycloakOI } provider.addAllowedRoles(opts.KeycloakConfig.Roles) + provider.setAllowedACR(opts.KeycloakConfig.ACRs) return provider } diff --git a/providers/provider_data.go b/providers/provider_data.go index 3c59c6da..e6c3c126 100644 --- a/providers/provider_data.go +++ b/providers/provider_data.go @@ -53,6 +53,7 @@ type ProviderData struct { // Universal Group authorization data structure // any provider can set to consume AllowedGroups map[string]struct{} + AllowedACRs map[string]struct{} getAuthorizationHeaderFunc func(string) http.Header loginURLParameterDefaults url.Values @@ -181,6 +182,14 @@ func (p *ProviderData) setAllowedGroups(groups []string) { } } +func (p *ProviderData) setAllowedACR(acrs string) { + p.AllowedACRs = make(map[string]struct{}) + var resultingACRs = strings.Split(acrs, ",") + for _, acr := range resultingACRs { + p.AllowedACRs[acr] = struct{}{} + } +} + type providerDefaults struct { name string loginURL *url.URL @@ -258,6 +267,7 @@ func (p *ProviderData) buildSessionFromClaims(rawIDToken, accessToken string) (* {p.UserClaim, &ss.User}, {p.EmailClaim, &ss.Email}, {p.GroupsClaim, &ss.Groups}, + {"acr", &ss.Acr}, // TODO (@NickMeves) Deprecate for dynamic claim to session mapping {"preferred_username", &ss.PreferredUsername}, } { diff --git a/providers/provider_default.go b/providers/provider_default.go index 756b5f69..53008a33 100644 --- a/providers/provider_default.go +++ b/providers/provider_default.go @@ -3,6 +3,7 @@ package providers import ( "bytes" "context" + "encoding/json" "errors" "fmt" "net/url" @@ -116,6 +117,16 @@ func (p *ProviderData) EnrichSession(_ context.Context, _ *sessions.SessionState // Authorize performs global authorization on an authenticated session. // This is not used for fine-grained per route authorization rules. func (p *ProviderData) Authorize(_ context.Context, s *sessions.SessionState) (bool, error) { + println("ACR level is", s.Acr) + bs, _ := json.Marshal(p.AllowedACRs) + fmt.Println("Current configured acr is", string(bs)) + if len(p.AllowedACRs) > 0 { + var _, ok = p.AllowedACRs[s.Acr] + if !ok { + return false, nil + } + } + if len(p.AllowedGroups) == 0 { return true, nil } From 16cfb49fc1f9b7f36f8a729dd377fedb0499cb5d Mon Sep 17 00:00:00 2001 From: SamTV12345 Date: Wed, 20 Nov 2024 18:03:22 +0100 Subject: [PATCH 2/4] Removed debug logs --- providers/provider_default.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/providers/provider_default.go b/providers/provider_default.go index 53008a33..841fbe24 100644 --- a/providers/provider_default.go +++ b/providers/provider_default.go @@ -3,7 +3,6 @@ package providers import ( "bytes" "context" - "encoding/json" "errors" "fmt" "net/url" @@ -117,9 +116,6 @@ func (p *ProviderData) EnrichSession(_ context.Context, _ *sessions.SessionState // Authorize performs global authorization on an authenticated session. // This is not used for fine-grained per route authorization rules. func (p *ProviderData) Authorize(_ context.Context, s *sessions.SessionState) (bool, error) { - println("ACR level is", s.Acr) - bs, _ := json.Marshal(p.AllowedACRs) - fmt.Println("Current configured acr is", string(bs)) if len(p.AllowedACRs) > 0 { var _, ok = p.AllowedACRs[s.Acr] if !ok { From 9bd6a1306a026b392d276e592a33a7bff1098560 Mon Sep 17 00:00:00 2001 From: SamTV12345 <40429738+samtv12345@users.noreply.github.com> Date: Sat, 23 Nov 2024 22:11:52 +0100 Subject: [PATCH 3/4] Added test cases --- providers/provider_data.go | 3 ++- providers/provider_default_test.go | 21 +++++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/providers/provider_data.go b/providers/provider_data.go index e6c3c126..304fbd47 100644 --- a/providers/provider_data.go +++ b/providers/provider_data.go @@ -22,6 +22,7 @@ import ( const ( // This is not exported as it's not currently user configurable oidcUserClaim = "sub" + oidcAcrClaim = "acr" ) // ProviderData contains information required to configure all implementations @@ -267,7 +268,7 @@ func (p *ProviderData) buildSessionFromClaims(rawIDToken, accessToken string) (* {p.UserClaim, &ss.User}, {p.EmailClaim, &ss.Email}, {p.GroupsClaim, &ss.Groups}, - {"acr", &ss.Acr}, + {oidcAcrClaim, &ss.Acr}, // TODO (@NickMeves) Deprecate for dynamic claim to session mapping {"preferred_username", &ss.PreferredUsername}, } { diff --git a/providers/provider_default_test.go b/providers/provider_default_test.go index 80d5b4ce..520c51d8 100644 --- a/providers/provider_default_test.go +++ b/providers/provider_default_test.go @@ -75,6 +75,8 @@ func TestProviderDataAuthorize(t *testing.T) { name string allowedGroups []string groups []string + acr string + userAcr string expectedAuthZ bool }{ { @@ -101,6 +103,23 @@ func TestProviderDataAuthorize(t *testing.T) { groups: []string{"baz", "foo"}, expectedAuthZ: false, }, + { + name: "UserNotAllowedForACRLevel", + acr: "1", + expectedAuthZ: false, + }, + { + name: "UserNotAllowedForACRLevel", + acr: "1", + userAcr: "1", + expectedAuthZ: true, + }, + { + name: "UserNotAllowedForACRLevel", + acr: "2", + userAcr: "somethingElse", + expectedAuthZ: false, + }, } for _, tc := range testCases { @@ -109,9 +128,11 @@ func TestProviderDataAuthorize(t *testing.T) { session := &sessions.SessionState{ Groups: tc.groups, + Acr: tc.userAcr, } p := &ProviderData{} p.setAllowedGroups(tc.allowedGroups) + p.setAllowedACR(tc.acr) authorized, err := p.Authorize(context.Background(), session) g.Expect(err).ToNot(HaveOccurred()) From a7937a81a7baeea47816555028798a55bb73b632 Mon Sep 17 00:00:00 2001 From: SamTV12345 <40429738+samtv12345@users.noreply.github.com> Date: Thu, 9 Jan 2025 18:09:08 +0100 Subject: [PATCH 4/4] fix: add feedback from review --- pkg/apis/middleware/session.go | 4 ++-- pkg/apis/options/legacy_options.go | 1 - pkg/apis/options/providers.go | 3 ++- pkg/apis/sessions/session_state.go | 4 ++-- providers/keycloak.go | 1 - providers/keycloak_oidc.go | 1 - providers/oidc.go | 1 + providers/provider_data.go | 2 +- providers/provider_default.go | 3 +-- providers/provider_default_test.go | 2 +- 10 files changed, 10 insertions(+), 12 deletions(-) diff --git a/pkg/apis/middleware/session.go b/pkg/apis/middleware/session.go index 4ef54b41..a7d32014 100644 --- a/pkg/apis/middleware/session.go +++ b/pkg/apis/middleware/session.go @@ -25,7 +25,7 @@ func CreateTokenToSessionFunc(verify VerifyFunc) TokenToSessionFunc { Verified *bool `json:"email_verified"` PreferredUsername string `json:"preferred_username"` Groups []string `json:"groups"` - Acr string `json:"acr"` + ACR string `json:"acr"` } idToken, err := verify(ctx, token) @@ -50,7 +50,7 @@ func CreateTokenToSessionFunc(verify VerifyFunc) TokenToSessionFunc { User: claims.Subject, Groups: claims.Groups, PreferredUsername: claims.PreferredUsername, - Acr: claims.Acr, + ACR: claims.ACR, AccessToken: token, IDToken: token, RefreshToken: "", diff --git a/pkg/apis/options/legacy_options.go b/pkg/apis/options/legacy_options.go index 8f539898..163aaa2c 100644 --- a/pkg/apis/options/legacy_options.go +++ b/pkg/apis/options/legacy_options.go @@ -727,7 +727,6 @@ func (l *LegacyProvider) convert() (Providers, error) { provider.KeycloakConfig = KeycloakOptions{ Groups: l.KeycloakGroups, Roles: l.AllowedRoles, - ACRs: l.AcrValues, } case "keycloak": provider.KeycloakConfig = KeycloakOptions{ diff --git a/pkg/apis/options/providers.go b/pkg/apis/options/providers.go index 11bbc2db..b1c8be2a 100644 --- a/pkg/apis/options/providers.go +++ b/pkg/apis/options/providers.go @@ -149,7 +149,6 @@ type KeycloakOptions struct { // Role enables to restrict login to users with role (only available when using the keycloak-oidc provider) Roles []string `json:"roles,omitempty"` - ACRs string `json:"acr,omitempty"` } type AzureOptions struct { @@ -261,6 +260,8 @@ type OIDCOptions struct { // ExtraAudiences is a list of additional audiences that are allowed // to pass verification in addition to the client id. ExtraAudiences []string `json:"extraAudiences,omitempty"` + // to pass acr values to the provider + ACRs string `json:"acr,omitempty"` } type LoginGovOptions struct { diff --git a/pkg/apis/sessions/session_state.go b/pkg/apis/sessions/session_state.go index 75c24a07..d3b5b0e3 100644 --- a/pkg/apis/sessions/session_state.go +++ b/pkg/apis/sessions/session_state.go @@ -28,7 +28,7 @@ type SessionState struct { User string `msgpack:"u,omitempty"` Groups []string `msgpack:"g,omitempty"` PreferredUsername string `msgpack:"pu,omitempty"` - Acr string `msgpack:"acr,omitempty"` + ACR string `msgpack:"acr,omitempty"` // Internal helpers, not serialized Clock clock.Clock `msgpack:"-"` @@ -150,7 +150,7 @@ func (s *SessionState) GetClaim(claim string) []string { case "preferred_username": return []string{s.PreferredUsername} case "acr": - return []string{s.Acr} + return []string{s.ACR} default: return []string{} } diff --git a/providers/keycloak.go b/providers/keycloak.go index cc2f1bc9..e36068c4 100644 --- a/providers/keycloak.go +++ b/providers/keycloak.go @@ -61,7 +61,6 @@ func NewKeycloakProvider(p *ProviderData, opts options.KeycloakOptions) *Keycloa provider := &KeycloakProvider{ProviderData: p} provider.setAllowedGroups(opts.Groups) - provider.setAllowedACR(opts.ACRs) return provider } diff --git a/providers/keycloak_oidc.go b/providers/keycloak_oidc.go index 353d0748..ab613752 100644 --- a/providers/keycloak_oidc.go +++ b/providers/keycloak_oidc.go @@ -26,7 +26,6 @@ func NewKeycloakOIDCProvider(p *ProviderData, opts options.Provider) *KeycloakOI } provider.addAllowedRoles(opts.KeycloakConfig.Roles) - provider.setAllowedACR(opts.KeycloakConfig.ACRs) return provider } diff --git a/providers/oidc.go b/providers/oidc.go index 43b5227e..103948a3 100644 --- a/providers/oidc.go +++ b/providers/oidc.go @@ -46,6 +46,7 @@ func NewOIDCProvider(p *ProviderData, opts options.OIDCOptions) *OIDCProvider { } p.setProviderDefaults(oidcProviderDefaults) + p.setAllowedACR(opts.ACRs) p.getAuthorizationHeaderFunc = makeOIDCHeader return &OIDCProvider{ diff --git a/providers/provider_data.go b/providers/provider_data.go index 304fbd47..9af504e6 100644 --- a/providers/provider_data.go +++ b/providers/provider_data.go @@ -268,7 +268,7 @@ func (p *ProviderData) buildSessionFromClaims(rawIDToken, accessToken string) (* {p.UserClaim, &ss.User}, {p.EmailClaim, &ss.Email}, {p.GroupsClaim, &ss.Groups}, - {oidcAcrClaim, &ss.Acr}, + {oidcAcrClaim, &ss.ACR}, // TODO (@NickMeves) Deprecate for dynamic claim to session mapping {"preferred_username", &ss.PreferredUsername}, } { diff --git a/providers/provider_default.go b/providers/provider_default.go index 841fbe24..96f47aa4 100644 --- a/providers/provider_default.go +++ b/providers/provider_default.go @@ -117,8 +117,7 @@ func (p *ProviderData) EnrichSession(_ context.Context, _ *sessions.SessionState // This is not used for fine-grained per route authorization rules. func (p *ProviderData) Authorize(_ context.Context, s *sessions.SessionState) (bool, error) { if len(p.AllowedACRs) > 0 { - var _, ok = p.AllowedACRs[s.Acr] - if !ok { + if _, ok := p.AllowedACRs[s.ACR]; !ok { return false, nil } } diff --git a/providers/provider_default_test.go b/providers/provider_default_test.go index 520c51d8..a818604f 100644 --- a/providers/provider_default_test.go +++ b/providers/provider_default_test.go @@ -128,7 +128,7 @@ func TestProviderDataAuthorize(t *testing.T) { session := &sessions.SessionState{ Groups: tc.groups, - Acr: tc.userAcr, + ACR: tc.userAcr, } p := &ProviderData{} p.setAllowedGroups(tc.allowedGroups)