Use golang.org/x/[email protected] to mitigate CVE-2022-32149

[knopt]

Signed-off-by: Tomasz Knopik [email protected]

What type of PR is this?
/kind bug

What this PR does / why we need it:
Fixes CVE-2022-32149 which is high severity vulnerability discovered in node-driver-registrar dependencies.

Which issue(s) this PR fixes:

Fixes #249

Special notes for your reviewer:

$  cat go.sum | grep golang.org/x/text
golang.org/x/text v0.4.0 h1:BrVqGRd7+k1DiOgtnFvAkoQEWQvBc25ouMJM6429SFg=
golang.org/x/text v0.4.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8=

Does this PR introduce a user-facing change?:

NONE

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: knopt
Once this PR has been reviewed and has the lgtm label, please assign jsafrane for approval by writing /assign @jsafrane in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

Welcome @knopt!

It looks like this is your first PR to kubernetes-csi/node-driver-registrar 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-csi/node-driver-registrar has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @knopt. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[knopt]

/cc @mauriciopoppe

[mauriciopoppe]

/ok-to-test
/lgtm
/assign @msau42

[msau42]

Why do we need the replace?

[msau42]

I see, it's a transitive dependency that has that requirement. I guess the potential concern is that version combination being untested.

[mauriciopoppe]

There's also an analysis that Jan did in https://kubernetes.slack.com/archives/C8EJ01Z46/p1669292513960349

[knopt]

To address the compatibility issues, we could change replace clause from v0.4.0 to v0.3.8. However, according to semantic versioning, which is followed by golang, upgrade of MINOR is backward compatible.

The analysis seems to be correct, but upgrading seems like a more trustworthy approach.

[msau42]

We discussed this at today's csi standup. It looks like go.sum lists all the possible versions that could be used, but that doesn't necessarily mean that the version is being used. So cve scanners that rely on go.sum may produce a lot of false positives.

https://stackoverflow.com/questions/70128040/content-of-go-sum-and-modules-really-used-by-a-go-application
https://stackoverflow.com/questions/69825533/why-does-go-sum-include-so-many-older-packages

Given that this change removed the entries from go.sum, but didn't actually change anything in the vendor directory (which reflects what's actually being used) confirms this.

What scanner are you using? It looks like trivy no longer relies on go.sum when the go version in go.mod is >= 1.17: aquasecurity/trivy#1973. But dependabot still does not: dependabot/dependabot-core#4740

[knopt]

I used a commercial security analysis tool to detect vulns. It is a good point that presence of a dependency in go.sum does not necessarily mean that it's actually used. However, the affected version (v0.3.7) is explicitly listed in go.mod of node-driver-registrar (indirect dependency but still a dependency) (it's not go.sum issue that was mentioned).

I agree that it looks like the affected part of the code is not used (even indirectly) by node-driver-registrar. However, afaik security scanners standards are not designed to go into analysis on a function level, they stop at reporting affected package version. Yet, they are still useful tool.

I believe that there is a choice to be made here by maintainers. Every time a new vulnerability is discovered either affected dependency should be updated regardless if the node-driver-registrar is using the affected, or do the vulnerability security audit every time and somehow maintain list of such vulnerabilities which are believed to be not affecting the repo and make all users aware that their scans might produce higher-than-usual number of false-positives.

[msau42]

In the go.mod, it looks like only 0.4.0 is listed: https://github.com/kubernetes-csi/node-driver-registrar/pull/251/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R42

Anyway, I think we should go ahead and merge this because it is a burden to have users manage exceptions in their scanners, but we should also notify the scanning tools of false-positives so that they can be improved.

[knopt]

Yeah, you are actually right, I was looking at 2.6.1 and 2.6.2 tags. 0.4.0 is not on master b/c of #243 which I've just noticed.

The above makes this PR redundant, thank you for the discussion though! I think it's important to recognize how vulns should be addressed.

[xing-yang]

@knopt Have you scanned the image after this fix? Do you see the CVE's gone after this?

[knopt]

@xing-yang Yes, I've run the scan against my branch and the vulnerability is no longer present.