Continue refactoring cert issuance code

upup/pkg/fi/fitasks/keypair.go

@@ -18,6 +18,7 @@ package fitasks
 
 import (
 	"crypto/x509"
+	"crypto/x509/pkix"
 	"fmt"
 	"math/big"
 	"net"
@@ -170,11 +171,6 @@ func (_ *Keypair) Render(c *fi.Context, a, e, changes *Keypair) error {
 		return fi.RequiredField("Name")
 	}
 
-	template, err := e.buildCertificateTemplate()
-	if err != nil {
-		return err
-	}
-
 	changeStoredFormat := false
 	createCertificate := false
 	if a == nil {
@@ -225,10 +221,38 @@ func (_ *Keypair) Render(c *fi.Context, a, e, changes *Keypair) error {
 
 		serial := pki.BuildPKISerial(time.Now().UnixNano())
 
+		subjectPkix, err := parsePkixName(e.Subject)
+		if err != nil {
+			return fmt.Errorf("error parsing Subject: %v", err)
+		}
+
+		if len(subjectPkix.ToRDNSequence()) == 0 {
+			return fmt.Errorf("subject name was empty for SSL keypair %q", *e.Name)
+		}
+
+		req := issueCertRequest{
+			Type:           e.Type,
+			Subject:        *subjectPkix,
+			AlternateNames: e.AlternateNames,
+		}
+		template, err := buildCertificateTemplate(&req)
+		if err != nil {
+			return err
+		}
 		cert, err := e.issueCert(c.Keystore, signer, name, serial, privateKey, template)
 		if err != nil {
 			return err
 		}
+		err = c.Keystore.StoreKeypair(name, cert, privateKey)
+		if err != nil {
+			return err
+		}
+
+		// Make double-sure it round-trips
+		_, _, _, err = c.Keystore.FindKeypair(name)
+		if err != nil {
+			return err
+		}
 
 		klog.V(8).Infof("created certificate with cn=%s", cert.Subject.CommonName)
 	}
@@ -253,43 +277,22 @@ func (_ *Keypair) Render(c *fi.Context, a, e, changes *Keypair) error {
 	return nil
 }
 
-// BuildCertificateTemplate is responsible for constructing a certificate template
+type issueCertRequest struct {
-func (e *Keypair) buildCertificateTemplate() (*x509.Certificate, error) {
+	// Signer is the keypair to use to sign.
-	template, err := buildCertificateTemplateForType(e.Type)
+	// Signer string
-	if err != nil {
+	// Type is the type of certificate i.e. CA, server, client etc.
-		return nil, err
+	Type string
-	}
+	// Subject is the certificate subject.
+	Subject pkix.Name
+	// AlternateNames is a list of alternative names for this certificate.
+	AlternateNames []string
 
-	subjectPkix, err := parsePkixName(e.Subject)
+	// Serial is the certificate serial number. If nil, a random number will be generated.
-	if err != nil {
+	// Serial *big.Int
-		return nil, fmt.Errorf("error parsing Subject: %v", err)
-	}
-
-	if len(subjectPkix.ToRDNSequence()) == 0 {
-		return nil, fmt.Errorf("Subject name was empty for SSL keypair %q", *e.Name)
-	}
-
-	template.Subject = *subjectPkix
-
-	var alternateNames []string
-	alternateNames = append(alternateNames, e.AlternateNames...)
-
-	for _, san := range alternateNames {
-		san = strings.TrimSpace(san)
-		if san == "" {
-			continue
-		}
-		if ip := net.ParseIP(san); ip != nil {
-			template.IPAddresses = append(template.IPAddresses, ip)
-		} else {
-			template.DNSNames = append(template.DNSNames, san)
-		}
-	}
-
-	return template, nil
 }
 
-func buildCertificateTemplateForType(certificateType string) (*x509.Certificate, error) {
+func buildCertificateTemplate(request *issueCertRequest) (*x509.Certificate, error) {
+	certificateType := request.Type
 	if expanded, found := wellKnownCertificateTypes[certificateType]; found {
 		certificateType = expanded
 	}
@@ -320,6 +323,23 @@ func buildCertificateTemplateForType(certificateType string) (*x509.Certificate,
 		}
 	}
 
+	template.Subject = request.Subject
+
+	var alternateNames []string
+	alternateNames = append(alternateNames, request.AlternateNames...)
+
+	for _, san := range alternateNames {
+		san = strings.TrimSpace(san)
+		if san == "" {
+			continue
+		}
+		if ip := net.ParseIP(san); ip != nil {
+			template.IPAddresses = append(template.IPAddresses, ip)
+		} else {
+			template.DNSNames = append(template.DNSNames, san)
+		}
+	}
+
 	return template, nil
 }
 
@@ -354,36 +374,23 @@ func (e *Keypair) issueCert(keystore fi.Keystore, signer string, id string, seri
 
 	template.SerialNumber = serial
 
-	var cert *pki.Certificate
+	var caCertificate *x509.Certificate
-	if template.IsCA {
+	var caPrivateKey *pki.PrivateKey
+	if !template.IsCA {
 		var err error
-		cert, err = pki.SignNewCertificate(privateKey, template, nil, nil)
+		var caCert *pki.Certificate
-		if err != nil {
+		caCert, caPrivateKey, _, err = keystore.FindKeypair(signer)
-			return nil, err
-		}
-	} else {
-		caCertificate, caPrivateKey, _, err := keystore.FindKeypair(signer)
 		if err != nil {
 			return nil, err
 		}
 		if caPrivateKey == nil {
 			return nil, fmt.Errorf("ca key for %q was not found; cannot issue certificates", signer)
 		}
-		if caCertificate == nil {
+		if caCert == nil {
 			return nil, fmt.Errorf("ca certificate for %q was not found; cannot issue certificates", signer)
 		}
-		cert, err = pki.SignNewCertificate(privateKey, template, caCertificate.Certificate, caPrivateKey)
+		caCertificate = caCert.Certificate
-		if err != nil {
-			return nil, err
-		}
 	}
 
-	err := keystore.StoreKeypair(id, cert, privateKey)
+	return pki.SignNewCertificate(privateKey, template, caCertificate, caPrivateKey)
-	if err != nil {
-		return nil, err
-	}
-
-	// Make double-sure it round-trips
-	certificate, _, _, err := keystore.FindKeypair(id)
-	return certificate, err
 }

upup/pkg/fi/vfs_castore.go

@@ -39,13 +39,8 @@ type VFSCAStore struct {
 	basedir vfs.Path
 	cluster *kops.Cluster
 
-	mutex     sync.Mutex
+	mutex    sync.Mutex
-	cachedCAs map[string]*cachedEntry
+	cachedCA *keyset
-}
-
-type cachedEntry struct {
-	certificates *keyset
-	privateKeys  *keyset
 }
 
 var _ CAStore = &VFSCAStore{}
@@ -53,9 +48,8 @@ var _ SSHCredentialStore = &VFSCAStore{}
 
 func NewVFSCAStore(cluster *kops.Cluster, basedir vfs.Path) *VFSCAStore {
 	c := &VFSCAStore{
-		basedir:   basedir,
+		basedir: basedir,
-		cluster:   cluster,
+		cluster: cluster,
-		cachedCAs: make(map[string]*cachedEntry),
 	}
 
 	return c
@@ -65,9 +59,8 @@ func NewVFSCAStore(cluster *kops.Cluster, basedir vfs.Path) *VFSCAStore {
 func NewVFSSSHCredentialStore(cluster *kops.Cluster, basedir vfs.Path) SSHCredentialStore {
 	// Note currently identical to NewVFSCAStore
 	c := &VFSCAStore{
-		basedir:   basedir,
+		basedir: basedir,
-		cluster:   cluster,
+		cluster: cluster,
-		cachedCAs: make(map[string]*cachedEntry),
 	}
 
 	return c
@@ -77,47 +70,6 @@ func (s *VFSCAStore) VFSPath() vfs.Path {
 	return s.basedir
 }
 
-// Retrieves the CA keypair.  No longer generates keypairs if not found.
-func (s *VFSCAStore) readCAKeypairs(id string) (*keyset, *keyset, error) {
-	s.mutex.Lock()
-	defer s.mutex.Unlock()
-
-	cached := s.cachedCAs[id]
-	if cached != nil {
-		return cached.certificates, cached.privateKeys, nil
-	}
-
-	caCertificates, err := s.loadCertificates(s.buildCertificatePoolPath(id))
-	if err != nil {
-		return nil, nil, err
-	}
-
-	var caPrivateKeys *keyset
-
-	if caCertificates != nil {
-		caPrivateKeys, err = s.loadPrivateKeys(s.buildPrivateKeyPoolPath(id))
-		if err != nil {
-			return nil, nil, err
-		}
-
-		if caPrivateKeys == nil {
-			klog.Warningf("CA private key was not found")
-			//return nil, fmt.Errorf("error loading CA private key - key not found")
-		}
-	}
-
-	if caPrivateKeys == nil {
-		// We no longer generate CA certificates automatically - too race-prone
-		return caCertificates, caPrivateKeys, nil
-	}
-
-	cached = &cachedEntry{certificates: caCertificates, privateKeys: caPrivateKeys}
-	s.cachedCAs[id] = cached
-
-	return cached.certificates, cached.privateKeys, nil
-
-}
-
 func (c *VFSCAStore) buildCertificatePoolPath(name string) vfs.Path {
 	return c.basedir.Join("issued", name)
 }
@@ -633,14 +585,28 @@ func (c *VFSCAStore) loadPrivateKeys(p vfs.Path) (*keyset, error) {
 
 func (c *VFSCAStore) findPrivateKeyset(id string) (*keyset, error) {
 	var keys *keyset
+	var err error
 	if id == CertificateId_CA {
-		_, caPrivateKeys, err := c.readCAKeypairs(id)
+		c.mutex.Lock()
+		defer c.mutex.Unlock()
+
+		cached := c.cachedCA
+		if cached != nil {
+			return cached, nil
+		}
+
+		keys, err = c.loadPrivateKeys(c.buildPrivateKeyPoolPath(id))
 		if err != nil {
 			return nil, err
 		}
-		keys = caPrivateKeys
+
+		if keys == nil {
+			klog.Warningf("CA private key was not found")
+			// We no longer generate CA certificates automatically - too race-prone
+		} else {
+			c.cachedCA = keys
+		}
 	} else {
-		var err error
 		p := c.buildPrivateKeyPoolPath(id)
 		keys, err = c.loadPrivateKeys(p)
 		if err != nil {

upup/pkg/fi/vfs_castore_test.go

@@ -55,8 +55,7 @@ func TestVFSCAStoreRoundTrip(t *testing.T) {
 	}
 
 	s := &VFSCAStore{
-		basedir:   basePath,
+		basedir: basePath,
-		cachedCAs: make(map[string]*cachedEntry),
 	}
 
 	privateKeyData := "-----BEGIN RSA PRIVATE KEY-----\nMIIEpAIBAAKCAQEA4JwpEprZ5n8RIEt6jT2lAh+UDgRgx/4px21gjgywQivYHVxH\nAZexVb/E9pBa9Q2G9B1Q7TCO7YsUVRQy4JMDZVt+McFnWVwexnqBYFNcVjkEmDgA\ngvCYGE0P9d/RwRL4KuLHo+u6fv7P0jXMN+CpOxyLhYZZNa0ZOZDHsSiJSQSj9WGF\nGHrbCf0KVDpKieR1uBqHrRO+mLR5zkX2L58m74kjK4dsBhmjeq/7OAoTmiG2QgJ/\nP2IjyhiA2mRqY+hl55lwEUV/0yHYEkJC8LdGkwwZz2eF77aSPGmi/A2CSKgMwDTx\n9m+P7jcpWreYw6NG9BueGoDIve/tgFKwvVFF6QIDAQABAoIBAA0ktjaTfyrAxsTI\nBezb7Zr5NBW55dvuII299cd6MJo+rI/TRYhvUv48kY8IFXp/hyUjzgeDLunxmIf9\n/Zgsoic9Ol44/g45mMduhcGYPzAAeCdcJ5OB9rR9VfDCXyjYLlN8H8iU0734tTqM\n0V13tQ9zdSqkGPZOIcq/kR/pylbOZaQMe97BTlsAnOMSMKDgnftY4122Lq3GYy+t\nvpr+bKVaQZwvkLoSU3rECCaKaghgwCyX7jft9aEkhdJv+KlwbsGY6WErvxOaLWHd\ncuMQjGapY1Fa/4UD00mvrA260NyKfzrp6+P46RrVMwEYRJMIQ8YBAk6N6Hh7dc0G\n8Z6i1m0CgYEA9HeCJR0TSwbIQ1bDXUrzpftHuidG5BnSBtax/ND9qIPhR/FBW5nj\n22nwLc48KkyirlfIULd0ae4qVXJn7wfYcuX/cJMLDmSVtlM5Dzmi/91xRiFgIzx1\nAsbBzaFjISP2HpSgL+e9FtSXaaqeZVrflitVhYKUpI/AKV31qGHf04sCgYEA6zTV\n99Sb49Wdlns5IgsfnXl6ToRttB18lfEKcVfjAM4frnkk06JpFAZeR+9GGKUXZHqs\nz2qcplw4d/moCC6p3rYPBMLXsrGNEUFZqBlgz72QA6BBq3X0Cg1Bc2ZbK5VIzwkg\nST2SSux6ccROfgULmN5ZiLOtdUKNEZpFF3i3qtsCgYADT/s7dYFlatobz3kmMnXK\nsfTu2MllHdRys0YGHu7Q8biDuQkhrJwhxPW0KS83g4JQym+0aEfzh36bWcl+u6R7\nKhKj+9oSf9pndgk345gJz35RbPJYh+EuAHNvzdgCAvK6x1jETWeKf6btj5pF1U1i\nQ4QNIw/QiwIXjWZeubTGsQKBgQCbduLu2rLnlyyAaJZM8DlHZyH2gAXbBZpxqU8T\nt9mtkJDUS/KRiEoYGFV9CqS0aXrayVMsDfXY6B/S/UuZjO5u7LtklDzqOf1aKG3Q\ndGXPKibknqqJYH+bnUNjuYYNerETV57lijMGHuSYCf8vwLn3oxBfERRX61M/DU8Z\nworz/QKBgQDCTJI2+jdXg26XuYUmM4XXfnocfzAXhXBULt1nENcogNf1fcptAVtu\nBAiz4/HipQKqoWVUYmxfgbbLRKKLK0s0lOWKbYdVjhEm/m2ZU8wtXTagNwkIGoyq\nY/C1Lox4f1ROJnCjc/hfcOjcxX5M8A8peecHWlVtUPKTJgxQ7oMKcw==\n-----END RSA PRIVATE KEY-----\n"