Skip to content
This repository has been archived by the owner on Sep 30, 2020. It is now read-only.

[wip] Kubelet TLS bootstrapping #414

Closed
wants to merge 2 commits into from
Closed

[wip] Kubelet TLS bootstrapping #414

wants to merge 2 commits into from

Conversation

danielfm
Copy link
Contributor

@danielfm danielfm commented Mar 15, 2017

This PR introduces an experimental feature for provisioning TLS certificates for worker nodes via the also experimental certificate signing requests workflow. Since this workflow is used by kube-adm, I think we can rest assured it won't be dropped anytime soon.

If RBAC is enabled in cluster.yaml, I also made sure to set up the appropriate cluster role bindings so that the token used for sending the certificate signing requests can only make requests related to certificate provisioning, as instructed by the official documentation.

The work is not complete, though. For now, I hard-coded the access token used in the bootstrap process, so I need some pointers regarding how to implement this properly. I think we have two options here:

  1. Generate a token file, i.e. ./credentials/csr-token, and handle it the same way we do with certificates, which is generating/encrypting when running kube-aws render credentials and kube-aws up, and handling the decryption in userdata templates
  2. Take a more generic approach by allowing the cluster administrator to specify a CSV token file, i.e. ./credentials/tokens.csv, (we can even generate a minimal one in kube-aws render credentials) in which he/she could add as many entries as needed alongside the one needed by the TLS bootstrap...
<csr-token>,kubelet-bootstrap,10001,"system:kubelet-bootstrap"
<some-token>,<username>,<id>,<groups>

...and have it encrypted/decrypted the usual way.

The first option is more to-the-point, but IMO the second option has the nice advantage of allowing the cluster administrator to manage other tokens for other purposes, perhaps even as the main authentication strategy for the cluster (e.g. by issuing tokens for other users and controlling who does what via RBAC).

Could you guys share your opinions on this so I can proceed?

Closes #406.

@codecov-io
Copy link

Codecov Report

Merging #414 into master will increase coverage by 0.18%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #414      +/-   ##
==========================================
+ Coverage   38.53%   38.71%   +0.18%     
==========================================
  Files          30       30              
  Lines        2351     2358       +7     
==========================================
+ Hits          906      913       +7     
  Misses       1324     1324              
  Partials      121      121
Impacted Files Coverage Δ
core/controlplane/config/tls_config.go 71.59% <100%> (+0.21%)
core/controlplane/config/config.go 65.7% <100%> (+0.27%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cdf0ef7...bdae020. Read the comment docs.

@mumoshu
Copy link
Contributor

mumoshu commented Mar 15, 2017

@danielfm Thanks for the great work!
I prefer the second option.

However, if a token file is subject to change by e.g. deletion of an user, how should we manage updates to a token file?

AFAIK, we have to restart every apiserver to apply changes in a token file.
If we embed ./credentials/tokens.csv to cloud-config-controller, probably kube-aws update replaces controller nodes one by one with new csv file to reflect changes to a token file without down time?

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 15, 2017
@danielfm
Copy link
Contributor Author

Hey @mumoshu,

However, if a token file is subject to change by e.g. deletion of an user, how should we manage updates to a token file?

AFAIK, we have to restart every apiserver to apply changes in a token file.
If we embed ./credentials/tokens.csv to cloud-config-controller, probably kube-aws update replaces controller nodes one by one with new csv file to reflect changes to a token file without down time?

Yes, that's how I've been doing this so far.

Although I'm sure there are better ways for granting and managing access to a Kubernetes cluster "at scale" (i.e. LDAP via dex), using a token file is simple enough for those with a more static set of tokens.

Since this is not currently supported by kube-aws, what I've been doing so far in my setup is to manually embed the encrypted token file in cloud-config-controller, decrypting it along the other encrypted strings when provisioning the instance, and providing the --token-auth-file flag to the apiserver.

@mumoshu
Copy link
Contributor

mumoshu commented Mar 16, 2017

Hi @danielfm, thanks for the info.

I agree with you about token files.
I have been dreamed of adding a support for dex into kube-aws but haven't had time to work on it.
In the meantime, the support for token files would be useful to many people.

Since this is not currently supported by kube-aws, what I've been doing so far in my setup is to manually embed the encrypted token file in cloud-config-controller, decrypting it along the other encrypted strings when provisioning the instance, and providing the --token-auth-file flag to the apiserver.

Is this something you can introduce to kube-aws as the second option we've talked above?
If so, I'd greatly appreciate it 👍

@danielfm
Copy link
Contributor Author

danielfm commented Mar 16, 2017

@mumoshu can I add the token file support as a non-experimental feature? Does this make sense to you?

In order to make things easier for the reviewers, I think it's best I send another PR just for that, instead of doing everything in the same PR. Then, once kube-aws supports token files, I can go back to this PR and implement the TLS provisioning on top of that.

@mumoshu
Copy link
Contributor

mumoshu commented Mar 16, 2017 via email

@danielfm danielfm mentioned this pull request Mar 17, 2017
4 tasks
mumoshu pushed a commit that referenced this pull request Mar 20, 2017
Adds support for authentication via static token files, as documented [here](https://kubernetes.io/docs/admin/authentication/#static-token-file).

The token file is a csv file with a minimum of 3 columns: token, user name, user uid, followed by optional group names. Note, if you have more than one group the column must be double quoted.

Authentication via static token files, although not very flexible, is a pretty simple solution for users getting started with Kubernetes and/or want to have some mechanism to give access to different users, but do not want the complexity overhead of a more flexible solution, such as [dex](https://github.com/coreos/dex).

This change will also be the foundation which #414 will be laid on.

A more detailed change list follows:

* Renamed function

Since we'll be encrypting things that are not TLS certificates, it's
time to rename the function in order to avoid any confusion.

The function name suffix implied it created a file, which was not the
case.

* Updated test error message

* Renamed systemd unit to match name changes

* Defined a location in which to put the token auth file

The decryption of the token file should also happen provided it is put
at the correct location.

* Should only attempt to decrypt tokens if the auth token file exists

* Fixed typo

* Added tests

* Updated docs

* Fixed script

* Validate auth token file

Before generating the encrypted version of the auth token file, parse
it as a CSV and look for problems, such as comments and unmatching
number of columns.

If any problem is found, an error is raised, causing the cluster not
to be created.
@mumoshu mumoshu mentioned this pull request Mar 22, 2017
22 tasks
@danielfm
Copy link
Contributor Author

For some reason, my commits are not showing up here... maybe it has to do with the repository moving from the CoreOS org to the Kubernetes Incubator. 😅

@mumoshu
Copy link
Contributor

mumoshu commented Mar 23, 2017

Hey @danielfm, thanks for the PR added the token file support!
Is your commit https://github.com/danielfm/kube-aws/commit/38e0664ee6df41751f8f3283df4e296a7c67fc32? So am I...
Perhaps you need to recreate this PR, or just rebase to the laster master and force push?

@cknowles
Copy link
Contributor

Same problem for #394, rebase and force push does not seem to solve the problem.

@danielfm
Copy link
Contributor Author

@c-knowles I sent a message to the GitHub staff and they said they'll try to find out what happened, but no ETA was given. 🤷‍♂️

@danielfm
Copy link
Contributor Author

danielfm commented Mar 23, 2017

I'll try moving to a new PR to see if it solves the problem.

Moved to #449.

@danielfm danielfm closed this Mar 23, 2017
kylehodgetts pushed a commit to HotelsDotCom/kube-aws that referenced this pull request Mar 27, 2018
Adds support for authentication via static token files, as documented [here](https://kubernetes.io/docs/admin/authentication/#static-token-file).

The token file is a csv file with a minimum of 3 columns: token, user name, user uid, followed by optional group names. Note, if you have more than one group the column must be double quoted.

Authentication via static token files, although not very flexible, is a pretty simple solution for users getting started with Kubernetes and/or want to have some mechanism to give access to different users, but do not want the complexity overhead of a more flexible solution, such as [dex](https://github.com/coreos/dex).

This change will also be the foundation which kubernetes-retired#414 will be laid on.

A more detailed change list follows:

* Renamed function

Since we'll be encrypting things that are not TLS certificates, it's
time to rename the function in order to avoid any confusion.

The function name suffix implied it created a file, which was not the
case.

* Updated test error message

* Renamed systemd unit to match name changes

* Defined a location in which to put the token auth file

The decryption of the token file should also happen provided it is put
at the correct location.

* Should only attempt to decrypt tokens if the auth token file exists

* Fixed typo

* Added tests

* Updated docs

* Fixed script

* Validate auth token file

Before generating the encrypted version of the auth token file, parse
it as a CSV and look for problems, such as comments and unmatching
number of columns.

If any problem is found, an error is raised, causing the cluster not
to be created.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants