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

Chore | upgrade Azure Identity from 1.5.0 to 1.6.0 #1611

Merged
merged 2 commits into from
May 26, 2022

Conversation

lcheunglci
Copy link
Contributor

As mentioned in #1601 , We can upgrade Azure Identity from 1.5.0 to 1.6.0 and it's dependency Microsoft.Identity.Client from 4.30.1 to 4.43.2

@AraHaan
Copy link
Member

AraHaan commented May 12, 2022

Btw take a look at the other dependency updates here as well (as there are others that are outdated)

#1536

note: with all of these updated: dotnet list package --vulnerable returns that there should be no vulnerable packages (could have it also check the dependencies of the packages as well too).

@JRahnama JRahnama added this to the 5.0.0-preview3 milestone May 12, 2022
…ta.SqlClient.AlwaysEncrypted.AzureKeyVaultProvider
@lcheunglci
Copy link
Contributor Author

Thanks @AraHaan just noticed the same changes are in your PR, so I'm closing this one.

@lcheunglci lcheunglci closed this May 12, 2022
@AraHaan
Copy link
Member

AraHaan commented May 12, 2022

I made those changes temporarily but I could rebase it to remove the package updates on it (to not block the updates to the packages).

That separate pr is blocked at least until .NET 6 is added to the CI.

@AraHaan
Copy link
Member

AraHaan commented May 12, 2022

Anyways I think updating the JsonWebTokens and OpenIdConnect to 6.17.0 should make this pr pass (hopefully). At least it does build on me locally.

@lcheunglci
Copy link
Contributor Author

ok, so the #1536 is blocked until the MDS project is upgraded to .NET6. I ran the dotnet list package -vulnerable and it didn't show OpenIdConnect and JsonWebTokens listed with vulnerabilities, and it builds fine with or without the upgrade from 6.8.0 to 6.17.0. @JRahnama any thoughts, and shall I reopen this PR and should I include the OIDC and JWT upgrade?

@JRahnama
Copy link
Contributor

yes please, reopen this PR and check for other dependencies related to this change.

@lcheunglci lcheunglci reopened this May 12, 2022
@AraHaan
Copy link
Member

AraHaan commented May 12, 2022

@lcheunglci still working on finding the other dependencies related to this change?

They should be the Microsoft.IdentityModel.* dependencies I believe.

@lcheunglci
Copy link
Contributor Author

lcheunglci commented May 13, 2022

@lcheunglci still working on finding the other dependencies related to this change?

They should be the Microsoft.IdentityModel.* dependencies I believe.

Thanks for the suggestion. In terms of dependencies directly affected by Azure.Identity upgrade from 1.5.0 to 1.6.0, only Microsoft.Identity.Client and Azure.Core were affected to upgrade the required minimum versions. Microsoft.IdentityModel.JsonWebTokens and Microsoft.IdentityModel.Protocols.OpenIdConnect are not directly referenced in it, so the upgrade for them from 6.8.0 to 6.17.0 isn't required at the moment. As for whether or not they should be upgraded, I checked with @JRahnama and he said we should keep this upgrade to a minimum to avoid any untested side effects, so I'll hold off on it at the moment.

@AraHaan
Copy link
Member

AraHaan commented May 13, 2022

Sounds good, those can be upgraded in a follow up pr to ensure they get tested.

@AraHaan
Copy link
Member

AraHaan commented May 13, 2022

Hopefully this gets merged soon so the follow up pr can happen before the end of preview.3's development stage.

@AraHaan
Copy link
Member

AraHaan commented May 20, 2022

Any ETA on when this will get merged?

@DavoudEshtehari DavoudEshtehari merged commit caaecea into dotnet:main May 26, 2022
@AraHaan
Copy link
Member

AraHaan commented May 26, 2022

@lcheunglci is the follow up pr in the works now to update the Microsoft.IdentityModel.* dependencies as well?

@lcheunglci
Copy link
Contributor Author

@lcheunglci is the follow up pr in the works now to update the Microsoft.IdentityModel.* dependencies as well?

I did try to upgrade both the Microsoft.IdentityModel.OpenIdConnect and JsonWebTokens locally, and it worked fine on the netcore project; however, it kept giving an error when it tries to restore the package Microsoft.IdentityModel.Abstrations which doesn't exist in the azure repo, so I was holding off until that's resolved until I opened a new PR for those upgrades

@lcheunglci
Copy link
Contributor Author

lcheunglci commented May 31, 2022

I think I found out why. The nuget repo that the solution/project points to i.e. azure nuget repo https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public/nuget/v3/index.json does not contain the required dependency Microsoft.IdentityModel.Abstrations; however, the nuget public repo does https://api.nuget.org/v3/index.json contain it. I wonder if there was a reason behind not using the nuget public repo and using the azure nuget repo instead.

@lcheunglci lcheunglci deleted the Upgrade-AzureIdentity1.6.0 branch July 19, 2022 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants