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 otel and other dependencies for CVE-2023-45142 #121338

Merged
merged 1 commit into from
Oct 26, 2023
Merged

Bump otel and other dependencies for CVE-2023-45142 #121338

merged 1 commit into from
Oct 26, 2023

Conversation

dims
Copy link
Member

@dims dims commented Oct 18, 2023 •
edited

So we start off here from the advisory:
GHSA-rcjv-mgp8-qvmr

This looks like the PR where it was fixed:
open-telemetry/opentelemetry-go-contrib#4277

Good news is the files/methods in question in the PR are NOT in our vendor/ directory. Though the advisory claims affected program has to use otelhttp.NewHandler wrapper (which we do have reference of! here) we do not have/use otelhttp metrics related code. So we are not affected AFAICT.

Here's the output from govulncheck:

$ govulncheck -C . ./...
Scanning your code and 1976 packages across 204 dependent modules for known vulnerabilities...

=== Informational ===

Found 1 vulnerability in packages that you import, but there are no call
stacks leading to the use of this vulnerability. You may not need to
take any action. See https://pkg.go.dev/golang.org/x/vuln/cmd/govulncheck
for details.

Vulnerability #1: GO-2023-2113
    Memory exhaustion in github.com/open-telemetry/opentelemetry-go-contrib
  More info: https://pkg.go.dev/vuln/GO-2023-2113
  Module: go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp
    Found in: go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.42.0
    Fixed in: go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.44.0

No vulnerabilities found.

Share feedback at https://go.dev/s/govulncheck-feedback.

So the ONLY reason to land this would be to silence scanners going forward (yuck!). We definitely should NOT be backporting this to stable branches.

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

NOTE: THIS set of dependency updates is NOT eligible for cherry-picks, so please don't propose one.

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 18, 2023
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Oct 18, 2023
@dims dims changed the title [WIP] working-config-otel [WIP] Bump otel Oct 18, 2023
@k8s-ci-robot k8s-ci-robot added area/apiserver area/cloudprovider area/code-generation area/dependency Issues or PRs related to dependency changes area/kube-proxy area/kubectl area/kubelet sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 18, 2023
@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Oct 25, 2023
@dashpole
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 35aa7b870861e579e81eafbae6cecc86247024e7

@liggitt
Copy link
Member

liggitt commented Oct 25, 2023

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, dims, liggitt

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@pacoxu
Copy link
Member

pacoxu commented Oct 26, 2023

/retest

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Oct 26, 2023 •
edited

@dims: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubernetes-e2e-kind-kms 702d911 link false /test pull-kubernetes-e2e-kind-kms

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

pacoxu
pacoxu reviewed Oct 26, 2023
View reviewed changes
staging/src/k8s.io/kms/internal/plugins/_mock/go.mod
@@ -1,27 +0,0 @@
module k8s.io/kms/plugins/mock

Copy link
Member

Choose a reason for hiding this comment

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

Why this is removed?

pull-kubernetes-e2e-kind-kms failed for this package?

https://prow.k8s.io/pr-history/?org=kubernetes&repo=kubernetes&pr=121338

Copy link
Member

@liggitt liggitt Oct 26, 2023 •
edited

Choose a reason for hiding this comment

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

oof... sorry I missed that

@dims mind opening a PR to add them back?

@k8s-ci-robot k8s-ci-robot merged commit d008435 into kubernetes:master Oct 26, 2023
18 of 19 checks passed
SIG Node PR Triage automation moved this from WIP to Done Oct 26, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.29 milestone Oct 26, 2023
@liggitt liggitt mentioned this pull request Oct 26, 2023
Merged
@dims dims mentioned this pull request Oct 26, 2023
Merged
@leilajal
Copy link
Contributor

/remove-sig api-machinery

@k8s-ci-robot k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Oct 26, 2023
@MartinForReal MartinForReal mentioned this pull request Oct 27, 2023
Closed
@feiskyer feiskyer mentioned this pull request Oct 30, 2023
Merged
@dims dims mentioned this pull request Oct 31, 2023
Closed
@dashpole dashpole mentioned this pull request Nov 10, 2023
Closed
@antoninbas antoninbas mentioned this pull request Nov 14, 2023
Merged
@kiashok kiashok mentioned this pull request Nov 29, 2023
Merged
@jerryhe1999
Copy link

Do we have any plan addressing this CVE to other release versions v1.26-v1.28 as well?

@liggitt
Copy link
Member

liggitt commented Dec 18, 2023 •
edited

Do we have any plan addressing this CVE to other release versions v1.26-v1.28 as well?

my understanding is that the CVE is in code we don't link or build, so does not impact Kubernetes at all, and the CVE is a false positive (for Kubernetes) reported by scanners. From the description of this PR:

Good news is the files/methods in question in the PR are NOT in our vendor/ directory. Though the advisory claims affected program has to use otelhttp.NewHandler wrapper (which we do have reference of! here) we do not have/use otelhttp metrics related code. So we are not affected AFAICT.
...
So the ONLY reason to land this would be to silence scanners going forward (yuck!). We definitely should NOT be backporting this to stable branches.
...
NOTE: THIS set of dependency updates is NOT eligible for cherry-picks, so please don't propose one.

@jerryhe1999
Copy link

@liggitt Appreciate your responding. Yeah I was thinking we could ignore this CVE as well, but this PR get merged to v1.29 Release which made me considering merging this to other release versions.

@dims
Copy link
Member Author

dims commented Dec 19, 2023

@jerryhe1999 also addressed in the commit message

So the ONLY reason to land this would be to silence scanners going forward (yuck!). We definitely should NOT be backporting this to stable branches.

@liggitt
Copy link
Member

liggitt commented Dec 19, 2023

this PR get merged to v1.29 Release

it was merged to master before 1.29.0 was cut, which is the only reason it was automatically included in 1.29

This was referenced Mar 18, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@pacoxu pacoxu pacoxu left review comments

@andrewsykim andrewsykim Awaiting requested review from andrewsykim

@ardaguclu ardaguclu Awaiting requested review from ardaguclu

Assignees

@liggitt liggitt

@dashpole dashpole

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/cloudprovider area/code-generation area/dependency Issues or PRs related to dependency changes area/kube-proxy area/kubectl area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
SIG Auth
Archived in project
SIG CLI
Archived in project
SIG Node PR Triage
Archived in project
SIG Node PR Triage
Done
Milestone
v1.29
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@dims @k8s-ci-robot @ericsmalling @Aneurysm9 @dashpole @antoninbas @leilajal @liggitt @pacoxu @jerryhe1999