Group/Role Access Restriction support in /oauth2/auth endpoint

[NickMeves]

#831

How Has This Been Tested?

Unit tests & Live Kubernetes Cluster

Checklist:

• My change requires a change to the documentation or CHANGELOG.
• I have updated the documentation/CHANGELOG accordingly.
• I have created a feature (non-master) branch for my PR.
• Interactive manual tests in Kubernetes for infinite redirect prevention design

[NickMeves]

This will conflict with the authZ refactor in #797 which will likely merge first.

When that happens, the helpers that are needed in oauthproxy & in provider.Authorize might be best refactored to a new authorization package that can host the common group membership methods.

[NickMeves]

This will potentially need to be added to the Sign In endpoint as well to avoid the infinite redirect issue users got on standard group authz support when the check was missing from the Redeem/Callback stage of setting up a session.

[JoelSpeed]

I'm a bit concerned about extending the Authenticate Only endpoint in this way, does this not add to much AuthZ to an AuthN endpoint? If the user is logged in but we don't like their groups, should we not be returning a 403? I don't think that's happening right now with this

[JoelSpeed]

One of these is pluralised, the other isn't, should they both be pluralised?

[NickMeves]

The intent was to mirror our plural behavior of configurations:

If you use the singular form, you can specify it multiple times with single values in each key=value pair.

If you use the plural form, specify multiple separated by commas.

[JoelSpeed]

The plural in the configuration is going away in the long term, not sure how I feel about having this here or whether we should just have allowed_groups and be done with it

[JoelSpeed]

I'd still lean towards having this with just one option for the query string, otherwise I think this PR is fine at this point

[NickMeves]

So go with the single comma delimited allowed_groups?

[JoelSpeed]

I would just use the same key, so allowed_groups=foo&allowed_groups=baz,bar results in [foo, bar, baz]

I think you can do something like this for the extraction process

q := url.Query()
for _, allowedGroups := range q["allowed_groups"] {
  for _, group := range strings.Split(allowedGroups, ",") {
    if group != "" {
      groups[group] = struct{}{}
    }
  }
}

[NickMeves]

Awesome, thanks! I missed the multi-case for allowed_groups when I originally ripped out allowed_group. Latest changes are pushed up.

[NickMeves]

I'm a bit concerned about extending the Authenticate Only endpoint in this way, does this not add to much AuthZ to an AuthN endpoint? If the user is logged in but we don't like their groups, should we not be returning a 403? I don't think that's happening right now with this

Technically the user endpoint is /oauth2/auth why are you assuming that is short for AuthN and not AuthZ 😝

Valid concern though, I'll need to make sure I have all the flows right -- and get everything documented well like this issue touches on #894

If we can get this right, this will be huge. This architecture completely eliminated the need for a BeyondCorp gateway vendor for my company. A seamless way for apps to customize the groups is one of the last features that some of the more mature players had -- so getting that in a scalable way would put us ahead of most COTS vendors in the space.

Otherwise we had to stand up multiple oauth2-proxies hooked into different IdP OIDC apps that had different authZ rules at that layer. Which led to less fine-grained control on a per app basis since we only stood up general categories of groups (all staff, engineering department, SREs only) to limit the number of OAuth2-Proxy deployments we stood up.

I'm wrapping up my service-to-service blog post today. Then next on my list is a post on using OAuth2-Proxy with kubernetes ingress annotations as a BeyondCorp corporate network gateway.

[NickMeves]

@JoelSpeed - this is still a WIP, but I'm curious your thoughts at my attempted mitigation for infinite redirects.

I haven't confirmed yet, but I have a strong suspicion if we return 401 from /oauth2/auth Kubernetes ingress annotation will redirect the user to the sign_in URL annotation. Which if there's already a session would zoom by and end up back at the /oauth2/auth endpoint which will return unauthorized again due to the groups membership being wrong. Hence infinity...

This is only an issue I think if the Sign In Page is disabled. So I've added a Context to the SessionState for the project to use to track internal adhoc details about the session.

We'd use that to store an unauthorized marker and save the session in the 401 codepath from /oauth2/auth. In /oauth2/start, if we are about to start the OAuth2 redeem/callback process, but we already have an existing valid session, check the session for the unauthorized marker. If it exists, make the page actually return an error page, preventing infinite loops (but clear out the marker so a refresh of the page starts the signin process again).

[JoelSpeed]

I haven't confirmed yet, but I have a strong suspicion if we return 401 from /oauth2/auth Kubernetes ingress annotation will redirect the user to the sign_in URL annotation.

That's correct to my knowledge yes

We'd use that to store an unauthorized marker and save the session in the 401 codepath from /oauth2/auth. In /oauth2/start, if we are about to start the OAuth2 redeem/callback process, but we already have an existing valid session, check the session for the unauthorized marker. If it exists, make the page actually return an error page, preventing infinite loops (but clear out the marker so a refresh of the page starts the signin process again).

I think this harks back to, if the user is logged in, but doesn't meet the Authz policy, we should return a 403 instead of a 401. I don't think that would necessarily break existing users as this would improve the UX if anything in the cases where redirect loops could exist.

When using Nginx auth request, you can handle the different errors separately as far as I'm aware, so users could configure a separate 403 handler that isn't redirecting to the sign in/start endpoint. Which I think would solve the problem.

If we are going to make this change however, we should definitely try to do so before the next major bump 🤔

I imagine the long term flow of the request through the (Session Load) > (Session AuthZ) > (ServeMux) > (AuthOnly Logic), so there would be a few ways through this:

1. Session Exists and is valid, Session Load loads session, Session AuthZ allows request through, AuthOnly logic sees session, returns 201
2. Session Exists and is invalid, Session Load loads session, Session AuthZ returns 403
3. Session does not exist, Session Load does nothing, Session AuthZ does nothing, AuthOnly logic sees no session, returns 401

[NickMeves]

I'll need to test, the use case I'm most interested in making sure we have simple since it seems to be the most popular is via Kubernetes ingress annotations to configure the nginx subrequest.

I can't seem to find documentation on how exactly nginx.ingress.kubernetes.io/auth-url works under the hood and what flavor of 4XX results in a redirect to nginx.ingress.kubernetes.io/auth-signin vs what would result in an access denied with no redirect loop.

You think 403 might be the key to nip the redirect loop in the bud?

[NickMeves]

Thanks @JoelSpeed ! StatusForbidden did the trick -- Woot!

This is ready to roll, I tested a bunch of scenarios manually on a Kubernetes cluster.

[JoelSpeed]

Have we thought about the security implications of dynamically allowing this AuthZ like this? I wonder if it might be safer if the policies could be set up in the config file and then the query string would contain the policy name to use. Does that suggestion actually make a difference, perhaps not.

Are other projects doing things similar to this do we know?

[JoelSpeed]

The plural in the configuration is going away in the long term, not sure how I feel about having this here or whether we should just have allowed_groups and be done with it

[JoelSpeed]

I can see future contributors wanting to add more to this and we will end up with a really complicated chain here, might be better to wrap this in some authz function that just callls this same logic for now, so that we clearly have authn and authz separated in the logic, WDYT?

[NickMeves]

The plural in the configuration is going away in the long term, not sure how I feel about having this here or whether we should just have allowed_groups and be done with it

I'm all for simplifying 😄

Which did you want to align on? One comma-delimited querystring of the groups (plural case). Or singular querstring case that can be specified multiple times?

[NickMeves]

I can see future contributors wanting to add more to this and we will end up with a really complicated chain here, might be better to wrap this in some authz function that just callls this same logic for now, so that we clearly have authn and authz separated in the logic, WDYT?

I'm up for making the change 👍 -- I'm having trouble visualizing the function/chain you are thinking to simplify this. Do you have a small pseudocode sample of what you had in mind?

[NickMeves]

Have we thought about the security implications of dynamically allowing this AuthZ like this? I wonder if it might be safer if the policies could be set up in the config file and then the query string would contain the policy name to use. Does that suggestion actually make a difference, perhaps not.

Are other projects doing things similar to this do we know?

From a security perspective, I've set it up so adding groups/roles via querystring can only further restrict access. It can never open up more access since this is after the central getAuthenticatedSession which will perform any global authorizations that are setup (email domain, allowed email list, allowed-groups config option). Hopefully I iterated on all the potential combinations in the unit tests so we ensure we never accidentally open up too much access.

While its an OR membership operations between groups in global & querystring allowed groups. The intersection of the two paradigms is an AND. So if global were set to R&D and Finance, while querystring turns into SRE and Backend, it would be this type of rule: session.Groups contains(R&D OR Finance) AND contains(SRE OR Backend)

I see the merit of an allowlist of customizable values to put into the querystring. But I'm also not sure if that buys us anything at the moment since I think we are secure against unwanted access attacks thinking through the threat model.

I think I'll punt for now to avoid configuration bloat, but a good idea to potentially keep in our back pockets.

[NickMeves]

Are other projects doing things similar to this do we know?

I'm not positive what other projects that intend to hook into subrequest architecture are doing to be honest or if any in the market have this added feature.

For the parallel architecture, where I've seen a lot of vendor platforms have 1 central access proxy that does host-based routing to the upstream based on the host name -- I've seen all configuration run through them, either in a config file where every registered app needs to be spelled out with their distinct rules or a web UI to manage it. -- But that is a different discussion related to the host-based routing feature that has been asked before.

I think the key distinction is in the host-based routing architecture, it is very much in the routing chain of a protected application (they even need to change CNAME to point at the proxy). So centralized configuration makes a ton of sense and truthfully is the only secure implementation path.

In the subrequest pattern, the deployment still manages its architecture and sort of uses oauth2-proxy as an addon/helper on the side (via the ingress annotations). So the querystring route is still configuration that is internal to them, but the implementation point is the ingress spec.

[NickMeves]

We're a 400 person company potentially looking at ~100 distinct web app URLs potentially wanting to hook into a central OAuth2-Proxy for BeyondCorp like access. Riding Kubernetes specs & terrform/helm for app developer controlled configuration & customization is ideal for scalability. And it is a nice, battle-tested route to manage configuration that we already have.

Even in V8 with structured configs, I am not signing up for managing 100 app's configs in the central structured configuration file as the system admin 😅

[NickMeves]

@JoelSpeed Rebased off master. Did this need any other changes?

[JoelSpeed]

LGTM, merge when you're ready

[JoelSpeed]

Just noticed one linter failure, you'll need to fix that before merge

[NickMeves]

Just noticed one linter failure, you'll need to fix that before merge

Lol - quite the recommendation: 'return !(!checkAllowedGroups(req, s))'

So this is a dummy wrapper I added per your suggestion to future proof extending authorization logic in case other scenarios were added: https://github.com/grnhse/oauth2-proxy/blob/2ceac96677d30ce6b606d1771ba90ca6c0f51c47/oauthproxy.go#L1029

I think the right fix for the linter is back to the old way of checkAllowedGroups called directly in authOnly but this way is more explicit how future contributors should add to this functionality.

Is there a magic comment to make the linter allow this style?

[JoelSpeed]

I think there is a magic comment

I think it's something like // nolint:<name of linter>, could give that a try? If we do that, add a comment why we need it and a TODO to remove it in the future

[NickMeves]

@JoelSpeed I fixed up the linter violation & added a TODO comment on when to remove it.

[JoelSpeed]

LGTM

[MnrGreg]

@JoelSpeed might we see a v7.0.0 release candidate with this commit at some stage- or will the project go straight to 7? Keen to test this out but been holding off for a release.

[NickMeves]

@MnrGreg We're likely stamping and releasing a v7 release in the next couple weeks. All the PRs we had tagged for it have merged.