AzureAD / microsoft-authentication-library-for-go

The MSAL library for Go is part of the Microsoft identity platform for developers (formerly named Azure AD) v2.0. It enables you to acquire security tokens to call protected APIs. It uses industry standard OAuth2 and OpenID Connect.
MIT License
228 stars 87 forks source link

Ensure cache interoperability with other MSALs #367

Closed chlowell closed 1 year ago

chlowell commented 1 year ago

Inconsistencies in cache data written by MSAL Go and MSAL Python could prevent interop between them. I've noticed these differences (there may be others):

The work here is writing unit tests to ensure MSAL Go writes correct cache entries and fixing any bugs causing it not to do so.

Related: #58

bgavrilMS commented 1 year ago
  1. MSAL.NET also has CamelCase naming. It should be expected that MSALs perform all string operations in a case insensitive way (although note that as per OIDC spec, some strings are case sensitive, like scopes, but we've never came across an issue in this area).

  2. token_type is an optional field. It's absence means that the token is Bearer. Once MSAL Go will support other token types (e.g. PoP), then this field becomes more important

  3. target is an optional field for RT. target is represents the scopes for which the token is defined. And AAD refresh tokens are what we call "Multi-Resource RT", i.e. the same RT can be used to get a token for several resources and scopes. Since MSAL Go only supports AAD authorites, this is fine (same applies for B2C and ADFS).

  4. last_modification_time - optional field. Not used and not set by MSAL.NET either. Not sure what MSAL Py uses it for. @rayluo ?

For full reference, please see https://identitydivision.visualstudio.com/DevEx/_git/AuthLibrariesApiReview?path=/SSO/Schema.md&_a=contents&version=GBdev

I think this issue can be closed.

element-of-surprise commented 1 year ago

If all SDKs are required to handle these fields regardless of case, then we should be good. Go will read in the value regardless of the case.

As the others are optional fields we don't need, can I close this @chlowell ?

rayluo commented 1 year ago

Casing of data types in keys e.g., in Go we have "AccessToken", "AppMetadata", "IDToken" where Python has the same but lowercased

MSAL.NET also has CamelCase naming. It should be expected that MSALs perform all string operations in a case insensitive way (although note that as per OIDC spec, some strings are case sensitive, like scopes, but we've never came across an issue in this area).

MSAL Python's "key makers" deliberately converts all token keys into lowercase (such as this one). This convertion is required by the internal token cache schema, specifically in the "key case sensitivity" section: "Each key component should be normalized before talking to the system persistence layer. Normalization includes: Lowercasing ...".

Therefore, I would suggest MSAL Go to change the CamelCase in persistence layer to lowercase, to comply with the schema. This move is expected to be smooth, because "it should be expected that MSALs perform all string operations in a case insensitive way". Same applies to MSAL .Net, too. CC: @bgavrilMS

MSAL Python writes more fields than MSAL Go

  • RefreshToken last_modification_time

last_modification_time - optional field. Not used and not set by MSAL.NET either. Not sure what MSAL Py uses it for. @rayluo ?

It is optional, so, by definition it is OK to not utilize it.

MSAL Python uses it in this situation (surprisingly, the github search result happens to also contain the inline comment, quoted below): "Since unfit RTs would not be aggressively removed (from token cache), we start from newer RTs which are more likely fit".