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

kubeadm join: Error out if CA pinning isn't used or opted out of #55468

Merged
merged 1 commit into from
Nov 16, 2017
Merged

kubeadm join: Error out if CA pinning isn't used or opted out of #55468

merged 1 commit into from
Nov 16, 2017

Conversation

yuexiao-wang
Copy link
Contributor

@yuexiao-wang yuexiao-wang commented Nov 10, 2017 •
edited by luxas

Signed-off-by: yuexiao-wang wang.yuexiao@zte.com.cn

What this PR does / why we need it:
convert the warning to an error in kubeadm

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#534

Special notes for your reviewer:
/cc @mattmoyer

Release note:

[action required] kubeadm join: Error out if CA pinning isn't used or opted out of
* kubeadm now requires the user to specify either the `--discovery-token-ca-cert-hash` flag or the `--discovery-token-unsafe-skip-ca-verification` flag.

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Nov 10, 2017
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 10, 2017
@yuexiao-wang
Copy link
Contributor Author

/assign @krousey

@yuexiao-wang
Copy link
Contributor Author

ping @krousey

@yuexiao-wang yuexiao-wang changed the title convert the warning to an error in kubeadm Fix TODO: convert the warning to an error in kubeadm Nov 13, 2017
@dims
Copy link
Member

dims commented Nov 13, 2017

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 13, 2017
mattmoyer
mattmoyer suggested changes Nov 13, 2017
View reviewed changes
Copy link
Contributor

@mattmoyer mattmoyer left a comment

Choose a reason for hiding this comment

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

@yuexiao-wang thanks for fixing this!

cmd/kubeadm/app/apis/kubeadm/validation/validation.go Outdated
if len(cfg.DiscoveryFile) == 0 && len(cfg.DiscoveryTokenCACertHashes) == 0 && !cfg.DiscoveryTokenUnsafeSkipCAVerification {
fmt.Println("[validation] WARNING: using token-based discovery without DiscoveryTokenCACertHashes can be unsafe (see https://kubernetes.io/docs/admin/kubeadm/#kubeadm-join).")
fmt.Println("[validation] WARNING: Pass --discovery-token-unsafe-skip-ca-verification to disable this warning. This warning will become an error in Kubernetes 1.9.")
allErrs = append(allErrs, field.Invalid(fldPath, "", "using token-based discovery without DiscoveryTokenCACertHashes can be unsafe."))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be helpful to still include a message like (set DiscoveryTokenUnsafeSkipCAVerification to continue). so the user knows how to bypass this check if they are willing to sacrifice some of the security validation.

I'm not sure if it would be better to refer to the CLI flag (--discovery-token-unsafe-skip-ca-verification) or the config parameter (DiscoveryTokenUnsafeSkipCAVerification). It seems like the rest of the errors refer to the config parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for review. The message has updated. PTAL

@yuexiao-wang
convert this warning to an error in kubeadm
4387d57 
Signed-off-by: yuexiao-wang <wang.yuexiao@zte.com.cn>
@mattmoyer
Copy link
Contributor

/lgtm

Thanks @yuexiao-wang!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2017
@mattmoyer mattmoyer mentioned this pull request Nov 14, 2017
Merged
luxas
luxas reviewed Nov 14, 2017
View reviewed changes
Copy link
Member

@luxas luxas left a comment

Choose a reason for hiding this comment

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

/assign @jbeda
for final approval
I think I'm ok here, but it is a breaking change indeed that I'm not very excited to see either tbh

@jbeda
Copy link
Contributor

jbeda commented Nov 14, 2017

I'm cool with this but @luxas wanted to cover it in the SIG meeting today.

@jbeda
Copy link
Contributor

jbeda commented Nov 14, 2017

We should couple this with a fix for kubernetes/kubeadm#519. It would be great if this error said "Run with --discovery-token-unsafe-skip-ca-verification or get the correct CA verification argument with kubeadm token get --print-join-command" or some such.

Also, this needs a release note. This is a change that users should be aware of.

@luxas luxas changed the title Fix TODO: convert the warning to an error in kubeadm kubeadm join: Error out if CA pinning isn't used or opted out of Nov 14, 2017
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note-none Denotes a PR that doesn't merit a release note. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Nov 14, 2017
luxas
luxas approved these changes Nov 14, 2017
View reviewed changes
Copy link
Member

@luxas luxas left a comment

Choose a reason for hiding this comment

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

/approve

This was approved in the SIG meeting

cc @mattmoyer FYI

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: luxas, mattmoyer, yuexiao-wang

Associated issue: 534

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 14, 2017
@luxas luxas mentioned this pull request Nov 14, 2017
Closed
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 55764, 55683, 55468, 54409, 55546). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 4962d29 into kubernetes:master Nov 16, 2017
@yuexiao-wang yuexiao-wang deleted the change-warning branch November 16, 2017 10:37
dims added a commit to dims/kubernetes-anywhere that referenced this pull request Nov 19, 2017
@dims
Add --discovery-token-unsafe-skip-ca-verification to prevent kubeadm …
0fe7681 
…join failure

Since kubernetes/kubernetes#55468 merged, kubeadm now requires the user to specify either the `--discovery-token-ca-cert-hash` flag or the `--discovery-token-unsafe-skip-ca-verification` flag
This was referenced Nov 19, 2017
Closed
Closed
alban added a commit to kinvolk/kube-spawn that referenced this pull request Nov 26, 2017
@alban
kubeadm join: fix TLS on Kubernetes >=1.9
0edaa40 
kubeadm got a new option in version 1.8:
--discovery-token-unsafe-skip-ca-verification
See: kubernetes/kubernetes#49520

Since version 1.9, it became mandatory to either use it to skip
verification, or to use --discovery-token-ca-cert-hash=...
See: kubernetes/kubernetes#55468

Since kube-spawn is a developer tool used on one physical machine, use
the former.
@alban alban mentioned this pull request Nov 26, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattmoyer mattmoyer mattmoyer requested changes

@luxas luxas luxas approved these changes

Assignees

@jbeda jbeda

@krousey krousey

@lukemarsden lukemarsden

@dmmcquay dmmcquay

@mattmoyer mattmoyer

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Start erroring out when CA pinning isn't used and not explicitely opted out of
10 participants
@yuexiao-wang @dims @mattmoyer @jbeda @k8s-github-robot @luxas @krousey @lukemarsden @dmmcquay @k8s-ci-robot