user api: accept bearer tokens with multiple audiences

[ericchiang]

closes #530

cc @squat @sym3tri @chancez

[squat]

After checking ok (line 278), shouldn't we reduce clientIDs to the set of client IDs that verify successfully? Otherwise, I can imagine a case where several clients who claim to be admins but do not verify, and one verified client who is not an admin, would pass the ok check; then then the bad admins would get filtered into this new splice even though they do not verify.

[ericchiang]

The check on line 278 is verifying the token is signed by dex, has the correct issuer, isn't expired, etc. This is a bit different, it's checking if the client itself has "admin" status. I think agree that the logic could be cleaned up a bit.

So maybe, a better comment on line 278, and more obvious logic in this chunk?

[squat]

Hmm that's what I suspected. Sorry if I misunderstood, I just want to
clarify what I meant: what if all the clients except for one are
admins and all but one (the non-admin) are expired -> then 278 returns ok.
Afterwards, on this line the clients would be trimmed down to hold only the
admins (which happens to be all expired clients). Then the splice of
clients that is left holds only invalid admins. Would these clients be
given access to endpoints they should not have access to? Let me know
if this is not actually an issue.

On Monday, August 1, 2016, Eric Chiang notifications@github.com wrote:

In server/user.go
#531 (comment):

}

• if requiresAdmin && !isAdmin {

• clientIDs = clientIDs[:i]

The check on line 278 is verifying the token is signed by dex, has the
correct issuer, isn't expired, etc. This is a bit different, it's checking
if the client itself has "admin" status. I think agree that the logic could
be cleaned up a bit.

So maybe, a better comment on line 278, and more obvious logic in this
chunk?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/coreos/dex/pull/531/files/57c58184dff08fd071686d9268ee8aff13d1d280#r73090912,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ATiQP3zpMeE76PpdJL-xAffFq-GVrhoZks5qbr8QgaJpZM4JZ_4s
.

[ericchiang]

The clients themselves can't be expired, only the token which holds a client_id. You can read some details on that token here[0].

Basically, the API gets that token and needs to verify the signature, make sure it hasn't expired, etc. Normally it would also verify that the client_id is for the client we expect, that's why all the current verification methods take a client_id. However, for this this case we don't need to verify a token was issued for a specific client, we just need to check if that client is an admin later on (the line we're having this conversation about), which is done outside this verify call.

Actually the 278 shouldn't be a for loop, probably just a single verification call. The method just needs the client_id being passed is just verifying that the aud claim is for a client you expect.

[0] https://openid.net/specs/openid-connect-core-1_0.html#IDToken

[ericchiang]

@squat I've update the code flow. Let me know if that looks better.

[squat]

LGTM