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 certificateKey field to v1beta2 config #77012

Merged
merged 1 commit into from
May 3, 2019

Conversation

rosti
Copy link
Contributor

@rosti rosti commented Apr 24, 2019

What type of PR is this?
/kind feature

What this PR does / why we need it:

This change introduces config fields to the v1beta2 format, that allow certificate key to be specified in the config file. This certificate key is a hex encoded AES key, that is used to encrypt certificates and keys, needed for secondary control plane nodes to join. The same key is used for the decryption
during control plane join.

It is important to note, that this key is never uploaded to the cluster. It can only be specified on either command line or the config file.

The new fields can be used like so:

apiVersion: kubeadm.k8s.io/v1beta2
kind: InitConfiguration
certificateKey: "yourSecretHere"
apiVersion: kubeadm.k8s.io/v1beta2
kind: JoinConfiguration
controlPlane:
  certificateKey: "yourSecretHere"

Note 1: I thought about adding validation of the key (should be hex encoded AES key + probably verify minimal key length), but since this was not done for the command line flag, I perceive it as out of scope for this PR. Hence, only a TODO is added.

Note 2: Currently, the command line option does not override the config file option. Instead an error is printed if both are specified. I am open to change for this behavior.

Note 3: This is the variant where a single field is added to NodeRegistrationOptions. Another possibility is to add certificateEncryptionKey to InitConfiguration and certificateDecryptionKey to JoinControlPlane. This may be a little bit more intuitive for users. I have no strong opinion, I can easily switch ways if needed.
Thanks to feedback from @mauilion and @joshrosso, I switched the PR to the change suggested in Note 3.

Which issue(s) this PR fixes:

Refs:

Special notes for your reviewer:

Please, review only the top commit. Depends on #76710

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm
/priority important-soon
/assign @fabriziopandini
/assign @timothysc
/assign @neolit123
/assign @yagonobre

Does this PR introduce a user-facing change?:

kubeadm: Add ability to specify certificate encryption and decryption key for the upload/download certificates phases as part of the new v1beta2 kubeadm config format.

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Apr 24, 2019
@k8s-ci-robot k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. area/kubeadm priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 24, 2019
Copy link
Member

@yagonobre yagonobre left a comment

Choose a reason for hiding this comment

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

Thanks @rosti, LGTM.

Note 1: +1
Note 2: +1
Note 3: Not a strong opinion, but I feel that put it on InitConfiguration/JoinControlPlane is better for ux, I also like the idea of keep the certificateEncryptionKey in the same "config" of tokens.

@@ -105,6 +105,7 @@ func ValidateNodeRegistrationOptions(nro *kubeadm.NodeRegistrationOptions, fldPa
}
allErrs = append(allErrs, ValidateSocketPath(nro.CRISocket, fldPath.Child("criSocket"))...)
// TODO: Maybe validate .Taints as well in the future using something like validateNodeTaints() in pkg/apis/core/validation
// TODO: Maybe validate that .CertificateKey is a valid hex encoded AES key
Copy link
Member

Choose a reason for hiding this comment

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

👍

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 25, 2019
@rosti
Copy link
Contributor Author

rosti commented Apr 25, 2019

An example between the two different UX approaches in practice (Note 3).
The nodeRegistration one:

apiVersion: kubeadm.k8s.io/v1beta2
kind: InitConfiguration
nodeRegistration:
  certificateKey: "yourSecretHere"
---
apiVersion: kubeadm.k8s.io/v1beta2
kind: JoinConfiguration
nodeRegistration:
  certificateKey: "yourSecretHere"

vs the InitConfiguration & JoinControlPlane one:

apiVersion: kubeadm.k8s.io/v1beta2
kind: InitConfiguration
certificateEncryptionKey: "yourSecretHere"
---
apiVersion: kubeadm.k8s.io/v1beta2
kind: JoinConfiguration
controlPlane:
  certificateDecryptionKey: "yourSecretHere"

@mauilion @joshrosso I would really appreciate an overview from UX perspective on this one.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 25, 2019
@neolit123
Copy link
Member

^ cc @seh

@neolit123
Copy link
Member

i'm starting to realize that the kubeadm configuration is a bit weird WRT to bootstrap tokens:

  • for InitConfiguration we have a root field for the tokens.
  • for Join we have Discovery that holds the tokens.
  • there is no common place for the tokens between the two configurations.

the same problem happens now for the certs key. where should we put it?
the certs key in terms of CLI UX is something quite similar to the tokens....users just feed them on the command line on Join.

A) so if we want to follow the existing patterns we should have it as a root field of Init and have it as a field under Discovery.
B) but then it also fits under JoinControlPlane because it's only related to that scenario and have it as a root element in Init.
C) NodeRegistrationOptions is something shared between the two configurations, but the scope of this struct needs better definition...

my vote goes for A), but i don't like any of the options.
unfortunately we are now in beta, so we cannot make major changes.

@rosti
Copy link
Contributor Author

rosti commented Apr 25, 2019

@neolit123 I think it would be more confusing if we put tokens in a central place. What if you have multiple ones, which one are you going to use for the discovery process? What if you don't want to use token for discovery (use kubeconfig instead)?
Having tokens in a single place is going to produce a lot of strangely looking YAML options, that are in the "periphery" of tokens.
So I think, that the problem is still with the certificate key only.

@mauilion
Copy link

From a UX perspective I am in favor of

apiVersion: kubeadm.k8s.io/v1beta2
kind: InitConfiguration
certificateEncryptionKey: "yourSecretHere"
---
apiVersion: kubeadm.k8s.io/v1beta2
kind: JoinConfiguration
controlPlane:
  certificateDecryptionKey: "yourSecretHere"

Mainly because this seems a better separation of concerns. Ideally I would not want this key to end up on worker nodes even by accident. Writing it to the node registration struct will likely mean that the "common" join config will be made available to all nodes.

@joshrosso
Copy link

I also prefer controlPlane.certificateDecryptionKey. I would like kubeadm join to fail if controlPlane is present when issuing a join for a non-control plane node.

The benefit of certificateDecriptionKey/certificateEncryptionKey over certificateKey is unclear to me. I suppose it is more descriptive, but I actually prefer certificateKey as a consistent attribute. I do not feel too strongly about this though.

So my suggestion is:

apiVersion: kubeadm.k8s.io/v1beta2
kind: InitConfiguration
certificateKey: "yourSecretHere"
---
apiVersion: kubeadm.k8s.io/v1beta2
kind: JoinConfiguration
controlPlane:
  certificateKey: "yourSecretHere"

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 30, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rosti

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 30, 2019
@rosti
Copy link
Contributor Author

rosti commented Apr 30, 2019

Thank you @mauilion and @joshrosso for the UX review!
I rebased and updated the PR with the proposed change (currently the "long" names are used for the options, but that can change too).
So this one is ready for another review pass on it.


// CertificateEncryptionKey sets the key with which certificates and keys are encrypted prior to being uploaded in
// a secret in the cluster during the uploadcerts init phase.
CertificateEncryptionKey string
Copy link
Member

Choose a reason for hiding this comment

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

+1 for going with a common key name between join/init
CertificateKey

Copy link
Member

Choose a reason for hiding this comment

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

+1 from me as well on the shared name, having separate naming makes it seem as if we are possibly using public key encryption rather than symmetric encryption

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My initial thought on that was, that users may get confused easily and questions like "I placed my key under JoinConfiguration, why is upload failing?" or "I placed my key in InitConfuguration, why is my download failing?" may arise.
But then, since this is the prevailing opinion, then I'll shorten the names.

@rosti
Copy link
Contributor Author

rosti commented May 2, 2019

PR updated again, moved to certificateKey names.

This change introduces config fields to the v1beta2 format, that allow
certificate key to be specified in the config file. This certificate key is a
hex encoded AES key, that is used to encrypt certificates and keys, needed for
secondary control plane nodes to join. The same key is used for the decryption
during control plane join.
It is important to note, that this key is never uploaded to the cluster. It can
only be specified on either command line or the config file.
The new fields can be used like so:

---
apiVersion: kubeadm.k8s.io/v1beta2
kind: InitConfiguration
certificateKey: "yourSecretHere"
---
apiVersion: kubeadm.k8s.io/v1beta2
kind: JoinConfiguration
controlPlane:
  certificateKey: "yourSecretHere"
---

Signed-off-by: Rostislav M. Georgiev <[email protected]>
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

the change seems good to me!

/lgtm
/hold
hold until EOD today on lazy consensus.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 2, 2019
@timothysc
Copy link
Member

lgtm

Copy link
Member

@yagonobre yagonobre left a comment

Choose a reason for hiding this comment

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

/lgtm

@neolit123
Copy link
Member

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 3, 2019
@fejta-bot
Copy link

/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 f29138c into kubernetes:master May 3, 2019
@rosti rosti mentioned this pull request Jun 10, 2019
4 tasks
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/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants