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

[Feature Request] Support force_refresh for service principal #650

Open
jiasli opened this issue Jan 17, 2024 · 3 comments · Fixed by #666
Open

[Feature Request] Support force_refresh for service principal #650

jiasli opened this issue Jan 17, 2024 · 3 comments · Fixed by #666

Comments

@jiasli
Copy link
Contributor

jiasli commented Jan 17, 2024

MSAL client type

Confidential

Problem Statement

Azure CLI currently faces this issue:

For service principal, after successfully login and logout, it is possible to re-authenticate using a random word as the password.

The root cause is the behavior change of acquire_token_for_client.

Before #581, acquire_token_for_client acquires an access token by making a web request to AAD eSTS. This forces MSAL to validate the service principals client ID and secrets against AAD eSTS. However, after this PR, acquire_token_for_client will check the token cache first and return the access token if the service principals client ID matches what's in the token cache, thus skipping the web request.

MSAL also forbids force_refresh to bypass the token cache, making it impossible to refresh the service principal token:

if kwargs.get("force_refresh"):
raise ValueError( # We choose to disallow force_refresh
"Historically, this method does not support force_refresh behavior. "
)

Proposed solution

It should be possible to force_refresh access tokens for service principals.

@bgavrilMS
Copy link
Member

This would be a consistency item with the rest of the MSALs, so approved.

@rayluo rayluo changed the title [Feature Request] Support force_refresh for service principal [Feature Request] Support ~force_refresh~ remove_tokens_for_client() for service principal Feb 10, 2024
@rayluo rayluo changed the title [Feature Request] Support ~force_refresh~ remove_tokens_for_client() for service principal [Feature Request] Support remove_tokens_for_client() for service principal Feb 10, 2024
@jiasli jiasli changed the title [Feature Request] Support remove_tokens_for_client() for service principal [Feature Request] Support force_refresh for service principal Mar 28, 2024
@jiasli
Copy link
Contributor Author

jiasli commented Mar 28, 2024

Sorry for changing the issue title back, as this issue is different from #649.

Even after #666, we still need the functionality to force refresh. To utilize remove_tokens_for_client, Azure CLI needs to check the MSAL token cache first before logging in a service principal and log out the existing one. This is a little bit complex.

Suppose a user runs 2 az login commands but with different passwords:

az login --service-principal --username app1 --password pass1
az login --service-principal --username app1 --password pass2

Intuitively, the second command should make a new web request and overwrite app1's access token in the token cache, but currently a user should do:

az login --service-principal --username app1 --password pass1
az logout
az login --service-principal --username app1 --password pass2

@bgavrilMS bgavrilMS reopened this Mar 28, 2024
@bgavrilMS
Copy link
Member

Realistically, this behavior is expected. There is already a token for that SP. I believe PowerShell folks were also hit by similar issues, and decided to educate customers instead.

I know that some apps (like SQL Server / Kusto) have decided to maintain a dictionary of (secret, CCA) or (cert.Thumbprint, CCA) in order to partition the cache by cert.

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

Successfully merging a pull request may close this issue.

3 participants