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: tolerate whitespace when validating user CA PEMs #86705

Merged

Conversation

@neolit123
Copy link
Member

neolit123 commented Dec 29, 2019

What this PR does / why we need it:

The function validateKubeConfig() can end up comparing
a user generated kubeconfig to a kubeconfig generated by kubeadm.

If a user kubeconfig has a CA that is base64 encoded with whitespace,
if said kubeconfig is loaded using clientcmd.LoadFromFile()
the CertificateAuthorityData bytes will be decoded from base64
and placed in the v1.Config raw. On the other hand a kubeconfig
generated by kubeadm will have the ca.crt parsed to a Certificate
object with whitespace ignored in the PEM input.

Make sure that validateKubeConfig() tolerates whitespace differences
when comparing CertificateAuthorityData.

Which issue(s) this PR fixes:

Fixes kubernetes/kubeadm#1991

Special notes for your reviewer:
NONE

Does this PR introduce a user-facing change?:

kubeadm: tolerate whitespace when validating certificate authority PEM data in kubeconfig files

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


/assign @fabriziopandini
/kind bug
/priority backlog

@neolit123

This comment has been minimized.

Copy link
Member Author

neolit123 commented Dec 29, 2019

/hold
for review

Copy link
Member

SataQiu left a comment

/lgtm

@SataQiu

This comment has been minimized.

Copy link
Member

SataQiu commented Dec 30, 2019

/test pull-kubernetes-integration
/test pull-kubernetes-e2e-gce

Copy link
Member

fabriziopandini left a comment

@neolit123 thanks for the PR!
Have you considered to use bytes.TrimSpaces so we can avoid the conversion to string?

@neolit123

This comment has been minimized.

Copy link
Member Author

neolit123 commented Dec 30, 2019

Have you considered to use bytes.TrimSpaces so we can avoid the conversation to string?

i must have forgotten about this function. will update.

The function validateKubeConfig() can end up comparing
a user generated kubeconfig to a kubeconfig generated by kubeadm.

If a user kubeconfig has a CA that is base64 encoded with whitespace,
if said kubeconfig is loaded using clientcmd.LoadFromFile()
the CertificateAuthorityData bytes will be decoded from base64
and placed in the v1.Config raw. On the other hand a kubeconfig
generated by kubeadm will have the ca.crt parsed to a Certificate
object with whitespace ignored in the PEM input.

Make sure that validateKubeConfig() tolerates whitespace differences
when comparing CertificateAuthorityData.
@neolit123 neolit123 force-pushed the neolit123:1.18-fix-ca-whitespace-comparison branch from 080da2e to 453ac80 Dec 30, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm label Dec 30, 2019
@neolit123

This comment has been minimized.

Copy link
Member Author

neolit123 commented Dec 30, 2019

updated to use bytes.TrimSpace()
/hold cancel

@fabriziopandini

This comment has been minimized.

Copy link
Member

fabriziopandini commented Dec 31, 2019

thanks @neolit123!
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Dec 31, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 31, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, neolit123

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

@neolit123

This comment has been minimized.

Copy link
Member Author

neolit123 commented Dec 31, 2019

/retest

1 similar comment
@neolit123

This comment has been minimized.

Copy link
Member Author

neolit123 commented Dec 31, 2019

/retest

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Dec 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.

2 similar comments
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jan 1, 2020

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

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jan 1, 2020

/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 09cb73a into kubernetes:master Jan 1, 2020
15 checks passed
15 checks passed
cla/linuxfoundation neolit123 authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
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-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
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
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Jan 1, 2020
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.