Update guava dependency to address CVE-2023-2976 #10249
Conversation
|
I am not sure what the correct course of action is here. The version of |
I checked and the latest non-RC version 3.23.2 also uses 1.3 so not sure which version you can update to. If everything works out without upgrading protobuf-java-util may be it's okay? |
bestbeforetoday
force-pushed
the
guava
branch
2 times, most recently
from
8baf8b4 to
54c8c47
Compare
|
Thank you for the input. I noticed there was no release version of protobuf-java-util with a more recent dependency and wondered if it was necessary to update protobuf-java-util first. I have changed the transitive dependency version checking logic slightly, which makes it more similar to Maven's and avoids the build failure. Also specified an explicit dependency in the Maven example project to ensure correct transitive dependency version selection. I am not sure if these changes are really satisfactory. Particularly the Gradle version check is using a deprecated Gradle class to parse package versions. I guess the alternative is to use a different version parser, or to implement some version comparison logic in this project. Any guidance or help would be appreciated. |
sanjaypujare
added
the
kokoro:run
grpc-kokoro
removed
the
kokoro:run
Hehe. You are giving Maven too much credit. It doesn't compare versions and choose the latest. Maven does a breadth-first scan of the dependency tree and always chooses the first version is finds ("nearest definition"). So it trivially makes bad decisions. The check is "would Maven downgrade dependencies," mirroring Maven Enforcer's requireUpperBoundDeps which detects the same. The check assumes Gradle made a good version decision, and just checks that Maven would have made the same decision. You could use a comparison (but not with VersionNumber because it is being removed), but you'd need to reverse the comparison: version < goldenVersion. The proper solution is to add an explicit dependency (like you did for the example) or to reorder the dependency list. Explicit dependencies always work, but reordering the dependency list can significantly reduce the burden at times. |
|
Thanks for the response. I was referring to Maven's requireUpperBoundDeps, not to Maven's version resolution in general, since the function I changed is supposed to implement similar checks to Maven's requireUpperBoundDeps. Are you sure that it is an error for a transitive dependency to want an older version than the one resolved by Gradle? I thought it was supposed to be the other way, where it is an error for the resolved version to be older than one required by a transitive dependency since it might be relying on functionality not available in the resolved version. In which case the check should be Adding an explicit dependency sounds reasonable. However, will that still cause the existing check to fail since the resolved version does not match exactly the one requested by all transitive dependencies? If using VersionNumber is not an acceptable implementation approach, please could you advise on what should be used? |
Don't touch the checker. It is doing exactly what it's supposed to be doing.
That means it is checking for cases that Maven would use a newer version than Gradle. Upgrading is okay, so this would only fail in cases we don't care about. But also, Gradle is already choosing the highest transitive dependency. Since Gradle is already choosing reasonable versions, simply checking that Maven would choose something else is simple and useful.
No, the check just verifies the version of the first transitive dependency it encounters of each artifact. So it only checks the version that Maven would have used. You resolve this failure exactly like you would in Maven, and then the check starts passing as it would in Maven. It is dissimilar from dependencyConvergence, which requires a lot of work for a very brittle system (each dependency excluded is just a future bug). |
|
Thank you for the clear explanation. As soon as I get a chance I'll push an update with an explicit dependency and the change to the checking code reverted. |
Maven requireUpperBoundDeps checks that the version for each dependency resolved during a build, is equal to or higher than all transitive dependency declarations. It is not required for a resolved dependency to exactly match all transitive dependency declarations. As a proof-of-concept, this implementation uses Gradle's VersionNumber utility, which is deprecated and scheduled to be removed in Gradle 9. An alternative (or custom) version comparison utility should be used.
Revert change to requireUpperBoundDepsMatch since explicit dependencies are the correct way to resolve the version inconsistency in transitive dependencies.
sanjaypujare
added
the
kokoro:run
grpc-kokoro
removed
the
kokoro:run
|
CC @sergiitk |
|
Will you backport this change to whichever branch will produce the next published release, or should I raise a PR with that backport? If it helps for me to raise a PR with a cherry-pick of this commit, please just let me know which branch(es) I should raise against. |
|
The release is going out today-ish. Last week I considered whether we'd want to backport this (before it was merged), but the Guava update is so big it scared me too much. 32.0.x is a bit too big to rush it out. And the fix in 32.0.1 was exactly because of the code change to fix the CVE. If the grpc release was in two or three more weeks, I'd be more comfortable with this. I just see too many potential issues that could prevent people from upgrading quickly. Also, users can upgrade Guava themselves, whereas with #10260 they can't. We lived with the CVE for a while as "it didn't actually matter" because the code wasn't used and most deployments wouldn't be vulnerable if it were. FileBackedOutputStream doesn't really change that. Downstream can upgrade Guava themselves, now, which is a better place to be. For the same reasoning, I don't think we'll want a patch release for this. That does make me uncomfortable, because we take vulnerabilities seriously and we want to help make the ecosystem healthy, but the practical side of me is even more uncomfortable because it seems too much risk for limited gain. |
Closes #10247