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

CVE-2024-24790 bump go to 1.23 #8146

Closed
wants to merge 2 commits into from

Conversation

sdx-jkataja
Copy link

@sdx-jkataja sdx-jkataja commented Aug 22, 2024

Thank you for contributing to Velero!

Please add a summary of your change

Fixes vulnerability CVE-2024-24790 by updating Go to 1.22 Edit: in place of 1.22.0

Does your change fix a particular issue?

Fixes #(issue)

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.

@github-actions github-actions bot added the Dependencies Pull requests that update a dependency file label Aug 22, 2024
go.mod Outdated Show resolved Hide resolved
Copy link
Contributor

@blackpiglet blackpiglet left a comment

Choose a reason for hiding this comment

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

Please don't merge this PR until reaching an agreement.

@sdx-jkataja sdx-jkataja changed the title CVE-2024-24790 bump go to 1.22.6 CVE-2024-24790 bump go to 1.22 Aug 23, 2024
Copy link

codecov bot commented Aug 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.05%. Comparing base (934b3ea) to head (ae4229e).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8146      +/-   ##
==========================================
- Coverage   59.07%   59.05%   -0.03%     
==========================================
  Files         364      364              
  Lines       30298    30307       +9     
==========================================
- Hits        17899    17898       -1     
- Misses      10956    10965       +9     
- Partials     1443     1444       +1     

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

@blackpiglet blackpiglet added the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Aug 26, 2024
@blackpiglet
Copy link
Contributor

@sdx-jkataja
Could you sign off your commit to pass the DCO check?

@sdx-jkataja
Copy link
Author

@sdx-jkataja Could you sign off your commit to pass the DCO check?

Sorry, had forgotten to do with the latest changes. Done.

blackpiglet
blackpiglet previously approved these changes Aug 26, 2024
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.

From prior experience, setting go to 1.22 without a patch version causes problems with the build system. Looks like this CVE is fixed in golang 1.22.4, so setting to 1.22 doesn't necessarily guarantee that. I think the right fix would be to either set this to 1.22.4, or maybe 1.22.5 since .5 also includes some CVE fixes. Also, note that we may want to update other files that specify golang versions to guarantee that we're 1.22.4 or later.

@sdx-jkataja
Copy link
Author

sdx-jkataja commented Aug 27, 2024

From prior experience, setting go to 1.22 without a patch version causes problems with the build system.

Yes. Should I revert it to how it was setting go to 1.22.6? Do other reviewers @blackpiglet and @mateusoliveira43 agree?

@mateusoliveira43
Copy link
Contributor

@sseago can you give an example of problem in build systems?

from what I checked, CI job ran from this branch already uses latest 1.22 possible (1.22.6), for example. Reference https://github.com/vmware-tanzu/velero/actions/runs/10556235652/job/29241563973?pr=8146#step:3:24

@sseago
Copy link
Collaborator

sseago commented Aug 27, 2024

@mateusoliveira43 I don't remember exactly -- @shubham-pampattiwar hit it too, but I think the result was an error complaining that if the go 1.22 line didn't specify a patch release, then a go toolchain line is required -- if the go line specifies .0 or .6 or whatever, then the toolchain line is not required. So go 1.22 without toolchain will fail at some point. I forget exactly where. This wasn't true with go 1.21 - it's only for 1.22 and later.

@sseago
Copy link
Collaborator

sseago commented Aug 27, 2024

@mateusoliveira43 we originally had "1.22" -- @shubham-pampattiwar changed it to "1.22.0" because he was having problems building. I reproduced the issue myself locally as well -- basically for at least some actions, if you don't have a patch version for 1.22 and later in your go.mod, then you must specify a go toolchain patch version, so you might as well specify the golang minimum version explicitly.

@shubham-pampattiwar
Copy link
Collaborator

@mateusoliveira43 @sseago yeah I had hit this issue on 1.22 IIRC. But I just tried running the make container command on this PR and it built the velero image for me.

@mateusoliveira43
Copy link
Contributor

I had a similar problem in the past, fixed by updating the Go version I had locally

If everyone can run velero (from default branch) without the need of specifying patch version in go.mod, that would be the best in my vision 😄

@sdx-jkataja sdx-jkataja changed the title CVE-2024-24790 bump go to 1.22 CVE-2024-24790 bump go to 1.23 Aug 29, 2024
@sdx-jkataja
Copy link
Author

Would you find acceptable to move to go 1.23 ?

@sseago
Copy link
Collaborator

sseago commented Aug 29, 2024

@sdx-jkataja I don't think we should move to 1.23 here. The CVE is fixed in 1.22.5. CVE fixes should be minimal in nature. At some point Velero will move to 1.23, but that should be driven more by development concerns, assuming that any necessary security fixes exist on 1.22.x.

@sdx-jkataja
Copy link
Author

sdx-jkataja commented Aug 29, 2024

I regret to withdraw the pull request, but didn't how I could contribute with the conflicting asks.
I thought this would be a straightforward version bump.
The specific CVE is no longer showing up with v1.14.1 but now some are remaining in the base image.

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.

5 participants