From 2aa655a28425fc80a478c42f7585523fc9e2d8a7 Mon Sep 17 00:00:00 2001 From: John Gardiner Myers Date: Thu, 14 May 2020 22:47:17 -0700 Subject: [PATCH] Continue refactoring cert issuance code --- upup/pkg/fi/fitasks/keypair.go | 127 +++++++++++++++++--------------- upup/pkg/fi/vfs_castore.go | 80 ++++++-------------- upup/pkg/fi/vfs_castore_test.go | 3 +- 3 files changed, 91 insertions(+), 119 deletions(-) diff --git a/upup/pkg/fi/fitasks/keypair.go b/upup/pkg/fi/fitasks/keypair.go index 621895e9ca..26f4c9a4bc 100644 --- a/upup/pkg/fi/fitasks/keypair.go +++ b/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 -func (e *Keypair) buildCertificateTemplate() (*x509.Certificate, error) { - template, err := buildCertificateTemplateForType(e.Type) - if err != nil { - return nil, err - } +type issueCertRequest struct { + // Signer is the keypair to use to sign. + // Signer string + // Type is the type of certificate i.e. CA, server, client etc. + 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) - if err != nil { - 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 + // Serial is the certificate serial number. If nil, a random number will be generated. + // Serial *big.Int } -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 - if template.IsCA { + var caCertificate *x509.Certificate + var caPrivateKey *pki.PrivateKey + if !template.IsCA { var err error - cert, err = pki.SignNewCertificate(privateKey, template, nil, nil) - if err != nil { - return nil, err - } - } else { - caCertificate, caPrivateKey, _, err := keystore.FindKeypair(signer) + var caCert *pki.Certificate + caCert, 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) - if err != nil { - return nil, err - } + caCertificate = caCert.Certificate } - err := keystore.StoreKeypair(id, cert, privateKey) - if err != nil { - return nil, err - } - - // Make double-sure it round-trips - certificate, _, _, err := keystore.FindKeypair(id) - return certificate, err + return pki.SignNewCertificate(privateKey, template, caCertificate, caPrivateKey) } diff --git a/upup/pkg/fi/vfs_castore.go b/upup/pkg/fi/vfs_castore.go index d29e300fa4..3579568988 100644 --- a/upup/pkg/fi/vfs_castore.go +++ b/upup/pkg/fi/vfs_castore.go @@ -39,13 +39,8 @@ type VFSCAStore struct { basedir vfs.Path cluster *kops.Cluster - mutex sync.Mutex - cachedCAs map[string]*cachedEntry -} - -type cachedEntry struct { - certificates *keyset - privateKeys *keyset + mutex sync.Mutex + cachedCA *keyset } var _ CAStore = &VFSCAStore{} @@ -53,9 +48,8 @@ var _ SSHCredentialStore = &VFSCAStore{} func NewVFSCAStore(cluster *kops.Cluster, basedir vfs.Path) *VFSCAStore { c := &VFSCAStore{ - basedir: basedir, - cluster: cluster, - cachedCAs: make(map[string]*cachedEntry), + basedir: basedir, + cluster: cluster, } 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, - cluster: cluster, - cachedCAs: make(map[string]*cachedEntry), + basedir: basedir, + cluster: cluster, } 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 { diff --git a/upup/pkg/fi/vfs_castore_test.go b/upup/pkg/fi/vfs_castore_test.go index 71e95afb1e..dd31db9af6 100644 --- a/upup/pkg/fi/vfs_castore_test.go +++ b/upup/pkg/fi/vfs_castore_test.go @@ -55,8 +55,7 @@ func TestVFSCAStoreRoundTrip(t *testing.T) { } s := &VFSCAStore{ - basedir: basePath, - cachedCAs: make(map[string]*cachedEntry), + basedir: basePath, } 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"