Fix #502 - Terraform loses access token and requires az login

[lawrencegripper]

This PR uses the callbacks present in the ADAL library along with a 'find and replace in file' function. These changes allow TF to use an expired token and persist the updated refresh token back to the AzureCLI file.

[tombuildsstuff]

hey @lawrencegripper

Thanks for this PR.

Given #1746 is going to be making some pretty major changes to the Azure API clients across the provider - I'm going to suggest we hold off reviewing this PR until #1746 is merged. I believe the general approach should work - however I'm now wondering if we should just parse and persist the file regardless (rather than replacing the auth token in it) given we're reliant on it / parsing it at the moment anyway 🤔

We're working through #1746 at the moment and should have this finished in the coming days; but once that's merged we'll circle back around to this :)

Thanks!

[joshgav]

FWIW the file format is an internal detail and could change (or the file be removed) in the future. But we'll offer more robust ways to reuse the CLI tokens before that happens and keep you informed.

cc @yugangw-msft

[lawrencegripper]

however I'm now wondering if we should just parse and persist the file regardless (rather than replacing the auth token in it) given we're reliant on it / parsing it at the moment anyway

Yeah I think this is another option, I went down the find and replace route as it means that the TF provider can weather certain changes to the file without causing issues. For example, if the CLI team added an additional property it would be lost if the code unmarshaled to the TF struct (missing the new property) and wrote out the update back to the file.

[tombuildsstuff]

hey @lawrencegripper

Sorry for the delayed response here - whilst the approach taken in this PR is valid, since this PR was opened there's a new mechanism become available in the Azure SDK for Go where access tokens can be retrieved via shelling out to the Azure CLI (which is then responsible for refreshing the token and persisting it - which means it's officially supported).

In the next couple of releases (currently scheduled for v1.20) - we're planning on refactoring the authentication framework out into it's own package/library - such that it can be reused across the AzureRM Backend too; at which time it'd make sense to switch over to using this new authentication method for the Azure CLI (it'd also be a good time to support authenticating using a Service Principal and a Certificate).

I spent a little bit of time recently prototyping this and it seems to work fairly well - such that I believe it should be possible to reuse the authentication logic across the AzureRM Provider, the Azure Stack Provider and the AzureRM Backend - with feature toggles for which authentication modes are supported in which location (e.g. there's no CloudShell in Azure Stack). As mentioned above - this is currently scheduled for a couple of releases time, so whilst I appreciate this may be a little longer to wait - I think it's worth the extra time to make this consistent across the Backend too.

As such - I hope you don't mind but I'm going to close this PR for the moment in favour of the upcoming refactoring detailed above which is scheduled for the near future - that said I'd like to thank you for this contribution :)

Thanks!

[lawrencegripper]

@tombuildsstuff Sure happy for a different approach, I do have one concern about using the az account get-access-token method which caused me to discount it when I was doing the PR. I'm guessing your way ahead of me on this one but just noting down in case it's useful.

I don't think the CLI method will handle an operation which runs longer than the life of the token, without some extra work. Calling get-access-token returns just a token, no refresh token like so..

az account get-access-token
{
  "accessToken": "tokenhere",
  "expiresOn": "2018-11-01 11:38:17.942332",
  "subscription": "subhere",
  "tenant": "tenanthere",
  "tokenType": "Bearer"
}

When that is used by the client, if the token has expired, the sdk client will return a 403 and error. Whereas in the this PR the client will see that a refresh_token is available and use this to get a new token then repeat the request (writing the update refresh token back to disk).

It looks like the default lifetime for a token is 1hour but this is configurable by the tenant so could be shorter (minimum 10mins max 1day).

So if a terraform apply process runs longer than the expiresOn then it would fail and this could be 10mins after starting.

One workaround would be to hook into each request made by the SDK and inject some retry logic with shells out to get a new token if the first attempt returns a 403.

[yugangw-msft]

To use az account get-access-token, 2 suggestions in case they help,

1. Leverage the expiresOn field, and only re-invoke the command when the expiration is <5 minutes away. JFYI, 5 minutes buffer is to handle clock skew.
2. Do the check right before send out every request payload to ARM. Since the token is good for 60 minutes, so most of such checks will be a no-op.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!