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

Follow the OpenID Connect Audiences spec #240

Merged
merged 1 commit into from
Sep 19, 2018

Conversation

OvermindDL1
Copy link
Contributor

This follows the OpenID Connect spec to support audiences with either a string or an array of strings instead of just a string.

This fixes the bug at go-gitea/gitea#4877 and the PR at go-gitea/gitea#4878 is dependent on this getting fixed and released.

I have tested this and am running it on my site now with the OpenID Connect with multiple audiences returned with no issue, the new code is not even run at all unless the old pure-string check fails.

Disclaimer: I've never used and barely looked at Go thus far, and I was unable to find common functions like filter or so forth (or any higher order functions at all...) and based on some googling it appears that these large and unwieldy loops are the standard way to do this kind of thing in Go, but if any changes are needed please request as such with information on what I should change it to (great learning opportunity!).

And of course any rights or whatever there-of related to this code I give up for whatever use this project currently wants or will want in the future. Etc... etc... Commit is signed, etc...

This follows the OpenID Connect spec to support audiences with either a string or an array of strings instead of just a string.

This fixes go-gitea/gitea#4877

Signed-off-by: Gabriel Robertson <overminddl1@gmail.com>
@OvermindDL1
Copy link
Contributor Author

For note, the OpenID Connect Spec URL that is actually referenced in this file (4 lines above the first diff line actually) confirms that the audience can be a string or an array and both must be handled, hence why this is not working on the OpenID Connect server that I have to use since it sends multiple audiences as an array of strings.

A quoted summary from go-gitea/gitea#4877

At this line of code:

if audience != p.ClientKey {

	if audience != p.ClientKey {

It is performing a string comparison between audience and p.ClientKey, however, according to the OpenID Connect spec that that very source file links to just 3 lines higher states (emphasis mine):

The Client MUST validate that the aud (audience) Claim contains its client_id value registered at the Issuer identified by the iss (issuer) Claim as an audience. The aud (audience) Claim MAY contain an array with more than one element. The ID Token MUST be rejected if the ID Token does not list the Client as a valid audience, or if it contains additional audiences not trusted by the Client.

And in fact an array is being passed in by some OpenID Connect servers, which is causing this to fail and be unusable.

@OvermindDL1
Copy link
Contributor Author

Is there anything else that I need to do to get this merged in and released?

@bentranter
Copy link
Collaborator

Sorry for the delay @OvermindDL1, I was on vacation when this initially came in and missed it in a sea of other notifications 😛

I'd say this looks good to me, and while I'd like to add a test for this so it doesn't get broken in the future, this is great for now, and I really appreciate the amount of detail put into both the comments -- made it very easy to review. Thanks for the contribution!

@bentranter bentranter merged commit f9c6649 into markbates:master Sep 19, 2018
@OvermindDL1
Copy link
Contributor Author

Sorry for the delay @OvermindDL1, I was on vacation when this initially came in and missed it in a sea of other notifications stuck_out_tongue

All good, just making sure I didn't miss anything. :-)

I'd say this looks good to me, and while I'd like to add a test for this so it doesn't get broken in the future, this is great for now, and I really appreciate the amount of detail put into both the comments -- made it very easy to review. Thanks for the contribution!

Thanks! I do apologize for the lack of tests, traditionally I would in C++ or so but I'm entirely new to Go so this PR took me an excess of time as it was as I kept trying to look up patterns that Go wasn't capable of... ^.^;

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.

2 participants