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

Feature/kubeadm 594 etcd TLS on init/upgrade #57415

Merged
merged 4 commits into from Feb 25, 2018

Conversation

@stealthybox
Copy link
Contributor

stealthybox commented Dec 19, 2017

What this PR does / why we need it:
On kubeadm init/kubeadm upgrade, this PR generates certificates for securing local etcd:

  • etcd serving cert
  • etcd peer cert
  • apiserver etcd client cert

Flags and hostMounts are added to the etcd and apiserver static-pods to load these certs.
For connections to etcd, https is now used in favor of http and tests have been added/updated.

Etcd only listens on localhost, so the serving cert SAN defaults to DNS:localhost,IP:127.0.0.1.
The etcd peer cert has SANs for <hostname>,<api-advertise-address>, but is unused.

New kubeadm config options, Etcd.ServerCertSANs and Etcd.PeerCertSANs, are used for user additions to the default certificate SANs for the etcd server and peer certs.

This feature continues to utilize the existence of MasterConfiguration.Etcd.Endpoints as a feature gate for external-etcd.
If the user passes flags to configure Etcd.{CAFile,CertFile,KeyFile} but they omit Endpoints, these flags will be unused, and a warning is printed.

New phase commands:

kubeadm alpha phase certs etcd-server
kubeadm alpha phase certs etcd-peer
kubeadm alpha phase certs apiserver-etcd-client 

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

Special notes for your reviewer:

on the master

these should fail:

curl localhost:2379/v2/keys  # no output
curl --cacert /etc/kubernetes/pki/ca.crt https://localhost:2379/v2/keys  # handshake error

these should succeed:

cd /etc/kubernetes/pki
curl --cacert ca.crt --cert apiserver-etcd-client.crt --key apiserver-etcd-client.key https://localhost:2379/v2/keys

Release note:

On cluster provision or upgrade, kubeadm now generates certs and secures all connections to the etcd static-pod with mTLS.
@stealthybox

This comment has been minimized.

Copy link
Contributor Author

stealthybox commented Dec 19, 2017

/assign @luxas

@stealthybox stealthybox force-pushed the stealthybox:feature/kubeadm_594-etcd_tls branch from ef708b6 to baf3a14 Dec 20, 2017

@luxas luxas added this to the v1.10 milestone Dec 22, 2017

@luxas

This comment has been minimized.

Copy link
Member

luxas commented Dec 22, 2017

/assign @kad @fabriziopandini
for initial review

@@ -36,6 +36,12 @@ package certs
- apiserver.key
- apiserver-kubelet-client.crt
- apiserver-kubelet-client.key
- etcd.crt

This comment has been minimized.

@kargakis

kargakis Dec 28, 2017

Member

You also need to update INPUTS to include EtcdCertSANs.

This comment has been minimized.

@stealthybox

stealthybox Dec 29, 2017

Author Contributor

Thanks. I've updated the doc-strings for both the SAN fields.

altNames.IPs = append(altNames.IPs, ip)
} else if len(validation.IsDNS1123Subdomain(altname)) == 0 {
altNames.DNSNames = append(altNames.DNSNames, altname)
}

This comment has been minimized.

@kargakis

kargakis Dec 28, 2017

Member

ux enhancement: log when this is not a valid SAN

altNames.IPs = append(altNames.IPs, ip)
} else if len(validation.IsDNS1123Subdomain(altname)) == 0 {
altNames.DNSNames = append(altNames.DNSNames, altname)
}

This comment has been minimized.

@kargakis

kargakis Dec 28, 2017

Member

same comment as above

@stealthybox stealthybox force-pushed the stealthybox:feature/kubeadm_594-etcd_tls branch from 121b135 to ed08db6 Dec 29, 2017

@stealthybox

This comment has been minimized.

Copy link
Contributor Author

stealthybox commented Dec 29, 2017

@kargakis, thanks for the suggestions -- changes are ready for another review.
The SAN warnings look like this:

[certificates] WARNING: 'hello-&&' was not added to the 'apiserver.crt' SAN, because it is not a valid IP or RFC-1123 compliant DNS entry
[certificates] WARNING: '129.2.3.L' was not added to the 'apiserver.crt' SAN, because it is not a valid IP or RFC-1123 compliant DNS entry

I implemented them for the API Server as well.

@xiangpengzhao
Copy link
Member

xiangpengzhao left a comment

some nits.

Another concern is that we may also want to backup etcd cert and key files as what is done for apiserver.
FYI:
#55998
kubernetes/kubeadm#548

certAndKeyDir := kubeadmapiext.DefaultCertificatesDir
shouldBackup, err := shouldBackupAPIServerCertAndKey(certAndKeyDir, newK8sVer)
// Don't fail the upgrade phase if failing to determine to backup kube-apiserver cert and key.
if err != nil {
fmt.Printf("[postupgrade] WARNING: failed to determine to backup kube-apiserver cert and key: %v", err)
} else if shouldBackup {
// Don't fail the upgrade phase if failing to backup kube-apiserver cert and key.
if err := backupAPIServerCertAndKey(certAndKeyDir); err != nil {
fmt.Printf("[postupgrade] WARNING: failed to backup kube-apiserver cert and key: %v", err)
}
if err := certsphase.CreateAPIServerCertAndKeyFiles(cfg); err != nil {
errs = append(errs, err)
}
}

// It assumes the cluster CA certificate and key file exist in the CertificatesDir
func CreateEtcdCertAndKeyFiles(cfg *kubeadmapi.MasterConfiguration) error {

caCert, caKey, err := loadCertificateAuthorithy(cfg.CertificatesDir, kubeadmconstants.CACertAndKeyBaseName)

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Jan 9, 2018

Member

s/loadCertificateAuthorithy/loadCertificateAuthority ?

This comment has been minimized.

@stealthybox

stealthybox Jan 17, 2018

Author Contributor

This requires a refactor -- I'll try it out as an additional commit.

This comment has been minimized.

@stealthybox

stealthybox Feb 19, 2018

Author Contributor

Pushed up a change for this :)
Easy because it's a private function

@@ -230,6 +305,64 @@ func NewAPIServerKubeletClientCertAndKey(caCert *x509.Certificate, caKey *rsa.Pr
return apiClientCert, apiClientKey, nil
}

// NewEtcdCertAndKey generate CA certificate for etcd, signed by the given CA.

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Jan 9, 2018

Member

s/generate/generates

This comment has been minimized.

@stealthybox

stealthybox Feb 19, 2018

Author Contributor

not going to do this in this PR -- all of the functions have similar phrasing for their docstrings

return etcdCert, etcdKey, nil
}

// NewEtcdPeerCertAndKey generate CA certificate for etcd peering, signed by the given CA.

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Jan 9, 2018

Member

s/generate/generates

This comment has been minimized.

@stealthybox

stealthybox Feb 19, 2018

Author Contributor

not going to do this in this PR -- all of the functions have similar phrasing for their docstrings

return etcdPeerCert, etcdPeerKey, nil
}

// NewAPIServerEtcdClientCertAndKey generate CA certificate for the apiservers to connect to etcd securely, signed by the given CA.

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Jan 9, 2018

Member

s/generate/generates

This comment has been minimized.

@stealthybox

stealthybox Feb 19, 2018

Author Contributor

not going to do this in this PR -- all of the functions have similar phrasing for their docstrings

@timothysc

This comment has been minimized.

Copy link
Member

timothysc commented Jan 9, 2018

@k8s-ci-robot k8s-ci-robot requested a review from mattmoyer Jan 9, 2018

@xiangpengzhao

This comment has been minimized.

Copy link
Member

xiangpengzhao commented Jan 9, 2018

Also, as is mentioned in https://docs.google.com/document/d/19eei3oly2BqlgWFcqMtrp9_Ktlb3cNlSN8ZLntSt76I/edit#

During the upgrade case from 1.9 → 1.10 we’d need to generate the new certs, but that can easily be achieved

@stealthybox

This comment has been minimized.

Copy link
Contributor Author

stealthybox commented Jan 9, 2018

@xiangpengzhao With this PR, the certs are generated on upgrade when they don't exist. It doesn't discriminate between versions. Is this the behavior we want?

This cert renewal feature is something we didn't think about. We can implement that.
I suppose you backup the certs and re-use the CA to generate the new certs?
How long is the CA good for? I haven't read the code for this.

@timothysc

This comment has been minimized.

Copy link
Member

timothysc commented Jan 9, 2018

I'll dig through pretty soon, but as a general rule, please squash commits.

@xiangpengzhao

This comment has been minimized.

Copy link
Member

xiangpengzhao commented Jan 12, 2018

With this PR, the certs are generated on upgrade when they don't exist. It doesn't discriminate between versions. Is this the behavior we want?

@stealthybox yeah correct, this PR will cover this case "During the upgrade case from 1.9 → 1.10 we’d need to generate the new certs, but that can easily be achieved"

This cert renewal feature is something we didn't think about. We can implement that.

We can implement it in a follow-up PR.

I suppose you backup the certs and re-use the CA to generate the new certs?

correct.

How long is the CA good for? I haven't read the code for this.

IMO, one year would be ok (it's the default value?). And so all the certs can have the same validity duration.

@jamiehannaford
Copy link
Member

jamiehannaford left a comment

made a first pass at review

@@ -52,6 +52,10 @@ type MasterConfiguration struct {

// APIServerCertSANs sets extra Subject Alternative Names for the API Server signing cert
APIServerCertSANs []string

// EtcdCertSANs sets extra Subject Alternative Names for the etcd server and peer signing certs
EtcdCertSANs []string

This comment has been minimized.

@jamiehannaford

jamiehannaford Jan 12, 2018

Member

will there ever be a case where SANs for peer and server will be different?

This comment has been minimized.

@stealthybox

stealthybox Feb 19, 2018

Author Contributor

It's possible to have them listen on different interfaces.
I guessed it would be okay to just have a union of all of the needed SANs for peers and the server, but I could be wrong.

We could add another config for this.
I'm thinking it would be best to just add EtcdPeerCertSANs []string and not do anything fancy.
This makes the config more verbose, but it's flexible.

Do you think this is correct?

This comment has been minimized.

@stealthybox

stealthybox Feb 20, 2018

Author Contributor

Renamed this to EtcdServerCertSANs and added EtcdPeerCertSANs

etcdCertLongDesc = fmt.Sprintf(normalizer.LongDesc(`
Generates the etcd serving certificate and key and saves them into %s and %s files.
The certificate includes default subject alternative names and additional sans eventually provided by the user;

This comment has been minimized.

@jamiehannaford

This comment has been minimized.

@stealthybox

stealthybox Feb 20, 2018

Author Contributor

fixed on all 3 (api, etcd, etcdPeer) -- thanks

@@ -157,6 +182,24 @@ func getCertsSubCommands(defaultKubernetesVersion string) []*cobra.Command {
long: apiServerKubeletCertLongDesc,
cmdFunc: certsphase.CreateAPIServerKubeletClientCertAndKeyFiles,
},
{
use: "etcd",

This comment has been minimized.

@jamiehannaford

jamiehannaford Jan 12, 2018

Member

Should we make this more verbose? e.g. etcd-serving-cert

This comment has been minimized.

@stealthybox

stealthybox Feb 14, 2018

Author Contributor

I agree we could make this clearer since there are so many certs.
I'll try etcd-server -- WDYT?

This comment has been minimized.

@stealthybox

stealthybox Feb 19, 2018

Author Contributor

updated this to etcd-server

cmdFunc: certsphase.CreateEtcdCertAndKeyFiles,
},
{
use: "etcd-peer",

This comment has been minimized.

@jamiehannaford

jamiehannaford Jan 12, 2018

Member

etcd-peer-cert?

This comment has been minimized.

@stealthybox

stealthybox Feb 14, 2018

Author Contributor

This is already nested under the certs phase. (kubeadm alpha phase certs etcd-peer)

cmdFunc: certsphase.CreateEtcdPeerCertAndKeyFiles,
},
{
use: "apiserver-etcd-client",

This comment has been minimized.

@jamiehannaford

jamiehannaford Jan 12, 2018

Member

etcd-client-cert?

This comment has been minimized.

@stealthybox

stealthybox Feb 14, 2018

Author Contributor

I made this match phase certs apiserver-kubelet-client.
This naming emphasizes it's an apiserver cert.

Do we want to flip this around?
We can but it won't match the other example of this relationship.
Phases are alpha, so we can also change the apiserver-kubelet-client cert command.

)
}

// CreateEtcdCertAndKeyFiles create a new certificate and key file for etcd.

This comment has been minimized.

@jamiehannaford

This comment has been minimized.

@stealthybox

stealthybox Feb 19, 2018

Author Contributor

I can create another PR to clean all of these up -- I'd prefer the docstrings to be uniform

}

return writeCertificateFilesIfNotExist(
cfg.CertificatesDir,

This comment has been minimized.

@jamiehannaford

jamiehannaford Jan 12, 2018

Member

I think I prefer nesting all etcd certs into a new dir, e.g. cfg.CertificatesDir + "/etcd"

This comment has been minimized.

@timothysc

timothysc Jan 17, 2018

Member

agreed.

This comment has been minimized.

@stealthybox

stealthybox Jan 17, 2018

Author Contributor

I'll take a look at this -- need to consider path joining and tests

This comment has been minimized.

@stealthybox

stealthybox Feb 20, 2018

Author Contributor

730e91c addresses this by hardcoding etcd/ as a prefix in kubeadmconstants.
I found this to be the cleanest implementation.
We're already hardcoding forward-slashes for defaults such as /etc/kubernetes/pki.

I had concerns about adding a subdirectory prefixed to a fileName, but it turns out to be fine:

  • certutils.WriteKey() does a MkdirAll(filepath.Dir(joinedPath), ...)
  • testutils.AssertFileExists() does a filepath.Join(dirName, fileName)

Not hardcoding this value requires us to add filepath.Join(cfg.CerticatesDir, kubeadmconstants.Etcd, ... multiple times in 4 files:

diff --git a/cmd/kubeadm/app/phases/certs/certs.go b/cmd/kubeadm/app/phases/certs/certs.go
index 86583f4cf5..c377a9e2b1 100644
--- a/cmd/kubeadm/app/phases/certs/certs.go
+++ b/cmd/kubeadm/app/phases/certs/certs.go
@@ -138,7 +138,7 @@ func CreateEtcdServerCertAndKeyFiles(cfg *kubeadmapi.MasterConfiguration) error
 	}
 
 	return writeCertificateFilesIfNotExist(
-		cfg.CertificatesDir,
+		filepath.Join(cfg.CertificatesDir, kubeadmconstants.Etcd),
 		kubeadmconstants.EtcdServerCertAndKeyBaseName,
 		caCert,
 		etcdServerCert,
@@ -162,7 +162,7 @@ func CreateEtcdPeerCertAndKeyFiles(cfg *kubeadmapi.MasterConfiguration) error {
 	}
 
 	return writeCertificateFilesIfNotExist(
-		cfg.CertificatesDir,
+		filepath.Join(cfg.CertificatesDir, kubeadmconstants.Etcd),
 		kubeadmconstants.EtcdPeerCertAndKeyBaseName,
 		caCert,
 		etcdPeerCert,
diff --git a/cmd/kubeadm/app/phases/certs/certs_test.go b/cmd/kubeadm/app/phases/certs/certs_test.go
index fbd77208ad..343c8c61f8 100644
--- a/cmd/kubeadm/app/phases/certs/certs_test.go
+++ b/cmd/kubeadm/app/phases/certs/certs_test.go
@@ -579,6 +579,11 @@ func TestCreateCertificateFilesMethods(t *testing.T) {
 				kubeadmconstants.CACertName, kubeadmconstants.CAKeyName,
 				kubeadmconstants.APIServerCertName, kubeadmconstants.APIServerKeyName,
 				kubeadmconstants.APIServerKubeletClientCertName, kubeadmconstants.APIServerKubeletClientKeyName,
+				filepath.Join(kubeadmconstants.Etcd, kubeadmconstants.EtcdServerCertName),
+				filepath.Join(kubeadmconstants.Etcd, kubeadmconstants.EtcdServerKeyName),
+				filepath.Join(kubeadmconstants.Etcd, kubeadmconstants.EtcdPeerCertName),
+				filepath.Join(kubeadmconstants.Etcd, kubeadmconstants.EtcdPeerKeyName),
+				kubeadmconstants.APIServerEtcdClientCertName, kubeadmconstants.APIServerEtcdClientKeyName,
 				kubeadmconstants.ServiceAccountPrivateKeyName, kubeadmconstants.ServiceAccountPublicKeyName,
 				kubeadmconstants.FrontProxyCACertName, kubeadmconstants.FrontProxyCAKeyName,
 				kubeadmconstants.FrontProxyClientCertName, kubeadmconstants.FrontProxyClientKeyName,
@@ -599,14 +604,20 @@ func TestCreateCertificateFilesMethods(t *testing.T) {
 			expectedFiles: []string{kubeadmconstants.APIServerKubeletClientCertName, kubeadmconstants.APIServerKubeletClientKeyName},
 		},
 		{
-			setupFunc:     CreateCACertAndKeyfiles,
-			createFunc:    CreateEtcdServerCertAndKeyFiles,
-			expectedFiles: []string{kubeadmconstants.EtcdServerCertName, kubeadmconstants.EtcdServerKeyName},
+			setupFunc:  CreateCACertAndKeyfiles,
+			createFunc: CreateEtcdServerCertAndKeyFiles,
+			expectedFiles: []string{
+				filepath.Join(kubeadmconstants.Etcd, kubeadmconstants.EtcdServerCertName),
+				filepath.Join(kubeadmconstants.Etcd, kubeadmconstants.EtcdServerKeyName),
+			},
 		},
 		{
-			setupFunc:     CreateCACertAndKeyfiles,
-			createFunc:    CreateEtcdPeerCertAndKeyFiles,
-			expectedFiles: []string{kubeadmconstants.EtcdPeerCertName, kubeadmconstants.EtcdPeerKeyName},
+			setupFunc:  CreateCACertAndKeyfiles,
+			createFunc: CreateEtcdPeerCertAndKeyFiles,
+			expectedFiles: []string{
+				filepath.Join(kubeadmconstants.Etcd, kubeadmconstants.EtcdPeerCertName),
+				filepath.Join(kubeadmconstants.Etcd, kubeadmconstants.EtcdPeerKeyName),
+			},
 		},
 		{
 			setupFunc:     CreateCACertAndKeyfiles,
diff --git a/cmd/kubeadm/app/phases/certs/doc.go b/cmd/kubeadm/app/phases/certs/doc.go
index 7e66cd344c..641907e6c8 100644
--- a/cmd/kubeadm/app/phases/certs/doc.go
+++ b/cmd/kubeadm/app/phases/certs/doc.go
@@ -38,12 +38,12 @@ package certs
 		 - apiserver.key
 		 - apiserver-kubelet-client.crt
 		 - apiserver-kubelet-client.key
-		 - etcd-server.crt
-		 - etcd-server.key
-		 - etcd-peer.crt
-		 - etcd-peer.key
 		 - apiserver-etcd-client.crt
 		 - apiserver-etcd-client.key
+		 - etcd/server.crt
+		 - etcd/server.key
+		 - etcd/peer.crt
+		 - etcd/peer.key
 		 - sa.pub
 		 - sa.key
 		 - front-proxy-ca.crt
diff --git a/cmd/kubeadm/app/phases/etcd/local.go b/cmd/kubeadm/app/phases/etcd/local.go
index d25503521f..4a78c8b7c2 100644
--- a/cmd/kubeadm/app/phases/etcd/local.go
+++ b/cmd/kubeadm/app/phases/etcd/local.go
@@ -75,12 +75,12 @@ func getEtcdCommand(cfg *kubeadmapi.MasterConfiguration) []string {
 		"listen-client-urls":    "https://127.0.0.1:2379",
 		"advertise-client-urls": "https://127.0.0.1:2379",
 		"data-dir":              cfg.Etcd.DataDir,
-		"cert-file":             filepath.Join(cfg.CertificatesDir, kubeadmconstants.EtcdServerCertName),
-		"key-file":              filepath.Join(cfg.CertificatesDir, kubeadmconstants.EtcdServerKeyName),
+		"cert-file":             filepath.Join(cfg.CertificatesDir, kubeadmconstants.Etcd, kubeadmconstants.EtcdServerCertName),
+		"key-file":              filepath.Join(cfg.CertificatesDir, kubeadmconstants.Etcd, kubeadmconstants.EtcdServerKeyName),
 		"trusted-ca-file":       filepath.Join(cfg.CertificatesDir, kubeadmconstants.CACertName),
 		"client-cert-auth":      "true",
-		"peer-cert-file":        filepath.Join(cfg.CertificatesDir, kubeadmconstants.EtcdPeerCertName),
-		"peer-key-file":         filepath.Join(cfg.CertificatesDir, kubeadmconstants.EtcdPeerKeyName),
+		"peer-cert-file":        filepath.Join(cfg.CertificatesDir, kubeadmconstants.Etcd, kubeadmconstants.EtcdPeerCertName),
+		"peer-key-file":         filepath.Join(cfg.CertificatesDir, kubeadmconstants.Etcd, kubeadmconstants.EtcdPeerKeyName),
 		"peer-trusted-ca-file":  filepath.Join(cfg.CertificatesDir, kubeadmconstants.CACertName),
 		"peer-client-cert-auth": "true",
 	}
diff --git a/cmd/kubeadm/app/phases/etcd/local_test.go b/cmd/kubeadm/app/phases/etcd/local_test.go
index 920680e059..425a96416c 100644
--- a/cmd/kubeadm/app/phases/etcd/local_test.go
+++ b/cmd/kubeadm/app/phases/etcd/local_test.go
@@ -82,12 +82,12 @@ func TestGetEtcdCommand(t *testing.T) {
 				"--listen-client-urls=https://127.0.0.1:2379",
 				"--advertise-client-urls=https://127.0.0.1:2379",
 				"--data-dir=/var/lib/etcd",
-				"--cert-file=etcd-server.crt",
-				"--key-file=etcd-server.key",
+				"--cert-file=etcd/server.crt",
+				"--key-file=etcd/server.key",
 				"--trusted-ca-file=ca.crt",
 				"--client-cert-auth=true",
-				"--peer-cert-file=etcd-peer.crt",
-				"--peer-key-file=etcd-peer.key",
+				"--peer-cert-file=etcd/peer.crt",
+				"--peer-key-file=etcd/peer.key",
 				"--peer-trusted-ca-file=ca.crt",
 				"--peer-client-cert-auth=true",
 			},
@@ -107,12 +107,12 @@ func TestGetEtcdCommand(t *testing.T) {
 				"--listen-client-urls=https://10.0.1.10:2379",
 				"--advertise-client-urls=https://10.0.1.10:2379",
 				"--data-dir=/var/lib/etcd",
-				"--cert-file=etcd-server.crt",
-				"--key-file=etcd-server.key",
+				"--cert-file=etcd/server.crt",
+				"--key-file=etcd/server.key",
 				"--trusted-ca-file=ca.crt",
 				"--client-cert-auth=true",
-				"--peer-cert-file=etcd-peer.crt",
-				"--peer-key-file=etcd-peer.key",
+				"--peer-cert-file=etcd/peer.crt",
+				"--peer-key-file=etcd/peer.key",
 				"--peer-trusted-ca-file=ca.crt",
 				"--peer-client-cert-auth=true",
 			},
@@ -126,12 +126,12 @@ func TestGetEtcdCommand(t *testing.T) {
 				"--listen-client-urls=https://127.0.0.1:2379",
 				"--advertise-client-urls=https://127.0.0.1:2379",
 				"--data-dir=/etc/foo",
-				"--cert-file=etcd-server.crt",
-				"--key-file=etcd-server.key",
+				"--cert-file=etcd/server.crt",
+				"--key-file=etcd/server.key",
 				"--trusted-ca-file=ca.crt",
 				"--client-cert-auth=true",
-				"--peer-cert-file=etcd-peer.crt",
-				"--peer-key-file=etcd-peer.key",
+				"--peer-cert-file=etcd/peer.crt",
+				"--peer-key-file=etcd/peer.key",
 				"--peer-trusted-ca-file=ca.crt",
 				"--peer-client-cert-auth=true",
 			},
@@ -545,6 +678,63 @@ func getAltNames(cfg *kubeadmapi.MasterConfiguration) (*certutil.AltNames, error
altNames.IPs = append(altNames.IPs, ip)
} else if len(validation.IsDNS1123Subdomain(altname)) == 0 {
altNames.DNSNames = append(altNames.DNSNames, altname)
} else {
fmt.Printf("[certificates] WARNING: '%s' was not added to the '%s' SAN, because it is not a valid IP or RFC-1123 compliant DNS entry\n", altname, kubeadmconstants.APIServerCertName)

This comment has been minimized.

@jamiehannaford

jamiehannaford Jan 12, 2018

Member

nit: might be worth splitting over several lines

This comment has been minimized.

@stealthybox

stealthybox Feb 14, 2018

Author Contributor

I'll add line-breaks between the args -- I think this is what you mean 👍

This comment has been minimized.

@stealthybox

stealthybox Feb 19, 2018

Author Contributor

also done in fcf0fef -- thanks :)

} else if len(validation.IsDNS1123Subdomain(altname)) == 0 {
altNames.DNSNames = append(altNames.DNSNames, altname)
} else {
fmt.Printf("[certificates] WARNING: '%s' was not added to the '%s' SAN, because it is not a valid IP or RFC-1123 compliant DNS entry\n", altname, kubeadmconstants.EtcdCertName)

This comment has been minimized.

@jamiehannaford

jamiehannaford Jan 12, 2018

Member

let's define this error once

This comment has been minimized.

@stealthybox

stealthybox Feb 19, 2018

Author Contributor

thanks for the suggestion 👍 this is done (fcf0fef)

}

// adds additional SAN
for _, altname := range cfg.EtcdCertSANs {

This comment has been minimized.

@jamiehannaford

jamiehannaford Jan 12, 2018

Member

can we re-use this logic?

This comment has been minimized.

@stealthybox

stealthybox Feb 19, 2018

Author Contributor

implemented this as appendSANsToAltNames() in certs/pkiutil (fcf0fef)

@timothysc
Copy link
Member

timothysc left a comment

Address other minor changes, squash commits, then lgtm.

I debate if we want to shift-out etcd logic if we are going to create mgmt code. /cc @jamiehannaford

@@ -64,6 +64,33 @@ const (
// APIServerKubeletClientCertCommonName defines kubelet client certificate common name (CN)
APIServerKubeletClientCertCommonName = "kube-apiserver-kubelet-client"

// EtcdCertAndKeyBaseName defines etcd's server certificate and key base name

This comment has been minimized.

@timothysc

timothysc Jan 17, 2018

Member

We should follow names outlined here: https://coreos.com/etcd/docs/latest/op-guide/security.html , there are a couple of conflicts.

This comment has been minimized.

@stealthybox

stealthybox Feb 14, 2018

Author Contributor

I'll update the etcd-server cert like @jamiehannaford suggests.

For peers, I see:

  • this PR names the Peer Cert "etcd-peer.crt"
  • the CoreOS example names it "member1.crt"

I find this naming strange. It doesn't match the flags: --peer-cert-file, --peer-key-file
The term "member" is used loosely in the docs I've come across.

We have an ansible example in the kubernetes/contrib repo that uses the peer naming:
https://github.com/kubernetes/contrib/blob/master/ansible/roles/etcd/defaults/main.yaml#L21-L31

This comment has been minimized.

@stealthybox

stealthybox Feb 18, 2018

Author Contributor

This CoreOS example mentions member.crt in the description but uses infra0-peer.crt in the code:
https://github.com/coreos/etcd/blob/master/Documentation/op-guide/clustering.md#self-signed-certificates

This other CoreOS example just reuses the server crt as the peer crt:
https://github.com/coreos/etcd/blob/master/hack/tls-setup/Procfile

}

return writeCertificateFilesIfNotExist(
cfg.CertificatesDir,

This comment has been minimized.

@timothysc

timothysc Jan 17, 2018

Member

agreed.

@luxas
Copy link
Member

luxas left a comment

IMO, one year would be ok (it's the default value?)

The default CA validity for the k8s root CA is ten years sigh. The individual API Server certs are then valid for one year.

I debate if we want to shift-out etcd logic if we are going to create mgmt code

I don't think this is lifecycle code. I'm okay with keeping this cert generation cert generation and then including what's needed for etcd here as well. The methods are public so anyone can import anyway.

Good job so far @stealthybox!

AltNames: *altNames,
Usages: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
}
etcdCert, etcdKey, err := pkiutil.NewCertAndKey(caCert, caKey, config)

This comment has been minimized.

@luxas

luxas Feb 2, 2018

Member

NewCertAndKey is duplicated across all new methods... any chance these methods could just return certutil.Config and then let the caller run this method? Do you think that would make sense?

This comment has been minimized.

@stealthybox

stealthybox Feb 14, 2018

Author Contributor

I can look at this in another PR to refactor all of these methods.
These are basically copy-paste.

Changing the return type would probably warrant a rename and new docstrings, and these are all public.

// localhost is included in the SAN since this is the interface the etcd static pod listens on
// hostname and API.AdvertiseAddress are excluded since etcd does not listen on this interface by default
// the user can override the listen address with Etcd.ExtraArgs and add SANs with EtcdCertSANs
func getEtcdAltNames(cfg *kubeadmapi.MasterConfiguration) (*certutil.AltNames, error) {

This comment has been minimized.

@luxas

luxas Feb 2, 2018

Member

this logic could make sense in an other package, like pkiutil or the general util instead of here in a phase package.

This comment has been minimized.

@stealthybox

stealthybox Feb 14, 2018

Author Contributor

I like this suggestion.
It touches code that's outside of the scope of making this patch function.
Let's do a follow-up PR

This comment has been minimized.

@stealthybox

stealthybox Feb 19, 2018

Author Contributor

@luxas -- I ended up implementing this while considering @jamiehannaford's comments.
See fcf0fef.

The one part I don't like about it is that I've added user-facing warnings to the pkiutil package.
Lmk what you think?

@stealthybox stealthybox force-pushed the stealthybox:feature/kubeadm_594-etcd_tls branch from ed08db6 to d2c5677 Feb 14, 2018

@stealthybox stealthybox force-pushed the stealthybox:feature/kubeadm_594-etcd_tls branch from eef1d38 to 4407fd9 Feb 21, 2018

@@ -60,8 +60,8 @@ var (
apiServerCertLongDesc = fmt.Sprintf(normalizer.LongDesc(`
Generates the API server serving certificate and key and saves them into %s and %s files.
The certificate includes default subject alternative names and additional sans eventually provided by the user;
default sans are: <node-name>, <apiserver-advertise-address>, kubernetes, kubernetes.default, kubernetes.default.svc,
The certificate includes default subject alternative names and additional SANs provided by the user;

This comment has been minimized.

@mattkelly

mattkelly Feb 21, 2018

Contributor

Maybe capitalize Subject Alternative Names so that it's more clear that SAN (used later) is abbreviating that (applies below too).

)
}

// CreateEtcdServerCertAndKeyFiles create a new certificate and key file for etcd.

This comment has been minimized.

@mattkelly

mattkelly Feb 21, 2018

Contributor

creates (applies for other function comments below)

}

// CreateEtcdServerCertAndKeyFiles create a new certificate and key file for etcd.
// If the etcd serving certificate and key file already exist in the target folder, they are used only if evaluated equal; otherwise an error is returned.

This comment has been minimized.

@mattkelly

mattkelly Feb 21, 2018

Contributor

Maybe evaluated to be equal (applies for other function comments below)

}
etcdServerCert, etcdServerKey, err := pkiutil.NewCertAndKey(caCert, caKey, config)
if err != nil {
return nil, nil, fmt.Errorf("failure while creating etcd key and certificate: %v", err)

This comment has been minimized.

@mattkelly

mattkelly Feb 21, 2018

Contributor

Maybe be more specific, etcd server key


etcdServerCert, _, err := NewEtcdServerCertAndKey(cfg, caCert, caKey)
if err != nil {
t.Fatalf("failed creation of cert and key: %v", err)

This comment has been minimized.

@mattkelly

mattkelly Feb 21, 2018

Contributor

More specific: of etcd server cert and key

@@ -246,3 +251,101 @@ func pathForKey(pkiPath, name string) string {
func pathForPublicKey(pkiPath, name string) string {
return filepath.Join(pkiPath, fmt.Sprintf("%s.pub", name))
}

// GetAPIServerAltNames builds an AltNames object for to be used when generating apiserver certificate

This comment has been minimized.

@mattkelly

mattkelly Feb 21, 2018

Contributor

Delete for: AltNames object to be used...

}

// GetEtcdAltNames builds an AltNames object for generating the etcd server certificate
// localhost is included in the SAN since this is the interface the etcd static pod listens on

This comment has been minimized.

@mattkelly

mattkelly Feb 21, 2018

Contributor

I don't think godoc will break this up into multiple lines when the docs are generated, so this may read as one giant run-on sentence. Consider breaking it up into actual sentences.

This comment has been minimized.

@stealthybox

stealthybox Feb 24, 2018

Author Contributor

Thanks for catching this one @mattkelly!
It's now fixed in the most recent rebase.

I have another PR ready to address the other typos over the whole codebase -- I'll make sure to check your other comments when we get that submitted.

@ericchiang

This comment has been minimized.

Copy link
Member

ericchiang commented Feb 22, 2018

After todays discussion, I'm not certain if we should plumb through options for a separate CA for etcd, but it seems prudent.

This is what the CIS benchmark recommends. If not, you have to use the --peer-cert-allowed-cn to restrict the set of client certs that can access API.

https://coreos.com/etcd/docs/latest/op-guide/security.html#notes-for-tls-authentication

@stealthybox

This comment has been minimized.

Copy link
Contributor Author

stealthybox commented Feb 22, 2018

@ericchiang Thanks -- Do you think we should implement the etcd CA then?
You mentioned there are some limitations with this regarding some CSR API in etcd-io/etcd#8262.

--peer-cert-allowed-cn is a new v3.3+ feature so that really complicates things.

Am I understanding that we would also have to enable etcd's RBAC and make a user for the apiserver so that we could prevent unauthorized clients from connecting?
(--client-cert-auth=true && etcdctl auth enable)
Would we just use the root etcd user or role?

We would also need to make sure that etcd-peer certs aren't permitted to use Kubernetes API's.

@ericchiang

This comment has been minimized.

Copy link
Member

ericchiang commented Feb 22, 2018

@ericchiang Thanks -- Do you think we should implement the etcd CA then?
You mentioned there are some limitations with this regarding some CSR API in etcd-io/etcd#8262.

Separate CAs make the policy simpler, but complicates the PKI management. I'm not familiar enough with kubeadm HA efforts to say which trade off makes sense.

If you don't have a good enough reason to centralize it (e.g. you're using a CSR API that doesn't support intermediate CAs), separate CAs probably makes sense.

Am I understanding that we would also have to enable etcd's RBAC and make a user for the apiserver so that we could prevent unauthorized clients from connecting?
(--client-cert-auth=true && etcdctl auth enable)
Would we just use the root etcd user or role?

You can just use the root user, or map the root role onto something more convenient for your X509 CN.

We would also need to make sure that etcd-peer certs aren't permitted to use Kubernetes API's.

Etcd peers can arbitrarily modify etcd data. They have backdoor access to the Kubernetes API already :)

@stealthybox stealthybox force-pushed the stealthybox:feature/kubeadm_594-etcd_tls branch from 4407fd9 to 530d945 Feb 23, 2018

@timothysc
Copy link
Member

timothysc left a comment

/approve

Please squash your commits, open a separate issue to follow on with a separate CA and I'll lgtm this one, b/c code freeze is Monday.

@timothysc timothysc added the approved label Feb 23, 2018

@stealthybox stealthybox force-pushed the stealthybox:feature/kubeadm_594-etcd_tls branch 2 times, most recently from 6c4ae20 to 5db8884 Feb 23, 2018

Secure etcd API /w TLS on kubeadm init [kubeadm/#594]
- Generate Server and Peer cert for etcd
- Generate Client cert for apiserver
- Add flags / hostMounts for etcd static pod
- Add flags / hostMounts for apiserver static pod

- Generate certs on upgrade of static-pods for etcd/kube-apiserver
- Modify logic for appending etcd flags to staticpod to be safer for external etcd

@stealthybox stealthybox force-pushed the stealthybox:feature/kubeadm_594-etcd_tls branch from 5db8884 to 2406f23 Feb 23, 2018

@stealthybox

This comment has been minimized.

Copy link
Contributor Author

stealthybox commented Feb 23, 2018

Down from 20 commits to 4 😅

seriously the most intense rebase I've ever done... I'm exhausted

❯ git branch | grep 594
* feature/kubeadm_594-etcd_tls
  feature/kubeadm_594-etcd_tls--subdir-draft
  feature/kubeadm_594-etcd_tls__rebase
  feature/kubeadm_594-etcd_tls__unsquashed
  feature/kubeadm_594-etcd_tls__unsquashed__2
  feature/kubeadm_594-etcd_tls__unsquashed__2__autogendocs
  feature/kubeadm_594-etcd_tls__unsquashed__3__move_typos_autogendocs
  feature/kubeadm_594-etcd_tls__unsquashed__3__move_typos_autogendocs__squash1
  feature/kubeadm_594-etcd_tls__unsquashed__3__move_typos_autogendocs__squash1__move_typos
  feature/kubeadm_594-etcd_tls__unsquashed__4
  feature/kubeadm_594-etcd_tls__unsquashed__5__veryvery_squashed

I made sure there were no code changes

stealthybox added some commits Dec 18, 2017

Change SANs for etcd serving and peer certs
- Place etcd server and peer certs & keys into pki subdir
- Move certs.altName functions to pkiutil + add appendSANstoAltNames()
    Share the append logic for the getAltName functions as suggested by
    @jamiehannaford.
    Move functions/tests to certs/pkiutil as suggested by @luxas.

    Update Bazel BUILD deps

- Warn when an APIServerCertSANs or EtcdCertSANs entry is unusable
- Add MasterConfiguration.EtcdPeerCertSANs
- Move EtcdServerCertSANs and EtcdPeerCertSANs under MasterConfiguration.Etcd
Fix typos
- Fix typos in tests for upgrade phase
- Rename loadCertificateAuthorithy() --> loadCertificateAuthority()
- Disambiguate apiKubeletClientCert & apiEtcdClientCert
- Parameterize hard-coded certs_test config + log tempCertsDir

@stealthybox stealthybox force-pushed the stealthybox:feature/kubeadm_594-etcd_tls branch from 2406f23 to 509e9af Feb 24, 2018

@stealthybox

This comment has been minimized.

Copy link
Contributor Author

stealthybox commented Feb 24, 2018

@timothysc I've opened kubernetes/kubeadm#710 for the separate CA.
I can't add labels in that repo -- feel free to triage it to the 1.10 milestone. I'll put up another PR.

@timothysc

This comment has been minimized.

Copy link
Member

timothysc commented Feb 24, 2018

/test pull-kubernetes-unit

@timothysc
Copy link
Member

timothysc left a comment

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 24, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 24, 2018

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Feb 25, 2018

Automatic merge from submit-queue (batch tested with PRs 59159, 60318, 60079, 59371, 57415). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit b32e9c4 into kubernetes:master Feb 25, 2018

13 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation stealthybox authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
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.