Skip to content

Don't return proxied redirects to the client #92941

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
Merged
merged 1 commit into from
Jul 12, 2020
Merged

Don't return proxied redirects to the client #92941

merged 1 commit into from
Jul 12, 2020

Conversation

tallclair
Copy link
Member

@tallclair tallclair commented Jul 9, 2020 •
edited by saschagrunert
Loading

What type of PR is this?
/kind bug

What this PR does / why we need it:

Treat all non-101 (switching protocol) responses to upgrade requests as errors. If the server responds with a 200 but the client is expecting an upgrade, the behavior can be unpredictable and lead to a stuck client.

Does this PR introduce a user-facing change?:

CVE-2020-8559 (Medium): Privilege escalation from compromised node to cluster. See https://github.com/kubernetes/kubernetes/issues/92914 for more details.
Action Required: The API Server will no longer proxy non-101 responses for upgrade requests. This could break proxied backends (such as an extension API server) that respond to upgrade requests with a non-101 response code.

/sig api-machinery
/milestone v1.19
/priority important-soon
/assign @liggitt @deads2k

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 9, 2020
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Jul 9, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Jul 9, 2020
@k8s-ci-robot
Copy link
Contributor

@tallclair: The label(s) sig/apimachinery cannot be applied, because the repository doesn't have them

In response to this:

What type of PR is this?
/kind bug

What this PR does / why we need it:

Treat all non-101 (switching protocol) responses to upgrade requests as errors. If the server responds with a 200 but the client is expecting an upgrade, the behavior can be unpredictable and lead to a stuck client.

Does this PR introduce a user-facing change?:

The apiserver will no longer proxy non-101 responses for upgrade requests. This could break proxied backends (such as an extension API server) that respond to upgrade requests with a non-101 response code.

/sig apimachinery
/milestone v1.19
/priority important-soon
/assign @liggitt @deads2k

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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 9, 2020
@tallclair tallclair added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jul 9, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Jul 9, 2020
@justaugustus
Copy link
Member

/retest

deads2k
deads2k reviewed Jul 9, 2020
View reviewed changes
staging/src/k8s.io/apimachinery/pkg/util/net/http.go
@@ -453,7 +453,7 @@ redirectLoop:

// Only follow redirects to the same host. Otherwise, propagate the redirect response back.
if requireSameHostRedirects && location.Hostname() != originalLocation.Hostname() {
break redirectLoop
return nil, nil, fmt.Errorf("hostname mismatch: expected %s, found %s", originalLocation.Hostname(), location.Hostname())
Copy link
Contributor

Choose a reason for hiding this comment

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

This change doesn't immediately seem related to the release note, but seems ok.

@deads2k
Copy link
Contributor

deads2k commented Jul 9, 2020

that respond to upgrade requests with a non-101 response code.

Based on my read, error responses (greater than 400) are still allowed. I like this, it does what I need, but I want to make sure it is intentional on your part.

/lgtm
/approve
/hold

holding in case you want to do something different with errors. if you're ok passing them back as failures (I like that), feel free to release the hold.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jul 9, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, tallclair

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 9, 2020
@tallclair
Copy link
Member Author

@deads2k yep, that's intentional, for the same reasons you highlighted. Thanks!

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 9, 2020
@justaugustus
Copy link
Member

/retest

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

This was referenced Jul 10, 2020
Merged
Merged
Merged
@k8s-ci-robot k8s-ci-robot merged commit dcf60e0 into kubernetes:master Jul 12, 2020
k8s-ci-robot added a commit that referenced this pull request Jul 12, 2020
@k8s-ci-robot
Merge pull request #92971 from tallclair/automated-cherry-pick-of-#92…
dda9914 
…941-upstream-release-1.16

Automated cherry pick of #92941: Don't return proxied redirects to the client
k8s-ci-robot added a commit that referenced this pull request Jul 12, 2020
@k8s-ci-robot
Merge pull request #92970 from tallclair/automated-cherry-pick-of-#92…
7555efd 
…941-upstream-release-1.17

Automated cherry pick of #92941: Don't return proxied redirects to the client
k8s-ci-robot added a commit that referenced this pull request Jul 12, 2020
@k8s-ci-robot
Merge pull request #92969 from tallclair/automated-cherry-pick-of-#92…
a9f7208 
…941-upstream-release-1.18

Automated cherry pick of #92941: Don't return proxied redirects to the client
@github-actions github-actions bot mentioned this pull request Jul 14, 2020
Open
@k8s-ci-robot k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 15, 2020
@tallclair tallclair mentioned this pull request Jul 15, 2020
Closed
@knight42 knight42 mentioned this pull request Jul 23, 2020
Closed
@tallclair tallclair mentioned this pull request Jul 24, 2020
Closed
@liggitt liggitt mentioned this pull request Sep 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k deads2k left review comments

@logicalhan logicalhan Awaiting requested review from logicalhan

@wojtek-t wojtek-t Awaiting requested review from wojtek-t

Assignees

@liggitt liggitt

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.
6 participants
@tallclair @k8s-ci-robot @justaugustus @deads2k @fejta-bot @liggitt