Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom OIDC groups claim along with allowed groups query parameter always returns 403 #1730

Closed
andrebarrett opened this issue Jul 15, 2022 · 20 comments

Comments

@andrebarrett
Copy link

andrebarrett commented Jul 15, 2022

  • I am using Auth0 as the OIDC provider. They only allow custom claims that are name-spaced. Therefore I can't use the default "groups" as the oidc_groups_claim. I override this in the configuration with a custom claim string (see below under steps to reproduce)

  • I get a successful 202 response from oauth proxy when there is no allowed_groups query parameter (see ngnix config below)

  • As soon as I add an allowed_groups query parameter with a single group to verify I get a forbidden 403 error

  • This response is incorrect as the user logged in is part of the group specified by the custom claim (see token below)

  • I have looked through the code and I believe oauthproxy.go checkAllowedGroups is returning false due to the session State not containing the group

Expected Behavior

The method checkAllowedGroups should return true if the ID Token contains a custom group claim matching the query parameter. This would then mean oauth proxy returns 202 and lets the request through

Current Behavior

Either the session state does not contain the groups based on the oidc_groups_claim OR it is not correctly matching from the query parameter

Possible Solution

The code seems pretty well unit tested so I'm struggling to see where this could be going wrong. Perhaps a test around the extractAllowedEntities function
The CreateSessionFromToken function seems to be well unit tested. However, I can't find a test that verifies token parsing with a custom groups claim actually populates the session state Group correctly.

I am sure this is a small fix. I imagine it to only be a small code change. Any assistance pointing me in the right direction would be great.

Steps to Reproduce (for bugs)

Legacy configuration

provider = "oidc" 
oidc_groups_claim = "https://members.com/groups"
email_domains = ["*"]
cookie_domains=".the-app-company.com"
whitelist_domains=[".the-app-company.com"]
reverse_proxy = true

...

Nginx configuration

  location / {

    auth_request /internal-auth/;

    error_page 401 = https://oauth2.the-app-company.com/oauth2/start?rd=$scheme://$host$request_uri;

    proxy_pass http:/allowed_upstream_server;
  }

  location /internal-auth/ {
    internal; # Ensure external users can't access this path

    # Make sure the OAuth2 Proxy knows where the original request came from.
    proxy_set_header Host       $host;
    proxy_set_header X-Real-IP  $remote_addr;
    proxy_pass_request_body off;
    proxy_set_header Content-Length "";
    proxy_set_header X-Original-URI $request_uri;

   # This is the only way to send allowed groups to oauth proxy otherwise the ? get's encoded to %3F and no longer matches the correct path
    proxy_pass http://oauth2-proxy:4180/oauth2/auth?allowed_groups=member;
  }

JWT ID Token

{
  "https://members.com/groups": [
    "member"
  ],
  "nickname": "andrebarrett",
  "name": "Andre",
  "picture": "https://s.gravatar.com/avatar/d461c627c81d53ac46c143ae49df2ed5?s=480&r=pg&d=https%3A%2F%2Fcdn-icons-png.flaticon.com%2F512%2F149%2F149071.png",
  "updated_at": "2022-07-15T06:06:48.428Z",
  "email": "andrebarrett@hotmail.co.uk",
  "email_verified": true,
  "iss": "https://dev-bka5l41i.auth0.com/",
  "sub": "auth0|5e9b5e5c63f8000c8c20d541",
  "aud": "uU2CJnAgxtI5yH81viwlrTlB9p8k5qMn",
  "iat": 1657865210,
  "exp": 1657901210
}

User Info Endpoint

{
	"sub": "auth0|5e9b5e5c63f8000c8c20d541",
	"nickname": "andrebarrett",
	"name": "Andre",
	"picture": "https://s.gravatar.com/avatar/d461c627c81d53ac46c143ae49df2ed5?s=480&r=pg&d=https%3A%2F%2Fcdn-icons-png.flaticon.com%2F512%2F149%2F149071.png",
	"updated_at": "2022-07-15T06:06:48.428Z",
	"email": "andrebarrett@hotmail.co.uk",
	"email_verified": true,
	"https://members.com/groups": ["member"]
}

Steps to reproduce

  1. Use OIDC provider such as Auth0
  2. Use a custom groups claim in config
  3. Use nginx auth_request module
  4. Have a allowed_groups query with a group go to the oauth proxy pass
  5. Login using a user that is part of the allowed group
  6. See the request fail with a 403

Context

I am trying to secure various upstream websites.

  • Only members of the admin group should be able to access some of the websites (metrics.domain.com for example).
  • Individuals without a group should be able to access the public website (www.domain.com for example)
  • And only subscribed members should be able to access the portal (members.domain.com for example)

Your Environment

Tested with a docker compose setup. Used Ubuntu for both containers running nginx and oauth proxy. Ran from an M1 Mac

  • Version used:
    Latest v7.3.0.linux-amd64 version, prebuilt binary downloaded from release
@andrebarrett
Copy link
Author

Ok, I found the issue. This function is where it goes wrong:

func getClaimFrom(claim string, src *simplejson.Json) interface{} {
	claimParts := strings.Split(claim, ".")
	return src.GetPath(claimParts...).Interface()
}

In my case the claim was https://members.com/groups
The string gets split by "." however which means the GetPath call ends up not finding the claim.

To workaround the issue I have changed the claim to remove the .com after which it works as expected

Not sure what to do about this one as I feel claims should be able to include a url as a namespace.

Any reason we split the string using the dot?

@anthr76
Copy link

anthr76 commented Jul 18, 2022

may be related #1686 I will have to check to see if this functionality was recently added.

@JoelSpeed
Copy link
Member

Any reason we split the string using the dot?

It's meant to be a basic implementation of JSON paths. I'd be happy to add support for escaping the period in the same way you can with JSON path (or better using some library for this) to allow this use case, but then users would need to be aware its JSON path and escape properly

@github-actions
Copy link
Contributor

github-actions bot commented Oct 8, 2022

This issue has been inactive for 60 days. If the issue is still relevant please comment to re-activate the issue. If no action is taken within 7 days, the issue will be marked closed.

@eloo
Copy link

eloo commented Oct 19, 2022

not stale

@JoelSpeed is there already any implementation of fixing the "basic json path" misbehaviour when using claims with dots?

just came across this issue while debugging here:
#1779

also it seems like this "feature" is not documented...
it also violates some best practices when working with custom claims like Auth0 documents
https://auth0.com/docs/get-started/apis/scopes/sample-use-cases-scopes-and-claims#add-custom-claims-to-a-token

@JoelSpeed JoelSpeed removed the Stale label Oct 19, 2022
@JoelSpeed JoelSpeed reopened this Oct 19, 2022
@eloo-abi eloo-abi mentioned this issue Dec 6, 2022
3 tasks
@github-actions
Copy link
Contributor

This issue has been inactive for 60 days. If the issue is still relevant please comment to re-activate the issue. If no action is taken within 7 days, the issue will be marked closed.

@github-actions github-actions bot added the Stale label Dec 19, 2022
@JoelSpeed JoelSpeed removed the Stale label Dec 19, 2022
@JoelSpeed
Copy link
Member

@JoelSpeed is there already any implementation of fixing the "basic json path" misbehaviour when using claims with dots?

I don't think there's any fix yet no

also it seems like this "feature" is not documented...

That's because it's not really complete or tested yet, the internal code is intended to support the various providers in a generic way, I hadn't really intended for it to be exposed to end users until later down the line

I scanned through the link but can't see what we are violating, can you be more explicit with what you mean with that statement?

@eloo
Copy link

eloo commented Dec 23, 2022

Hi @JoelSpeed
The current implementation violates the best practice of using namespaces as mentioned in the provided link...

E.g.

https://my.app/groups: abc

With the current code you will never get the abc group because the code splits at every dot.

@JoelSpeed
Copy link
Member

Yeah that's a fair point, I'd consider that a bug that we can fix hopefully, are you aware of any fixes that you could suggest or any libraries that aren't violating the namespacing rules?
I guess an alternative is to implement some sort of escape character in the current implementation 🤔

@eloo
Copy link

eloo commented Feb 3, 2023

maybe the lib i used in my PR could do the trick: github.com/ohler55/ojg/jp

another solution could be to make a flag to enable or disable jsonpath parsing..
so if you wanna use jsonpath parsing you should enable it (maybe thats the default) or others if you have namespaces or some special case you just disable jsonpath parsing

i guess this would be the easiest solution right now...

another approach could be the PR i have provided which is going to detect if the passed value is valid json path or not
https://github.com/oauth2-proxy/oauth2-proxy/pull/1921/files

@github-actions
Copy link
Contributor

github-actions bot commented Apr 5, 2023

This issue has been inactive for 60 days. If the issue is still relevant please comment to re-activate the issue. If no action is taken within 7 days, the issue will be marked closed.

@github-actions github-actions bot added the Stale label Apr 5, 2023
@eloo
Copy link

eloo commented Apr 5, 2023

Not stale

@JoelSpeed JoelSpeed removed the Stale label Apr 6, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

This issue has been inactive for 60 days. If the issue is still relevant please comment to re-activate the issue. If no action is taken within 7 days, the issue will be marked closed.

@github-actions github-actions bot added the Stale label Jun 6, 2023
@eloo-abi
Copy link
Contributor

eloo-abi commented Jun 6, 2023

not stale -.-

@github-actions github-actions bot removed the Stale label Jun 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 6, 2023

This issue has been inactive for 60 days. If the issue is still relevant please comment to re-activate the issue. If no action is taken within 7 days, the issue will be marked closed.

@github-actions github-actions bot added the Stale label Aug 6, 2023
@eloo
Copy link

eloo commented Aug 6, 2023

not stale

@github-actions github-actions bot removed the Stale label Aug 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2023

This issue has been inactive for 60 days. If the issue is still relevant please comment to re-activate the issue. If no action is taken within 7 days, the issue will be marked closed.

@github-actions github-actions bot added the Stale label Oct 7, 2023
@anthr76
Copy link

anthr76 commented Oct 10, 2023

not stale

@tuunit tuunit removed the Stale label Oct 10, 2023
@mruoss
Copy link

mruoss commented Oct 20, 2023

Wasn't this fixed with #1921 (and deployed with v7.5.0)?

Copy link
Contributor

This issue has been inactive for 60 days. If the issue is still relevant please comment to re-activate the issue. If no action is taken within 7 days, the issue will be marked closed.

@github-actions github-actions bot added the Stale label Dec 20, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants