Use the MS Graph API for atomic add/remove password operations #59
Conversation
|
Relates to #63 |
|
Any way to get this reviewed/merged? @mdgreenfield I guess the current conflicting files need to be fixed first. I would also think the change needs to be backward compatible, eg. compatibility with the Azure AD graph API needs to be maintained (to not break existing use-cases) and allow for using the MS Graph API with a config flag. EDIT: On further review I see the toggling is already present. 👍 |
|
I'm happy to get this PR updated but would like some feedback from the maintainers about this approach first. #63 is a bit duplicative so we need to come to a consensus on the best way to add support for the new API. I do believe a config flag to toggle is necessary given the different permissions that are required for the new API otherwise there is a risk of breaking users simply from a Vault upgrade. |
|
@mdgreenfield We're looking at both of these PRs internally and should have some comments on both PRs soon. Sorry for the delay but this definitely something we're looking to change soon 😄 |
There was a problem hiding this comment.
Hi @mdgreenfield, thanks for the contribution! I've taken a closer look at this and did some manual testing. In general this looks really good and we do agree that having the msgraph configurable is the right approach. Unfortunately during my testing I could not generate a dynamic application with a group. I've tried various permissions but keep getting:
* 1 error occurred:
* unable to lookup Azure group: graphrbac.GroupsClient#List: Failure responding to request: StatusCode=403 -- Original Error: autorest/azure: Service returned an error. Status=403 Code="Unknown" Message="Unknown service error" Details=[{"odata.error":{"code":"Authorization_RequestDenied","date":"2021-08-16T15:51:15","message":{"lang":"en","value":"Insufficient privileges to complete the operation."},"requestId":"37e2f04f-b0de-4bb6-8548-8c7c9885818c"}}]
This may be an issue on my end but I wanted to ask if you were able to get this to work and what permissions did you give the service principal? I suspect these errors are due to not updating the group code but wanted to double check.
|
@jasonodonnell, do you only have Microsoft Graph permissions assigned? I think you may be running into the fact that I only implemented the client for AAD Applications so at the moment it requires both Microsoft Graph permissions and Azure Active Directory Graph permissions. If your team is comfortable with the changes I've laid out I can probably find some time to implement the remaining functionality for Groups, RoleAssignments, ServicePrincipals, etc... |
|
Ah yes, that was it, I removed all AAD permissions to test just the MS graph work. I missed that the groups client wasn't updated as well. I'm meeting with my team tomorrow to discuss which approach we're moving forward with. Thanks! |
|
@mdgreenfield Sorry for the delay, but we've decided to move forward with this implementation since its closer to other Vault projects using MS graph. The next steps would be:
If you aren't available to add extra support, we can take over the PR and add the functionality. Please let me know how you would like to proceed. |
|
@jasonodonnell, I should be able to get the PR rebased to resolve the conflicts today but if your team is able to run with this PR that would probably be best because I don't think I'll have the time to contribute for a few weeks. |
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 hashicorp#58
mdgreenfield
force-pushed
the
atomically-addremove-app-passwords
branch
from
b115d8d
to
b5fa247
Compare
|
Closing this with #67 |
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-godoes not yet offer a MS Graph APIclient, therefore, this PR utilizes
Azure/go-autorestto construct a client thesame as
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