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

feat: Add support for audience in authorization request #68

Merged
merged 3 commits into from
Oct 14, 2024

Conversation

abhisek
Copy link
Contributor

@abhisek abhisek commented Oct 8, 2024

Add support for audience in authorization request. This is required for IDPs that mandate having an audience field to be able to issue JWT in access token.

@jtmcg
Copy link
Contributor

jtmcg commented Oct 8, 2024

Hey @abhisek, I'm not familiar with this requirement. Could you provide some more details (and expanded acronyms) about what problem this is solving and how it solves it?

@jtmcg jtmcg self-assigned this Oct 8, 2024
@abhisek
Copy link
Contributor Author

abhisek commented Oct 9, 2024

@jtmcg Sure. Thanks for looking at this PR. I will take a stab at explaining the rationale behind this PR, apologies if it is unnecessarily verbose.

As I understand, cli/oauth helps cli applications integrate with an OAuth2 capable Identity Provider (IDP). While primarily this is built for integrating cli applications with github.com as the Identity Provider and obtain an access token to call GitHub API, the implementation seems generic enough to work with other OAuth2 capable Identity Providers as well.

OpenID Connect (OIDC) compliant Identity Providers usually encode the identity of the authenticating user (resource owner in OAuth2 terminology) in a JSON Web Token (JWT) with standard claims for email, name etc. While interacting with OAuth2/OIDC compliant Identity Provider, client applications can include an optional parameter called audience in the authorization request. The Identity Provider should validate and optionally include the audience request within the JWT as aud claim, indicating which service(s) (resource servers in OAuth2 terminology) are meant to be the consumer of the token.

I am using this package in my project vet to authenticate with Auth0 Identity Provider. As it turns out, audience is not so optional as I expected. If the audience parameter is missing in the authorization request, Auth0 ends up providing an opaque token and not a JWT based identity token. It seems Auth0 requires audience of the token to be explicitly mentioned for it to have OIDC compliant behaviour.

This PR is to add the support for users of the package to "optionally" add an audience parameter in the authorization request sent to the identity provider.

Copy link
Contributor

@jtmcg jtmcg left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation on what's going on - it was very helpful. I had no idea about most of this before now, so thanks for taking the time to teach me 🙇

Anyway, this looks good. I spent some time tinkering around with your implementation and I like where you landed 👍 My only feedback is the small variable name change, otherwise LGTM.

Thanks for the contribution!

device/device_flow.go Outdated Show resolved Hide resolved
@jtmcg
Copy link
Contributor

jtmcg commented Oct 11, 2024

@abhisek, I tried to fix the linter errors for you but my commit got rejected 😞 Once those are fixed we can :shipit:

@abhisek
Copy link
Contributor Author

abhisek commented Oct 12, 2024

@jtmcg Oops. Did not notice the linter errors. Fixed it. Verified by running locally as well

➜  oauth git:(main) golangci-lint run -n

Hope the CI passes when you approve the run.

Copy link
Contributor

@jtmcg jtmcg left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution!

@jtmcg jtmcg merged commit 6c44f68 into cli:main Oct 14, 2024
10 checks passed
@abhisek
Copy link
Contributor Author

abhisek commented Oct 15, 2024

Thanks @jtmcg Will it be possible for you to make a release so that I can switch back to this repo in my project and get rid of this?

https://github.com/safedep/vet/blob/main/go.mod#L42

@jtmcg
Copy link
Contributor

jtmcg commented Oct 15, 2024

Thanks @jtmcg Will it be possible for you to make a release so that I can switch back to this repo in my project and get rid of this?

https://github.com/safedep/vet/blob/main/go.mod#L42

Done! Thanks again for the contribution 🍻

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