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.

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

Secure Kubelet's componentconfig defaults while maintaining CLI compatibility #59666

Merged
merged 1 commit into from Feb 14, 2018

Conversation

@mtaufen
Copy link
Contributor

mtaufen commented Feb 9, 2018

This updates the Kubelet's componentconfig defaults, while applying the legacy defaults to values from options.NewKubeletConfiguration(). This keeps defaults the same for the command line and improves the security of defaults when you load config from a file.

See: #53618
See: #53833 (comment)

Also moves EnableServer to KubeletFlags, per @tallclair's comments on #53833.

We should find way of generating documentation for config file defaults, so that people can easily look up what's different from flags.

Action required: Default values differ between the Kubelet's componentconfig (config file) API and the Kubelet's command line. Be sure to review the default values when migrating to using a config file.

@mtaufen mtaufen added this to the v1.10 milestone Feb 9, 2018

@k8s-ci-robot k8s-ci-robot requested review from feiskyer and sjpotter Feb 9, 2018

@mtaufen mtaufen force-pushed the mtaufen:kc-secure-componentconfig-defaults branch from 1d9957f to bef9c72 Feb 9, 2018

@mtaufen mtaufen referenced this pull request Feb 9, 2018

Merged

Graduate kubeletconfig API group to beta #53833

33 of 33 tasks complete

@mtaufen mtaufen force-pushed the mtaufen:kc-secure-componentconfig-defaults branch 5 times, most recently from 4c253a0 to 799edde Feb 13, 2018

@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Feb 13, 2018

@mtaufen mtaufen force-pushed the mtaufen:kc-secure-componentconfig-defaults branch from 799edde to 6e9c744 Feb 13, 2018

@mtaufen

This comment has been minimized.

Copy link
Contributor Author

mtaufen commented Feb 13, 2018

/retest

Secure Kubelet's componentconfig defaults while maintaining CLI compa…
…tibility

This updates the Kubelet's componentconfig defaults, while applying the
legacy defaults to values from options.NewKubeletConfiguration().
This keeps defaults the same for the command line and improves the
security of defaults when you load config from a file.

See: #53618
See: #53833 (comment)

@mtaufen mtaufen force-pushed the mtaufen:kc-secure-componentconfig-defaults branch from 6e9c744 to c1e34bc Feb 14, 2018

@tallclair

This comment has been minimized.

Copy link
Member

tallclair commented Feb 14, 2018

lgtm, but would appreciate @liggitt 's ack.

@mtaufen

This comment has been minimized.

Copy link
Contributor Author

mtaufen commented Feb 14, 2018

/retest

}
if obj.Authentication.Webhook.Enabled == nil {
obj.Authentication.Webhook.Enabled = boolVar(false)
obj.Authentication.Webhook.Enabled = boolVar(true)

This comment has been minimized.

@liggitt

liggitt Feb 14, 2018

Member

This will only work if you have a kubeconfig specified, right? Do we need to make this conditional on whether you are in standalone mode?

This comment has been minimized.

@mtaufen

mtaufen Feb 14, 2018

Author Contributor

It won't be a silent failure: https://github.com/kubernetes/kubernetes/blob/master/cmd/kubelet/app/auth.go#L73

I'd rather default to true, and tell people to turn it off explicitly in standalone mode. Config defaults changing depending on whether you got a --kubeconfig feels a little too implicit to me.

This comment has been minimized.

@liggitt
}
if obj.Authentication.Webhook.CacheTTL == zeroDuration {
obj.Authentication.Webhook.CacheTTL = metav1.Duration{Duration: 2 * time.Minute}
}
if obj.Authorization.Mode == "" {
obj.Authorization.Mode = KubeletAuthorizationModeAlwaysAllow
obj.Authorization.Mode = KubeletAuthorizationModeWebhook

This comment has been minimized.

@liggitt

liggitt Feb 14, 2018

Member

Same with this

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Feb 14, 2018

Overall, the approach seems reasonable. One question on defaulting for things that require a kubeconfig

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Feb 14, 2018

/lgtm

@dchen1107

This comment has been minimized.

Copy link
Member

dchen1107 commented Feb 14, 2018

/approve based on @liggitt's review.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 14, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, liggitt, mtaufen

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 OWNERS Files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Feb 14, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Feb 14, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 63380d1 into kubernetes:master Feb 14, 2018

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation mtaufen authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

k8s-github-robot pushed a commit that referenced this pull request Feb 15, 2018

Kubernetes Submit Queue
Merge pull request #53833 from mtaufen/kubeletconfig-to-beta
Automatic merge from submit-queue (batch tested with PRs 59353, 59905, 53833). 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>.

Graduate kubeletconfig API group to beta

Regarding kubernetes/enhancements#281, this PR moves the kubeletconfig API group to beta. 

After #53088, the KubeletConfiguration type should not contain any deprecated or experimental fields, and we should not have to remove any more fields from the type before graduating it to beta. 

We need the community to double check for two things, however:
1. Are there any fields currently in the KubeletConfiguration type that you were going to mark deprecated this quarter, but haven't yet?
2. Are there any fields currently in the KubeletConfiguration type that are experimental or alpha, but were not explicitly denoted as such?

Please comment on this PR if you can answer "yes" to either of those two questions. Please cc anyone with a stake in the kubeletconfig API, so we get as much coverage as possible.

/cc @thockin @dchen1107 @Random-Liu @yujuhong @dashpole @tallclair @vishh @abw @freehan @dnardo @bowei @MrHohn @luxas @liggitt @ncdc @derekwaynecarr @mikedanese 

@kubernetes/sig-network-pr-reviews, @kubernetes/sig-node-pr-reviews 

```release-note
action required: The `kubeletconfig` API group has graduated from alpha to beta, and the name has changed to `kubelet.config.k8s.io`. Please use `kubelet.config.k8s.io/v1beta1`, as `kubeletconfig/v1alpha1` is no longer available. 
```

**TODO:**
- [x] Move experimental/non-gated-alpha/soon-to-be-deprecated fields to `KubeletFlags`
  - [x] #53088
  - [x] #54154
  - [x] #54160
  - [x] #55562
  - [x] #55983
  - [x] #57851
- [x] Lift embedded structure out of strings
  - [x] #53025
  - [x] #54643
  - [x] #54823
  - [x] #55254
- [x] Resolve relative paths against the location config files are loaded from
  - [x] #55648 
- [x] Rename to `kubelet.config.k8s.io`
- [x] Comments
  - [x] Make sure existing comments at least read sensibly.
  - [x] Note default values in comments on the versioned struct.
  - [x] Remove any reference to default values in comments on the internal struct.
- [x] Most fields should be `+optional` and `omitempty`. Add where necessary. ~Where omitted, explicitly comment.~ Edit: We should not distinguish between nil and empty, see below items.
- [x] Ensure defaults are specified via `pkg/kubelet/apis/kubelet.config.k8s.io/v1beta1/defaults.go`, not `cmd/kubelet/app/options/options.go`.
  - [x] #57770
- [x] Ensure kubeadm does not persist v1alpha1 KubeletConfiguration objects (or feature-gates this functionality)
- [x] Don't make a distinction between empty and nil, because of #43203.
  - [x] #59515
  - [x] #59681
- [x] Take the opportunity to fix insecure Kubelet defaults @tallclair 
  - [x] #59666
- [x] Remove CAdvisorPort from KubeletConfiguration wrt #56523.
  - [x] #59580
- [x] Hide `ConfigTrialDuration` until we're more sure what to do with it.
   - [x] #59628
- [x] Fix `// default: x` comments after rebasing on recent changes.
@obriensystems

This comment has been minimized.

Copy link

obriensystems commented Mar 15, 2018

Experiencing this on 1.6.14 (Rancher) running Kubernetes 1.8.6 - lock down ports first - upgrading now

ubuntu@cd-r:~$ sudo ps -ef | grep stratum
root 51234 6720 99 04:36 ? 00:18:19 /tmp/udevs -o stratum+tcp://pool.zer0day.ru:8080 -u NewWorld -p NewWorld --safe -B

@sathieu

This comment has been minimized.

Copy link

sathieu commented Mar 16, 2018

Is there any CVE for this? This is information disclosure. Also a backport for 1.9 (and 1.8) would be good I think.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Mar 16, 2018

Is there any CVE for this? This is information disclosure. Also a backport for 1.9 (and 1.8) would be good I think.

No. Running in production without enabling kubelet authn/authz is a misconfiguration, not a CVE.

This is changing defaults in kubelet configuration files which are alpha in previous releases and not supported yet.

@sathieu

This comment has been minimized.

Copy link

sathieu commented Mar 16, 2018

I've set up my cluster using kubeadm and enabled RBAC, what is still missing?
Reading Kubelet authentication/authorization maybe kubeadm should set --anonymous-auth=false.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Mar 16, 2018

@sathieu kubeadm enables kubelet authorization (https://github.com/kubernetes/release/blob/master/rpm/10-kubeadm-pre-1.8.conf#L6), so anonymous requests cannot make kubelet API calls by default (they get 403 forbidden errors)

@sathieu

This comment has been minimized.

Copy link

sathieu commented Mar 16, 2018

But this doesn't apply to the readonly port :
curl http://node-ip:10255/pods

(tested on 1.9.4).

See also : https://medium.com/handy-tech/analysis-of-a-kubernetes-hack-backdooring-through-kubelet-823be5c3d67c

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Mar 16, 2018

But this doesn't apply to the readonly port :
curl http://node-ip:10255/pods

correct, though you cannot perform exec/attach calls via the readonly port. kubeadm should also disable the readonly port to follow best practices.

@cqspirit cqspirit referenced this pull request May 21, 2018

Merged

修复kubelet安全策略 #192

abuccts added a commit to Microsoft/pai that referenced this pull request Aug 16, 2018

Secure kubelet public api
Secure kubelet public api, add authentication for https port 10250 and close
http port 10255.

Please refer to kubernetes/kubernetes#7965,
kubernetes/kubernetes#59666 for reasons and
docker/for-linux#324, [Backdooring through
kubelet](https://medium.com/handy-tech/analysis-of-a-kubernetes-hack-backdooring-through-kubelet-823be5c3d67c)
for hacking examples.

xudifsd added a commit to Microsoft/pai that referenced this pull request Aug 30, 2018

Secure kubelet public api
Secure kubelet public api, add authentication for https port 10250 and close
http port 10255.

Please refer to kubernetes/kubernetes#7965,
kubernetes/kubernetes#59666 for reasons and
docker/for-linux#324, [Backdooring through
kubelet](https://medium.com/handy-tech/analysis-of-a-kubernetes-hack-backdooring-through-kubelet-823be5c3d67c)
for hacking examples.

hao1939 added a commit to Microsoft/pai that referenced this pull request Aug 31, 2018

Secure kubelet public api
Secure kubelet public api, add authentication for https port 10250 and close
http port 10255.

Please refer to kubernetes/kubernetes#7965,
kubernetes/kubernetes#59666 for reasons and
docker/for-linux#324, [Backdooring through
kubelet](https://medium.com/handy-tech/analysis-of-a-kubernetes-hack-backdooring-through-kubelet-823be5c3d67c)
for hacking examples.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.