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.

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

kubelet's cAdvisor binds to all interfaces #11710

Closed
farcaller opened this Issue Jul 22, 2015 · 20 comments

Comments

Projects
None yet
9 participants
@farcaller
Copy link

farcaller commented Jul 22, 2015

cAdvisor provides potentially sensitive data and there's currently no way to block access with anything other than iptables.

@erictune

This comment has been minimized.

Copy link
Member

erictune commented Jul 22, 2015

What type of cluster are you running (GKE, or what instructions did you use to setup your cluster?)

A short term solution might be to try adding --cadvisor-port=0 flag to kubelet and restarting the kubelet service and see if the sensitive information is no longer leaked.

Doing this may the UI and heapster which expect to be able to poll the cadvisor port.

I think @dchen1107 probably knows if there is a longer term solution planned, such as:

  • only serve on specified interface. if this is localhost, then it breaks the ui, but maybe that is okay. maybe @lavalamp has thought about this.
  • require authentication/authorization to connect to cAdvisor port.

@vmarmol

@erictune erictune self-assigned this Jul 22, 2015

@farcaller

This comment has been minimized.

Copy link

farcaller commented Aug 6, 2015

Self deployed cluster, the kubelet is started via

[Unit]
Description=Kubelet
After=docker.service
Requires=docker.service

[Service]
TimeoutStartSec=0
TimeoutStopSec=10
ExecStartPre=/sbin/iptables -A INPUT -p tcp --dport 4194 -j REJECT
ExecStart=/usr/local/bin/kubelet \
--address={{ salt.netinfo.ip() }} \
--api-servers=https://10.200.0.1:6443 \
--auth-path=/etc/kubernetes/kubelet_auth.json \
--cluster-dns=10.100.1.1 \
--config={{ pillar['kubernetes']['config_dir'] }}/manifests \
--hostname-override={{ grains['host'] }} \
--tls-cert-file=/etc/kubernetes/kubelet-tls.crt \
--tls-private-key-file=/etc/kubernetes/kubelet-tls.key
ExecStopPost=/sbin/iptables -D INPUT -p tcp --dport 4194 -j REJECT

Restart=on-failure
RestartSec=5s

[Install]
WantedBy=multi-user.target
@vishh

This comment has been minimized.

Copy link
Member

vishh commented Aug 7, 2015

We are working on improving the kubelet APIs which will subsume cadvisor APIs. Until then setting cadvisor port to '0' is probably the way to go.

@crawford crawford referenced this issue Aug 8, 2015

Closed

add kubelet #418

@philips

This comment has been minimized.

Copy link
Contributor

philips commented Aug 8, 2015

@vishh Is there an issue and target date for doing this?

@vishh

This comment has been minimized.

Copy link
Member

vishh commented Aug 10, 2015

Filed #12483. The current plan is to complete this for v1.1. cc @dchen1107

@jimmidyson

This comment has been minimized.

Copy link
Member

jimmidyson commented Aug 13, 2015

@philips Do you mean the read-only kubelet port (10255 by default IIRC)? If so, then we really need to disable this & provide authorisation options on the kubelet to be able to retrieve stats fro the kubelet via the normal port with a restricted token/cert/auth options. We've discussed this for OpenShift (openshift/origin#3020) too.

@jimmidyson

This comment has been minimized.

Copy link
Member

jimmidyson commented Aug 13, 2015

@philips Sorry I didn't realise that cadvisor was bound to port 4194 as well. Thought this was all embedded inside the kubelet process without exposing any ports.

@jimmidyson

This comment has been minimized.

Copy link
Member

jimmidyson commented Aug 13, 2015

@vmarmol @vishh Any reason why we start up cadvisor http server at all? https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/cadvisor/cadvisor_linux.go#L99-L103 Seems unnecessary now kubernetes embeds cadvisor & exposes via /stats & /metrics?

@vishh

This comment has been minimized.

Copy link
Member

vishh commented Aug 13, 2015

/stats does not expose all the containers (/docker-daemon, /kubelet, etc). @mvdan is working on introducing a better stats API in kubelet (#12483) that should help us shut down cadvisor's http server in kubelet.

@jimmidyson

This comment has been minimized.

Copy link
Member

jimmidyson commented Aug 13, 2015

Couldn't we just add cadvisor as a handler in the kubelet anyway? No need for separate http server & would mean access to raw data is available if anyone wants it.

@vishh

This comment has been minimized.

Copy link
Member

vishh commented Aug 13, 2015

Indeed that is an option. But we need a better stats API in kubelet anyways for reasons mentioned in #12483.

@jimmidyson

This comment has been minimized.

Copy link
Member

jimmidyson commented Aug 13, 2015

Absolutely agree but thinking quick win.

@philips

This comment has been minimized.

Copy link
Contributor

philips commented Aug 14, 2015

+1 on a quick win. We just disabled it by default instead: https://github.com/coreos/coreos-overlay/blob/master/app-admin/kubelet/files/kubelet.service

Which reminds me it would be great to have #12418 so we could make it easy to turn on/off stuff like this w/o rewriting the entire command-line in a service file.

@vishh

This comment has been minimized.

Copy link
Member

vishh commented May 23, 2016

Bregor added a commit to evilmartians/chef-kubernetes that referenced this issue Oct 26, 2016

@antoineco

This comment has been minimized.

Copy link
Contributor

antoineco commented May 29, 2017

@vishh it's not 100% clear to me whether or not #12483 made it safe to use --cadvisor-port=0, could you please clarify? On a fresh k8s 1.6.4 cluster kubelet still exposes cAdvisor to the world:

State   Recv-Q  Send-Q  Local Address:Port  Peer Address:Port
LISTEN  0       128       :::4194              :::*

@philips:

@vishh

This comment has been minimized.

Copy link
Member

vishh commented May 30, 2017

Ahh. That's a bug that the new component config based defaulting scheme introduced.
@mtaufen CadvisorPort being 0 is a valid setting.

@mtaufen

This comment has been minimized.

Copy link
Contributor

mtaufen commented May 30, 2017

I think we can change this. The KubeletConfiguration type is still technically alpha, so it should be ok to make this field a pointer and keep the current default when it's not provided (but allow 0). This won't affect anyone who presently omits the flag and expects port 4194. Theoretically people could be explicitly passing 0 and expecting 4194... I guess... but that's sort of an insane case, and I'd consider that behavior a bug.

@antoineco

This comment has been minimized.

Copy link
Contributor

antoineco commented May 30, 2017

Thanks for clarifying guys 👍
The original problem remains though: enabling the cAdvisor port makes the kubelet listen on all interfaces.

@vishh

This comment has been minimized.

Copy link
Member

vishh commented May 30, 2017

k8s-merge-robot added a commit that referenced this issue Jun 6, 2017

Merge pull request #46876 from mtaufen/fix-cadvisorport
Automatic merge from submit-queue (batch tested with PRs 46787, 46876, 46621, 46907, 46819)

Fix cAdvisorPort, 0 is a valid option

wrt #11710, this maintains the current default if nobody provides the flag, but allows explicitly passing 0.

/cc @farcaller @vishh @liggitt @antoineco @philips 
/assign @liggitt @vishh 

```release-note
Fixes a bug with cAdvisorPort in the KubeletConfiguration that prevented setting it to 0, which is in fact a valid option, as noted in issue #11710.
```

dims added a commit to dims/kubernetes that referenced this issue Jun 8, 2017

Run cAdvisor on the same interface as kubelet
cAdvisor currently binds to all interfaces. Currently the only
solution is to use iptables to block access to the port. We
are better off making cAdvisor to bind to the interface that
kubelet uses for better security.

Fixes kubernetes#11710

dims added a commit to dims/kubernetes that referenced this issue Jun 8, 2017

Run cAdvisor on the same interface as kubelet
cAdvisor currently binds to all interfaces. Currently the only
solution is to use iptables to block access to the port. We
are better off making cAdvisor to bind to the interface that
kubelet uses for better security.

Fixes kubernetes#11710
@dims

This comment has been minimized.

Copy link
Member

dims commented Jun 9, 2017

/assign

k8s-merge-robot added a commit that referenced this issue Jun 23, 2017

Merge pull request #47195 from dims/bind-cadvisor-on-kubelet-interface
Automatic merge from submit-queue (batch tested with PRs 47922, 47195, 47241, 47095, 47401)

Run cAdvisor on the same interface as kubelet

**What this PR does / why we need it**:

cAdvisor currently binds to all interfaces. Currently the only
solution is to use iptables to block access to the port. We
are better off making cAdvisor to bind to the interface that
kubelet uses for better security.

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #

Fixes #11710

**Special notes for your reviewer**:

**Release note**:

```release-note
cAdvisor binds only to the interface that kubelet is running on instead of all interfaces.
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment