Make certificates per-profile and consistent until IP or names change

pkg/minikube/bootstrapper/certs.go

@@ -17,6 +17,7 @@ limitations under the License.
 package bootstrapper
 
 import (
+	"crypto/sha1"
 	"encoding/pem"
 	"fmt"
 	"io/ioutil"
@@ -25,9 +26,11 @@ import (
 	"os/exec"
 	"path"
 	"path/filepath"
+	"sort"
 	"strings"
 
 	"github.com/golang/glog"
+	"github.com/otiai10/copy"
 	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/tools/clientcmd/api"
@@ -40,63 +43,50 @@ import (
 	"k8s.io/minikube/pkg/minikube/localpath"
 	"k8s.io/minikube/pkg/minikube/vmpath"
 	"k8s.io/minikube/pkg/util"
-	"k8s.io/minikube/pkg/util/lock"
-
-	"github.com/juju/mutex"
-)
-
-var (
-	certs = []string{
-		"ca.crt", "ca.key", "apiserver.crt", "apiserver.key", "proxy-client-ca.crt",
-		"proxy-client-ca.key", "proxy-client.crt", "proxy-client.key",
-	}
 )
 
 // SetupCerts gets the generated credentials required to talk to the APIServer.
-func SetupCerts(cmd command.Runner, k8s config.KubernetesConfig, n config.Node) error {
-
-	localPath := localpath.MiniPath()
+func SetupCerts(cmd command.Runner, k8s config.KubernetesConfig, n config.Node) ([]assets.CopyableFile, error) {
+	localPath := localpath.Profile(k8s.ClusterName)
 	glog.Infof("Setting up %s for IP: %s\n", localPath, n.IP)
 
-	// WARNING: This function was not designed for multiple profiles, so it is VERY racey:
-	//
-	// It updates a shared certificate file and uploads it to the apiserver before launch.
-	//
-	// If another process updates the shared certificate, it's invalid.
-	// TODO: Instead of racey manipulation of a shared certificate, use per-profile certs
-	spec := lock.PathMutexSpec(filepath.Join(localPath, "certs"))
-	glog.Infof("acquiring lock: %+v", spec)
-	releaser, err := mutex.Acquire(spec)
+	ccs, err := generateSharedCACerts()
 	if err != nil {
-		return errors.Wrapf(err, "unable to acquire lock for %+v", spec)
+		return nil, errors.Wrap(err, "shared CA certs")
 	}
-	defer releaser.Release()
 
-	if err := generateCerts(k8s, n); err != nil {
-		return errors.Wrap(err, "Error generating certs")
+	xfer, err := generateProfileCerts(k8s, n, ccs)
+	if err != nil {
+		return nil, errors.Wrap(err, "profile certs")
 	}
+
+	xfer = append(xfer, ccs.caCert)
+	xfer = append(xfer, ccs.caKey)
+	xfer = append(xfer, ccs.proxyCert)
+	xfer = append(xfer, ccs.proxyKey)
+
 	copyableFiles := []assets.CopyableFile{}
-	for _, cert := range certs {
-		p := filepath.Join(localPath, cert)
+	for _, p := range xfer {
+		cert := filepath.Base(p)
 		perms := "0644"
 		if strings.HasSuffix(cert, ".key") {
 			perms = "0600"
 		}
 		certFile, err := assets.NewFileAsset(p, vmpath.GuestKubernetesCertsDir, cert, perms)
 		if err != nil {
-			return err
+			return nil, errors.Wrapf(err, "key asset %s", cert)
 		}
 		copyableFiles = append(copyableFiles, certFile)
 	}
 
 	caCerts, err := collectCACerts()
 	if err != nil {
-		return err
+		return nil, err
 	}
 	for src, dst := range caCerts {
 		certFile, err := assets.NewFileAsset(src, path.Dir(dst), path.Base(dst), "0644")
 		if err != nil {
-			return err
+			return nil, errors.Wrapf(err, "ca asset %s", src)
 		}
 
 		copyableFiles = append(copyableFiles, certFile)
@@ -114,11 +104,11 @@ func SetupCerts(cmd command.Runner, k8s config.KubernetesConfig, n config.Node)
 	kubeCfg := api.NewConfig()
 	err = kubeconfig.PopulateFromSettings(kcs, kubeCfg)
 	if err != nil {
-		return errors.Wrap(err, "populating kubeconfig")
+		return nil, errors.Wrap(err, "populating kubeconfig")
 	}
 	data, err := runtime.Encode(latest.Codec, kubeCfg)
 	if err != nil {
-		return errors.Wrap(err, "encoding kubeconfig")
+		return nil, errors.Wrap(err, "encoding kubeconfig")
 	}
 
 	if n.ControlPlane {
@@ -128,46 +118,74 @@ func SetupCerts(cmd command.Runner, k8s config.KubernetesConfig, n config.Node)
 
 	for _, f := range copyableFiles {
 		if err := cmd.Copy(f); err != nil {
-			return errors.Wrapf(err, "Copy %s", f.GetAssetName())
+			return nil, errors.Wrapf(err, "Copy %s", f.GetAssetName())
 		}
 	}
 
 	if err := installCertSymlinks(cmd, caCerts); err != nil {
-		return errors.Wrapf(err, "certificate symlinks")
+		return nil, errors.Wrapf(err, "certificate symlinks")
 	}
-	return nil
+	return copyableFiles, nil
 }
 
-func generateCerts(k8s config.KubernetesConfig, n config.Node) error {
-	serviceIP, err := util.GetServiceClusterIP(k8s.ServiceCIDR)
-	if err != nil {
-		return errors.Wrap(err, "getting service cluster ip")
-	}
-
-	localPath := localpath.MiniPath()
-	caCertPath := filepath.Join(localPath, "ca.crt")
-	caKeyPath := filepath.Join(localPath, "ca.key")
+type CACerts struct {
+	caCert    string
+	caKey     string
+	proxyCert string
+	proxyKey  string
+}
 
-	proxyClientCACertPath := filepath.Join(localPath, "proxy-client-ca.crt")
-	proxyClientCAKeyPath := filepath.Join(localPath, "proxy-client-ca.key")
+// generateSharedCACerts generates CA certs shared among profiles, but only if missing
+func generateSharedCACerts() (CACerts, error) {
+	globalPath := localpath.MiniPath()
+	cc := CACerts{
+		caCert:    localpath.CACert(),
+		caKey:     filepath.Join(globalPath, "ca.key"),
+		proxyCert: filepath.Join(globalPath, "proxy-client-ca.crt"),
+		proxyKey:  filepath.Join(globalPath, "proxy-client-ca.key"),
+	}
 
 	caCertSpecs := []struct {
 		certPath string
 		keyPath  string
 		subject  string
 	}{
 		{ // client / apiserver CA
-			certPath: caCertPath,
-			keyPath:  caKeyPath,
+			certPath: cc.caCert,
+			keyPath:  cc.caKey,
 			subject:  "minikubeCA",
 		},
 		{ // proxy-client CA
-			certPath: proxyClientCACertPath,
-			keyPath:  proxyClientCAKeyPath,
+			certPath: cc.proxyCert,
+			keyPath:  cc.proxyKey,
 			subject:  "proxyClientCA",
 		},
 	}
 
+	for _, ca := range caCertSpecs {
+		if canRead(ca.certPath) && canRead(ca.keyPath) {
+			glog.Infof("skipping %s CA generation: %s", ca.subject, ca.keyPath)
+			continue
+		}
+
+		glog.Infof("generating %s CA: %s", ca.subject, ca.keyPath)
+		if err := util.GenerateCACert(ca.certPath, ca.keyPath, ca.subject); err != nil {
+			return cc, errors.Wrap(err, "generate ca cert")
+		}
+	}
+
+	return cc, nil
+}
+
+// generateProfileCerts generates profile certs for a profile
+func generateProfileCerts(k8s config.KubernetesConfig, n config.Node, ccs CACerts) ([]string, error) {
+	profilePath := localpath.Profile(k8s.ClusterName)
+
+	serviceIP, err := util.GetServiceClusterIP(k8s.ServiceCIDR)
+	if err != nil {
+		return nil, errors.Wrap(err, "getting service cluster ip")
+	}
+
 	apiServerIPs := append(
 		k8s.APIServerIPs,
 		[]net.IP{net.ParseIP(n.IP), serviceIP, net.ParseIP(oci.DefaultBindIPV4), net.ParseIP("10.0.0.1")}...)
@@ -176,66 +194,97 @@ func generateCerts(k8s config.KubernetesConfig, n config.Node) error {
 		apiServerNames,
 		util.GetAlternateDNS(k8s.DNSDomain)...)
 
-	signedCertSpecs := []struct {
-		certPath       string
-		keyPath        string
+	// Generate a hash input for certs that depend on ip/name combinations
+	hi := []string{}
+	hi = append(hi, apiServerAlternateNames...)
+	for _, ip := range apiServerIPs {
+		hi = append(hi, ip.String())
+	}
+	sort.Strings(hi)
+
+	specs := []struct {
+		certPath string
+		keyPath  string
+		hash     string
+
 		subject        string
 		ips            []net.IP
 		alternateNames []string
 		caCertPath     string
 		caKeyPath      string
 	}{
 		{ // Client cert
-			certPath:       filepath.Join(localPath, "client.crt"),
-			keyPath:        filepath.Join(localPath, "client.key"),
+			certPath:       localpath.ClientCert(k8s.ClusterName),
+			keyPath:        localpath.ClientKey(k8s.ClusterName),
 			subject:        "minikube-user",
 			ips:            []net.IP{},
 			alternateNames: []string{},
-			caCertPath:     caCertPath,
-			caKeyPath:      caKeyPath,
+			caCertPath:     ccs.caCert,
+			caKeyPath:      ccs.caKey,
 		},
 		{ // apiserver serving cert
-			certPath:       filepath.Join(localPath, "apiserver.crt"),
-			keyPath:        filepath.Join(localPath, "apiserver.key"),
+			hash:           fmt.Sprintf("%x", sha1.Sum([]byte(strings.Join(hi, "/"))))[0:8],
+			certPath:       filepath.Join(profilePath, "apiserver.crt"),
+			keyPath:        filepath.Join(profilePath, "apiserver.key"),
 			subject:        "minikube",
 			ips:            apiServerIPs,
 			alternateNames: apiServerAlternateNames,
-			caCertPath:     caCertPath,
-			caKeyPath:      caKeyPath,
+			caCertPath:     ccs.caCert,
+			caKeyPath:      ccs.caKey,
 		},
 		{ // aggregator proxy-client cert
-			certPath:       filepath.Join(localPath, "proxy-client.crt"),
-			keyPath:        filepath.Join(localPath, "proxy-client.key"),
+			certPath:       filepath.Join(profilePath, "proxy-client.crt"),
+			keyPath:        filepath.Join(profilePath, "proxy-client.key"),
 			subject:        "aggregator",
 			ips:            []net.IP{},
 			alternateNames: []string{},
-			caCertPath:     proxyClientCACertPath,
-			caKeyPath:      proxyClientCAKeyPath,
+			caCertPath:     ccs.proxyCert,
+			caKeyPath:      ccs.proxyKey,
 		},
 	}
 
-	for _, caCertSpec := range caCertSpecs {
-		if !(canReadFile(caCertSpec.certPath) &&
-			canReadFile(caCertSpec.keyPath)) {
-			if err := util.GenerateCACert(
-				caCertSpec.certPath, caCertSpec.keyPath, caCertSpec.subject,
-			); err != nil {
-				return errors.Wrap(err, "Error generating CA certificate")
-			}
+	xfer := []string{}
+	for _, spec := range specs {
+		if spec.subject != "minikube-user" {
+			xfer = append(xfer, spec.certPath)
+			xfer = append(xfer, spec.keyPath)
+		}
+
+		cp := spec.certPath
+		kp := spec.keyPath
+		if spec.hash != "" {
+			cp = cp + "." + spec.hash
+			kp = kp + "." + spec.hash
+		}
+
+		if canRead(cp) && canRead(kp) {
+			glog.Infof("skipping %s signed cert generation: %s", spec.subject, kp)
+			continue
 		}
-	}
 
-	for _, signedCertSpec := range signedCertSpecs {
-		if err := util.GenerateSignedCert(
-			signedCertSpec.certPath, signedCertSpec.keyPath, signedCertSpec.subject,
-			signedCertSpec.ips, signedCertSpec.alternateNames,
-			signedCertSpec.caCertPath, signedCertSpec.caKeyPath,
-		); err != nil {
-			return errors.Wrap(err, "Error generating signed apiserver serving cert")
+		glog.Infof("generating %s signed cert: %s", spec.subject, kp)
+		err := util.GenerateSignedCert(
+			cp, kp, spec.subject,
+			spec.ips, spec.alternateNames,
+			spec.caCertPath, spec.caKeyPath,
+		)
+		if err != nil {
+			return xfer, errors.Wrapf(err, "generate signed cert for %q", spec.subject)
+		}
+
+		if spec.hash != "" {
+			glog.Infof("copying %s -> %s", cp, spec.certPath)
+			if err := copy.Copy(cp, spec.certPath); err != nil {
+				return xfer, errors.Wrap(err, "copy cert")
+			}
+			glog.Infof("copying %s -> %s", kp, spec.keyPath)
+			if err := copy.Copy(kp, spec.keyPath); err != nil {
+				return xfer, errors.Wrap(err, "copy key")
+			}
 		}
 	}
 
-	return nil
+	return xfer, nil
 }
 
 // isValidPEMCertificate checks whether the input file is a valid PEM certificate (with at least one CERTIFICATE block)
@@ -357,9 +406,9 @@ func installCertSymlinks(cr command.Runner, caCerts map[string]string) error {
 	return nil
 }
 
-// canReadFile returns true if the file represented
+// canRead returns true if the file represented
 // by path exists and is readable, otherwise false.
-func canReadFile(path string) bool {
+func canRead(path string) bool {
 	f, err := os.Open(path)
 	if err != nil {
 		return false

pkg/minikube/bootstrapper/certs_test.go

@@ -24,7 +24,6 @@ import (
 	"k8s.io/minikube/pkg/minikube/command"
 	"k8s.io/minikube/pkg/minikube/config"
 	"k8s.io/minikube/pkg/minikube/constants"
-	"k8s.io/minikube/pkg/minikube/localpath"
 	"k8s.io/minikube/pkg/minikube/tests"
 	"k8s.io/minikube/pkg/util"
 )
@@ -58,20 +57,8 @@ func TestSetupCerts(t *testing.T) {
 	f := command.NewFakeCommandRunner()
 	f.SetCommandToOutput(expected)
 
-	var filesToBeTransferred []string
-	for _, cert := range certs {
-		filesToBeTransferred = append(filesToBeTransferred, filepath.Join(localpath.MiniPath(), cert))
-	}
-	filesToBeTransferred = append(filesToBeTransferred, filepath.Join(localpath.MiniPath(), "ca.crt"))
-	filesToBeTransferred = append(filesToBeTransferred, filepath.Join(localpath.MiniPath(), "certs", "mycert.pem"))
-
-	if err := SetupCerts(f, k8s, config.Node{}); err != nil {
+	_, err := SetupCerts(f, k8s, config.Node{})
+	if err != nil {
 		t.Fatalf("Error starting cluster: %v", err)
 	}
-	for _, cert := range filesToBeTransferred {
-		_, err := f.GetFileToContents(cert)
-		if err != nil {
-			t.Errorf("Cert not generated: %s", cert)
-		}
-	}
 }

pkg/minikube/bootstrapper/kubeadm/kubeadm.go

@@ -445,7 +445,8 @@ func (k *Bootstrapper) DeleteCluster(k8s config.KubernetesConfig) error {
 
 // SetupCerts sets up certificates within the cluster.
 func (k *Bootstrapper) SetupCerts(k8s config.KubernetesConfig, n config.Node) error {
-	return bootstrapper.SetupCerts(k.c, k8s, n)
+	_, err := bootstrapper.SetupCerts(k.c, k8s, n)
+	return err
 }
 
 // UpdateCluster updates the cluster.