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

Change | Updating Azure.Identity version to 1.11.2 #2462

Merged
merged 10 commits into from
Apr 26, 2024

Conversation

JRahnama
Copy link
Member

Addresses the issue mentioned at #2460 for CVE-2024-29992

Copy link
Contributor

@arellegue arellegue left a comment

Choose a reason for hiding this comment

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

lgtm

@cheenamalhotra
Copy link
Member

cheenamalhotra commented Apr 12, 2024

Based on NuGet's docs, use * instead. See Use Floating Versions

The comments in example show that smallest stable versions are resolved with version ranges whereas we want highest applicable version.

@SimonCropp
Copy link
Contributor

this seems to change significantly more than "Updating Azure.Identity version to 1.11.0". it is also assigning upper version limits for many references

@SimonCropp
Copy link
Contributor

also. can we get a patch out for this. people should not need to wait for v6 to get this fix

@JRahnama
Copy link
Member Author

JRahnama commented Apr 15, 2024

also. can we get a patch out for this. people should not need to wait for v6 to get this fix

Sure, it is in our to do list for next patch release. Date yet TBD.

this seems to change significantly more than "Updating Azure.Identity version to 1.11.0". it is also assigning upper version limits for many references

This is going to change to floating versions. something like 1.11.* or 1.* to pick the latest stable release of the dependency.

@SimonCropp
Copy link
Contributor

This is going to change to floating versions. something like 1.11.* or 1.* to pick the latest stable release of the dependency.

Doesnt nuget always resolve to the lowest transitive? so, in this case, people would still need an explicit reference to Azure.Identity so as to avoid the CVE?

@JRahnama
Copy link
Member Author

This is going to change to floating versions. something like 1.11.* or 1.* to pick the latest stable release of the dependency.

Doesnt nuget always resolve to the lowest transitive? so, in this case, people would still need an explicit reference to Azure.Identity so as to avoid the CVE?

Something similar to this documentation.

@JRahnama
Copy link
Member Author

This is going to change to floating versions. something like 1.11.* or 1.* to pick the latest stable release of the dependency.

Doesnt nuget always resolve to the lowest transitive? so, in this case, people would still need an explicit reference to Azure.Identity so as to avoid the CVE?

Something similar to this documentation.

For example if we had 1.* in our Versions.props, when 1.10.3 was marked as deprecated, the next available version would have been picked which is 1.11.0

@roji
Copy link
Member

roji commented Apr 15, 2024

@JRahnama @cheenamalhotra as far as I know, it's discouraged to use floating nuget dependency versions in most production software, because it leads to non-deterministic builds. That is, the moment a dependency releases a version, your next CI run automatically uses that, possibly causing failures, performance regressions or whatever. This can make sense in various prototyping or internal development scenarios, but is less recommended for stable, public software.

The alternative mechanism for keeping versions up to date is dependabot, which is an official github feature; a bot submits PRs updating dependency versions whenever new ones become available, and team members merge these as usual. This ensures a git commit for the dependency version change, allowing tracking exactly what changed and when, e.g. when looking at the project history or diagnosing some problem. I'd recommend looking into enabling dependabot - it's widely used in many .NET projects (including EF and other Microsoft .NET projects) and works very well.

Let me know if the above makes sense and if I've missed something specific to SqlClient.

@JRahnama
Copy link
Member Author

JRahnama commented Apr 15, 2024

@JRahnama @cheenamalhotra as far as I know, it's discouraged to use floating nuget dependency versions in most production software, because it leads to non-deterministic builds. That is, the moment a dependency releases a version, your next CI run automatically uses that, possibly causing failures, performance regressions or whatever. This can make sense in various prototyping or internal development scenarios, but is less recommended for stable, public software.

The alternative mechanism for keeping versions up to date is dependabot, which is an official github feature; a bot submits PRs updating dependency versions whenever new ones become available, and team members merge these as usual. This ensures a git commit for the dependency version change, allowing tracking exactly what changed and when, e.g. when looking at the project history or diagnosing some problem. I'd recommend looking into enabling dependabot - it's widely used in many .NET projects (including EF and other Microsoft .NET projects) and works very well.

Let me know if the above makes sense and if I've missed something specific to SqlClient.

Thanks, @roji.

It appears that our quest to mitigate the rush for hotfix releases when a dependency becomes deprecated doesn't have a viable solution, based on your insights, and we're left with no choice but to release hotfixes whenever a dependency version is deprecated.

Copy link

codecov bot commented Apr 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.72%. Comparing base (61e89f9) to head (9512262).
Report is 10 commits behind head on main.

❗ Current head 9512262 differs from pull request most recent head ca86126. Consider uploading reports for the commit ca86126 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2462      +/-   ##
==========================================
- Coverage   73.67%   72.72%   -0.96%     
==========================================
  Files         313      313              
  Lines       68590    61737    -6853     
==========================================
- Hits        50535    44897    -5638     
+ Misses      18055    16840    -1215     
Flag Coverage Δ
addons 92.88% <ø> (ø)
netcore 77.04% <100.00%> (-0.64%) ⬇️
netfx 70.40% <100.00%> (-1.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SimonCropp
Copy link
Contributor

@JRahnama

It appears that our quest to mitigate the rush for hotfix releases when a dependency becomes deprecated doesn't have a viable solution, based on your insights, and we're left with no choice but to release hotfixes whenever a dependency version is deprecated.

IMO the fix is to remove the unnecessarily dependency: #110

@roji
Copy link
Member

roji commented Apr 16, 2024

@SimonCropp assuming you meant #1108, I don't think that's related. Even if the Azure stuff were split into a different package for use only by Azure users (which is what #1108 is about), that "SqlClient Azure" package would still have the exact same issue. In other words, this would stop affecting all SqlClient users, but it would still affect some.

But at the end of the day, it seems reasonable/standard that if/when a vulnerability is found in a dependency of your package, you release a new version of your package referencing a new version - IMHO that's simply how things are supposed to work. I'd definitely not want users of my package to always automatically get the latest patch version of my package's dependencies, since that would expose them to sudden bugs and behavior changes without them having done any change on their side.

@SimonCropp
Copy link
Contributor

@roji

Even if the Azure stuff were split into a different package for use only by Azure users (which is what #1108 is about), that "SqlClient Azure" package would still have the exact same issue. In other words, this would stop affecting all SqlClient users, but it would still affect some.

that is true. but the split would enabled the packages to have a different release cadence. it would be more reasonable for the "SqlClient Azure" package to do a release in response to a release of Azure.Identity. in fact the "SqlClient Azure" package could be shipped from the Azure.Identity in lock step.

@AraHaan
Copy link
Member

AraHaan commented Apr 16, 2024

Or just wildcard the azure dependencies so then every build of SqlClient would pull in the latest Azure package versions.

@roji
Copy link
Member

roji commented Apr 17, 2024

@AraHaan please see discussion above about floating versions.

@JRahnama JRahnama changed the title Change | Updating Azure.Identity version to 1.11.0 Change | Updating Azure.Identity version to 1.11.1 Apr 17, 2024
@SimonCropp
Copy link
Contributor

Azure.Identity is now at 1.11.2

@JRahnama JRahnama changed the title Change | Updating Azure.Identity version to 1.11.1 Change | Updating Azure.Identity version to 1.11.2 Apr 26, 2024
@JRahnama JRahnama merged commit c33bde2 into dotnet:main Apr 26, 2024
148 checks passed
@JRahnama JRahnama deleted the updating-azure.identity branch May 6, 2024 21:46
@BlythMeister
Copy link

when is this likely to hit the shelves?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 Enhancement New feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants