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

Check for sub claim, not username, when validating #310

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

rhansen
Copy link

@rhansen rhansen commented Aug 12, 2020

Not all IdPs provide a username or email claim in the UserInfo response.

This is a partial fix to #309. A more complete fix would:

  • Use sub and iss for whitelisting instead of username. (Or at least add sub+iss filtering as a separate option, and add warnings to username whitelisting that explain its security flaws.)
  • Extract iss from id_token and save it in structs.User and jwtmanager.VouchClaims.

bnfinet and others added 7 commits November 21, 2021 20:09
GitHub doesn't provide username, createdon, lastupdate, or sub in the
user info JSON it returns.
Not all IdPs provide a `username` or `email` claim in the UserInfo
response, and many IdPs allow users to change their username or email
address.

Co-authored-by: Benjamin Foote <git@bnf.net>
Copy link

@erictapen erictapen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using this code in my setup, I was able to solve the exact problem described in #309, so thank you!

@Firstyear
Copy link

@rhansen I think that there should also be a check here for preferred_username which is the defined claim in OIDC per:

https://openid.net/specs/openid-connect-basic-1_0.html#StandardClaims

sub should always be the "primary key" but preferred_username is the correct username claim to use, and then you should fall back to username if preferred_username was not presented.

@aaronpk
Copy link
Collaborator

aaronpk commented Feb 22, 2022

From the openid spec about the preferred_username:

The RP MUST NOT rely upon this value being unique

This is meant for display purposes, but not meant to differentiate between accounts. This is not at all the same as sub

@Firstyear
Copy link

I ... mentioned that. sub is the primary key, and preferred username is for display.

@aaronpk
Copy link
Collaborator

aaronpk commented Feb 22, 2022

Unless I'm horribly out of date, Vouch doesn't have the concept of a display username, so it should never use preferred_username

@bnfinet
Copy link
Member

bnfinet commented Feb 22, 2022

@aaronpk that's correct, VP does not utilize preferred_username. It can be passed downstream in a header but VP doesn't use it.

@mig5
Copy link
Contributor

mig5 commented Mar 30, 2022

that's correct, VP does not utilize preferred_username. It can be passed downstream in a header but VP doesn't use it.

From my experience though, Vouch does however expect a generic OIDC provider to return 'Username' in order to evaluate things such as its username 'whitelist', which is equally as wrong. This is overridden for certain specific vendor OPs supported in Vouch, but not for the generic provider.

In my own OP, the claim is called something like foobarID (the user cannot change it or influence the value of it, so I willfully ignore the warning in the spec about relying on something like it, like a username). I had to change my OP to also send a claim called Username with the same value as the foobarID claim, before I could use things like the Username 'whitelist' in the Vouch config.

The best solution would be to allow specifying the claim param that is to be treated as the Username. As has been mentioned here, the technically-correct value per the spec is that of the 'sub'. But in practical terms, for things like 'whitelist' or other evaluations on a 'human friendly' name, it would be good to be able to configure it.

Apache's mod_authz_openid for example lets you do Require claim someclaim:somevalue as an equivalent of Vouch's user 'whitelist'.

@turboMaCk
Copy link

Hello. I would like to use vouch-proxy but seeing this open since Aug 12, 2020 is sort of blocking for me. Is there any plan on how to address the issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants