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

Using msal token cache to fetch refresh token inside proxy command for az cli version >= 2.30 #4194

Closed
wants to merge 11 commits into from

Conversation

sirireddy12
Copy link
Contributor


This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your PR is merged into master branch, a new PR will be created to update src/index.json automatically.
The precondition is to put your code inside this repo and upgrade the version in the PR but do not modify src/index.json.

@zhoxing-ms
Copy link
Contributor

@sirireddy12 May I ask do you need to write the description of this change in to the history note and release a new extension for this change? If so, please modify the HISTORY.rst to record the this change history and modify setup.py to release a new version

Comment on lines 1650 to 1651
msal_token_cache_user = r'.azure/msal_token_cache.bin'
msal_token_cache_spn = r'.azure/service_principal_entries.bin'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DON'T DO THIS. THIS IS NOT SUPPORTED. See Azure/azure-cli#19853 (comment)

token_cache._reload_if_necessary()
home_account_id = response_user_objectid + "." + tenantId
owned_by_home_account = {"home_account_id": home_account_id}
creds_info = token_cache.find(PersistedTokenCache.CredentialType.REFRESH_TOKEN, query=owned_by_home_account)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rayluo, I think this is another instance where people are hacking MSAL cache to get the refresh token.

Copy link
Member

@rayluo rayluo Dec 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is really unfortunate.

@sirireddy12 , why is refresh token (RT) needed? Can the usage pattern be replaced by periodically requesting an access token, possibly via Azure CLI? (@jiasli , does az get-access-token ... has an equivalent helper function to be called by extensions?)

FYI: Not only the token cache helpers are considered internal, MSAL would probably NOT save RT in token cache in near future.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rayluo Refresh token is needed to retrieve PoP token. Does PoP implementation support available in MSAL python which az extension can utilize?

Copy link
Member

@rayluo rayluo Dec 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rayluo Refresh token is needed to retrieve PoP token. Does PoP implementation support available in MSAL python which az extension can utilize?

MSAL Python does not currently support PoP. However, I got an impression that the PoP implementation might be similar to another feature named "Ssh Cert" which MSAL Python does support, and then there is also an az extension "ssh" already handles the key management and feeds signed content to MSAL. You may take a look and see whether that pattern could be repurposed for PoP token.

(Oh, I did not notice your alias, @krdhruva :-). The content above was the idea that I already mentioned in our email conversation.)

token_cache_location = os.path.expanduser(os.path.join('~', msal_token_cache_spn))
try:
if operating_system == 'Windows':
persistence = msal_extensions.FilePersistenceWithDataProtection(token_cache_location)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You really should not be doing this. MSAL's internal cache is not a public artifact and may change without notice.

In fact, this is changing as we speak - MSAL py will use the Windows broker (WAM). Brokers don't even return RT and cache is going to be stored elsewhere (not shared anymore).

@yonzhan yonzhan added this to the Backlog milestone Dec 20, 2021
@sirireddy12 sirireddy12 closed this Feb 7, 2022
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.

8 participants