feat: filter sso groups based on regex #11774

basanthjenuhb · 2023-09-08T03:59:44Z

Fixes #10153 #9530

Motivation

Currently users logging in via SSO with lot of groups, exceed the 4KB Cookie limit
This happens because we send the full set of claims as part of cookie
Adding capability to filter sso groups matching a regex.
This would eliminate all SSO groups which won't be used in SSO RBAC and reduce the number of groups in the authorization cookie

Modifications

Add a property filterGroupsRegex in SSO config
Filter groups based on the property

Verification

Tested locally with filterGroupsRegex

Example usage:

scopes:
- groups
- email
- profile
rbac:
  enabled: true
filterGroupsRegex:
- ".*argowf.*"

Signed-off-by: bjenuhb <Basanth_JenuHB@intuit.com>

agilgur5 · 2023-09-08T06:22:16Z

config/sso.go

@@ -21,6 +21,7 @@ type SSOConfig struct {
 	CustomGroupClaimName string `json:"customGroupClaimName,omitempty"`
 	UserInfoPath         string `json:"userInfoPath,omitempty"`
 	InsecureSkipVerify   bool   `json:"insecureSkipVerify,omitempty"`
+	FilterSSOGroupsRegex string `json:"filterSSOGroupsRegex,omitempty"`


Suggested change

FilterSSOGroupsRegex string `json:"filterSSOGroupsRegex,omitempty"`

FilterGroupsRegex string `json:"filterGroupsRegex,omitempty"`

"SSO" is redundant here as this is a field of SSOConfig

agilgur5

Would an allowlist of groups suffice as well? Or is a regex necessary for the amount of relevant groups?

server/auth/sso/sso.go

basanthjenuhb · 2023-09-08T06:34:12Z

Would an allowlist of groups suffice as well? Or is a regex necessary for the amount of relevant groups?

Reason for regex is to be able to add groups dynamically. And be able to filter our groups which will be relevant for argo SSO rbac. And reduce number of groups to bring the cookie size below 4kb

with allowlist we would need to update config and restart everytime we want to add a new group

Signed-off-by: bjenuhb <Basanth_JenuHB@intuit.com>

juliev0

Good idea. So, for a given cluster there would be only one particular regex that can be matched, right? I wonder if we should make it an array of regex?

sarabala1979

LGTM

sarabala1979 · 2023-09-08T15:13:41Z

@basanthjenuhb Can you add the document for this?
@terrytangyuan can we include this feature in v3.5?

basanthjenuhb · 2023-09-08T15:16:01Z

Good idea. So, for a given cluster there would be only one particular regex that can be matched, right? I wonder if we should make it an array of regex?

yeah
we could make it an array
but wanted to keep it simple and see how it works out for us
we can take it up as an enhancement after we see this one’s usage

basanthjenuhb · 2023-09-08T15:27:24Z

@basanthjenuhb Can you add the document for this? @terrytangyuan can we include this feature in v3.5?

#11778

agilgur5

So, for a given cluster there would be only one particular regex that can be matched, right? I wonder if we should make it an array of regex?

Julie brings up a good point here

we could make it an array
but wanted to keep it simple and see how it works out for us
we can take it up as an enhancement after we see this one’s usage

Adding a feature afterward is a bit easier said than done -- there would then be two features that need to be maintained for backward-compatibility.

If we can get ahead of a problem now, it would certainly aid maintainability

agilgur5 · 2023-09-08T15:59:14Z

with allowlist we would need to update config

correct. that is not necessarily a bad thing though. I'm not opposed to regex but there is a security trade-off.

restart everytime we want to add a new group

there is a separate issue to improve that: #10970

Signed-off-by: bjenuhb <Basanth_JenuHB@intuit.com>

agilgur5 · 2023-09-08T20:50:59Z

So, for a given cluster there would be only one particular regex that can be matched, right? I wonder if we should make it an array of regex?

Julie brings up a good point here

Wait a minute, maybe I spoke too soon. This is a regex after all, you can do "or" conditions within a regex. Would that suffice? That would actually be simpler than having to tell users whether the array is "or" vs. "and"

Sorry I haven't been home all day, responded from my phone. Apologies if this results in rework 🙇
Will take a closer look later. Also want to check how Dex handles this case

juliev0 · 2023-09-08T22:14:47Z

Wait a minute, maybe I spoke too soon. This is a regex after all, you can do "or" conditions within a regex. Would that suffice? That would actually be simpler than having to tell users whether the array is "or" vs. "and"

You're right. Just looked that up. It's more doable than I thought it was.

basanthjenuhb · 2023-09-09T00:55:48Z

Wait a minute, maybe I spoke too soon. This is a regex after all, you can do "or" conditions within a regex. Would that suffice? That would actually be simpler than having to tell users whether the array is "or" vs. "and"

yes, I had looked it up
regex does support OR. it would look like (.*patternA.*|.*patternB.*), although doable, its just not very clean compared to

filterGroupsRegex:
- ".*patternA.*"
- ".*patternB.*"

I would prefer list over single regex if the idea is to support multiple patterns.
anyways, let me know your preference. Will make the changes accordingly

Signed-off-by: bjenuhb <Basanth_JenuHB@intuit.com>

juliev0 · 2023-09-09T01:42:29Z

I would prefer list over single regex if the idea is to support multiple patterns. anyways, let me know your preference. Will make the changes accordingly

I agree if you’re open to changing it. Thanks

basanthjenuhb · 2023-09-09T02:06:00Z

I agree if you’re open to changing it. Thanks

already did

Signed-off-by: bjenuhb <Basanth_JenuHB@intuit.com>

agilgur5

there's some stylistic issues in the code (it was spliced in between another block of related code), see below

server/auth/sso/sso.go

agilgur5 · 2023-09-09T20:58:40Z

I agree if you’re open to changing it. Thanks

I don't have too strong an opinion on this so long as the documentation makes clear that the list is "OR"'d together. Otherwise it may be ambiguous to users. I mentioned this in my docs review

A singular regex does not have any ambiguity; that would be the main benefit (also simpler code)

agilgur5 · 2023-09-09T21:21:25Z

server/auth/sso/sso.go

@@ -280,6 +293,18 @@ func (s *sso) HandleCallback(w http.ResponseWriter, r *http.Request) {
 			return
 		}
 	}
+	if s.filterGroupsRegex != nil && len(s.filterGroupsRegex) > 0 {
+		var filteredGroups []string


it would be great if the provider could do this logic so that the response from it is not so large, but it does not seem there is a uniform provider interface for that.

Azure AD implements group filtering per app

LDAP more generally has group search

There is a related Dex issue on this (dexidp/dex#1476) and generalizing it was basically closed out due to provider-specific nuances.

So I think there may be no way around filtering in the Server code unfortunately.

If a user has an IdP proxy like Dex though, they can do this group filter logic within Dex. As we get into more complex scenarios like these, we may want to consider limiting the scope of the internal implementation and forwarding users to Dex et al instead.

Signed-off-by: bjenuhb <Basanth_JenuHB@intuit.com>

server/auth/sso/sso.go

Signed-off-by: bjenuhb <Basanth_JenuHB@intuit.com>

agilgur5

LGTM. Thanks for iterating on this!

terrytangyuan · 2023-09-10T23:28:40Z

@terrytangyuan can we include this feature in v3.5?

@sarabala1979 Sorry just saw it. Let's discuss this in the next contributors meeting on v3.5 release strategy. #11381

Signed-off-by: bjenuhb <Basanth_JenuHB@intuit.com> Co-authored-by: bjenuhb <Basanth_JenuHB@intuit.com>

Signed-off-by: bjenuhb <Basanth_JenuHB@intuit.com> Co-authored-by: bjenuhb <Basanth_JenuHB@intuit.com> Signed-off-by: Dillen Padhiar <dillen_padhiar@intuit.com>

bjenuhb added 2 commits September 8, 2023 09:09

feat: filter sso groups based on regex

d8dfc5a

Signed-off-by: bjenuhb <Basanth_JenuHB@intuit.com>

ci: empty commit for build

44b84a3

Signed-off-by: bjenuhb <Basanth_JenuHB@intuit.com>

agilgur5 added the area/sso-rbac label Sep 8, 2023

agilgur5 reviewed Sep 8, 2023

View reviewed changes

server/auth/sso/sso.go Outdated Show resolved Hide resolved

feat: pre initialize regex

38c2b77

Signed-off-by: bjenuhb <Basanth_JenuHB@intuit.com>

basanthjenuhb requested a review from agilgur5 September 8, 2023 06:52

ci: empty commit for build

e15387a

Signed-off-by: bjenuhb <Basanth_JenuHB@intuit.com>

juliev0 reviewed Sep 8, 2023

View reviewed changes

sarabala1979 approved these changes Sep 8, 2023

View reviewed changes

basanthjenuhb mentioned this pull request Sep 8, 2023

feat: document usage of filterGroupsRegex #11778

Merged

agilgur5 reviewed Sep 8, 2023

View reviewed changes

bjenuhb added 2 commits September 8, 2023 21:51

feat: make filterGroupRegex a list

1dfd1eb

Signed-off-by: bjenuhb <Basanth_JenuHB@intuit.com>

feat: make filterGroupRegex a list

b1ea29b

Signed-off-by: bjenuhb <Basanth_JenuHB@intuit.com>

ci: empty commit for build

e469614

Signed-off-by: bjenuhb <Basanth_JenuHB@intuit.com>

ci: empty commit for build

157b137

Signed-off-by: bjenuhb <Basanth_JenuHB@intuit.com>

juliev0 approved these changes Sep 9, 2023

View reviewed changes

agilgur5 reviewed Sep 9, 2023

View reviewed changes

server/auth/sso/sso.go Show resolved Hide resolved

server/auth/sso/sso.go Show resolved Hide resolved

agilgur5 reviewed Sep 9, 2023

View reviewed changes

bjenuhb added 2 commits September 10, 2023 03:29

fix: stylistic issues

4194ac8

Signed-off-by: bjenuhb <Basanth_JenuHB@intuit.com>

fix: stylistic issues

6b6742b

Signed-off-by: bjenuhb <Basanth_JenuHB@intuit.com>

agilgur5 reviewed Sep 9, 2023

View reviewed changes

server/auth/sso/sso.go Outdated Show resolved Hide resolved

feat: log filterGroupsRegex

3358f57

Signed-off-by: bjenuhb <Basanth_JenuHB@intuit.com>

agilgur5 approved these changes Sep 10, 2023

View reviewed changes

juliev0 merged commit 477b3ca into argoproj:master Sep 10, 2023
24 checks passed

agilgur5 mentioned this pull request Sep 22, 2023

REQUEST: Promotion to Reviewer for @agilgur5 argoproj/argoproj#242

Closed

8 tasks

terrytangyuan mentioned this pull request Oct 10, 2023

Release v3.4 patch releases discussion #11851

Open

terrytangyuan pushed a commit that referenced this pull request Oct 19, 2023

feat: filter sso groups based on regex (#11774)

f87aba3

Signed-off-by: bjenuhb <Basanth_JenuHB@intuit.com> Co-authored-by: bjenuhb <Basanth_JenuHB@intuit.com>

agilgur5 mentioned this pull request Jul 8, 2024

token not valid for running mode because more than 130 AD groups and token too large #11277

Closed

3 tasks

agilgur5 added this to the v3.4.x patches milestone Jul 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: filter sso groups based on regex #11774

feat: filter sso groups based on regex #11774

basanthjenuhb commented Sep 8, 2023 •

edited by agilgur5

Loading

agilgur5 Sep 8, 2023

basanthjenuhb Sep 8, 2023

agilgur5 left a comment

basanthjenuhb commented Sep 8, 2023 •

edited

Loading

juliev0 left a comment

sarabala1979 left a comment

sarabala1979 commented Sep 8, 2023

basanthjenuhb commented Sep 8, 2023

basanthjenuhb commented Sep 8, 2023

agilgur5 left a comment

agilgur5 commented Sep 8, 2023 •

edited

Loading

agilgur5 commented Sep 8, 2023 •

edited

Loading

juliev0 commented Sep 8, 2023 •

edited by agilgur5

Loading

basanthjenuhb commented Sep 9, 2023 •

edited by agilgur5

Loading

juliev0 commented Sep 9, 2023 •

edited by agilgur5

Loading

basanthjenuhb commented Sep 9, 2023 •

edited

Loading

agilgur5 left a comment

agilgur5 commented Sep 9, 2023 •

edited

Loading

agilgur5 Sep 9, 2023 •

edited

Loading

agilgur5 left a comment

terrytangyuan commented Sep 10, 2023

	FilterSSOGroupsRegex string `json:"filterSSOGroupsRegex,omitempty"`
	FilterGroupsRegex string `json:"filterGroupsRegex,omitempty"`

feat: filter sso groups based on regex #11774

feat: filter sso groups based on regex #11774

Conversation

basanthjenuhb commented Sep 8, 2023 • edited by agilgur5 Loading

Motivation

Modifications

Verification

Example usage:

agilgur5 Sep 8, 2023

Choose a reason for hiding this comment

basanthjenuhb Sep 8, 2023

Choose a reason for hiding this comment

agilgur5 left a comment

Choose a reason for hiding this comment

basanthjenuhb commented Sep 8, 2023 • edited Loading

juliev0 left a comment

Choose a reason for hiding this comment

sarabala1979 left a comment

Choose a reason for hiding this comment

sarabala1979 commented Sep 8, 2023

basanthjenuhb commented Sep 8, 2023

basanthjenuhb commented Sep 8, 2023

agilgur5 left a comment

Choose a reason for hiding this comment

agilgur5 commented Sep 8, 2023 • edited Loading

agilgur5 commented Sep 8, 2023 • edited Loading

juliev0 commented Sep 8, 2023 • edited by agilgur5 Loading

basanthjenuhb commented Sep 9, 2023 • edited by agilgur5 Loading

juliev0 commented Sep 9, 2023 • edited by agilgur5 Loading

basanthjenuhb commented Sep 9, 2023 • edited Loading

agilgur5 left a comment

Choose a reason for hiding this comment

agilgur5 commented Sep 9, 2023 • edited Loading

agilgur5 Sep 9, 2023 • edited Loading

Choose a reason for hiding this comment

agilgur5 left a comment

Choose a reason for hiding this comment

terrytangyuan commented Sep 10, 2023

basanthjenuhb commented Sep 8, 2023 •

edited by agilgur5

Loading

basanthjenuhb commented Sep 8, 2023 •

edited

Loading

agilgur5 commented Sep 8, 2023 •

edited

Loading

agilgur5 commented Sep 8, 2023 •

edited

Loading

juliev0 commented Sep 8, 2023 •

edited by agilgur5

Loading

basanthjenuhb commented Sep 9, 2023 •

edited by agilgur5

Loading

juliev0 commented Sep 9, 2023 •

edited by agilgur5

Loading

basanthjenuhb commented Sep 9, 2023 •

edited

Loading

agilgur5 commented Sep 9, 2023 •

edited

Loading

agilgur5 Sep 9, 2023 •

edited

Loading