Prevent data races with multiple PKCS7 encryption operations

scep/authority.go

@@ -28,7 +28,8 @@ type Authority struct {
 	decrypterCertificate *x509.Certificate
 	scepProvisionerNames []string
 
-	mu sync.RWMutex
+	provisionersMutex        sync.RWMutex
+	encryptionAlgorithmMutex sync.Mutex
 }
 
 type authorityKey struct{}
@@ -86,8 +87,8 @@ func (a *Authority) Validate() error {
 		return nil
 	}
 
-	a.mu.RLock()
-	defer a.mu.RUnlock()
+	a.provisionersMutex.RLock()
+	defer a.provisionersMutex.RUnlock()
 
 	noDefaultDecrypterAvailable := a.defaultDecrypter == nil
 	for _, name := range a.scepProvisionerNames {
@@ -118,8 +119,8 @@ func (a *Authority) UpdateProvisioners(scepProvisionerNames []string) {
 		return
 	}
 
-	a.mu.Lock()
-	defer a.mu.Unlock()
+	a.provisionersMutex.Lock()
+	defer a.provisionersMutex.Unlock()
 
 	a.scepProvisionerNames = scepProvisionerNames
 }
@@ -307,20 +308,13 @@ func (a *Authority) SignCSR(ctx context.Context, csr *x509.CertificateRequest, m
 	// and create a degenerate cert structure
 	deg, err := microscep.DegenerateCertificates([]*x509.Certificate{cert})
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed generating degenerate certificate: %w", err)
 	}
 
-	// apparently the pkcs7 library uses a global default setting for the content encryption
-	// algorithm to use when en- or decrypting data. We need to restore the current setting after
-	// the cryptographic operation, so that other usages of the library are not influenced by
-	// this call to Encrypt(). We are not required to use the same algorithm the SCEP client uses.
-	encryptionAlgorithmToRestore := pkcs7.ContentEncryptionAlgorithm
-	pkcs7.ContentEncryptionAlgorithm = p.GetContentEncryptionAlgorithm()
-	e7, err := pkcs7.Encrypt(deg, msg.P7.Certificates)
+	e7, err := a.encrypt(deg, msg.P7.Certificates, p.GetContentEncryptionAlgorithm())
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed encrypting degenerate certificate: %w", err)
 	}
-	pkcs7.ContentEncryptionAlgorithm = encryptionAlgorithmToRestore
 
 	// PKIMessageAttributes to be signed
 	config := pkcs7.SignerInfoConfig{
@@ -391,6 +385,28 @@ func (a *Authority) SignCSR(ctx context.Context, csr *x509.CertificateRequest, m
 	return crepMsg, nil
 }
 
+func (a *Authority) encrypt(content []byte, recipients []*x509.Certificate, algorithm int) ([]byte, error) {
+	// apparently the pkcs7 library uses a global default setting for the content encryption
+	// algorithm to use when en- or decrypting data. We need to restore the current setting after
+	// the cryptographic operation, so that other usages of the library are not influenced by
+	// this call to Encrypt(). We are not required to use the same algorithm the SCEP client uses.
+	a.encryptionAlgorithmMutex.Lock()
+	defer a.encryptionAlgorithmMutex.Unlock()
+
+	encryptionAlgorithmToRestore := pkcs7.ContentEncryptionAlgorithm
+	defer func() {
+		pkcs7.ContentEncryptionAlgorithm = encryptionAlgorithmToRestore
+	}()
+
+	pkcs7.ContentEncryptionAlgorithm = algorithm
+	e7, err := pkcs7.Encrypt(content, recipients)
+	if err != nil {
+		return nil, err
+	}
+
+	return e7, nil
+}
+
 // CreateFailureResponse creates an appropriately signed reply for PKI operations
 func (a *Authority) CreateFailureResponse(ctx context.Context, _ *x509.CertificateRequest, msg *PKIMessage, info FailInfoName, infoText string) (*PKIMessage, error) {
 	config := pkcs7.SignerInfoConfig{

scep/authority_test.go

@@ -0,0 +1,73 @@
+package scep
+
+import (
+	"crypto/x509"
+	"crypto/x509/pkix"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.mozilla.org/pkcs7"
+	"go.step.sm/crypto/keyutil"
+	"go.step.sm/crypto/minica"
+	"go.step.sm/crypto/randutil"
+)
+
+func generateContent(t *testing.T, size int) []byte {
+	t.Helper()
+	b, err := randutil.Bytes(size)
+	require.NoError(t, err)
+	return b
+}
+
+func generateRecipients(t *testing.T) []*x509.Certificate {
+	ca, err := minica.New()
+	require.NoError(t, err)
+	s, err := keyutil.GenerateSigner("RSA", "", 2048)
+	require.NoError(t, err)
+	tmpl := &x509.Certificate{
+		PublicKey: s.Public(),
+		Subject:   pkix.Name{CommonName: "Test PKCS#7 Encryption"},
+	}
+	cert, err := ca.Sign(tmpl)
+	require.NoError(t, err)
+	return []*x509.Certificate{cert}
+}
+
+func TestAuthority_encrypt(t *testing.T) {
+	t.Parallel()
+	a := &Authority{}
+	recipients := generateRecipients(t)
+	type args struct {
+		content    []byte
+		recipients []*x509.Certificate
+		algorithm  int
+	}
+	tests := []struct {
+		name    string
+		args    args
+		wantErr bool
+	}{
+		{"alg-0", args{generateContent(t, 32), recipients, pkcs7.EncryptionAlgorithmDESCBC}, false},
+		{"alg-1", args{generateContent(t, 32), recipients, pkcs7.EncryptionAlgorithmAES128CBC}, false},
+		{"alg-2", args{generateContent(t, 32), recipients, pkcs7.EncryptionAlgorithmAES256CBC}, false},
+		{"alg-3", args{generateContent(t, 32), recipients, pkcs7.EncryptionAlgorithmAES128GCM}, false},
+		{"alg-4", args{generateContent(t, 32), recipients, pkcs7.EncryptionAlgorithmAES256GCM}, false},
+		{"alg-unknown", args{generateContent(t, 32), recipients, 42}, true},
+	}
+	for _, tt := range tests {
+		tc := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			got, err := a.encrypt(tc.args.content, tc.args.recipients, tc.args.algorithm)
+			if tc.wantErr {
+				assert.Error(t, err)
+				assert.Nil(t, got)
+				return
+			}
+
+			assert.NoError(t, err)
+			assert.NotEmpty(t, got)
+		})
+	}
+}