Rework the credential validation to use azidentity #1381
Conversation
Credential validation now relies on listing of the resource principles to determine success. If the client credentials can successfully list the principles once then the credentials provided from Vault are deemed to be valid. In the case where the 401 HTTP status code is returned from the Azure API, the validation check is terminated since this means that the credentials were not authorized to perform any API requests.
| if err != nil { | ||
| return fmt.Errorf("error reading from Vault: %s", err) | ||
| } | ||
| log.Printf("[DEBUG] Read %q from Vault", configPath) | ||
| log.Printf("[DEBUG] Successfully read %q from Vault", configPath) | ||
|
|
||
| subscriptionID := "" |
There was a problem hiding this comment.
Instead of leveraging the subscriptionId and tenantId returned from the Vault configuration, can these be optionally defined in the data source? Otherwise it'll require all Azure SPs in Vault to be granted access to a centralized Subscription that is defined in the Vault configuration, which is likely not needed by the SP that's generated for anything else. This will likely cause the provider to break for anyone with existing access defined in Vault and they'll need to update their SP definitions.
There was a problem hiding this comment.
Sure, we can add that, but I think that it is out of scope for this PR. I can open another PR, but it probably won't make it into 3.4.0. Sound okay?
There was a problem hiding this comment.
I'm concerned it'll cause the provider to break for any existing vault roles that don't have access to the subscription that's defined in the Azure Secrets engine config in Vault.
| overallSuccess = true | ||
|
|
||
| if pager.Err() == nil { | ||
| log.Printf("[DEBUG] Credential validation succeeded") |
There was a problem hiding this comment.
With num_sequential_successes being removed this will mean the moment a single Azure AD endpoint returns success then it'll assume that the credential is valid, however there can be a propogation delay between endpoints. with num_sequential_successes this allows for the provider to initiate multiple attempts to ensure credentials are replicated.
There was a problem hiding this comment.
Interesting, I was not able to reproduce that in my testing with a 5 minute poll. I'm happy to add it back.
There was a problem hiding this comment.
Is there a way to know which endpoint we hit?
There was a problem hiding this comment.
I don't believe there is. Replication time seems to take longer when using an the existing SP functionality as documented here https://www.vaultproject.io/docs/secrets/azure#application-object-ids
|
I'm happy to raise a separate issue for this, but we ran headlong directly into the concern that @mb290 raised yesterday: #1381 (comment) This seems to be a semi-breaking change for existing roles - could something be added to CHANGELOG.md? |
Hi @robison I don't believe we introduced any breaking changes with this fix. The Would you mind unpacking the Thanks, Ben |
|
Per @mb290's comment #1381 (comment), we had an existing Vault role that did not have access to the subscription that's defined in the Azure Secrets engine config in Vault. Thankfully ran into this in a dev environment, where the |
Ah, I see now. Thanks for the clarification. #1391 should be going out on Thursday in a patch release |
|
Excellent. Could we also get a update/note in CHANGELOG.md for v3.4.1 indicating the potential necessity of specifying |
Updated docs in #1398 |
Credential validation now relies on listing of the resource principles
to determine success. If the client credentials can successfully list
the principles once then the credentials provided from Vault are deemed
to be valid. In the case where the 401 HTTP status code is returned from
the Azure API, the validation check is terminated since this means that
the credentials were not authorized to perform any API requests.
Community Note
Relates OR Closes #881
Release note for CHANGELOG:
Output from acceptance testing: