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

etcdserver: upgrade the golang.org/x/crypto dependency #13669

Conversation

maxsokolovsky
Copy link

To rectify the vulnerability found in a version of golang.org/x/crypto
(https://avd.aquasec.com/nvd/cve-2020-29652), upgrade the dependency to
its latest version.
Alternatively, v0.0.0-20201216223049-8b5274cf687f could be used,
where the fixed was introduced, but the latest is preferable.

@ahrtr
Copy link
Member

ahrtr commented Feb 6, 2022

There are multiple go.mod files, so can you update all of them?

To rectify the vulnerability found in a version of golang.org/x/crypto
(https://avd.aquasec.com/nvd/cve-2020-29652), upgrade the dependency to
its latest version.
Alternatively, version v0.0.0-20201216223049-8b5274cf687f could be used,
where the fixed was introduced, but the latest is preferable.
@maxsokolovsky maxsokolovsky force-pushed the upgrade-server-dependency-golang.org/x/crypto branch from c9d8832 to f4708ae Compare February 7, 2022 15:12
@maxsokolovsky
Copy link
Author

@ahrtr, updated the dependency version in server and tests, and also ran go mod tidy for the module and sub-modules.

@@ -341,8 +341,8 @@ golang.org/x/crypto v0.0.0-20190510104115-cbcb75029529/go.mod h1:yigFU9vqHzYiE8U
golang.org/x/crypto v0.0.0-20190605123033-f99c8df09eb5/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
Copy link
Member

Choose a reason for hiding this comment

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

Why didn't you update etcdctl/go.mod?

Copy link
Author

Choose a reason for hiding this comment

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

This is for the release-3.5 branch, the dependency is not listed in etcdctl/go.mod.

Copy link
Member

Choose a reason for hiding this comment

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

Got it, thanks for the clarification.

I see some go.sum files also contain old crypto versions, such as api/go.sum#L57. Can you fix them?

Copy link
Author

Choose a reason for hiding this comment

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

I had run go mod tidy in the api module, but this produced no changes at all. If I remove the references to the crypto dependency manually from go.sum, then running go mod tidy brings them back in.

Copy link
Member

Choose a reason for hiding this comment

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

They might be dependencies' dependency.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I've looked at the api module dependency graph, looks like no top-level dependency bump will cause those transient dependencies to be upgraded the way we want. Some are already at their latest versions, like protobuf.

Copy link
Author

Choose a reason for hiding this comment

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

@ahrtr, is there still something that needs to be done on my part?

Copy link
Member

Choose a reason for hiding this comment

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

Ideally it would be better if you can fix all the old crypto versions, especially which are only in go.sum files. It may need some effort to figure out what's the exact dependencies which causes the golang.org/x/crypto being added into go.sum files.

It's OK to do it in a separate PR(s) or raise a ticket to track it.

Copy link
Contributor

@ptabor ptabor Feb 21, 2022

Choose a reason for hiding this comment

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

$ cd etcdctl
$go get golang.org/x/crypto@v0.0.0-20220131195533-30dcbda58838

Forces the dependency to be upgraded, even if its a transitive dependency.

go.mod gets modified by:

golang.org/x/crypto v0.0.0-20220131195533-30dcbda58838 // indirect

and go.sum as well.

go mod tidy is not removing that change.

etcdutl/go.sum Show resolved Hide resolved
go.sum Show resolved Hide resolved
@ptabor
Copy link
Contributor

ptabor commented Feb 21, 2022

Hi Max. Thank you for the fix.
Usually we first commit a change to main and later backport it to the release branches. Would that work here ?

@maxsokolovsky
Copy link
Author

Hi Piotr, thanks to you and Benjamin for reviewing! Sure, that would work!

@serathius serathius mentioned this pull request Apr 6, 2022
28 tasks
@serathius
Copy link
Member

Would like to have the fix in v3.5.3, as the author is not responding will handle the change on main.

@dims
Copy link
Contributor

dims commented Apr 8, 2022

@serathius i can do this :) let me file a PR against master/main!

@serathius
Copy link
Member

Sorry @dims, I have already pushed PR #13910

@dims
Copy link
Contributor

dims commented Apr 8, 2022

no worries @serathius i was joking with @ahrtr that this was probably the only thing i could help with :) glad you all are working hard to set the ship right. thanks for doing that!

@serathius serathius merged commit 383eceb into etcd-io:release-3.5 Apr 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants