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

Support OAuth Access tokens #147

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mdgreenfield
Copy link
Contributor

@mdgreenfield mdgreenfield commented May 5, 2023

Overview

A high level description of the contribution, including:

Adds new token endpoint for retrieving Azure OAuth access tokens (see #50).

Design of Change

How was this change implemented?

Related Issues/Pull Requests

[ ] Issue #50

Contributor Checklist

[ ] Add relevant docs to upstream Vault repository, or sufficient reasoning why docs won’t be added yet
My Docs PR Link
Example
[ ] Add output for any tests not ran in CI to the PR description (eg, acceptance tests)
[ ] Backwards compatible

Adds the <mountPath>/token/<role> endpoint to return an Oauth access
token. This access token is not leased because these tokens have a TTL
of 60m and are not revokable upstream.

Caveats:
- The <mountPath>/roles/<role> backend will create a separate App/SP
  with the same logic as the <mountPath>/roles/<role> creds. So, a
  unified App/Service Principal is not used between the various
  endpoints for a given role.
- No changes were made to how deleting a role revokes the cloud
  resources used by the <mountPath>/creds/<role> endpoint.
- An "existing Service Principal" still creates an App password as
  opposed to a service principal password.
@mdgreenfield
Copy link
Contributor Author

@fairclothjm, I have been looking at rebasing this PR. Since I last touched my branch there have been some changes to this plugin which I think warrants bringing up before rebasing (actually, probably cherry-picking):

Persisted Azure Applications

#98 introduced the ability to persist the Azure Application and Service Principal for dynamic roles, however, this behavior is opt-in. To support #50 the Application and Service Principal need to be persisted for every dynamic role, not just those with persist_app=true.

Also, as part of #98, any update to a plugin role removes all Azure roles and groups and then re-adds the correct ones. I believe this is problematic because running applications would briefly lose their permissions. But because RoleAssinments are not stored with their respective roles it is not currently possible to leave untouched Azure roles or groups in place. It should be possible to fix for existing persist_app users by looking up the Role+RoleAssignment and storing that mapping (this should be a one time operation).

Resource Owner Password Credentials

Supporting OAuth access tokens requires the application ID (client ID), and client_secret per Azure's Resource Owner Password Credentials "flow", when fetching tokens. I went back to double checked to see if Azure supports anything like GCP's Impersonated Service Accounts and roles/iam.serviceAccountTokenCreator functionality. The closest I could find is Azure's OAuth 2.0 On-Behalf-Of flow which might work and would allow the plugin's backend configuration to generate access tokens for other applications (given the right scope configuration). This would be useful because it would avoid application secrets from needing to be stored and rotated separately from the backend's secret. We'll see how things go with my availability but I may need to hand this investigation/implementation over to you all.

@fairclothjm
Copy link
Contributor

@mdgreenfield Thanks for the update and sorry for the delay! I think your assessment makes sense for the Persisted Azure Applications. It does look like the OBO flow could work here. I discussed it with the team and we currently don't have the bandwidth to pick it up at the moment. If this is something you want to continue to carry forward we will do our best to prioritize this PR.

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.

None yet

2 participants