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

[main] Fix #6429 #6453

Closed
wants to merge 1 commit into from
Closed

Conversation

blackpiglet
Copy link
Contributor

@blackpiglet blackpiglet commented Jul 3, 2023

Remove CVE-2021-3121 affected github.com/gogo/protobuf version from go.sum

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #6429

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

Remove CVE-2021-3121 affected github.com/gogo/protobuf version from go.sum

Signed-off-by: Xun Jiang <[email protected]>
@github-actions github-actions bot added the Dependencies Pull requests that update a dependency file label Jul 3, 2023
@blackpiglet blackpiglet changed the title Fix #6429 [main] Fix #6429 Jul 3, 2023
@blackpiglet blackpiglet added the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Jul 3, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #6453 (a382016) into main (84eca51) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #6453      +/-   ##
==========================================
+ Coverage   59.14%   59.15%   +0.01%     
==========================================
  Files         229      229              
  Lines       24184    24193       +9     
==========================================
+ Hits        14303    14311       +8     
- Misses       8880     8882       +2     
+ Partials     1001     1000       -1     

see 3 files with indirect coverage changes

@blackpiglet blackpiglet marked this pull request as ready for review July 3, 2023 08:54
@anshulahuja98
Copy link
Collaborator

Thank you for the fix!

@anshulahuja98
Copy link
Collaborator

LGTM

@sseago
Copy link
Collaborator

sseago commented Jul 3, 2023

This seems to be a false positive. As of go 1.17, all direct and indirect dependencies are included in go.mod. go.sum lists every dependency of every package prior to package version resolution, so multiple versions can be listed. Only one version of a package will be built, though, and that version should be specified in go.mod:

$ grep gogo go.mod 
	github.com/gogo/protobuf v1.3.2 // indirect

Also, go list -m all will list all modules for the build list, again only the (fixed) version is included:

$ go list -m all|grep gogo
github.com/gogo/protobuf v1.3.2

I also confirmed this by running go mod vendor locally:

$ grep gogo vendor/modules.txt 
# github.com/gogo/protobuf v1.3.2
github.com/gogo/protobuf/proto
github.com/gogo/protobuf/sortkeys

Note that Dependabot has stopped using go.sum for vulnerability scanning as well: https://github.blog/changelog/2023-03-07-dependency-graph-removes-go-sum-support/

@sseago
Copy link
Collaborator

sseago commented Jul 3, 2023

Also, if you check PR/issue history, we fixed this CVE a while back, which is why the currently-included package for the build is the fixed version. I'd rather avoid replace directives when they're unnecessary as then later if a package requires a newer version of this package, the replace becomes a forced downgrade, which could introduce unexpected behavior or bugs.

Copy link
Collaborator

@sseago sseago left a comment

Choose a reason for hiding this comment

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

I'd recommend that we close this PR as unnecessary, but holding off on that until the reported issue is closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dependencies Pull requests that update a dependency file kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CVE-2021-3121 - gogo/protobuf
5 participants