Add support for Managed Identity auth for physical/Azure

[sfc-gh-jelsesiy]

Obtain OAuth token from IMDS to allow for access to Azure Blob with
short-lived dynamic credentials

Fix #7322

[hashicorp-cla]

All committers have signed the CLA.

[sfc-gh-jelsesiy]

@calvn Please have a look. I'm hoping we can get this into the 1.6 milestone, thanks!

[sfc-gh-jelsesiy]

@calvn Did you have a chance to triage this? Thank you!

[calvn]

@sfc-gh-jelsesiy sorry, I missed the ping from your earlier messages! I'll be taking a look at this.

[calvn]

Looks good overall! We should also adjust the tests within azure_test.go so that they can run using creds from this flow if the account key is not provided.

Can you do a run of the tests afterwards and share the results over here?

[sfc-gh-jelsesiy]

Looks good overall! We should also adjust the tests within azure_test.go so that they can run using creds from this flow if the account key is not provided.

Can you do a run of the tests afterwards and share the results over here?

Sure I can add that, what are you thinking specifically? Running these tests without mocking would require you run it on a host that has the IMDS available and a valid identity attached to it. This seems tricky for most CI systems which is why I haven't included any

[calvn]

Definitely agree on the trickiness bit! We'd want to verify at some capacity that this works as intended, even if things are set up manually at this point, so that's what we might end up doing.

[sfc-gh-jelsesiy]

Definitely agree on the trickiness bit! We'd want to verify at some capacity that this works as intended, even if things are set up manually at this point, so that's what we might end up doing.

Okay let me see what I can come up with. I also noticed that the current tests are duplicating logic that is actually part of NewAzureBackend implementation. This is done to get access to the underlying containerURL object it seems but also means that you don't get to test any of the error handling that happens prior to containerURL creation. I'm going to think about how to improve this as I'm adding the new tests

[sfc-gh-jelsesiy]

@calvn I added a separate commit for the tests, let me know what you think. I documented what's needed to successfully run the tests using managed identities and did that myself, see:

go test -count=1 -v -run TestAzureBackend ./physical/azure
=== RUN   TestAzureBackend
    azure_test.go:57: using managed identity to authenticate against storage account
2020-10-28T18:09:21.738Z [INFO]  accountKey not set, using managed identity auth
2020-10-28T18:09:21.770Z [DEBUG] token refreshed, new token expires in: access_token_expiry=84287
--- PASS: TestAzureBackend (0.52s)
=== RUN   TestAzureBackend_ListPaging
    azure_test.go:71: using managed identity to authenticate against storage account
2020-10-28T18:09:22.255Z [INFO]  accountKey not set, using managed identity auth
2020-10-28T18:09:22.293Z [DEBUG] token refreshed, new token expires in: access_token_expiry=84286
--- PASS: TestAzureBackend_ListPaging (70.05s)
PASS
ok  	github.com/hashicorp/vault/physical/azure	70.574s

[calvn]

Perfect, thanks for sharing the test result as well!

Do the new test changes remove the ability to test the physical backend locally? Ideally we would want to be able to do both. That is, if ran on an Azure VM it will use IMDS, and if ran locally all the creds need to be provided as they were before in order be able to use Azure storage for local testing.

[sfc-gh-jelsesiy]

Perfect, thanks for sharing the test result as well!

Do the new test changes remove the ability to test the physical backend locally? Ideally we would want to be able to do both. That is, if ran on an Azure VM it will use IMDS, and if ran locally all the creds need to be provided as they were before in order be able to use Azure storage for local testing.

The way it's implemented is that if AZURE_ACCOUNT_KEY is specified it will always use that and only if empty attempts to use managed identity auth via IMDS. Even if you specify AZURE_ACCOUNT_KEY on a VM that has IMDS available it will still use the access key unless removed

[calvn]

Why are the env vars AZURE_ENVIRONMENT and AZURE_ARM_ENDPOINT removed from the test consumption?

[sfc-gh-jelsesiy]

Why are the env vars AZURE_ENVIRONMENT and AZURE_ARM_ENDPOINT removed from the test consumption?

The way the tests have been set up before wasn't correct. The credential creation has been duplicated in the tests instead of relying on the logic in NewAzureBackend which is done now

[calvn]

Ah, gotcha!

[sfc-gh-jelsesiy]

@calvn Please let me know if you have any code comments so I can address them today as I got the time allocated from my company for it :)

[calvn]

Looks good, thanks for the contribution!

Ran the test locally and they passed as well:

=== RUN   TestAzureBackend
    azure_test.go:63: using account key provided to authenticate against storage account
--- PASS: TestAzureBackend (3.07s)
=== RUN   TestAzureBackend_ListPaging
    azure_test.go:73: using account key provided to authenticate against storage account
--- PASS: TestAzureBackend_ListPaging (256.73s)
PASS

[sfc-gh-jelsesiy]

Great!! Should I be worried about the test failures (PR checks) or is that not blocking the merge?

[calvn]

Doesn't look like it's related to your changes, but can you merge/rebase origin/master into this branch if you haven't done so recently?

[sfc-gh-jelsesiy]

Doesn't look like it's related to your changes, but can you merge/rebase from origin/master if you haven't done so recently?

I'm up to date to origin/master