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: delete bootstrap-kubelet.conf after TLS bootstrap #80676

Merged

Conversation

@fabriziopandini
Copy link
Member

commented Jul 27, 2019

What type of PR is this?
/kind bug

What this PR does / why we need it:
#66482 enabled kubeadm join --discovery-file with a discovery file with embedded credentials. This PR makes sure that kubeadm removes the bootstrap-kubelet.conf after completing the TLS bootstrap process, so copies of the above credentials won't be left around.

Which issue(s) this PR fixes:
Fixes #kubernetes/kubeadm#1689

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

ACTION REQUIRED:
kubeadm now deletes the bootstrap-kubelet.conf file after TLS bootstrap
User relying on bootstrap-kubelet.conf should switch to kubelet.conf that contains node credentials

/sig cluster-lifecycle
/priority important-soon
/assign @timothysc
/assign @neolit123

Copy link
Member

left a comment

i think this is mostly OK, but i recommend adding a ACTION REQUIRED: prefix to the release note.
the boostrap credential can be bound to existing infrastructures, which was probably not a great idea in the first place, but we should visibly notify about such changes.

/lgtm
/hold
for other reviews.

@neolit123

This comment has been minimized.

Copy link
Member

commented Jul 28, 2019

if we really want to be on the safe side there should be a grace period of say 1 release while kubeadm join supports a knob for this that defaults to false.

EDIT and of course another option is not add this change and leave it to user infrastructure to call rm /etc/kubernetes/kubelet-bootstrap.conf.

Copy link
Member

left a comment

/lgtm
/approve
/hold cancel

Given that this is a security hole I'd be ok with a backport of this.
I don't think an action is required on the release note.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, 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

@fabriziopandini

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

/test pull-kubernetes-dependencies

@neolit123

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Given that this is a security hole I'd be ok with a backport of this.

i'm -1 given the workaround is very simple:
kubeadm join .... && sudo rm /etc/kubernetes/kubelet-bootstrap.conf

I don't think an action is required on the release note.

i worry about somebody is copying or doing something with the bootstrap credentials.
with kubeadm being GA we should avoid such changes without notification.

@fabriziopandini

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

AFAIK there is no risk in backporting this fix

  • if users are using token discovery, then bootstrap-kubelet can’t be used because it expires
  • if users are using file discovery, there is no way to get other credentials in until #80675 merge

But I leave the final call on this on @timothysc / @neolit123

@neolit123

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

/retest

1 similar comment
@neolit123

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

/retest

@fejta-bot

This comment has been minimized.

Copy link

commented Jul 30, 2019

/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 or /hold comment for consistent failures.

Copy link
Member

left a comment

LGTM

@fejta-bot

This comment has been minimized.

Copy link

commented Jul 31, 2019

/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 or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 82a252a into kubernetes:master Jul 31, 2019
23 checks passed
23 checks passed
cla/linuxfoundation fabriziopandini authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Jul 31, 2019
@stealthybox

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

the delete should be moved to defer here:
https://github.com/kubernetes/kubernetes/pull/80676/files#diff-3c566a0c1e3b213a34b39503abf4fc11R104

otherwise it's possible for the token to be left behind when the function errors

@neolit123

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

yep, +1 to use defer.
let me log a ticket.

@fabriziopandini fabriziopandini deleted the fabriziopandini:delete-bootstrap-kubelet.conf branch Aug 8, 2019
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.