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

Disable the kubelet readonly port #732

Closed
luxas opened this issue Mar 16, 2018 · 17 comments · Fixed by kubernetes/kubernetes#64187
Closed

Disable the kubelet readonly port #732

luxas opened this issue Mar 16, 2018 · 17 comments · Fixed by kubernetes/kubernetes#64187
Assignees
@luxas
Labels
area/security lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Milestone
v1.11

Comments

@luxas
Copy link
Member

luxas commented Mar 16, 2018

@liggitt said, very reasonably:

kubeadm should also disable the readonly port to follow best practices.

ref: kubernetes/kubernetes#59666 (comment)

While we already protect the most essential parts (e.g. disable cAdvisor, protect the API using the authorizer, etc.), we could lock this down by default more as well before going to GA.

cc @timothysc @kad @stealthybox
@luxas luxas added kind/enhancement area/security priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Mar 16, 2018
@kad
Copy link
Member

kad commented Mar 16, 2018

http://localhost:10255/healthz is used to check that kubelet started. Should we drop those and just keep WaitForAPI() ?

@detiber
Copy link
Member

detiber commented Mar 29, 2018

Since we have access to the generated certificates, it is possible to use the secure port.

@stealthybox
Copy link
Member

^ worth noting we will need to be using the secure port more for other planned uses of the kubelet API.

@timothysc
Copy link
Member

/assign @detiber

@timothysc timothysc added this to the v1.11 milestone Apr 6, 2018
This was referenced May 11, 2018
Closed
Closed
@luxas
Copy link
Member Author

luxas commented May 11, 2018

@woopstar do you want to work on this issue (given #804?).
Please do in that case, I don't think @detiber has started working on this yet.

@woopstar
Copy link
Member

Depends on what's expected.

Do we simply just want to update the WaitForHealthyKubelet function to use https instead of http? And apply the client certificates to the request?

How would we find the kubelet address and port? It is usually configured in a systemd service? Do we simply just expect it to be https://localhost:10250 ?

@luxas
Copy link
Member Author

luxas commented May 11, 2018

Do we simply just want to update the WaitForHealthyKubelet function to use https instead of http? And apply the client certificates to the request?

That's the first step 👍

Do we simply just expect it to be https://localhost:10250 ?

For now at least, yes

@luxas luxas mentioned this issue May 14, 2018
Merged
@luxas luxas assigned luxas and unassigned detiber May 14, 2018
@luxas luxas added the lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. label May 14, 2018
@luxas
Copy link
Member Author

luxas commented May 14, 2018

Sent first required PR kubernetes/kubernetes#63812

k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this issue May 15, 2018
Merge pull request #63812 from luxas/kubeadm_cut_kubelet_readonly_dep
dc62a73 
Automatic merge from submit-queue (batch tested with PRs 57536, 63812). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

kubeadm: Contact the kubelet on its healthz port instead of its readonly port

**What this PR does / why we need it**:
In order for us to disable the kubelet's readonly port in v1.11 (kubernetes/kubeadm#732), we need to cut the dependency on that port being open. Instead, we can use the dedicated healthz port (using the defaults `--healthz-bind-address=127.0.0.1` and `--healthz-port=10248`, xref: https://kubernetes.io/docs/reference/command-line-tools-reference/kubelet/)

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Part of kubernetes/kubeadm#732

**Special notes for your reviewer**:

**Release note**:

```release-note
NONE
```
@kubernetes/sig-cluster-lifecycle-pr-reviews @liztio
@woopstar
Copy link
Member

Oi. Using the the dedicated health endpoint is actually way better than using the https endpoint I guess :)

@luxas
Copy link
Member Author

luxas commented May 15, 2018

@woopstar I first learned about the healthz endpoint server yesterday :). Using the secure port wouldn't have worked, as its authentication/authorization there depends on the API server, which at this point isn't running. So kubernetes/kubernetes#63812 was actually the only thing we can do at this stage.

@stealthybox
Copy link
Member

@luxas will we not be able to use the kubelet API using the secure port if the apiserver is not running?

@luxas
Copy link
Member Author

luxas commented May 15, 2018

will we not be able to use the kubelet API using the secure port if the apiserver is not running?

right. The kubelet exec's out a SubjectAccessReview call to the API server when it gets a request to its own API in order to verify whether the caller is authorized to talk to the kubelet. So when there's no API server we can't talk to the kubelet secure API either. However, this was fixed with using the dedicated healthz port

@detiber
Copy link
Member

detiber commented May 15, 2018

That does complicate things a bit... Although, I really don't see an issue with relying on the api server and validating the mirror pods after the api server is already up and responding...

@luxas
Copy link
Member Author

luxas commented May 15, 2018

Anyway, it's already fixed with the healthz port on localhost:10248 😄

@stealthybox
Copy link
Member

The kubelet exec's out a SubjectAccessReview call to the API server when it gets a request to its own API in order to verify whether the caller is authorized to talk to the kubelet. So when there's no API server we can't talk to the kubelet secure API either. However, this was fixed with using the dedicated healthz port

This is probably not true though the apiserver-kubelet cert is used, right?

@luxas
Copy link
Member Author

luxas commented May 19, 2018

This is probably not true though the apiserver-kubelet cert is used, right?

This happens always, even though the apiserver-kubelet-client is used AFAIK, check it up in the code, but I haven't seen any exceptions 😉

This was referenced May 21, 2018
Closed
Merged
k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this issue May 24, 2018
Merge pull request #64187 from luxas/kubeadm_kubelet_improve_security
698ac55 
Automatic merge from submit-queue (batch tested with PRs 64174, 64187, 64216, 63265, 64223). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

kubeadm: Improve the kubelet default configuration security-wise

**What this PR does / why we need it**:
 - Disables the readonly port for the kubelets in the cluster
 - Enables delegated SA token authentication for the secure kubelet port (GCE also did this ref: #58178)
 - Follows up #63912 to move the last flag from the system dropin to the ComponentConfig

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes kubernetes/kubeadm#732
Fixes kubernetes/kubeadm#650
Replaces #57997

**Special notes for your reviewer**:
In order to make sure this actually works, or that clusters actually are secure, we're adding e2e tests for this: kubernetes/kubeadm#838 & #64140
Depends on #63912

**Release note**:

```release-note
[action required] kubeadm: kubelets in kubeadm clusters now disable the readonly port (10255). If you're relying on unauthenticated access to the readonly port, please switch to using the secure port (10250). Instead, you can now use ServiceAccount tokens when talking to the secure port, which will make it easier to get access to e.g. the `/metrics` endpoint of the kubelet securely.
```
@kubernetes/sig-cluster-lifecycle-pr-reviews 
@kubernetes/sig-auth-pr-reviews FYI
@dixudx dixudx mentioned this issue Jun 4, 2018
Merged
@vmendi vmendi mentioned this issue Oct 12, 2018
Closed
@elemental-lf elemental-lf mentioned this issue Oct 18, 2018
Closed
@micahhausler micahhausler mentioned this issue Dec 26, 2018
Closed
@onlybakam onlybakam mentioned this issue Jan 10, 2019
Closed
@dpankros dpankros mentioned this issue Feb 25, 2019
Closed
@syst0m syst0m mentioned this issue Feb 26, 2019
Closed
@drewgonzales360
Copy link

I know this is old, but can someone explain what

While we already protect the most essential parts (e.g. disable cAdvisor, protect the API using the authorizer, etc.), we could lock this down by default more as well before going to GA.

Means?

@r0ckarong r0ckarong mentioned this issue May 7, 2019
Closed
@poidag-zz poidag-zz mentioned this issue Jun 26, 2019
Closed
@pbecotte pbecotte mentioned this issue Jan 7, 2020
Closed
@mykaul mykaul mentioned this issue Jan 18, 2021
Merged
@danielmotaleite danielmotaleite mentioned this issue Aug 22, 2022
Open
@crigertg crigertg mentioned this issue Dec 5, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@luxas luxas

Labels
area/security lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
None yet
Milestone
v1.11
Development

Successfully merging a pull request may close this issue.

kubeadm: Improve the kubelet default configuration security-wise luxas/kubernetes
7 participants
@kad @timothysc @detiber @stealthybox @woopstar @luxas @drewgonzales360