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

kubeadm: Improve the kubelet default configuration security-wise #64187

Merged

Conversation

luxas
Copy link
Member

@luxas luxas commented May 23, 2018

What this PR does / why we need it:

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:

[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

@k8s-ci-robot k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. sig/auth Categorizes an issue or PR as relevant to SIG Auth. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/kubeadm approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 23, 2018
@dixudx
Copy link
Member

dixudx commented May 23, 2018

/retest


// Disable the readonly port of the kubelet, in order to not expose unnecessary information
// TODO: Enable in a future PR
// obj.KubeletConfiguration.BaseConfig.ReadOnlyPort = 0
obj.KubeletConfiguration.BaseConfig.ReadOnlyPort = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that ReadOnlyPort is already 0 by default when you load a config from a file, or use dynamic config.
Similar for a few others too, like Webhook auth. The v1beta1 defaulter is the canonical source of defaults, and we maintain the old defaults for flags via applyLegacyDefaults in cmd/kubelet/app/options/options.go.

So if you're just generating the default Kubelet config, then you'll get the file/dynamic config defaults if you do it like NewKubeletConfiguration in options.go.

Ah, noticed above you're doing this:

if obj.KubeletConfiguration.BaseConfig == nil {
	obj.KubeletConfiguration.BaseConfig = &kubeletconfigv1beta1.KubeletConfiguration{}
 }

You might want to run it through the defaulter, like NewKubeletConfiguration in options.go.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do run these through the defaulter, see later down the func where we construct the scheme.
But I like to enforce this here as well (double-checking basically to be safe), and be explicit with what we require. Does that make sense? Yeah I saw these are way more secure by default when not using the flags 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, that's totally reasonable.

@luxas luxas force-pushed the kubeadm_kubelet_improve_security branch from d80fc95 to 935835b Compare May 23, 2018 18:01
@luxas
Copy link
Member Author

luxas commented May 23, 2018

/retest

Copy link
Member

@timothysc timothysc left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

obj.KubeletConfiguration.BaseConfig.ReadOnlyPort = 0

// Enables client certificate rotation for the kubelet
obj.KubeletConfiguration.BaseConfig.RotateCertificates = true
Copy link
Member

Choose a reason for hiding this comment

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

For my own edification what's the default policy for kubelet cert rotation?

Copy link
Member Author

Choose a reason for hiding this comment

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

false, don't rotate an expired cert. It's beta, not GA yet. kubernetes/enhancements#266

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: luxas, timothysc

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 files:

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

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@luxas luxas force-pushed the kubeadm_kubelet_improve_security branch from 935835b to efc4089 Compare May 23, 2018 20:52
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2018
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@luxas
Copy link
Member Author

luxas commented May 23, 2018

just fixed a fuzzing/roundtrip test

@luxas luxas added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2018
@@ -79,6 +79,7 @@ func Funcs(codecs runtimeserializer.CodecFactory) []interface{} {
Enabled: utilpointer.BoolPtr(false),
},
},
RotateCertificates: true,
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused by this function. It doesn't look like it's fuzzing anything, just hardcoding the values?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's making sure these values from the fuzzer / these defaults are preserved between conversions using the different API versions.

Copy link
Member

Choose a reason for hiding this comment

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

I was looking at this a while back, and this overrides the default fuzzing for the attributes and hardcodes specific values. We should most likely only be setting values for attributes here if we need to handle cases where randomized fuzzing would generate invalid configs (attributes that are mutually exclusive, dependent on other attributes, etc).

Copy link
Member Author

Choose a reason for hiding this comment

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

@detiber can you open an issue for improving that? I guess we're setting this for a bit too many items then...


// Disable the readonly port of the kubelet, in order to not expose unnecessary information
// TODO: Enable in a future PR
// obj.KubeletConfiguration.BaseConfig.ReadOnlyPort = 0
obj.KubeletConfiguration.BaseConfig.ReadOnlyPort = 0
Copy link
Member

Choose a reason for hiding this comment

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

I assume you already configure heapster / metrics server to scrape from the 10255 port? (Can you point me to the config, to satisfy my curiosity?)

Copy link
Member Author

Choose a reason for hiding this comment

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

We actually don't configure heapster / the metrics server in our default configs. We do the bare minimum needed for conformance which doesn't require that kind of monitoring at the moment.

@luxas
Copy link
Member Author

luxas commented May 24, 2018

/retest

1 similar comment
@luxas
Copy link
Member Author

luxas commented May 24, 2018

/retest

@k8s-github-robot
Copy link

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 here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable the kubelet readonly port Enable support for authenticating tokens (e.g. SAs) in the Kubelet API
9 participants