Skip to content

Disable proxy to loopback and linklocal #71980

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
k8s-ci-robot merged 1 commit into from
Dec 12, 2018
Merged

Disable proxy to loopback and linklocal #71980

k8s-ci-robot merged 1 commit into from
Dec 12, 2018
+128 −0

Conversation

@micahhausler
Copy link
Member

@micahhausler micahhausler commented Dec 12, 2018

What type of PR is this?

/kind bug

What this PR does / why we need it:
Disable proxy to loopback and linklocal

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Disable proxy to loopback and linklocal

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. 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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 12, 2018
@k8s-ci-robot k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 12, 2018
This was referenced Dec 12, 2018
Merged
Merged
Merged
Merged
@lavalamp
Copy link
Contributor

lavalamp commented Dec 12, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 12, 2018
@micahhausler
Copy link
Member Author

micahhausler commented Dec 12, 2018

/test pull-kubernetes-e2e-kops-aws

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Dec 12, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, micahhausler

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

The pull request process is described here

Details 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 Dec 12, 2018
@micahhausler
Copy link
Member Author

micahhausler commented Dec 12, 2018

/test pull-kubernetes-e2e-kops-aws

krmayankk
krmayankk reviewed Dec 12, 2018
View reviewed changes
pkg/proxy/util/utils_test.go
{"127.0.0.2", ErrAddressNotAllowed},
{"169.254.169.254", ErrAddressNotAllowed},
{"169.254.1.1", ErrAddressNotAllowed},
{"224.0.0.0", ErrAddressNotAllowed},
Copy link

@krmayankk krmayankk Dec 12, 2018

Choose a reason for hiding this comment

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

it would be helpful to say in the comment why the above CIDR's are not allowed ?

krmayankk
krmayankk reviewed Dec 12, 2018
View reviewed changes
pkg/registry/core/pod/strategy.go

if err := proxyutil.IsProxyableIP(pod.Status.PodIP); err != nil {
return nil, nil, errors.NewBadRequest(err.Error())
}
Copy link

@krmayankk krmayankk Dec 12, 2018

Choose a reason for hiding this comment

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

how can pod.Status.PodIP be not proxyable ? If so , isnt the fix required in CNI or whatever component assigns the Pod IP @kubernetes/sig-network-api-reviews

@k8s-ci-robot k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Dec 12, 2018
@micahhausler
Copy link
Member Author

micahhausler commented Dec 12, 2018

/test pull-kubernetes-e2e-kops-aws

@k8s-ci-robot k8s-ci-robot merged commit ab1c9a4 into kubernetes:master Dec 12, 2018
k8s-ci-robot added a commit that referenced this pull request Dec 12, 2018
@k8s-ci-robot
Merge pull request #71981 from micahhausler/automated-cherry-pick-of-…
1c51137 
…#71980-upstream-release-1.13

Automated cherry pick of #71980: Disable proxy to loopback and linklocal
k8s-ci-robot added a commit that referenced this pull request Dec 12, 2018
@k8s-ci-robot
Merge pull request #71982 from micahhausler/automated-cherry-pick-of-…
67e3f78 
…#71980-upstream-release-1.12

Automated cherry pick of #71980: Disable proxy to loopback and linklocal
k8s-ci-robot added a commit that referenced this pull request Dec 12, 2018
@k8s-ci-robot
Merge pull request #71984 from micahhausler/automated-cherry-pick-of-…
54a061a 
…#71980-upstream-release-1.10

Automated cherry pick of #71980: Disable proxy to loopback and linklocal
k8s-ci-robot added a commit that referenced this pull request Dec 12, 2018
@k8s-ci-robot
Merge pull request #71983 from micahhausler/automated-cherry-pick-of-…
1992b4c 
…#71980-upstream-release-1.11

Automated cherry pick of #71980: Disable proxy to loopback and linklocal
@liggitt liggitt mentioned this pull request Jan 2, 2019
Merged
@micahhausler micahhausler deleted the proxy-patch branch January 4, 2019 17:09
@bjhaid
Copy link
Contributor

bjhaid commented Jan 5, 2019 •
edited
Loading

is there any genuine reason why the api-server should be proxying to addresses not belonging to the pod cidr or service cidr?

Should this just reject addresses not in the pod cidr or service-ip cidr?

EDIT:

I figured aggregated apis and friends, regardless I believe the apiserver should build up a whitelist that includes the domain names of the aggregated apis, pod-ips and service-ips and use that to make decision and not blacklist a few ip cidrs

@micahhausler
Copy link
Member Author

micahhausler commented Jan 8, 2019

@bjhaid Nodes can also be proxied, and their IPs are not know to the API server, except what is self-reported

@bjhaid
Copy link
Contributor

bjhaid commented Jan 8, 2019

their IPs are not know to the API server

Nodes send their ip address when they register to the api-server, so this can make to the whitelist if there's an internal whitelist

I am curious what is the use case for proxying nodes?

@micahhausler
Copy link
Member Author

micahhausler commented Jan 8, 2019

Nodes send their ip address when they register to the api-server, so this can make to the whitelist if there's an internal whitelist

What if I register a fake node with a malicious IP address? What about nodes that are in legitimate but
various different networks? What about nodes with public IPs? There are too many topologies and use cases to infer any kind of trusted whitelist. Personally, I think there could be room for a new apiserver flag with a whitelist of trusted CIDR ranges for proxying.

I am curious what is the use case for proxying nodes?

I believe node proxy is mostly for debugging (See list of kubelet endpoints here). In addition to proxy, the API server reaches out to nodes for all kinds of operations (pod proxy, logs, exec, etc).

@simonswine simonswine mentioned this pull request Jan 8, 2019
Open
@bjhaid
Copy link
Contributor

bjhaid commented Jan 9, 2019

I think there could be room for a new apiserver flag with a whitelist of trusted CIDR ranges for proxying.

I'll love to see this happen, is there some related issue tracking this?

I believe node proxy is mostly for debugging

Thanks!

@antoninbas antoninbas mentioned this pull request Feb 15, 2024
Merged
@aojea aojea mentioned this pull request Oct 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@caesarxuchao caesarxuchao Awaiting requested review from caesarxuchao

@dcbw dcbw Awaiting requested review from dcbw

1 more reviewer

@krmayankk krmayankk krmayankk left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@lavalamp lavalamp

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/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API 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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/network Categorizes an issue or PR as relevant to SIG Network. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@micahhausler @lavalamp @k8s-ci-robot @bjhaid @krmayankk