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

AzureCliCredential: add support for expires_on field. #5180

Merged
merged 13 commits into from
Dec 15, 2023

Conversation

antkmsft
Copy link
Member

@antkmsft antkmsft commented Nov 23, 2023

Closes #5116.

It works the following way - if expires_on (the new one) property is present, it gets parsed before ExpiresOn (the old one) property.

This PR is not a replacement for #5179, which mitigates the issue if the user has older version of Azure CLI installed. We can't expect users to always have the latest installed. The one without time zone info has been there first, for many years, and only now they are adding the expires_on property.

#5179 is good enough, but not ideal - once a year, one day in fall, when it is close to moving from DST, the token will be off for one hour, and then everything should work, but an extra token request will be sent when the auth policy will supply a token which was obtained when the DST was on, but the service will reject it, and then the policy will tell the credential to get a new token, and everything will work once again.
And one day in spring, when the clock moves to DST, the tokens obtained will have 1 hour shorter expiration than what they actually are, and some extra requests to refresh tokens will be made.

--
UWP CI is currently blocked until microsoft/vcpkg#35279 is fixed / microsoft/vcpkg#35116 is merged (#5181 would unblock)

@antkmsft antkmsft self-assigned this Nov 27, 2023
@ahsonkhan
Copy link
Member

Let's wait on this until we have a conclusion on Az Cli version support.

@antkmsft
Copy link
Member Author

Let's wait on this until we have a conclusion on Az Cli version support.

Sure.
When will we know?
BTW, if the decision is to require the newer azure CLI version, I would want to talk to them, because there is no reason to drop the support entirely - we perfectly can parse timestamps in the old format - especially with #5179 we could do that, and it would be excessive in getting tokens twice a year in the locales that move to DST and back.

@ahsonkhan
Copy link
Member

ahsonkhan commented Nov 30, 2023

When will we know?
BTW, if the decision is to require the newer azure CLI version, I would want to talk to them, because there is no reason to drop the support entirely - we perfectly can parse timestamps in the old format - especially with #5179 we could do that, and it would be excessive in getting tokens twice a year in the locales that move to DST and back.

I discussed this today and shared the conclusion on the issue:
#5116 (comment)

The other languages don't have the DST concern, let's discuss that separately. It does seem like that's an OK trade-off given where C++ language support is, and given AzureCliCredential is a dev time credential anyway.

@antkmsft antkmsft requested a review from ahsonkhan November 30, 2023 20:52
Copy link
Member

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

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

Otherwise, LGTM.

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

Successfully merging this pull request may close these issues.

[Identity] Evaluate expires_on field in AzureCliCredential
2 participants