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

Race Condition in Password Creation Results in Invalid Password #58

Closed
mdgreenfield opened this issue Mar 15, 2021 · 0 comments
Closed

Comments

@mdgreenfield
Copy link
Contributor

When generating Azure credentials using a role configured with an existing service principal, there is a race condition in the password creation logic which can result in a password being returned to the caller but not set on the Azure application.

The logic is as follows:

  • List Passwords (note that keyIds are returned here, no actual passwords)
  • [add/remove password from list}
  • Update Passwords (if a keyId is not present, the password is removed)

As a result, it is possible for request A and request B to retrieve the same list of existing passwords (i.e. keyIds) but the last request from A or B to update the passwords will win.

Although, there is locking in place to prevent that, this does not handle the case where an existing service principal is configured on multiple Azure secret engine mounts or across multiple Vault clusters.

The current logic is using the deprecated Azure AD Graph API, however, the newer Microsoft Graph API has endpoints to atomically addPassword and removePassword.

Because the MS Graph API has a different set of permissions required for the plugin configuration, it is probably better to have a flag on the plugin configuration that defaults to the Azure AD Graph API but which allows users to upgrade to the MS Graph API once their permission changes have been added.

mdgreenfield added a commit to mdgreenfield/vault-plugin-secrets-azure that referenced this issue Mar 16, 2021
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 added a commit to mdgreenfield/vault-plugin-secrets-azure that referenced this issue Aug 30, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant