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.

Sign up for GitHub

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

Jump to bottom

Bump go toolchain version to address CVE-2023-45288 for release-1.3 #713

Merged
merged 2 commits into from
Apr 5, 2024
Merged

Bump go toolchain version to address CVE-2023-45288 for release-1.3 #713

merged 2 commits into from
Apr 5, 2024

Conversation

henrybear327
Copy link
Contributor

@henrybear327 henrybear327 commented Apr 4, 2024 •
edited

Changes:

  • Bump toolchain version to 1.21.9 due to CVE-2023-45288
  • run go mod tidy

Address linter changes (align with the main branch):

  • remove rand.Seed(s)
  • use rand.Read from "crypto/rand"
  • add //nolint:all for (*reflect.SliceHeader)(slice) -> will be fixed in a new PR

Reference:

@henrybear327
Copy link
Contributor Author

henrybear327 commented Apr 4, 2024 •
edited

@ahrtr it's a cross-minor-version bump from 1.17.13 to 1.21.9, and thus quite some linter errors to fix!

I would like to make sure the version bump is done correctly before moving on to addressing the linter issues!

@henrybear327 henrybear327 changed the title Bump go toolchain version to address CVE-2023-45288 Bump go toolchain version to address CVE-2023-45288 for release-1.3 Apr 4, 2024
@henrybear327 henrybear327 force-pushed the cve/CVE-2023-45288-release-1.3 branch from 59f0fb7 to 957e011 Compare April 4, 2024 08:53
@ivanvc
Copy link
Member

ivanvc commented Apr 4, 2024

Cross linking etcd-io/etcd#17703

@ivanvc ivanvc mentioned this pull request Apr 4, 2024
Closed
10 tasks
@ivanvc
Copy link
Member

ivanvc commented Apr 4, 2024

Should we adopt the main branch's .go-version approach to defining the Go version to avoid mismatches in different files? What do you think @ahrtr?

@ahrtr
Copy link
Member

ahrtr commented Apr 4, 2024

Should we adopt the main branch's .go-version approach to defining the Go version to avoid mismatches in different files? What do you think @ahrtr?

Sounds good to me. Thanks

@henrybear327
Copy link
Contributor Author

Should we adopt the main branch's .go-version approach to defining the Go version to avoid mismatches in different files? What do you think @ahrtr?

Sounds good to me. Thanks

Thanks @ivanvc and @ahrtr, I will implement it.

@faddat faddat mentioned this pull request Apr 4, 2024
Merged
@henrybear327 henrybear327 force-pushed the cve/CVE-2023-45288-release-1.3 branch 2 times, most recently from b0fc38a to 980f2a6 Compare April 4, 2024 18:18
@henrybear327
Copy link
Contributor Author

henrybear327 commented Apr 4, 2024 •
edited

.go-version approach

@ivanvc @ahrtr, I have made my attempt at the .go-version approach in another PR!

@henrybear327 henrybear327 force-pushed the cve/CVE-2023-45288-release-1.3 branch from 7770b1a to 6c8bac3 Compare April 4, 2024 18:23
ivanvc
ivanvc reviewed Apr 4, 2024
View reviewed changes
Copy link
Member

@ivanvc ivanvc left a comment

Choose a reason for hiding this comment

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

The workflow changes LGTM. I defer to Benjamin regarding the other changes ✌️

manydbs_test.go Outdated Show resolved Hide resolved
@henrybear327 henrybear327 force-pushed the cve/CVE-2023-45288-release-1.3 branch from 194d4d8 to d62ad4b Compare April 4, 2024 19:18
ahrtr
ahrtr reviewed Apr 4, 2024
View reviewed changes
cmd/bbolt/main_test.go Show resolved Hide resolved
unsafe.go Show resolved Hide resolved
@henrybear327 henrybear327 mentioned this pull request Apr 4, 2024
Merged
@henrybear327 henrybear327 force-pushed the cve/CVE-2023-45288-release-1.3 branch from d62ad4b to 138719c Compare April 4, 2024 20:47
@henrybear327
Copy link
Contributor Author

After second thought, I bumped the go toolchain to 1.22.2, as we were at 1.17.x and there isn't a reason for us to just bump the version to 1.21.x.

@ivanvc
Copy link
Member

ivanvc commented Apr 5, 2024

Please refer to etcd-io/etcd#17348 (comment). Unless @ahrtr thinks otherwise, I think it would be better to stay on 1.21 for the release-1.3 branch.

@henrybear327
Copy link
Contributor Author

henrybear327 commented Apr 5, 2024 •
edited

Please refer to etcd-io/etcd#17348 (comment). Unless @ahrtr thinks otherwise, I think it would be better to stay on 1.21 for the release-1.3 branch.

@ivanvc, thanks for the pointer. I will rework the PR

  • use go version 1.21.9
  • split the go version patch

@henrybear327 henrybear327 force-pushed the cve/CVE-2023-45288-release-1.3 branch from 138719c to 63172d0 Compare April 5, 2024 07:57
@henrybear327 henrybear327 mentioned this pull request Apr 5, 2024
Merged
@henrybear327
Copy link
Contributor Author

Please refer to etcd-io/etcd#17348 (comment). Unless @ahrtr thinks otherwise, I think it would be better to stay on 1.21 for the release-1.3 branch.

@ahrtr which go toolchain version do we want to go with on release-1.3 :) 1.21.9 or 1.22.2

henrybear327 and others added 2 commits April 5, 2024 14:21
@henrybear327
Bump go toolchain version to address CVE-2023-45288
0b59bc1 
Changes:
- Bump toolchain version to 1.21.9 due to CVE-2023-45288
- run `go mod tidy`

Reference:
- PR etcd #17703

Signed-off-by: Chun-Hung Tseng <henrybear327@gmail.com>
@henrybear327 @ivanvc
Address linter issues
9b60c13 
Changes (align with the main branch):
- remove rand.Seed(s)
- use rand.Read from "crypto/rand"
- add //nolint:all for (*reflect.SliceHeader)(slice) -> will fix in a 
follow-up PR

Signed-off-by: Chun-Hung Tseng <henrybear327@gmail.com>
Co-authored-by: Iván Valdés Castillo <iv@nvald.es>
@henrybear327 henrybear327 force-pushed the cve/CVE-2023-45288-release-1.3 branch from 63172d0 to 9b60c13 Compare April 5, 2024 12:22
@henrybear327
Copy link
Contributor Author

@ahrtr rebased and using go version 1.21.9!

ivanvc
ivanvc approved these changes Apr 5, 2024
View reviewed changes
Copy link
Member

@ivanvc ivanvc left a comment

Choose a reason for hiding this comment

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

Thanks @henrybear327

@ahrtr ahrtr merged commit 2c29534 into etcd-io:release-1.3 Apr 5, 2024
9 checks passed
@henrybear327 henrybear327 deleted the cve/CVE-2023-45288-release-1.3 branch April 6, 2024 19:21
henrybear327 added a commit to henrybear327/bbolt that referenced this pull request Apr 6, 2024
@henrybear327
Remove UnsafeSlice and use unsafe.Slice
a624c19 
References:
- etcd-io#713
- etcd-io#566
@henrybear327 henrybear327 mentioned this pull request Apr 6, 2024
Merged
henrybear327 added a commit to henrybear327/bbolt that referenced this pull request Apr 6, 2024
@henrybear327
Remove UnsafeSlice and use unsafe.Slice
9e3d732 
References:
- etcd-io#713
- etcd-io#566

Signed-off-by: Chun-Hung Tseng <henrybear327@gmail.com>
@ahrtr ahrtr mentioned this pull request May 3, 2024
Closed
@serathius
Copy link
Member

We should avoid bumping go version in go.mod on stable releases as it means we drop support of older Go version which is a breaking change.

Instead of bumping go version in go.mod we should add a separate CI job that tests multiple go version for compatibility.

@ahrtr
Copy link
Member

ahrtr commented May 6, 2024

We should avoid bumping go version in go.mod on stable releases as it means we drop support of older Go version which is a breaking change.

Instead of bumping go version in go.mod we should add a separate CI job that tests multiple go version for compatibility.

#747 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivanvc ivanvc ivanvc approved these changes

@ahrtr ahrtr ahrtr approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@henrybear327 @ivanvc @ahrtr @serathius