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

Replace satori/go.uuid with gofrs/uuid #16952

Closed
wants to merge 1 commit into from

Conversation

vincentni
Copy link
Contributor

@vincentni vincentni commented Jun 7, 2022

Thank you for contributing to Harbor!

Comprehensive Summary of your change

Upgraded azure-sdk-for-go to a version that no longer uses satori/go.uuid, which is no longer maintained and has an unresolved CVE (https://nvd.nist.gov/vuln/detail/CVE-2021-3538).

Issue being fixed

Fixes #(issue)
Replaced satori/go.uuid with gofrs/uuid.

Please indicate you've done the following:

  • Well Written Title and Summary of the PR
  • Label the PR as needed. "release-note/ignore-for-release, release-note/new-feature, release-note/update, release-note/enhancement, release-note/community, release-note/breaking-change, release-note/docs, release-note/infra, release-note/deprecation"
  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Made sure tests are passing and test coverage is added if needed.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.

@vincentni vincentni requested a review from a team as a code owner June 7, 2022 02:48
@vincentni
Copy link
Contributor Author

@YangJiao0817 Could you please take a look at this PR as it tries to resolve a critical CVE (https://nvd.nist.gov/vuln/detail/CVE-2021-3538)? Thank you!

@codecov
Copy link

codecov bot commented Jun 8, 2022

Codecov Report

Merging #16952 (0536bde) into main (0dc7a68) will increase coverage by 0.04%.
The diff coverage is n/a.

❗ Current head 0536bde differs from pull request most recent head 3145662. Consider uploading reports for the commit 3145662 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16952      +/-   ##
==========================================
+ Coverage   67.27%   67.31%   +0.04%     
==========================================
  Files         970      970              
  Lines       81288    81288              
  Branches     2550     2550              
==========================================
+ Hits        54685    54718      +33     
+ Misses      22898    22863      -35     
- Partials     3705     3707       +2     
Flag Coverage Δ
unittests 67.31% <ø> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/lib/cache/util.go 73.68% <0.00%> (-15.79%) ⬇️
src/common/rbac/system/namespace.go 33.33% <0.00%> (-11.12%) ⬇️
src/common/utils/passports.go 84.61% <0.00%> (-5.13%) ⬇️
src/portal/src/app/shared/units/shared.utils.ts 36.47% <0.00%> (+2.35%) ⬆️
...es/vulnerability/vulnerability-config.component.ts 58.51% <0.00%> (+4.44%) ⬆️
src/controller/event/topic.go 9.00% <0.00%> (+7.20%) ⬆️
...-nav/gc-page/gc/gc-history/gc-history.component.ts 61.11% <0.00%> (+9.25%) ⬆️
src/core/api/internal.go 53.33% <0.00%> (+13.33%) ⬆️
src/controller/event/handler/auditlog/auditlog.go 60.86% <0.00%> (+52.17%) ⬆️

@vincentni
Copy link
Contributor Author

@YangJiao0817 It fails in release note label check as no label is currently associated with this PR. Could you please help add an appropriate release note to this PR as it seems I don't have the required permission? Thank you!

@vincentni
Copy link
Contributor Author

@YangJiao0817 @wy65701436 If it looks good to you, could you please give a thumbsup and merge it? Thank you!

@YangJiao0817
Copy link
Member

YangJiao0817 commented Jun 8, 2022

@vincentni Please rebase your code squash the two commits into one, Thank you!

@vincentni
Copy link
Contributor Author

@YangJiao0817 Done. Could you help re-trigger all checks again? Thank you!

@vincentni
Copy link
Contributor Author

@YangJiao0817 @wy65701436 How does this PR look like now?

@MinerYang
Copy link
Contributor

MinerYang commented Jun 17, 2022

Hi @vincentni ,
We appreciate your contribution but we still have some questions need to discuss.
We are not using github.com/satori/go.uuid directly, and it seems like all of these following dependencies also reference this module github.com/satori/go.uuid which causes the CVE.
For most of our practice, if we need to fix this , we have to bumpup all of these modules to the version which already fixed this CVE .
So I am wondering what's your process to achieve this commit?

  1. if I conduct go get github.com/gofrs/uuid and remove github.com/satori/go.uuid manually, thengo mod tidy/ go mod vendor, it wouldn't delete the github.com/satori/go.uuid(this would be add back).
  2. I assume you bump up the github.com/Azure/azure-sdk-for-go manaully to the latest version, and running go mod vendor, I also can not get the same result as you commited in this PR, especially it wouldn't change the github.com/distribution/distribution checksum in go.sum file.
ubuntu@miner:~/goharbor/harbor/src$ go mod graph | grep github.com/satori/go.uuid
github.com/goharbor/harbor/src github.com/satori/[email protected]
github.com/aliyun/[email protected] github.com/satori/[email protected]
github.com/containerd/[email protected] github.com/satori/[email protected]
github.com/containerd/[email protected] github.com/satori/[email protected]
github.com/distribution/distribution/[email protected] github.com/satori/[email protected]
github.com/containerd/[email protected] github.com/satori/[email protected]
github.com/jackc/[email protected] github.com/satori/[email protected]
github.com/containerd/[email protected] github.com/satori/[email protected]
github.com/jackc/pgx/[email protected] github.com/satori/[email protected]
github.com/jackc/[email protected] github.com/satori/[email protected]
github.com/jackc/pgx/[email protected] github.com/satori/[email protected]
github.com/jackc/[email protected] github.com/satori/[email protected]
github.com/jackc/pgx/[email protected] github.com/satori/[email protected]

So I am wondering how you replace it?

Note
golang version: v1.18.3

@YangJiao0817
Copy link
Member

Hi @vincentni ,
According to https://nvd.nist.gov/vuln/detail/CVE-2021-3538, the v1.2.0 used by Harbor is not in the 0ef6afb2f6cdd6cdaeee3885a95099c63f18fc8c to d91630c8510268e75203009fe7daf2b8e1d60c45 interval, why does this CVE affect Harbor?

uuid-log

@vincentni
Copy link
Contributor Author

vincentni commented Jun 20, 2022

Hi @vincentni , We appreciate your contribution but we still have some questions need to discuss. We are not using github.com/satori/go.uuid directly, and it seems like all of these following dependencies also reference this module github.com/satori/go.uuid which causes the CVE. For most of our practice, if we need to fix this , we have to bumpup all of these modules to the version which already fixed this CVE . So I am wondering what's your process to achieve this commit?

  1. if I conduct go get github.com/gofrs/uuid and remove github.com/satori/go.uuid manually, thengo mod tidy/ go mod vendor, it wouldn't delete the github.com/satori/go.uuid(this would be add back).
  2. I assume you bump up the github.com/Azure/azure-sdk-for-go manaully to the latest version, and running go mod vendor, I also can not get the same result as you commited in this PR, especially it wouldn't change the github.com/distribution/distribution checksum in go.sum file.
ubuntu@miner:~/goharbor/harbor/src$ go mod graph | grep github.com/satori/go.uuid
github.com/goharbor/harbor/src github.com/satori/[email protected]
github.com/aliyun/[email protected] github.com/satori/[email protected]
github.com/containerd/[email protected] github.com/satori/[email protected]
github.com/containerd/[email protected] github.com/satori/[email protected]
github.com/distribution/distribution/[email protected] github.com/satori/[email protected]
github.com/containerd/[email protected] github.com/satori/[email protected]
github.com/jackc/[email protected] github.com/satori/[email protected]
github.com/containerd/[email protected] github.com/satori/[email protected]
github.com/jackc/pgx/[email protected] github.com/satori/[email protected]
github.com/jackc/[email protected] github.com/satori/[email protected]
github.com/jackc/pgx/[email protected] github.com/satori/[email protected]
github.com/jackc/[email protected] github.com/satori/[email protected]
github.com/jackc/pgx/[email protected] github.com/satori/[email protected]

So I am wondering how you replace it?

Note golang version: v1.18.3

I simply manually updated github.com/Azure/azure-sdk-for-go version in both go.mod and vendor/modules.txt, and then ran go mod tidy followed by go mod vendor.
Yeah, I missed out a couple of other packages that reference github.com/satori/go.uuid as well.

@vincentni
Copy link
Contributor Author

Hi @vincentni , According to https://nvd.nist.gov/vuln/detail/CVE-2021-3538, the v1.2.0 used by Harbor is not in the 0ef6afb2f6cdd6cdaeee3885a95099c63f18fc8c to d91630c8510268e75203009fe7daf2b8e1d60c45 interval, why does this CVE affect Harbor?

uuid-log

Interesting! You are right that 1.2.0 version doesn't contain commits from the affected range. Somehow our image scanner reported that vulnerability and there is an open issue (satori/go.uuid#115) for that.
But regardless of that, I think it is much better to switch to maintained github.com/gofrs/uuid as suggested there satori/go.uuid#73 (comment).

@MinerYang
Copy link
Contributor

MinerYang commented Jul 6, 2022

Hi @vincentni ,

Appreciate your suggestion and contribution !
We are aware of there are some risks to keep github.com/satori/go.uuid in our mod, and will replace all the reference using this package with
well maintained github.com/gofrs/uuid in the future if necessary.
Since this would not impact harbor according to the above discussion for the moment, we would like to close this pr for now. If you have any concerns or suggestions, let's keep in touch!

Best,
Miner

@MinerYang MinerYang closed this Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants