From bf55b2a5fb3938fee486ca9a9cc898f181e90eba Mon Sep 17 00:00:00 2001 From: Hayden Blauzvern Date: Fri, 10 Jun 2022 04:55:57 +0000 Subject: [PATCH] Add interface for certs/signer fetching to remove mutex For all but the file CA implementation, locking when retrieving the certs/signer is not necessary. This refactors the intermediate CA struct by embedding an interface to fetch the certs/signer. Each CA type can use either the basic structure that simply returns certs/signer, or one that has a mutex to protect access to the variables. Also small test fix - go 1.18 checks for duplicate extensions in certs now, so I had to regenerate a test certificate. Signed-off-by: Hayden Blauzvern --- pkg/ca/fileca/fileca.go | 13 +++-- pkg/ca/fileca/fileca_test.go | 9 +-- pkg/ca/fileca/load.go | 15 ++--- pkg/ca/fileca/load_test.go | 6 +- pkg/ca/fileca/testdata/openssl-cert.pem | 15 +++-- pkg/ca/fileca/testdata/openssl-key.pem | 6 +- pkg/ca/fileca/watch.go | 4 +- pkg/ca/intermediateca/intermediateca.go | 25 ++------- pkg/ca/intermediateca/intermediateca_test.go | 15 ++--- pkg/ca/kmsca/kmsca.go | 10 +++- pkg/ca/kmsca/kmsca_test.go | 7 ++- pkg/ca/signercerts.go | 58 ++++++++++++++++++++ pkg/ca/signercerts_test.go | 51 +++++++++++++++++ 13 files changed, 169 insertions(+), 65 deletions(-) create mode 100644 pkg/ca/signercerts.go create mode 100644 pkg/ca/signercerts_test.go diff --git a/pkg/ca/fileca/fileca.go b/pkg/ca/fileca/fileca.go index 7f65e243c..35c83cc93 100644 --- a/pkg/ca/fileca/fileca.go +++ b/pkg/ca/fileca/fileca.go @@ -35,7 +35,7 @@ func NewFileCA(certPath, keyPath, keyPass string, watch bool) (ca.CertificateAut var fca fileCA var err error - fca.Certs, fca.Signer, err = loadKeyPair(certPath, keyPath, keyPass) + fca.SignerWithChain, err = loadKeyPair(certPath, keyPath, keyPass) if err != nil { return nil, err } @@ -61,12 +61,13 @@ func NewFileCA(certPath, keyPath, keyPass string, watch bool) (ca.CertificateAut } func (fca *fileCA) updateX509KeyPair(certs []*x509.Certificate, signer crypto.Signer) { - fca.Lock() - defer fca.Unlock() + scm := fca.SignerWithChain.(*ca.SignerCertsMutex) + scm.Lock() + defer scm.Unlock() - // NB: We use the RWLock to unsure a reading thread can't get a mismatching + // NB: We use a lock to ensure a reading thread can't get a mismatching // cert / key pair by reading the attributes halfway through the update // below. - fca.Certs = certs - fca.Signer = signer + scm.Certs = certs + scm.Signer = signer } diff --git a/pkg/ca/fileca/fileca_test.go b/pkg/ca/fileca/fileca_test.go index 49f29b5ba..d297ae983 100644 --- a/pkg/ca/fileca/fileca_test.go +++ b/pkg/ca/fileca/fileca_test.go @@ -57,18 +57,19 @@ func TestCertUpdate(t *testing.T) { t.Fatal(`Bad CA type`) } - key := fca.Signer + _, key := fca.GetSignerWithChain() + if _, ok = key.(ed25519.PrivateKey); !ok { t.Error(`first key should have been an ed25519 key`) } - cert, key, err := loadKeyPair(newCert, newKey, testKeyPass) + signerWithMutex, err := loadKeyPair(newCert, newKey, testKeyPass) if err != nil { t.Fatal(`Failed to load new keypair`) } - fca.updateX509KeyPair(cert, key) - key = fca.Signer + fca.updateX509KeyPair(signerWithMutex.Certs, signerWithMutex.Signer) + _, key = fca.GetSignerWithChain() if _, ok = key.(*ecdsa.PrivateKey); !ok { t.Fatal(`file CA should have been updated with ecdsa key`) diff --git a/pkg/ca/fileca/load.go b/pkg/ca/fileca/load.go index ed8cbf638..dff6b1fe7 100644 --- a/pkg/ca/fileca/load.go +++ b/pkg/ca/fileca/load.go @@ -23,12 +23,13 @@ import ( "os" "path/filepath" + "github.com/sigstore/fulcio/pkg/ca" "github.com/sigstore/fulcio/pkg/ca/intermediateca" "github.com/sigstore/sigstore/pkg/cryptoutils" "go.step.sm/crypto/pemutil" ) -func loadKeyPair(certPath, keyPath, keyPass string) ([]*x509.Certificate, crypto.Signer, error) { +func loadKeyPair(certPath, keyPath, keyPass string) (*ca.SignerCertsMutex, error) { var ( certs []*x509.Certificate err error @@ -37,29 +38,29 @@ func loadKeyPair(certPath, keyPath, keyPass string) ([]*x509.Certificate, crypto data, err := os.ReadFile(filepath.Clean(certPath)) if err != nil { - return nil, nil, err + return nil, err } certs, err = cryptoutils.LoadCertificatesFromPEM(bytes.NewReader(data)) if err != nil { - return nil, nil, err + return nil, err } { opaqueKey, err := pemutil.Read(keyPath, pemutil.WithPassword([]byte(keyPass))) if err != nil { - return nil, nil, err + return nil, err } var ok bool key, ok = opaqueKey.(crypto.Signer) if !ok { - return nil, nil, errors.New(`fileca: loaded private key can't be used to sign`) + return nil, errors.New(`fileca: loaded private key can't be used to sign`) } } if err := intermediateca.VerifyCertChain(certs, key); err != nil { - return nil, nil, err + return nil, err } - return certs, key, nil + return &ca.SignerCertsMutex{Certs: certs, Signer: key}, nil } diff --git a/pkg/ca/fileca/load_test.go b/pkg/ca/fileca/load_test.go index c6fc9f552..f3525d602 100644 --- a/pkg/ca/fileca/load_test.go +++ b/pkg/ca/fileca/load_test.go @@ -34,9 +34,9 @@ func TestValidLoadKeyPair(t *testing.T) { keyPath := fmt.Sprintf("testdata/%s-key.pem", keypair) certPath := fmt.Sprintf("testdata/%s-cert.pem", keypair) - _, _, err := loadKeyPair(certPath, keyPath, testKeyPass) + _, err := loadKeyPair(certPath, keyPath, testKeyPass) if err != nil { - t.Errorf("Failed to load key pair of type %s", keypair) + t.Errorf("Failed to load key pair of type %s: %v", keypair, err) } } } @@ -52,7 +52,7 @@ func TestInvalidLoadKeyPair(t *testing.T) { keyPath := fmt.Sprintf("testdata/%s-key.pem", keypair) certPath := fmt.Sprintf("testdata/%s-cert.pem", keypair) - _, _, err := loadKeyPair(certPath, keyPath, testKeyPass) + _, err := loadKeyPair(certPath, keyPath, testKeyPass) if err == nil { t.Errorf("Expected invalid key pair of type %s to fail to load", keypair) } diff --git a/pkg/ca/fileca/testdata/openssl-cert.pem b/pkg/ca/fileca/testdata/openssl-cert.pem index 92686539b..e66d72880 100644 --- a/pkg/ca/fileca/testdata/openssl-cert.pem +++ b/pkg/ca/fileca/testdata/openssl-cert.pem @@ -1,10 +1,9 @@ -----BEGIN CERTIFICATE----- -MIIBTzCCAQGgAwIBAgIUBO1MTUDAijUkpFiDr53xJJmPxLAwBQYDK2VwMBIxEDAO -BgNVBAMMB29wZW5zc2wwIBcNMjIwMTE4MjAzOTU1WhgPMjEyMTEyMjUyMDM5NTVa -MBIxEDAOBgNVBAMMB29wZW5zc2wwKjAFBgMrZXADIQBCrnxleddPY3TkNV8DtEWJ -9Avw/w17xtYhgVqzBOdOTaNnMGUwHQYDVR0OBBYEFA9ssW0mYnwHXMmdlADKaJaY -Ie7TMB8GA1UdIwQYMBaAFA9ssW0mYnwHXMmdlADKaJaYIe7TMA8GA1UdEwEB/wQF -MAMBAf8wEgYDVR0TAQH/BAgwBgEB/wIBATAFBgMrZXADQQCo2r09KAdu78YhQF1m -SVgxm1jgsoA/tAjKmzLi6sAuOV57WDd1vmMpu1ggqeaeQhzXNH4Qm76c+XUPBY2n -fkUB +MIIBPTCB8KADAgECAhQOFCGq1F/UFACHhW2z51EE+dodDTAFBgMrZXAwEjEQMA4G +A1UEAwwHb3BlbnNzbDAgFw0yMjA2MTAwNDUzMzZaGA8yMTIyMDUxNzA0NTMzNlow +EjEQMA4GA1UEAwwHb3BlbnNzbDAqMAUGAytlcAMhAHoP6VgvmjkF7TQktmsqA2WD +FKuEus/Uf1IV+heG91lQo1YwVDAdBgNVHQ4EFgQUZx7Tvdvg0FtL4NwBHyq+vEdA +KUswHwYDVR0jBBgwFoAUZx7Tvdvg0FtL4NwBHyq+vEdAKUswEgYDVR0TAQH/BAgw +BgEB/wIBATAFBgMrZXADQQDWdhDWFYcX5dDmHuPi3MpgX5lyR+7yOA5keUVWQU8U +62DPFRRsfcpmWELx/RNQD/OgdIUZ9/YTPgFBoTngeD4G -----END CERTIFICATE----- diff --git a/pkg/ca/fileca/testdata/openssl-key.pem b/pkg/ca/fileca/testdata/openssl-key.pem index 97dce41cb..981a4c855 100644 --- a/pkg/ca/fileca/testdata/openssl-key.pem +++ b/pkg/ca/fileca/testdata/openssl-key.pem @@ -1,5 +1,5 @@ -----BEGIN ENCRYPTED PRIVATE KEY----- -MIGKME4GCSqGSIb3DQEFDTBBMCkGCSqGSIb3DQEFDDAcBAifqugsEv+5eQICCAAw -DAYIKoZIhvcNAgkFADAUBggqhkiG9w0DBwQIsETz73+d0CkEOEfccSS2FSSCYchJ -61nyishJ4/AFxpsG5935bt+UvfcaUALH1RKQkwNvoGbry6afYY+qvW+LNy6g +MIGKME4GCSqGSIb3DQEFDTBBMCkGCSqGSIb3DQEFDDAcBAjrbQwLQsaVYgICCAAw +DAYIKoZIhvcNAgkFADAUBggqhkiG9w0DBwQIo5IbVAAU0f8EOEZix/CL4zM7FCfN +RlNM1GD99ZEouGO5jEae7q0medijaG1IbD4y4B90nLuQfc4aDlIMG5AyA8wP -----END ENCRYPTED PRIVATE KEY----- diff --git a/pkg/ca/fileca/watch.go b/pkg/ca/fileca/watch.go index 2a8e68404..8eff8a088 100644 --- a/pkg/ca/fileca/watch.go +++ b/pkg/ca/fileca/watch.go @@ -25,7 +25,7 @@ import ( func ioWatch(certPath, keyPath, keyPass string, watcher *fsnotify.Watcher, callback func([]*x509.Certificate, crypto.Signer)) { for event := range watcher.Events { if event.Op&fsnotify.Write == fsnotify.Write { - certs, key, err := loadKeyPair(certPath, keyPath, keyPass) + signerWithMutex, err := loadKeyPair(certPath, keyPath, keyPass) if err != nil { // Don't sweat it if this errors out. One file might // have updated and the other isn't causing a key-pair @@ -33,7 +33,7 @@ func ioWatch(certPath, keyPath, keyPass string, watcher *fsnotify.Watcher, callb continue } - callback(certs, key) + callback(signerWithMutex.Certs, signerWithMutex.Signer) } } } diff --git a/pkg/ca/intermediateca/intermediateca.go b/pkg/ca/intermediateca/intermediateca.go index 31c058158..259201f8d 100644 --- a/pkg/ca/intermediateca/intermediateca.go +++ b/pkg/ca/intermediateca/intermediateca.go @@ -23,7 +23,6 @@ import ( "crypto/x509/pkix" "encoding/asn1" "errors" - "sync" ct "github.com/google/certificate-transparency-go" cttls "github.com/google/certificate-transparency-go/tls" @@ -42,11 +41,8 @@ var ( ) type IntermediateCA struct { - sync.RWMutex - - // certs is a chain of certificates from intermediate to root - Certs []*x509.Certificate - Signer crypto.Signer + // contains the chain of certificates and signer + ca.SignerWithChain } func (ica *IntermediateCA) CreatePrecertificate(ctx context.Context, principal identity.Principal, publicKey crypto.PublicKey) (*ca.CodeSigningPreCertificate, error) { @@ -55,7 +51,7 @@ func (ica *IntermediateCA) CreatePrecertificate(ctx context.Context, principal i return nil, err } - certChain, privateKey := ica.getX509KeyPair() + certChain, privateKey := ica.GetSignerWithChain() // Append poison extension cert.ExtraExtensions = append(cert.ExtraExtensions, pkix.Extension{ @@ -136,7 +132,7 @@ func (ica *IntermediateCA) CreateCertificate(ctx context.Context, principal iden return nil, err } - certChain, privateKey := ica.getX509KeyPair() + certChain, privateKey := ica.GetSignerWithChain() finalCertBytes, err := x509.CreateCertificate(rand.Reader, cert, certChain[0], publicKey, privateKey) if err != nil { @@ -147,17 +143,8 @@ func (ica *IntermediateCA) CreateCertificate(ctx context.Context, principal iden } func (ica *IntermediateCA) Root(ctx context.Context) ([]byte, error) { - ica.RLock() - defer ica.RUnlock() - - return cryptoutils.MarshalCertificatesToPEM(ica.Certs) -} - -func (ica *IntermediateCA) getX509KeyPair() ([]*x509.Certificate, crypto.Signer) { - ica.RLock() - defer ica.RUnlock() - - return ica.Certs, ica.Signer + certs, _ := ica.GetSignerWithChain() + return cryptoutils.MarshalCertificatesToPEM(certs) } func VerifyCertChain(certs []*x509.Certificate, signer crypto.Signer) error { diff --git a/pkg/ca/intermediateca/intermediateca_test.go b/pkg/ca/intermediateca/intermediateca_test.go index bc67f8896..4da53ca47 100644 --- a/pkg/ca/intermediateca/intermediateca_test.go +++ b/pkg/ca/intermediateca/intermediateca_test.go @@ -27,6 +27,7 @@ import ( "testing" ct "github.com/google/certificate-transparency-go" + "github.com/sigstore/fulcio/pkg/ca" "github.com/sigstore/fulcio/pkg/ca/x509ca" "github.com/sigstore/fulcio/pkg/test" "github.com/sigstore/sigstore/pkg/cryptoutils" @@ -48,8 +49,7 @@ func TestIntermediateCARoot(t *testing.T) { } ica := IntermediateCA{ - Certs: certChain, - Signer: signer, + SignerWithChain: &ca.SignerCerts{Certs: certChain, Signer: signer}, } rootBytes, err := ica.Root(context.TODO()) @@ -62,7 +62,7 @@ func TestIntermediateCARoot(t *testing.T) { } } -func TestIntermediateCAGetX509KeyPair(t *testing.T) { +func TestIntermediateCAGetSignerWithChain(t *testing.T) { signer, _, err := signature.NewDefaultECDSASignerVerifier() if err != nil { t.Fatalf("unexpected error generating signer: %v", err) @@ -73,11 +73,10 @@ func TestIntermediateCAGetX509KeyPair(t *testing.T) { certChain := []*x509.Certificate{subCert, rootCert} ica := IntermediateCA{ - Certs: certChain, - Signer: signer, + SignerWithChain: &ca.SignerCerts{Certs: certChain, Signer: signer}, } - foundCertChain, foundSigner := ica.getX509KeyPair() + foundCertChain, foundSigner := ica.GetSignerWithChain() if !reflect.DeepEqual(certChain, foundCertChain) { t.Fatal("expected cert chains to be equivalent") @@ -177,7 +176,9 @@ func TestCreatePrecertificateAndIssueFinalCertificate(t *testing.T) { priv, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) certChain := []*x509.Certificate{subCert, rootCert} - ica := IntermediateCA{Certs: certChain, Signer: subKey} + ica := IntermediateCA{ + SignerWithChain: &ca.SignerCerts{Certs: certChain, Signer: subKey}, + } precsc, err := ica.CreatePrecertificate(context.TODO(), testPrincipal{}, priv.Public()) diff --git a/pkg/ca/kmsca/kmsca.go b/pkg/ca/kmsca/kmsca.go index dc1bb323c..6c52c266f 100644 --- a/pkg/ca/kmsca/kmsca.go +++ b/pkg/ca/kmsca/kmsca.go @@ -49,17 +49,21 @@ func NewKMSCA(ctx context.Context, kmsKey, certPath string) (ca.CertificateAutho if err != nil { return nil, err } - ica.Signer = signer + + sc := ca.SignerCerts{} + ica.SignerWithChain = &sc + + sc.Signer = signer data, err := os.ReadFile(filepath.Clean(certPath)) if err != nil { return nil, err } - ica.Certs, err = cryptoutils.LoadCertificatesFromPEM(bytes.NewReader(data)) + sc.Certs, err = cryptoutils.LoadCertificatesFromPEM(bytes.NewReader(data)) if err != nil { return nil, err } - if err := intermediateca.VerifyCertChain(ica.Certs, ica.Signer); err != nil { + if err := intermediateca.VerifyCertChain(sc.Certs, sc.Signer); err != nil { return nil, err } diff --git a/pkg/ca/kmsca/kmsca_test.go b/pkg/ca/kmsca/kmsca_test.go index eb8eb6b0d..2fcdfeb94 100644 --- a/pkg/ca/kmsca/kmsca_test.go +++ b/pkg/ca/kmsca/kmsca_test.go @@ -65,11 +65,12 @@ func TestNewKMSCA(t *testing.T) { // Expect signer and certificate's public keys match ica := ca.(*kmsCA) - if err := cryptoutils.EqualKeys(ica.Signer.Public(), subKey.Public()); err != nil { + certs, signer := ica.GetSignerWithChain() + if err := cryptoutils.EqualKeys(signer.Public(), subKey.Public()); err != nil { t.Fatalf("keys between CA and signer do not match: %v", err) } - if !reflect.DeepEqual(ica.Certs, []*x509.Certificate{subCert, rootCert}) { - t.Fatalf("expected certificate chains to match: %v", err) + if !reflect.DeepEqual(certs, []*x509.Certificate{subCert, rootCert}) { + t.Fatalf("expected certificate chains to match") } // Failure: Mismatch between signer and certificate key diff --git a/pkg/ca/signercerts.go b/pkg/ca/signercerts.go new file mode 100644 index 000000000..149cc8918 --- /dev/null +++ b/pkg/ca/signercerts.go @@ -0,0 +1,58 @@ +// Copyright 2022 The Sigstore Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +package ca + +import ( + "crypto" + "crypto/x509" + "sync" +) + +// SignerWithChain provides a getter for a CA's certificate chain and signing key. +type SignerWithChain interface { + GetSignerWithChain() ([]*x509.Certificate, crypto.Signer) +} + +// SignerCerts holds a certificate chain and signer. +type SignerCerts struct { + // Signer signs issued certificates + Signer crypto.Signer + // Certs contains the chain of certificates from intermediate to root + Certs []*x509.Certificate +} + +func (s *SignerCerts) GetSignerWithChain() ([]*x509.Certificate, crypto.Signer) { + return s.Certs, s.Signer +} + +// SignerCertsMutex holds a certificate chain and signer, and holds a reader lock +// when accessing the chain and signer. Use if a separate thread can concurrently +// update the chain and signer. +type SignerCertsMutex struct { + sync.RWMutex + + // Certs contains the chain of certificates from intermediate to root + Certs []*x509.Certificate + // Signer signs issued certificates + Signer crypto.Signer +} + +func (s *SignerCertsMutex) GetSignerWithChain() ([]*x509.Certificate, crypto.Signer) { + s.RLock() + defer s.RUnlock() + + return s.Certs, s.Signer +} diff --git a/pkg/ca/signercerts_test.go b/pkg/ca/signercerts_test.go new file mode 100644 index 000000000..916ff6d84 --- /dev/null +++ b/pkg/ca/signercerts_test.go @@ -0,0 +1,51 @@ +// Copyright 2022 The Sigstore Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +package ca + +import ( + "crypto/x509" + "reflect" + "testing" + + "github.com/sigstore/fulcio/pkg/test" + "github.com/sigstore/sigstore/pkg/cryptoutils" +) + +func TestSignerCerts(t *testing.T) { + rootCert, rootKey, _ := test.GenerateRootCA() + s := SignerCerts{Certs: []*x509.Certificate{rootCert}, Signer: rootKey} + + certs, signer := s.GetSignerWithChain() + if err := cryptoutils.EqualKeys(signer.Public(), rootKey.Public()); err != nil { + t.Fatalf("keys between CA and signer do not match: %v", err) + } + if !reflect.DeepEqual(certs, []*x509.Certificate{rootCert}) { + t.Fatalf("expected certificate chains to match") + } +} + +func TestSignerCertsMutex(t *testing.T) { + rootCert, rootKey, _ := test.GenerateRootCA() + s := SignerCertsMutex{Certs: []*x509.Certificate{rootCert}, Signer: rootKey} + + certs, signer := s.GetSignerWithChain() + if err := cryptoutils.EqualKeys(signer.Public(), rootKey.Public()); err != nil { + t.Fatalf("keys between CA and signer do not match: %v", err) + } + if !reflect.DeepEqual(certs, []*x509.Certificate{rootCert}) { + t.Fatalf("expected certificate chains to match") + } +}