Skip to content

Add KID check when getting a token from cache#692

Closed
JorgeDaboub wants to merge 2 commits intoAzureAD:devfrom
JorgeDaboub:pop-token-cache-bug
Closed

Add KID check when getting a token from cache#692
JorgeDaboub wants to merge 2 commits intoAzureAD:devfrom
JorgeDaboub:pop-token-cache-bug

Conversation

@JorgeDaboub
Copy link

We have encountered an issue with how MSAL is caching Tokens that have a KID.

The issue is:

  • Say you have a Token with ClientID (AAAA..) and KID (11111..) which gets cached. And returned when needed. So far no problem.
  • If you then come and try to get a token for ClientID (AAAA..) and KID (22222..) the expected behavior would be to get a new token since the KID has changed.
  • This is does not happen. Although the _find method takes in the "key_id" as a query parameter the query will still return tokens that only match the client ID. And there is no additional check to ensure the "key_id" is the same one.

The solution:

A quick check when getting a token from cache. If a KeyID is present it should match the one provided.

I validated these changes locally and they solve the issue. For reference here is the code we use: https://github.com/Azure/azure-cli-extensions/blob/2a1898ff1b70153df2311ccb784a90751914c0b1/src/connectedk8s/azext_connectedk8s/_clientproxyutils.py#L86

We have an open ICM with more context: https://portal.microsofticm.com/imp/v3/incidents/incident/494360176/summary

@JorgeDaboub JorgeDaboub requested a review from a team as a code owner April 26, 2024 23:15
@JorgeDaboub JorgeDaboub marked this pull request as draft May 1, 2024 20:33
@rayluo
Copy link
Contributor

rayluo commented Jul 12, 2024

Thanks for bringing this to our attention, @JorgeDaboub . This was the same issue that we recently fixed in #717 already. Please install our dev branch to confirm that our fix also works for you.

pip install --force-reinstall "git+https://github.com/AzureAD/microsoft-authentication-library-for-python.git@dev"

It would be great if you can let us know your test result within a day or two, because we are approaching a code-freeze period for another release.

Closing for now. Please reopen if our fix won't work for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants