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

Use the cap/oidc library for OIDC based authentication #158

Merged
merged 13 commits into from
Feb 10, 2021

Conversation

austingebauer
Copy link
Contributor

@austingebauer austingebauer commented Feb 1, 2021

Description

This PR transitions the plugin to use the cap/oidc library for OIDC based authentication.

This PR also includes changes to force the Azure-specific handling of the groups overage claim to use the Microsoft Graph API instead of the deprecated Azure Active Directory (AAD) graph API. This change was necessary as a part of the transition, since it required behavior (requesting an additional access token) that's not exposed by cap/oidc.

Testing

All existing tests are passing against the changes in this PR.

I've manually tested OIDC authentication using the following identity providers:

  • Auth0
  • Google
    • Tested with and without additional G Suite groups handling
  • Azure Active Directory
    • Tested with and without additional 200+ groups handling

I also tested that the namespace_in_state parameter works as expected.

Additional Context

The cap/oidc library introduces a Request type that represents a single OIDC auth flow for a user. A Request is uniquely identified by a State. This PR includes changes to align terminology used in the plugin to that used in the cap/oidc library by renaming State to Request in certain instances. I'm happy to consider alternatives or leave the naming unchanged in this plugin.

Related Pull Requests

path_config.go Outdated Show resolved Hide resolved
path_oidc.go Outdated Show resolved Hide resolved
path_oidc.go Outdated Show resolved Hide resolved
path_oidc.go Outdated Show resolved Hide resolved
path_oidc.go Outdated Show resolved Hide resolved
path_oidc.go Outdated Show resolved Hide resolved
@DrDaveD
Copy link

DrDaveD commented Feb 2, 2021

When this is getting pretty close to being final can you please say so here? I would like to test it in my environment.

@DrDaveD
Copy link

DrDaveD commented Feb 2, 2021

I also wonder if it's going to complicate or simplify adding code flow callback direct to vault #130 and device flow #131.

@austingebauer austingebauer requested a review from calvn February 2, 2021 22:37
provider_azure.go Outdated Show resolved Hide resolved
path_oidc_test.go Outdated Show resolved Hide resolved
Copy link
Member

@tvoran tvoran left a comment

Choose a reason for hiding this comment

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

I tried this with an azure setup and a user with 200 groups, and with namespaces and it still works 👍

@austingebauer
Copy link
Contributor Author

@DrDaveD - You can go ahead and test this in your environment now. Please let me know if you find any regressions. I'm having a look at the PRs you've linked to see how this change may affect them. Will respond when I've gathered more context.

@austingebauer
Copy link
Contributor Author

@DrDaveD - I took a look at the PRs that you've linked to. I don't see the usage of cap/oidc in this plugin as something that will complicate (or simplify..) the changes in the PRs. I do know that those use cases are being considered as a part of continued development on cap/oidc, so I expect that supporting them will eventually be simplified.

@austingebauer austingebauer merged commit 0e04f42 into master Feb 10, 2021
@austingebauer austingebauer deleted the cap-oidc-lib branch February 10, 2021 00:47
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.

4 participants