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

AutoRefreshingTokenCredential token override issue #1225

Closed
Nohac opened this issue Feb 21, 2023 · 4 comments · Fixed by #1229
Closed

AutoRefreshingTokenCredential token override issue #1225

Nohac opened this issue Feb 21, 2023 · 4 comments · Fixed by #1229
Labels
Azure.Identity The azure_identity crate

Comments

@Nohac
Copy link

Nohac commented Feb 21, 2023

Hi.

I have a program that streams some data from an http request to azure storage blob as well as accessing some other api's that also needs to authenticate using the same AutoRefreshingTokenCredential.

This is the logic flow:

  1. Stream all data to storage using put_block (wait to run put_block_list for now)
  2. Access other api's and do some work (uses get_token("<scope>") on the same AutoRefreshingTokenCredential that the storage client uses)
  3. If everything succeeds in step 2, run put_block_list to "finalize" the upload.

The problem I'm facing is that, after running get_token in step 2, step 3. ( put_block_list) returns the following error:

server returned error status which will not be retried: 401

Caused by:
    HttpError {  Status: 401,  Error Code: InvalidAuthenticationInfo,  Body: "b"\xef\xbb\xbf<?xml version=\"1.0\" encoding=\"utf-8\"?><Error><Code>InvalidAuthenticationInfo</Code><Message>Server failed to authenticate the request. Please refer to the information in the www-authenticate header.\nRequestId:<omitted>\nTime:2023-02-21T16:11:53.5576343Z</Message><AuthenticationErrorDetail>Issuer validation failed. Issuer did not match.</AuthenticationErrorDetail></Error>"",  Headers: [   date:Tue, 21 Feb 2023 16:11:53 GMT   content-type:application/xml   server:Microsoft-HTTPAPI/2.0   content-length:402   x-ms-error-code:InvalidAuthenticationInfo   www-authenticate:Bearer authorization_uri=https://login.microsoftonline.com/<omitted>/oauth2/authorize resource_id=https://storage.azure.com   x-ms-request-id:<omitted> ], } 

If I skip step 2. then step 3. works.

Another observation I made is that, if I don't use AutoRefreshingTokenCredential, all the steps work:

let creds = Arc::new(DefaultAzureCredential::with_sources(defaul_credentials));
// let creds = Arc::new(AutoRefreshingTokenCredential::new(creds));
return creds;

I'm not really sure what's going on here, but I suspect AutoRefreshingTokenCredential overwrites the current token when I run get_token to call the API's, then when it gets to put_block_list, it uses the token from step 2?

Another thing I'm not sure about is if I even need AutoRefreshingTokenCredential, the documentation is really sparse, all it says is:

Wraps a TokenCredential and handles token refresh on token expiry

Does that mean that, if I don't wrap DefaultAzureCredential in AutoRefreshingTokenCredential, the token might expire? Or does it just keep the token "up to date", so the next time I run get_token or something else, it will be faster?

I hope the explanation is clear enough, if not, I can try to put together some example code.

Also, as a side note, not directly related to this. I'm having trouble calling get_token("<scope>") using managed identity after updating from 0.6.0 to 0.10.0, getting this error message:

Multiple errors were encountered while attempting to authenticate: getting managed identity credential timed out

The code is completely unchanged, all I did was update and rebuild. Downgrading makes it work again.

@johnbatty
Copy link
Contributor

Thanks for the detailed report.

As you suggest, it looks like the problem is that AutoRefreshingTokenCredential only caches a single token, and the token is at the scope of a particular resource:
https://github.com/Azure/azure-sdk-for-rust/blob/main/sdk/identity/src/token_credentials/auto_refreshing_credentials.rs#L58

There is no check that the resource matches on subsequent requests when the cached token may be returned:
https://github.com/Azure/azure-sdk-for-rust/blob/main/sdk/identity/src/token_credentials/auto_refreshing_credentials.rs#L42

One workaround may be to create separate credentials for your storage client and your other non-storage client.

The "proper" fix is to update the SDK's AzureRefreshingTokenCredential to take account of the resource - perhaps maintaining per-resource tokens.

Your managed identity get_token() issue sounds concerning and needs further investigation - clearly something seems to have regressed.

@Nohac
Copy link
Author

Nohac commented Feb 22, 2023

Thanks for getting back to me so quickly.

If there's anything I can do the help move this forward, please let me know.

Regarding the workaround you proposed, I thought about that, but again, I'm still not sure I need AzureRefreshingTokenCredential at all. The docs aren't exactly clear on why and when to use it. I assume get_token will fetch a new token once the old one has expired regardless, it will just take longer? My guess is that AzureRefreshingTokenCredential just acts as a cache and makes sure there's always a valid token available when needed, to make requests faster?

As for the get_token() timeout problem on 0.10.0, I will try to isolate the issue to make sure that I'm not doing anything wrong. I might open a separate issue once I can confirm for sure that it is the update that causes the timeout to occur.

@johnbatty
Copy link
Contributor

In the current implementation, the Credential implementations of get_token() always do the work to fetch a new token, even if the previous token has not expired. It does no caching of tokens. I was recently surprised to discover this, and will be raising a new issue to address this as I think we should be caching by default.

For example, see:

This will clearly impact performance if you are making many API calls. Using the AutoRefreshingTokenCredential does provide caching and renewing on expiry, but turns out that it suffers from the issue you reported.

@johnbatty
Copy link
Contributor

As for the get_token() timeout problem on 0.10.0
If this is easily reproducible, it would be very helpful if you could determine which release between 0.6 and 0.10.0 breaks the function for you. If you determine that it is broken in newer version(s) then yes, please do create a new issue to track it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Identity The azure_identity crate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants