[WIP] KEP: make RESTClient not to follow redirects by default #1908
Conversation
Signed-off-by: knight42 <anonymousknight96@gmail.com>
k8s-ci-robot
added
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: knight42 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
kind/kep
knight42
changed the title
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
@knight42: The following test failed, say
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. |
| --> | ||
| Generally the RESTClient does not need to follow any redirects, but there might be some cases where it is required to follow redirects. | ||
|
|
||
| 1. If it is required to follow redirects on the client side, i.e. kubectl, we could add a global flag to kubectl, say `--follow-redirect`, to mitigate breakage. |
There was a problem hiding this comment.
Would the linked example of the type of vulnerability this is trying to prevent have been mitigated by preventing redirects in the apiserver outgoing request, but continuing to follow them in the kubectl client?
We could also expose this as an option in the kubeconfig rather than providing it as a flag.
| // other unchanged fields | ||
|
|
||
| // FollowRedirect makes the internal HTTP client follow redirects automatically | ||
| FollowRedirect bool |
There was a problem hiding this comment.
Could this be a list of regular expressions of URI's it will follow. For example the client may expect redirects for a set of hosts, but not arbitrary redirects to the public internet.
| with and without the feature, are necessary. At the very least, think about | ||
| conversion tests if API types are being modified. | ||
|
|
||
| ### Rollout, Upgrade and Rollback Planning |
There was a problem hiding this comment.
We could rollout this out as "deprecating redirect following" -- e.g. start with logging warnings to stderr in cases that would begin to fail in a future release, and make it opt-in. Then in a subsequent release make it opt-out. This way folks will have some indication that things will start to fail for them and be able to take action ahead of time.
| - [CVE-2020-8559](https://github.com/kubernetes/kubernetes/issues/92914) | ||
| - ...(there may be other unrevealed CVEs) | ||
|
|
||
| Generally speaking, the RESTClient should work without following any HTTP redirects and we would have an immediate mitigation of the above CVEs if the RESTClient do not follow redirects by default. |
There was a problem hiding this comment.
Have we seen examples that are not apiserver -> kubelet requests? We could start by disabling redirects for requests made by the apiserver rather than for all clients, or disabling redirects for requests made to the kubelet endpoints that are result in exploits.
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Send feedback to sig-contributor-experience at kubernetes/community. |
|
@fejta-bot: Closed this PR. In response to this:
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. |
Signed-off-by: knight42 anonymousknight96@gmail.com
Motivation: kubernetes/kubernetes#93129
Fixes: #1906
Rendered
/cc @liggitt