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.

This also fixes a bug with the mutex. File CA used a different lock than
the intermediate CA struct, so locking while writing wouldn't have
actually protected against concurrent reads. This fixes it by moving the
mutex to the certs/signer struct, and file CA reaches into that struct
to lock while writing.

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 <hblauzvern@google.com>

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
 	// cert / key pair by reading the attributes halfway through the update
 	// below.
-	fca.Certs = certs
-	fca.Signer = signer
+	scm.Certs = certs
+	scm.Signer = signer
 }

pkg/ca/fileca/fileca_test.go

@@ -57,18 +57,21 @@ func TestCertUpdate(t *testing.T) {
 		t.Fatal(`Bad CA type`)
 	}
 
-	key := fca.Signer
+	_, key := fca.GetSignerWithChain()
+
+	// key := fca.Signer
 	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()
+	// key = fca.Signer
 
 	if _, ok = key.(*ecdsa.PrivateKey); !ok {
 		t.Fatal(`file CA should have been updated with ecdsa key`)

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
 }

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)
 		}

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-----

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-----

pkg/ca/fileca/watch.go

@@ -25,15 +25,15 @@ 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
 				// mismatch
 				continue
 			}
 
-			callback(certs, key)
+			callback(signerWithMutex.Certs, signerWithMutex.Signer)
 		}
 	}
 }

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 {

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())
 

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
 	}
 

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

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
+}