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

Add support for using MS Graph instead of AAD #67

Merged
merged 8 commits into from Sep 24, 2021
Merged

Add support for using MS Graph instead of AAD #67

merged 8 commits into from Sep 24, 2021

Conversation

pcman312
Copy link
Contributor

@pcman312 pcman312 commented Sep 15, 2021

Overview

The Azure Active Directory Graph API (AAD) is being deprecated in favor of the Microsoft Graph API. The AAD API is slated to be removed from Azure mid-2022. This PR updates the secrets engine to support using either API so users can migrate to using MS Graph.

NOTE: Since the AAD API is going to be removed entirely, this engine will stop supporting using the AAD API at some point in the future. All users of this engine should migrate to using the MS Graph API. The exact date for the removal of the AAD API from this engine has not be set.

Big thanks to @mdgreenfield for doing a large part of the heavy lifting for this PR

Design of Change

  • Adds a new config field (use_microsoft_graph_api) that toggles between using AAD and MS Graph
  • The provider has been updated so it can switch between using the AAD API (via the graphrbac library) and using the autorest library to use the MS Graph API
  • The API logic has been extracted to a separate package (api)

Related Issues/Pull Requests

#63
#59

Contributor Checklist

  • Add relevant docs to upstream Vault repository, or sufficient reasoning why docs won’t be added yet
    • TBD
  • Backwards compatible
  • Add output for any tests not ran in CI to the PR description (eg, acceptance tests)
Test Output

AAD:

$ VAULT_ACC=1 richgo test -v -run TestCredentialInteg_aad
START| CredentialInteg_aad
START|   CredentialInteg_aad/service_principals
     | === PAUSE TestCredentialInteg_aad/service_principals
START|   CredentialInteg_aad/static_service_principals
     | === PAUSE TestCredentialInteg_aad/static_service_principals
     | === CONT  TestCredentialInteg_aad/service_principals
     | === CONT  TestCredentialInteg_aad/static_service_principals
PASS | CredentialInteg_aad (0.00s)
PASS |   CredentialInteg_aad/static_service_principals (57.65s)
PASS |   CredentialInteg_aad/service_principals (82.23s)
PASS
PASS | github.com/hashicorp/vault-plugin-secrets-azure 82.446s

MS Graph:

$ VAULT_ACC=1 richgo test -v -run TestCredentialInteg_msgraph
START| CredentialInteg_msgraph
START|   CredentialInteg_msgraph/service_principals
     | === PAUSE TestCredentialInteg_msgraph/service_principals
     | === CONT  TestCredentialInteg_msgraph/service_principals
PASS | CredentialInteg_msgraph (0.00s)
PASS |   CredentialInteg_msgraph/service_principals (30.54s)
PASS
PASS | github.com/hashicorp/vault-plugin-secrets-azure 30.753s

mdgreenfield and others added 3 commits August 30, 2021 10:02
Azure Active Directory Graph API, now deprecated, does not provide
support for atomically creating/removing passwords on an application. As
a result, there is a race conditions that can occur when creds are being
created for roles configured with an existing service principal that
is configured on multiple mounts or across multiple Vault clusters.

Unfortunately,
[`Azure/azure-sdk-for-go`](https://github.com/Azure/azure-sdk-for-go)
does not yet offer a MS Graph API client, therefore, this PR utilizes
[`Azure/go-autorest`](https://github.com/Azure/go-autorest) to construct
a client the same as
[`Azure/azure-sdk-for-go`](https://github.com/Azure/azure-sdk-for-go).

This changeset preserves using the AAD Graph API by default but provides
a mount configuration option for toggling to the new MS Graph API. This
is because the two APIs require different API permissions. This allows
users to upgrade to the new plugin version and then switch to the new
API.

Additionally, although using the MS Graph API is a net benefit, it
itself has reliability issues when handling multiple requests in
parallel. More details can be found in
https://github.com/mdgreenfield/microsoft-graph-api-reliability and I am
working with Microsoft to try to get some of these reliability issues
resolved.

Fixes #58
api/groups.go Outdated Show resolved Hide resolved
@pcman312 pcman312 marked this pull request as ready for review September 15, 2021 21:50
Copy link
Contributor

@jasonodonnell jasonodonnell left a comment

Choose a reason for hiding this comment

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

LGTM and tested well!

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

3 participants