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

Always secure api -> kubelet communication #11185

Merged
merged 1 commit into from
Apr 13, 2021

Conversation

olemarkus
Copy link
Member

@olemarkus olemarkus commented Apr 7, 2021

Without secure node auth enabled, commands like kubectl logs may fail with certain configurations.

As of kubernetes 1.11, we explicitly disable anonymousauth on kubelet for new clusters. We also always disable AlwaysAllow auth mode for kubelet as of 1.19. This combination works for new clusters and those who have already disabled anonymousAuth for kubelet, which accounts for most users, it seems.

But old clusters upgrading from pre 1.11 face issues with commands like kubectl logs because for these clusters, apiserver is anonymous and anonymous users are not allowed to talk to kubelet per our RBAC rules.

The logic for this is

// UseSecureKubelet checks if the kubelet api should be protected by a client certificate.
func (c *NodeupModelContext) UseSecureKubelet() bool {
	return c.NodeupConfig.KubeletConfig.AnonymousAuth != nil && !*c.NodeupConfig.KubeletConfig.AnonymousAuth
}

anonymousAuth and secure kubelet comms are two different things as far as I can tell. So not sure why we made the check this way. Checking for AuthorizationMode="" or AlwaysAllow would make more sense.

This PR enables secure comms unconditionally

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 7, 2021
@hakman
Copy link
Member

hakman commented Apr 7, 2021

/test pull-kops-e2e-cni-cilium

@olemarkus
Copy link
Member Author

/retest

1 similar comment
@olemarkus
Copy link
Member Author

/retest

@olemarkus olemarkus added this to the v1.21 milestone Apr 8, 2021
@olemarkus
Copy link
Member Author

/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Apr 8, 2021
@@ -399,7 +399,11 @@ func (c *NodeupModelContext) UseBootstrapTokens() bool {

// UseSecureKubelet checks if the kubelet api should be protected by a client certificate.
func (c *NodeupModelContext) UseSecureKubelet() bool {
return c.NodeupConfig.KubeletConfig.AnonymousAuth != nil && !*c.NodeupConfig.KubeletConfig.AnonymousAuth
if c.IsKubernetesGTE("1.21") {
return true
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see much logging elsewhere in this file, but should we warn users if they had enabled anonymous authentication, and we're overriding their intention here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I just saw #11180, which I think takes care of catching that discrepancy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. Need to think a little about how to approach that. But for now, this should take care of some of the bugs people have been reporting.

nodeup/pkg/model/context.go Outdated Show resolved Hide resolved
nodeup/pkg/model/context.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 10, 2021
@johngmyers
Copy link
Member

/retest

4 similar comments
@johngmyers
Copy link
Member

/retest

@johngmyers
Copy link
Member

/retest

@olemarkus
Copy link
Member Author

/retest

@olemarkus
Copy link
Member Author

/retest

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 13, 2021
@olemarkus olemarkus changed the title Use secure node auth as of k8s 1.21 Always secure api -> kubelet communication Apr 13, 2021
Without secure node auth enabled, commands like `kubectl logs` may fail
with certain configurations.

Previously, we checked if anonymousAuth was enabled on the kubelet
before securing node communication, but this isn't really relevant. We
can still authenticate even if anonymous access is allowed.
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

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

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 13, 2021
@olemarkus
Copy link
Member Author

/retest

@olemarkus
Copy link
Member Author

/test pull-kops-e2e-k8s-containerd

@k8s-ci-robot k8s-ci-robot merged commit 044a59a into kubernetes:master Apr 13, 2021
k8s-ci-robot added a commit that referenced this pull request Apr 13, 2021
…185-origin-release-1.20

Automated cherry pick of #11185: Use secure kubelet auth
k8s-ci-robot added a commit that referenced this pull request Apr 13, 2021
…185-origin-release-1.19

Automated cherry pick of #11185: Use secure kubelet auth
@olemarkus olemarkus deleted the always-secure-node branch April 19, 2021 16:27
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/documentation area/nodeup cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants