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

The documentation for the TokenCredential is non-existent #20221

Closed
leidegre opened this issue Apr 8, 2021 · 9 comments
Closed

The documentation for the TokenCredential is non-existent #20221

leidegre opened this issue Apr 8, 2021 · 9 comments
Assignees
Labels
Azure.Identity Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. Docs needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team question The issue doesn't require a change to the product in order to be resolved. Most issues start as that

Comments

@leidegre
Copy link

leidegre commented Apr 8, 2021

It's quite typical that the MSDN documentation is just regurgitating what is obvious from the type information that's already present. Such documentation doesn't add any value and there's no point in adding it. Rather, I'd like to know stuff that is not obvious from the type information alone.

For example, in this case, with respect to the TokenCredential (from the Azure.Identity package), I like to know what I can expect from the token I get back. It doesn't say that the ExpiresOn property of the access token has some tolerance but it must have otherwise this API would create subtle race errors where the token you get back from the API expires before you have a chance to use it.

For example, I really need to know if the token is valid for at least 1 minute, or 5 minutes or 10 minutes? Which one is it and why? None of this is documented anywhere and it's a problem because I cannot conclude what will happen when I use this API in these circumstances. I'm currently testing this through experimentation but I think it would be better if you used the documentation to explain in more detail what problem each API is meant to solve, and what you may expect from the APIs (i.e. what promises do you give?). Without this it's hard to know if you've implemented the right thing or not and what additional issues you may need to take into account.


Document Details

Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

@ghost ghost added needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. customer-reported Issues that are reported by GitHub users external to the Azure organization. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Apr 8, 2021
@jsquire
Copy link
Member

jsquire commented Apr 8, 2021

Hi @leidegre. Thank you for your feedback and we regret that you're experiencing difficulties. TokenCredential is an abstraction in the Azure.Core library meant to allow the Azure SDK packages to take advantage of credential-based authorization in a provider-neutral way without forcing a dependency on Azure.Identiy. As a result, it isn't bound to any specific type of token nor does it have knowledge of any concrete implementation; there are no specifics that can be documented.

Unfortunately, many of the details that you're interested in are controlled by the specific service performing authentication and aren't defined or controlled by the Azure.Identity library. The service documentation would be the authoritative source. That said, looking through the documentation for the concrete credential types, I think there is opportunity to provide more context in some cases, and make it easier to locate the service documentation.

I'm going to route this over to the folks working on Azure.Identiy for consideration.

@jsquire jsquire added Azure.Identity Client This issue points to a problem in the data-plane of the library. Docs needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team labels Apr 8, 2021
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Apr 8, 2021
@leidegre
Copy link
Author

leidegre commented Apr 8, 2021

I was able to identify the following behavior with respect to token expiry, so it's 5 minutes in this case but this is important stuff, it's not like the different services performing authentication can do what they want. There's an intent in the abstraction which right now is just undocumented.

I would expect there to be a little bit more context around why the various abstractions exist and what problems they aim to solve and what assumptions you can make about them. Abstractions that don't allow you to make any assumptions are not helpful so I'm sure there's a lot of details here that just left out for some reason.

Moreover, I often consult the source code and I'm surprise at how little comments/documentation there is. It's difficult to gauge what the intent is behind all these designs when there's so little to go on.

image

@christothes
Copy link
Member

Hi @leidegre - The behavior details of how the token is created and its expiration is mostly controlled by the authority, which in this case is usually Azure Active Directory.

Is there more context you could provide about what problem you are trying to solve?

@christothes christothes added the needs-author-feedback Workflow: More information is needed from author to address the issue. label Apr 9, 2021
@ghost ghost removed the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Apr 9, 2021
@leidegre
Copy link
Author

leidegre commented Apr 12, 2021

@christothes My point was just that it would be helpful if the documentation would be more extensive regarding the token you get back from the TokenCredential abstraction.

For example,

When you call GetToken... you will get back a token that will be valid for at least 5 minutes and not 60 minutes which is the default you get from Azure AD.

See, the abstraction has changed the guarantees that I have about the token expiry but this is undocumented behavior.

I have some additional information in this comment, if you wanna take a look.

@ghost ghost added needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team and removed needs-author-feedback Workflow: More information is needed from author to address the issue. labels Apr 12, 2021
@christothes
Copy link
Member

For example,

When you call GetToken... you will get back a token that will be valid for at least 5 minutes and not 60 minutes which is the default you get from Azure AD.

See, the abstraction has changed the guarantees that I have about the token expiry but this is undocumented behavior.

Hi @leidegre - Could you clarify where you are seeing this statement of behavior guarantees? The TokenCredential abstraction does not define any behavior about the token that will be returned by GetToken. If I understand correctly, it sounds like what you are looking for is better documentation on the access token lifetime from AAD.

@christothes christothes added the needs-author-feedback Workflow: More information is needed from author to address the issue. label Apr 12, 2021
@ghost ghost removed the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Apr 12, 2021
@leidegre
Copy link
Author

@christothes ok, please read carefully. I believe I have been misunderstood twice now.

This has absolutely nothing to do with the AAD infrastructure.

The problem is that the different implementations of TokenCredential employ different mechanisms of caching. While tokens are issued with a 60 minutes validity by default they can be held in cache up to 55 minutes.

If you allowed yourself to cache the token longer, clock skew between services could cause the token to fail validation. Not because it was invalid but because it's expiry cannot be determined with the precision of an atomic clock.

In my previous comment I included a screenshot of what happens if you sample the ExpiresOn property over a period of an hour. As you can see from the screenshot, the token never comes back with 60 minutes of validity, it comes back with between 60-5 minutes of validity depending on how long it has been sitting in cache. This is the default behavior of the DefaultAzureCredential when running locally on my machine.

Neither the DefaultAzureCredential or the GetToken... API is documented with any of this. Through experimentation I can observe this undocumented behavior and I believe your products could be improved by documenting such behaviors.

@ghost ghost added needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team and removed needs-author-feedback Workflow: More information is needed from author to address the issue. labels Apr 13, 2021
@christothes
Copy link
Member

TokenCredential is an abstraction, so we can't generally document details of how the token will be acquired, cached or refreshed, since that's up to the implementer. In general if you're using the TokenCredential abstraction you can't make assumptions about the lifetime or underlying caching, and should cache and handle lifetime in your code. We're working to improve the documentation of specific credentials with respect to how they cache (or don't), but this is particularly tricky in the case of the DefaultAzureCredential which actually is comprised of many credentials each of which have their own caching and refresh behavior.

Also, we're considering library updates to provide some indication in the API as to whether a particular credential will handle caching for you, and possibly configure the behavior of that caching. However, we haven't committed to these additions yet, and it's not clear that this would apply to all credentials in the Azure.Identity library.

@christothes christothes added the needs-author-feedback Workflow: More information is needed from author to address the issue. label Apr 13, 2021
@ghost ghost removed the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Apr 13, 2021
@leidegre
Copy link
Author

I don't like being surprised.

It was surprising to me that the SharedTokenCacheCredential (which does caching) was included in the DefaultAzureCredential chain.

I just want the ability to be able to formulate a valid theory on what will happen when I use these APIs, for that to be possible the surprising stuff needs to be taken out of the default path.

Not clear to me why the SharedTokenCacheCredential is included by default.


I appreciate you taking the time. I think I've said all I can say. ❤️

If you wanna keep the issue open for some tracking purpose please do, otherwise close away!

@ghost ghost added needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team and removed needs-author-feedback Workflow: More information is needed from author to address the issue. labels Apr 14, 2021
@christothes
Copy link
Member

No problem. One thing to add though - as of version 1.4.0.beta-4, the SharedTokenCacheCredential is disabled by default.

https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/identity/Azure.Identity/CHANGELOG.md#breaking-changes-1

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Identity Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. Docs needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team question The issue doesn't require a change to the product in order to be resolved. Most issues start as that
Projects
None yet
Development

No branches or pull requests

4 participants