oidc provider assumes username or email claim exists and is stable, VP should use `sub`

[rhansen]

When oauth.provider is set to oidc, Vouch assumes that either username or email exists in the UserInfo response. If neither exists (as is the case with GitLab when scope=openid), structs.User.Username is the empty string and the /validate endpoint fails with "no User found in jwt".

Furthermore, the username (or email) claim is used as a unique identifier for the user (e.g., in the user whitelist). This is forbidden by section 5.7 of the core spec: "The sub (subject) and iss (issuer) Claims, used together, are the only Claims that an RP can rely upon as a stable identifier for the End-User, [...] other Claims such as email, phone_number, and preferred_username MUST NOT be used as unique identifiers for the End-User."

[bnfinet]

@rhansen Thanks for opening this issue.

There has been talk from time to time of moving to sub and it's clearly the right choice.

I know you've tested the PR against gitlab, have you had an opportunity to test any other providers?

[rhansen]

I know you've tested the PR against gitlab, have you had an opportunity to test any other providers?

No, I have not.

[bnfinet]

@rhansen sorry for the delay in reviewing this PR

Unfortunately #310 breaks github support. All of the GetUserInfo calls should be audited for each provider to ensure that User.Sub is populated. Are you able to support such an effort?

[bnfinet]

@rhansen I've added the Sub adjustment for github and a rudimentary fix in structs.PrepareUserData but it should still be looked at a bit closer IMHO.

[UberKitten]

I noticed this issue today when playing around with authenticating guest users in Azure AD through OIDC. In AAD guest users are not in the directory themselves, just authorized to log in to the directory with an external email. The user info has sub and email but no username so it fails.

[bnfinet]

@UberKitten that sounds like #314

[UberKitten]

@bnfinet Ah yes, it is. I've been getting the "no User found in jwt" error but that's the root cause. Thanks!

[rissson]

I ran against this problem too. I need this fix, because I need to have multiple domains in vouch.domains. Unfortunately not all my users have an email address in those domains and thus get denied login.

[bnfinet]

@rissson have you tried 'allowAllUsers' config item?

[rissson]

Yes, that works, but then I can't use vouch proxy under different domains.

[bnfinet]

@rissson If your users are not all within the set of protected domains you may need to run multiple VPs, one for each domain, each with vouch.allowAllUsers and its own oauth.callback_url.

domains is meant for orgs in which all users have email addresses within the specified domain.

[bnfinet]

@rhansen are you still interested in supporting PR #310 ? I'd love to work with you to get it merged.

[djcrabhat]

We designed a custom OIDC system where that's all UserInfo is giving back (and checking token's validity). Per https://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse

The sub (subject) Claim MUST always be returned in the UserInfo Response.

VouchCookie gets set, when /validate is called, it's errors "no User found in jwt", because the Username field is blank https://github.com/vouch/vouch-proxy/blob/master/handlers/validate.go#L48 Would love if we could get this to work. I know moving Sub up to the common user object is breaking github (that's not an "official" UserInfo endpoint, so I understand). Can we make a new custom OidcUser struct, add Sub to that? Then a PrepareUserData like

func (u *OidcUser) PrepareUserData() {
	if u.Username == "" {
	        if u.email == "" {
                   u.Username = u.Sub
		} else {
		    u.Username = u.Email
               }
	}
}

that way, at least /validate has something to fall back on.

edit: sorry, finally read PR #310 and saw @bnfinet 's fix for github, think you've got the problem described well. Think you solved it better than my little hack of jamming Sub in to Username, and instead making Sub the real heart of the user.

[bnfinet]

@djcrabhat thanks for the kind words and for taking a look at this.

Have you tested #310 against your OIDC setup? Does it work?

from Dec 6th to @rhansen ..

Unfortunately #310 breaks github support. All of the GetUserInfo calls should be audited for each provider to ensure that User.Sub is populated. Are you able to support such an effort?

Would you be in a position to perform such an audit and/or add tests for such?

[djcrabhat]

@bnfinet will compile and test locally tomorrow (hopefully). Once I get that going I'd be happy to take a whack at getting it well exercised in the tests.

[plachor]

Hi @bnfinet, any idea when can we except this to be merged ?

In our scenario we do not expose email claim and sub is only unique identifier of an user (which is true according to spec). So in order to work with Vouch we would need to customize our internals, where in our system not everyone has email attached. When it comes to username it seems it is not a standard claim for oidc https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims.

Also for case when authentication is enough (vouch.allowAllUsers is true) sub should be sufficient so user-info endpoint should not be required ?

[bnfinet]

@plachor no I can't provide you with an estimate at this time. Thanks for your patience.

Everything else you say seems correct.

[plachor]

Thnx for clarification.

[rhansen]

@bnfinet I'm not using vouch anymore and unfortunately don't have much time to invest in fixing this issue. I rebased PR #310 onto latest master and cleaned up the commits a bit in case someone wants to pick up where I left off. The changes are untested (other than GitHub actions).

[ShyLionTjmn]

I have allowAllUsers: true, but still, user gets redirect loop, if it does not have email. This makes it pain in back, when using AD authentication.