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: add support for ECDSA keys #76390

Merged
merged 1 commit into from Apr 27, 2019

Conversation

@rojkov
Copy link
Member

commented Apr 10, 2019

What type of PR is this?
/kind feature

What this PR does / why we need it:
Currently kubeadm generates or accepts pre-generated RSA keys only. This PR lets kubeadm accept aslo pre-generated ECDSA keys and cerificates.

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

Does this PR introduce a user-facing change?:

kubeadm: add optional ECDSA support.
kubeadm still generates RSA keys when deploying a node, but also accepts ECDSA
keys if they exist already in the directory specified in --cert-dir option.
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

Hi @rojkov. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@bart0sh

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

/ok-to-test

@neolit123

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

kubeadm still generates RSA keys when deploying a node, but also accepts ECDSA
keys if they exist already in the directory specified in --cert-dir option.

i would change the release note in the lines of kubeadm: add optional ECDSA support. <further details>

@neolit123

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

@neolit123

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

thanks for the PR @rojkov
i will try have a closer look in the next following days.

@rojkov

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

i would change the release note in the lines of kubeadm: add optional ECDSA support. <further details>

Sure, updated the note.

Also I guess the current code can be simplified a bit since duplicated functionality exists in k8s.io/client-go/util (e.g. certificate storage can be reused). Then I wonder if we want to add an option to generate ECDSA keys. Or to take client-go's route and to generate ECDSA keys only instead of RSA ones as they consume less bandwidth providing the same level of security.

@rojkov rojkov force-pushed the rojkov:ecdsa-v2 branch 2 times, most recently from ba0eef7 to 5247fe6 Apr 11, 2019

@rosti
Copy link
Member

left a comment

Thanks @rojkov !
Looks OK on the first pass, though I am not an expert in this field.


expectedKeyPEM, err := keyutil.MarshalPrivateKeyToPEM(test.expectedKey)
if err != nil {
t.Fatal("Failed to marshal expected private key")

This comment has been minimized.

Copy link
@rosti

rosti Apr 11, 2019

Member

Dump err here?

This comment has been minimized.

Copy link
@rojkov

rojkov Apr 12, 2019

Author Member

Thanks! Dumping the error now.

@detiber

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

Overall this change lgtm.

Also I guess the current code can be simplified a bit since duplicated functionality exists in k8s.io/client-go/util (e.g. certificate storage can be reused).

I do think reducing the duplication would be a good idea.

Then I wonder if we want to add an option to generate ECDSA keys. Or to take client-go's route and to generate ECDSA keys only instead of RSA ones as they consume less bandwidth providing the same level of security.

I'm not sure that I would be comfortable with switching to ECDSA-only without having some type of transition period where we first allow ECDSA as an option, then switching the default but still allowing RSA as an option, then finally going ECDSA-only. This should probably be done through config rather than a feature flag, since it would require inverting the logic at some point.

@rosti

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

We can start with a phase that generates ECDSA keys. Later on we can simply flip this with the RSA key generation phase.
But this needs a broader discussion and a KEP (I think).

@rojkov rojkov force-pushed the rojkov:ecdsa-v2 branch from 5247fe6 to cd9086c Apr 12, 2019

@rojkov

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

I'll try to come up with a KEP. Hopefully soon.

@rojkov

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

/test pull-kubernetes-e2e-gce-100-performance

@rojkov rojkov force-pushed the rojkov:ecdsa-v2 branch from cd9086c to 6fdaf6c Apr 23, 2019

@rojkov

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

I've created a provisional KEP to ignite discussion.

This PR doesn't depend on the KEP.

@rojkov

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

/retest

@rojkov rojkov referenced this pull request Apr 25, 2019
kubeadm: add support for ECDSA keys
kubeadm still generates RSA keys when deploying a node, but also
accepts ECDSA keys if they already exist pregenerated in the
directory specified in --cert-dir.

@rojkov rojkov force-pushed the rojkov:ecdsa-v2 branch from 6fdaf6c to d125f3b Apr 25, 2019

@rojkov

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

  • added test cases that generate and use ECDSA keys;
  • rebased to the latest master.
@bart0sh

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

/lgtm

@rojkov

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

/test pull-kubernetes-integration

@bart0sh

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

@fabriziopandini, @timothysc please approve
/assign @fabriziopandini
/assign @timothysc

@timothysc
Copy link
Member

left a comment

/approve

@timothysc

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

thx @rojkov

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

[APPROVALNOTIFIER] This PR is APPROVED

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

This comment has been minimized.

Copy link

commented Apr 26, 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.

@timothysc

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

/test pull-kubernetes-e2e-gce

@neolit123

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

LGTM
/test pull-kubernetes-e2e-kind

@neolit123

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

/test pull-kubernetes-e2e-gce

@fejta-bot

This comment has been minimized.

Copy link

commented Apr 27, 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 3148eb7 into kubernetes:master Apr 27, 2019

21 checks passed

cla/linuxfoundation rojkov 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-storage-slow Skipped.
pull-kubernetes-e2e-kind Job succeeded.
Details
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-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@luxas

This comment has been minimized.

Copy link
Member

commented May 8, 2019

Thanks!

@neolit123

This comment has been minimized.

Copy link
Member

commented Jun 5, 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.