-
Notifications
You must be signed in to change notification settings - Fork 141
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
Use golang.org/x/[email protected] to mitigate CVE-2022-32149 #251
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the replace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, it's a transitive dependency that has that requirement. I guess the potential concern is that version combination being untested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also an analysis that Jan did in https://kubernetes.slack.com/archives/C8EJ01Z46/p1669292513960349
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To address the compatibility issues, we could change replace clause from v0.4.0 to v0.3.8. However, according to semantic versioning, which is followed by golang, upgrade of MINOR is backward compatible.
The analysis seems to be correct, but upgrading seems like a more trustworthy approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this at today's csi standup. It looks like go.sum lists all the possible versions that could be used, but that doesn't necessarily mean that the version is being used. So cve scanners that rely on go.sum may produce a lot of false positives.
https://stackoverflow.com/questions/70128040/content-of-go-sum-and-modules-really-used-by-a-go-application
https://stackoverflow.com/questions/69825533/why-does-go-sum-include-so-many-older-packages
Given that this change removed the entries from go.sum, but didn't actually change anything in the vendor directory (which reflects what's actually being used) confirms this.
What scanner are you using? It looks like trivy no longer relies on go.sum when the go version in go.mod is >= 1.17: aquasecurity/trivy#1973. But dependabot still does not: dependabot/dependabot-core#4740
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used a commercial security analysis tool to detect vulns. It is a good point that presence of a dependency in go.sum does not necessarily mean that it's actually used. However, the affected version (v0.3.7) is explicitly listed in
go.modof node-driver-registrar (indirect dependency but still a dependency) (it's not go.sum issue that was mentioned).I agree that it looks like the affected part of the code is not used (even indirectly) by node-driver-registrar. However, afaik security scanners standards are not designed to go into analysis on a function level, they stop at reporting affected package version. Yet, they are still useful tool.
I believe that there is a choice to be made here by maintainers. Every time a new vulnerability is discovered either affected dependency should be updated regardless if the node-driver-registrar is using the affected, or do the vulnerability security audit every time and somehow maintain list of such vulnerabilities which are believed to be not affecting the repo and make all users aware that their scans might produce higher-than-usual number of false-positives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the go.mod, it looks like only 0.4.0 is listed: https://github.com/kubernetes-csi/node-driver-registrar/pull/251/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R42
Anyway, I think we should go ahead and merge this because it is a burden to have users manage exceptions in their scanners, but we should also notify the scanning tools of false-positives so that they can be improved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you are actually right, I was looking at 2.6.1 and 2.6.2 tags.
0.4.0is not on master b/c of #243 which I've just noticed.The above makes this PR redundant, thank you for the discussion though! I think it's important to recognize how vulns should be addressed.