From a96f7963a6f3d912e8f3c5ab328db2c5066e0151 Mon Sep 17 00:00:00 2001 From: John Gardiner Myers Date: Thu, 14 May 2020 22:16:22 -0700 Subject: [PATCH] Pull cert issuance code up into fitasks.Keypair --- nodeup/pkg/model/kube_apiserver_test.go | 5 - pkg/kubeconfig/create_kubecfg_test.go | 8 -- upup/pkg/fi/ca.go | 3 - upup/pkg/fi/clientset_castore.go | 122 ++---------------------- upup/pkg/fi/fitasks/keypair.go | 49 +++++++++- upup/pkg/fi/vfs_castore.go | 62 ------------ 6 files changed, 52 insertions(+), 197 deletions(-) diff --git a/nodeup/pkg/model/kube_apiserver_test.go b/nodeup/pkg/model/kube_apiserver_test.go index e7dbf03db7..e988356bf8 100644 --- a/nodeup/pkg/model/kube_apiserver_test.go +++ b/nodeup/pkg/model/kube_apiserver_test.go @@ -18,7 +18,6 @@ package model import ( "bytes" - "crypto/x509" "strings" "testing" @@ -74,10 +73,6 @@ func (k fakeKeyStore) FindKeypair(name string) (*pki.Certificate, *pki.PrivateKe panic("implement me") } -func (k fakeKeyStore) CreateKeypair(signer string, name string, template *x509.Certificate, privateKey *pki.PrivateKey) (*pki.Certificate, error) { - panic("implement me") -} - func (k fakeKeyStore) StoreKeypair(id string, cert *pki.Certificate, privateKey *pki.PrivateKey) error { panic("implement me") } diff --git a/pkg/kubeconfig/create_kubecfg_test.go b/pkg/kubeconfig/create_kubecfg_test.go index 5c34a8b468..6c949f63f6 100644 --- a/pkg/kubeconfig/create_kubecfg_test.go +++ b/pkg/kubeconfig/create_kubecfg_test.go @@ -20,8 +20,6 @@ import ( "reflect" "testing" - "crypto/x509" - "k8s.io/client-go/tools/clientcmd" "k8s.io/kops/pkg/apis/kops" "k8s.io/kops/pkg/pki" @@ -50,8 +48,6 @@ func (f fakeStatusStore) GetApiIngressStatus(cluster *kops.Cluster) ([]kops.ApiI type fakeKeyStore struct { FindKeypairFn func(name string) (*pki.Certificate, *pki.PrivateKey, bool, error) - CreateKeypairFn func(signer string, name string, template *x509.Certificate, privateKey *pki.PrivateKey) (*pki.Certificate, error) - // StoreKeypair writes the keypair to the store StoreKeypairFn func(id string, cert *pki.Certificate, privateKey *pki.PrivateKey) error @@ -63,10 +59,6 @@ func (f fakeKeyStore) FindKeypair(name string) (*pki.Certificate, *pki.PrivateKe return f.FindKeypairFn(name) } -func (f fakeKeyStore) CreateKeypair(signer string, name string, template *x509.Certificate, privateKey *pki.PrivateKey) (*pki.Certificate, error) { - return f.CreateKeypairFn(signer, name, template, privateKey) -} - func (f fakeKeyStore) StoreKeypair(id string, cert *pki.Certificate, privateKey *pki.PrivateKey) error { return f.StoreKeypairFn(id, cert, privateKey) } diff --git a/upup/pkg/fi/ca.go b/upup/pkg/fi/ca.go index 4a8e7de236..51660212b4 100644 --- a/upup/pkg/fi/ca.go +++ b/upup/pkg/fi/ca.go @@ -18,7 +18,6 @@ package fi import ( "bytes" - "crypto/x509" "fmt" "k8s.io/kops/pkg/apis/kops" @@ -53,8 +52,6 @@ type Keystore interface { // task to convert a Legacy Keypair to the new Keypair API format. FindKeypair(name string) (*pki.Certificate, *pki.PrivateKey, bool, error) - CreateKeypair(signer string, name string, template *x509.Certificate, privateKey *pki.PrivateKey) (*pki.Certificate, error) - // StoreKeypair writes the keypair to the store StoreKeypair(id string, cert *pki.Certificate, privateKey *pki.PrivateKey) error diff --git a/upup/pkg/fi/clientset_castore.go b/upup/pkg/fi/clientset_castore.go index b131f383e4..ce7db8b703 100644 --- a/upup/pkg/fi/clientset_castore.go +++ b/upup/pkg/fi/clientset_castore.go @@ -19,11 +19,8 @@ package fi import ( "bytes" "context" - "crypto/x509" "fmt" "math/big" - "sync" - "time" "golang.org/x/crypto/ssh" "k8s.io/apimachinery/pkg/api/errors" @@ -40,9 +37,6 @@ type ClientsetCAStore struct { cluster *kops.Cluster namespace string clientset kopsinternalversion.KopsInterface - - mutex sync.Mutex - cachedCaKeysets map[string]*keyset } var _ CAStore = &ClientsetCAStore{} @@ -51,10 +45,9 @@ var _ SSHCredentialStore = &ClientsetCAStore{} // NewClientsetCAStore is the constructor for ClientsetCAStore func NewClientsetCAStore(cluster *kops.Cluster, clientset kopsinternalversion.KopsInterface, namespace string) CAStore { c := &ClientsetCAStore{ - cluster: cluster, - clientset: clientset, - namespace: namespace, - cachedCaKeysets: make(map[string]*keyset), + cluster: cluster, + clientset: clientset, + namespace: namespace, } return c @@ -64,38 +57,14 @@ func NewClientsetCAStore(cluster *kops.Cluster, clientset kopsinternalversion.Ko func NewClientsetSSHCredentialStore(cluster *kops.Cluster, clientset kopsinternalversion.KopsInterface, namespace string) SSHCredentialStore { // Note: currently identical to NewClientsetCAStore c := &ClientsetCAStore{ - cluster: cluster, - clientset: clientset, - namespace: namespace, - cachedCaKeysets: make(map[string]*keyset), + cluster: cluster, + clientset: clientset, + namespace: namespace, } return c } -// readCAKeypairs retrieves the CA keypair. -// (No longer generates a keypair if not found.) -func (c *ClientsetCAStore) readCAKeypairs(ctx context.Context, id string) (*keyset, error) { - c.mutex.Lock() - defer c.mutex.Unlock() - - cached := c.cachedCaKeysets[id] - if cached != nil { - return cached, nil - } - - keyset, err := c.loadKeyset(ctx, id) - if err != nil { - return nil, err - } - - if keyset == nil { - return nil, nil - } - c.cachedCaKeysets[id] = keyset - return keyset, nil -} - // keyset is a parsed Keyset type keyset struct { legacyFormat bool @@ -319,67 +288,6 @@ func (c *ClientsetCAStore) ListSSHCredentials() ([]*kops.SSHCredential, error) { return items, nil } -func (c *ClientsetCAStore) issueCert(signer string, name string, serial *big.Int, privateKey *pki.PrivateKey, template *x509.Certificate) (*pki.Certificate, error) { - ctx := context.TODO() - - klog.Infof("Issuing new certificate: %q", name) - - template.SerialNumber = serial - - caKeyset, err := c.readCAKeypairs(ctx, signer) - if err != nil { - return nil, err - } - - if caKeyset == nil { - return nil, fmt.Errorf("ca keyset was not found; cannot issue certificates") - } - if caKeyset.primary == nil { - return nil, fmt.Errorf("ca keyset did not have any key data; cannot issue certificates") - } - if caKeyset.primary.certificate == nil { - return nil, fmt.Errorf("ca certificate was not found; cannot issue certificates") - } - if caKeyset.primary.privateKey == nil { - return nil, fmt.Errorf("ca privateKey was not found; cannot issue certificates") - } - cert, err := pki.SignNewCertificate(privateKey, template, caKeyset.primary.certificate.Certificate, caKeyset.primary.privateKey) - if err != nil { - return nil, err - } - - if _, err := c.storeAndVerifyKeypair(ctx, name, cert, privateKey); err != nil { - return nil, err - } - - return cert, nil -} - -// storeAndVerifyKeypair writes the keypair, also re-reading it to double-check it -func (c *ClientsetCAStore) storeAndVerifyKeypair(ctx context.Context, name string, cert *pki.Certificate, privateKey *pki.PrivateKey) (*keyset, error) { - id := cert.Certificate.SerialNumber.String() - if err := c.storeKeypair(ctx, name, id, cert, privateKey); err != nil { - return nil, err - } - - // Make double-sure it round-trips - keyset, err := c.loadKeyset(ctx, name) - if err != nil { - return nil, fmt.Errorf("error fetching stored certificate: %v", err) - } - - if keyset == nil { - return nil, fmt.Errorf("stored certificate not found: %v", err) - } - if keyset.primary == nil { - return nil, fmt.Errorf("stored certificate did not have data: %v", err) - } - if keyset.primary.id != id { - return nil, fmt.Errorf("stored certificate changed concurrently (id mismatch)") - } - return keyset, nil -} - // StoreKeypair implements CAStore::StoreKeypair func (c *ClientsetCAStore) StoreKeypair(name string, cert *pki.Certificate, privateKey *pki.PrivateKey) error { ctx := context.TODO() @@ -429,18 +337,6 @@ func (c *ClientsetCAStore) FindPrivateKeyset(name string) (*kops.Keyset, error) return o, nil } -// CreateKeypair implements CAStore::CreateKeypair -func (c *ClientsetCAStore) CreateKeypair(signer string, id string, template *x509.Certificate, privateKey *pki.PrivateKey) (*pki.Certificate, error) { - serial := c.buildSerial() - - cert, err := c.issueCert(signer, id, serial, privateKey, template) - if err != nil { - return nil, err - } - - return cert, nil -} - // addKey saves the specified key to the registry func (c *ClientsetCAStore) addKey(ctx context.Context, name string, keysetType kops.KeysetType, item *kops.KeysetItem) error { create := false @@ -573,12 +469,6 @@ func (c *ClientsetCAStore) storeKeypair(ctx context.Context, name string, id str return c.addKey(ctx, name, kops.SecretTypeKeypair, item) } -// buildSerial returns a serial for use when issuing certificates -func (c *ClientsetCAStore) buildSerial() *big.Int { - t := time.Now().UnixNano() - return pki.BuildPKISerial(t) -} - // AddSSHPublicKey implements CAStore::AddSSHPublicKey func (c *ClientsetCAStore) AddSSHPublicKey(name string, pubkey []byte) error { ctx := context.TODO() diff --git a/upup/pkg/fi/fitasks/keypair.go b/upup/pkg/fi/fitasks/keypair.go index e8a775c4bc..621895e9ca 100644 --- a/upup/pkg/fi/fitasks/keypair.go +++ b/upup/pkg/fi/fitasks/keypair.go @@ -19,9 +19,11 @@ package fitasks import ( "crypto/x509" "fmt" + "math/big" "net" "sort" "strings" + "time" "k8s.io/klog" "k8s.io/kops/pkg/pki" @@ -168,7 +170,7 @@ func (_ *Keypair) Render(c *fi.Context, a, e, changes *Keypair) error { return fi.RequiredField("Name") } - template, err := e.BuildCertificateTemplate() + template, err := e.buildCertificateTemplate() if err != nil { return err } @@ -221,7 +223,9 @@ func (_ *Keypair) Render(c *fi.Context, a, e, changes *Keypair) error { signer = fi.StringValue(e.Signer.Name) } - cert, err := c.Keystore.CreateKeypair(signer, name, template, privateKey) + serial := pki.BuildPKISerial(time.Now().UnixNano()) + + cert, err := e.issueCert(c.Keystore, signer, name, serial, privateKey, template) if err != nil { return err } @@ -250,7 +254,7 @@ func (_ *Keypair) Render(c *fi.Context, a, e, changes *Keypair) error { } // BuildCertificateTemplate is responsible for constructing a certificate template -func (e *Keypair) BuildCertificateTemplate() (*x509.Certificate, error) { +func (e *Keypair) buildCertificateTemplate() (*x509.Certificate, error) { template, err := buildCertificateTemplateForType(e.Type) if err != nil { return nil, err @@ -344,3 +348,42 @@ func buildTypeDescription(cert *x509.Certificate) string { return s } + +func (e *Keypair) issueCert(keystore fi.Keystore, signer string, id string, serial *big.Int, privateKey *pki.PrivateKey, template *x509.Certificate) (*pki.Certificate, error) { + klog.Infof("Issuing new certificate: %q", id) + + template.SerialNumber = serial + + var cert *pki.Certificate + 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) + 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 { + 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 + } + } + + 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 +} diff --git a/upup/pkg/fi/vfs_castore.go b/upup/pkg/fi/vfs_castore.go index 971f5740bb..d29e300fa4 100644 --- a/upup/pkg/fi/vfs_castore.go +++ b/upup/pkg/fi/vfs_castore.go @@ -18,13 +18,11 @@ package fi import ( "bytes" - "crypto/x509" "fmt" "math/big" "os" "strings" "sync" - "time" "k8s.io/apimachinery/pkg/runtime" "k8s.io/klog" @@ -43,10 +41,6 @@ type VFSCAStore struct { mutex sync.Mutex cachedCAs map[string]*cachedEntry - - // SerialGenerator is the function for generating certificate serial numbers - // It can be replaced for testing purposes. - SerialGenerator func() *big.Int } type cachedEntry struct { @@ -64,11 +58,6 @@ func NewVFSCAStore(cluster *kops.Cluster, basedir vfs.Path) *VFSCAStore { cachedCAs: make(map[string]*cachedEntry), } - c.SerialGenerator = func() *big.Int { - t := time.Now().UnixNano() - return pki.BuildPKISerial(t) - } - return c } @@ -586,46 +575,6 @@ func mirrorSSHCredential(cluster *kops.Cluster, basedir vfs.Path, sshCredential return nil } -func (c *VFSCAStore) issueCert(signer string, id string, serial *big.Int, privateKey *pki.PrivateKey, template *x509.Certificate) (*pki.Certificate, error) { - klog.Infof("Issuing new certificate: %q", id) - - template.SerialNumber = serial - - var cert *pki.Certificate - if template.IsCA { - var err error - cert, err = pki.SignNewCertificate(privateKey, template, nil, nil) - if err != nil { - return nil, err - } - } else { - caCertificates, caPrivateKeys, err := c.readCAKeypairs(signer) - if err != nil { - return nil, err - } - - if caPrivateKeys == nil || caPrivateKeys.primary == nil || caPrivateKeys.primary.privateKey == nil { - return nil, fmt.Errorf("ca key for %q was not found; cannot issue certificates", signer) - } - if caCertificates == nil || caCertificates.primary == nil || caCertificates.primary.certificate == nil { - return nil, fmt.Errorf("ca certificate for %q was not found; cannot issue certificates", signer) - } - cert, err = pki.SignNewCertificate(privateKey, template, caCertificates.primary.certificate.Certificate, caPrivateKeys.primary.privateKey) - if err != nil { - return nil, err - } - } - - err := c.StoreKeypair(id, cert, privateKey) - if err != nil { - return nil, err - } - - // Make double-sure it round-trips - p := c.buildCertificatePath(id, serial.String()) - return c.loadOneCertificate(p) -} - func (c *VFSCAStore) StoreKeypair(name string, cert *pki.Certificate, privateKey *pki.PrivateKey) error { serial := cert.Certificate.SerialNumber.String() @@ -729,17 +678,6 @@ func (c *VFSCAStore) FindPrivateKeyset(name string) (*kops.Keyset, error) { return o, nil } -func (c *VFSCAStore) CreateKeypair(signer string, id string, template *x509.Certificate, privateKey *pki.PrivateKey) (*pki.Certificate, error) { - serial := c.SerialGenerator() - - cert, err := c.issueCert(signer, id, serial, privateKey, template) - if err != nil { - return nil, err - } - - return cert, nil -} - func (c *VFSCAStore) storePrivateKey(name string, ki *keysetItem) error { if ki.privateKey == nil { return fmt.Errorf("privateKey not provided to storeCertificate")