Crypto: alter Storage interface to create keys inside key store (#3120)

crypto/crypto.go

@@ -21,9 +21,6 @@ package crypto
 import (
 	"context"
 	"crypto"
-	"crypto/ecdsa"
-	"crypto/elliptic"
-	"crypto/rand"
 	"errors"
 	"fmt"
 	"github.com/lestrrat-go/jwx/v2/jwk"
@@ -153,24 +150,13 @@ func (client *Crypto) Configure(config core.ServerConfig) error {
 // Stores the private key, returns the public basicKey.
 // It returns an error when a key with the resulting ID already exists.
 func (client *Crypto) New(ctx context.Context, namingFunc KIDNamingFunc) (Key, error) {
-	keyPair, kid, err := generateKeyPairAndKID(namingFunc)
+	publicKey, kid, err := client.storage.NewPrivateKey(ctx, namingFunc)
 	if err != nil {
 		return nil, err
 	}
-
-	audit.Log(ctx, log.Logger(), audit.CryptoNewKeyEvent).Infof("Generating new key pair: %s", kid)
-	exists, err := client.storage.PrivateKeyExists(ctx, kid)
-	if err != nil {
-		return nil, err
-	}
-	if exists {
-		return nil, errors.New("key with the given ID already exists")
-	}
-	if err = client.storage.SavePrivateKey(ctx, kid, keyPair); err != nil {
-		return nil, fmt.Errorf("could not create new keypair: could not save private key: %w", err)
-	}
+	audit.Log(ctx, log.Logger(), audit.CryptoNewKeyEvent).Infof("Generated new key pair: %s", kid)
 	return basicKey{
-		publicKey: keyPair.Public(),
+		publicKey: publicKey,
 		kid:       kid,
 	}, nil
 }
@@ -181,30 +167,10 @@ func (client *Crypto) Delete(ctx context.Context, kid string) error {
 	return client.storage.DeletePrivateKey(ctx, kid)
 }
 
-func generateKeyPairAndKID(namingFunc KIDNamingFunc) (*ecdsa.PrivateKey, string, error) {
-	keyPair, err := generateECKeyPair()
-	if err != nil {
-		return nil, "", err
-	}
-
-	kid, err := namingFunc(keyPair.Public())
-	if err != nil {
-		return nil, "", err
-	}
-	log.Logger().
-		WithField(core.LogFieldKeyID, kid).
-		Debug("Generated new key pair")
-	return keyPair, kid, nil
-}
-
-func generateECKeyPair() (*ecdsa.PrivateKey, error) {
-	return ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
-}
-
 // GenerateJWK a new in-memory key pair and returns it as JWK.
 // It sets the alg field of the JWK.
 func GenerateJWK() (jwk.Key, error) {
-	keyPair, err := generateECKeyPair()
+	keyPair, err := spi.GenerateKeyPair()
 	if err != nil {
 		return nil, nil
 	}

crypto/crypto_test.go

@@ -20,8 +20,6 @@ package crypto
 
 import (
 	"context"
-	"crypto"
-	"errors"
 	"github.com/nuts-foundation/nuts-node/audit"
 	"github.com/nuts-foundation/nuts-node/crypto/storage/fs"
 	"github.com/nuts-foundation/nuts-node/crypto/storage/spi"
@@ -81,7 +79,7 @@ func TestCrypto_New(t *testing.T) {
 		assert.NoError(t, err)
 		assert.NotNil(t, key.Public())
 		assert.Equal(t, kid, key.KID())
-		auditLogs.AssertContains(t, ModuleName, "CreateNewKey", audit.TestActor, "Generating new key pair: kid")
+		auditLogs.AssertContains(t, ModuleName, "CreateNewKey", audit.TestActor, "Generated new key pair: kid")
 	})
 
 	t.Run("error - invalid KID", func(t *testing.T) {
@@ -92,38 +90,6 @@ func TestCrypto_New(t *testing.T) {
 		assert.ErrorContains(t, err, "invalid key ID")
 		assert.Nil(t, key)
 	})
-
-	t.Run("error - NamingFunction returns err", func(t *testing.T) {
-		errorNamingFunc := func(key crypto.PublicKey) (string, error) {
-			return "", errors.New("b00m!")
-		}
-		_, err := client.New(ctx, errorNamingFunc)
-		assert.Error(t, err)
-	})
-
-	t.Run("error - save public key returns an error", func(t *testing.T) {
-		ctrl := gomock.NewController(t)
-		storageMock := spi.NewMockStorage(ctrl)
-		storageMock.EXPECT().PrivateKeyExists(ctx, "123").Return(false, nil)
-		storageMock.EXPECT().SavePrivateKey(ctx, gomock.Any(), gomock.Any()).Return(errors.New("foo"))
-
-		client := &Crypto{storage: storageMock}
-		key, err := client.New(ctx, StringNamingFunc("123"))
-		assert.Nil(t, key)
-		assert.Error(t, err)
-		assert.Equal(t, "could not create new keypair: could not save private key: foo", err.Error())
-	})
-
-	t.Run("error - ID already in use", func(t *testing.T) {
-		ctrl := gomock.NewController(t)
-		storageMock := spi.NewMockStorage(ctrl)
-		storageMock.EXPECT().PrivateKeyExists(ctx, "123").Return(true, nil)
-
-		client := &Crypto{storage: storageMock}
-		key, err := client.New(ctx, StringNamingFunc("123"))
-		assert.Nil(t, key)
-		assert.EqualError(t, err, "key with the given ID already exists", err)
-	})
 }
 
 func TestCrypto_Delete(t *testing.T) {

crypto/ecies_test.go

@@ -19,13 +19,14 @@
 package crypto
 
 import (
+	"github.com/nuts-foundation/nuts-node/crypto/storage/spi"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 )
 
 func TestEciesEncrypt(t *testing.T) {
-	key, err := generateECKeyPair()
+	key, err := spi.GenerateKeyPair()
 	assert.NoError(t, err)
 
 	cipherText1, err := EciesEncrypt(&key.PublicKey, []byte("hello world"))
@@ -38,7 +39,7 @@ func TestEciesEncrypt(t *testing.T) {
 }
 
 func TestEciesDecrypt(t *testing.T) {
-	key, err := generateECKeyPair()
+	key, err := spi.GenerateKeyPair()
 	assert.NoError(t, err)
 
 	cipherText, err := EciesEncrypt(&key.PublicKey, []byte("hello world"))

crypto/ephemeral.go

@@ -19,9 +19,13 @@
 
 package crypto
 
+import (
+	"github.com/nuts-foundation/nuts-node/crypto/storage/spi"
+)
+
 // NewEphemeralKey returns a Key for single use.
 func NewEphemeralKey(namingFunc KIDNamingFunc) (Key, error) {
-	keyPair, kid, err := generateKeyPairAndKID(namingFunc)
+	keyPair, kid, err := spi.GenerateKeyPairAndKID(namingFunc)
 	if err != nil {
 		return nil, err
 	}

crypto/storage/external/client.go

@@ -42,6 +42,10 @@ type APIClient struct {
 	httpClient *ClientWithResponses
 }
 
+func (c APIClient) NewPrivateKey(ctx context.Context, namingFunc func(crypto.PublicKey) (string, error)) (crypto.PublicKey, string, error) {
+	return spi.GenerateAndStore(ctx, c, namingFunc)
+}
+
 func (c APIClient) Name() string {
 	return "Crypto"
 }

crypto/storage/fs/fs.go

@@ -92,6 +92,10 @@ func NewFileSystemBackend(fspath string) (spi.Storage, error) {
 	return fsc, nil
 }
 
+func (fsc fileSystemBackend) NewPrivateKey(ctx context.Context, namingFunc func(crypto.PublicKey) (string, error)) (crypto.PublicKey, string, error) {
+	return spi.GenerateAndStore(ctx, fsc, namingFunc)
+}
+
 func (fsc fileSystemBackend) PrivateKeyExists(_ context.Context, kid string) (bool, error) {
 	_, err := os.Stat(fsc.getEntryPath(kid, privateKeyEntry))
 	if errors.Is(err, os.ErrNotExist) {

crypto/storage/spi/interface.go

@@ -21,6 +21,9 @@ package spi
 import (
 	"context"
 	"crypto"
+	"crypto/ecdsa"
+	"crypto/elliptic"
+	"crypto/rand"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -42,11 +45,15 @@ var KidPattern = regexp.MustCompile(`^(?:(?:[\da-zA-Z_\- :#.])|(?:%[0-9a-fA-F]{2
 // Storage interface containing functions for storing and retrieving keys.
 type Storage interface {
 	core.HealthCheckable
+	// NewPrivateKey creates a new private key and returns its handler as an implementation of crypto.Signer.
+	// It should be preferred over generating a key in the application and saving it to the storage,
+	// as it allows for unexportable (safer) keys. If the resulting kid already exists, it returns an error
+	NewPrivateKey(ctx context.Context, namingFunc func(crypto.PublicKey) (string, error)) (crypto.PublicKey, string, error)
 	// GetPrivateKey from the storage backend and return its handler as an implementation of crypto.Signer.
 	GetPrivateKey(ctx context.Context, kid string) (crypto.Signer, error)
 	// PrivateKeyExists checks if the private key indicated with the kid is stored in the storage backend.
 	PrivateKeyExists(ctx context.Context, kid string) (bool, error)
-	// SavePrivateKey stores the key under the kid in the storage backend.
+	// SavePrivateKey imports the key under the kid in the storage backend.
 	SavePrivateKey(ctx context.Context, kid string, key crypto.PrivateKey) error
 	// ListPrivateKeys returns the KIDs of the private keys that are present. Returns a []string(nil) if there was a problem.
 	ListPrivateKeys(ctx context.Context) []string
@@ -96,3 +103,44 @@ func (pke *PublicKeyEntry) UnmarshalJSON(bytes []byte) error {
 func (pke PublicKeyEntry) JWK() jwk.Key {
 	return pke.parsedJWK
 }
+
+// GenerateAndStore generates a new key pair and stores it in the provided storage.
+func GenerateAndStore(ctx context.Context, store Storage, namingFunc func(crypto.PublicKey) (string, error)) (crypto.PublicKey, string, error) {
+	keyPair, kid, err := GenerateKeyPairAndKID(namingFunc)
+	if err != nil {
+		return nil, "", err
+	}
+	exists, err := store.PrivateKeyExists(ctx, kid)
+	if err != nil {
+		return nil, "", err
+	}
+	if exists {
+		return nil, "", errors.New("key with the given ID already exists")
+	}
+	if err = store.SavePrivateKey(ctx, kid, keyPair); err != nil {
+		return nil, "", fmt.Errorf("could not create new keypair: could not save private key: %w", err)
+	}
+	return keyPair.Public(), kid, nil
+}
+
+// GenerateKeyPair generates a new key pair using the default key type.
+// It's intended to be used by crypto backends that don't create unexportable keys.
+func GenerateKeyPair() (*ecdsa.PrivateKey, error) {
+	return ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+}
+
+// GenerateKeyPairAndKID generates a new key pair and a KID using the provided naming function.
+// It's intended to be used by crypto backends that don't create unexportable keys.
+func GenerateKeyPairAndKID(namingFunc func(crypto.PublicKey) (string, error)) (*ecdsa.PrivateKey, string, error) {
+	keyPair, err := GenerateKeyPair()
+	if err != nil {
+		return nil, "", err
+	}
+
+	kid, err := namingFunc(keyPair.Public())
+	if err != nil {
+		return nil, "", err
+	}
+
+	return keyPair, kid, nil
+}

crypto/storage/spi/interface_test.go

@@ -17,11 +17,15 @@
 package spi
 
 import (
+	"context"
+	"crypto"
 	"crypto/ecdsa"
 	"crypto/elliptic"
 	"crypto/rand"
+	"errors"
 	"github.com/lestrrat-go/jwx/v2/jwk"
 	"github.com/stretchr/testify/require"
+	"go.uber.org/mock/gomock"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -49,3 +53,70 @@ func TestPublicKeyEntry_FromJWK(t *testing.T) {
 	assert.NotEmpty(t, entry.Key)
 	assert.Same(t, pk, entry.parsedJWK)
 }
+
+func TestGenerateKeyPairAndKID(t *testing.T) {
+	t.Run("success", func(t *testing.T) {
+		keyPair, kid, err := GenerateKeyPairAndKID(func(_ crypto.PublicKey) (string, error) {
+			return "keyname", nil
+		})
+		require.NoError(t, err)
+		assert.NotNil(t, keyPair)
+		assert.Equal(t, "keyname", kid)
+	})
+}
+
+func TestGenerateAndStore(t *testing.T) {
+	ctx := context.Background()
+	t.Run("ok", func(t *testing.T) {
+		ctrl := gomock.NewController(t)
+		store := NewMockStorage(ctrl)
+		store.EXPECT().PrivateKeyExists(ctx, "123").Return(false, nil)
+		store.EXPECT().SavePrivateKey(ctx, gomock.Any(), gomock.Any()).Return(nil)
+		kid := "123"
+
+		key, kid, err := GenerateAndStore(ctx, store, func(_ crypto.PublicKey) (string, error) {
+			return kid, nil
+		})
+
+		assert.NoError(t, err)
+		assert.NotNil(t, key)
+		assert.Equal(t, "123", kid)
+	})
+	t.Run("error - NamingFunction returns err", func(t *testing.T) {
+		ctrl := gomock.NewController(t)
+		store := NewMockStorage(ctrl)
+
+		_, _, err := GenerateAndStore(ctx, store, func(_ crypto.PublicKey) (string, error) {
+			return "", errors.New("foo")
+		})
+
+		assert.ErrorContains(t, err, "foo")
+	})
+
+	t.Run("error - save public key returns an error", func(t *testing.T) {
+		ctrl := gomock.NewController(t)
+		store := NewMockStorage(ctrl)
+		store.EXPECT().PrivateKeyExists(ctx, "123").Return(false, nil)
+		store.EXPECT().SavePrivateKey(ctx, gomock.Any(), gomock.Any()).Return(errors.New("foo"))
+		kid := "123"
+
+		_, _, err := GenerateAndStore(ctx, store, func(_ crypto.PublicKey) (string, error) {
+			return kid, nil
+		})
+
+		assert.ErrorContains(t, err, "could not create new keypair: could not save private key: foo")
+	})
+
+	t.Run("error - ID already in use", func(t *testing.T) {
+		ctrl := gomock.NewController(t)
+		store := NewMockStorage(ctrl)
+		store.EXPECT().PrivateKeyExists(ctx, "123").Return(true, nil)
+		kid := "123"
+
+		_, _, err := GenerateAndStore(ctx, store, func(_ crypto.PublicKey) (string, error) {
+			return kid, nil
+		})
+
+		assert.ErrorContains(t, err, "key with the given ID already exists")
+	})
+}

crypto/storage/spi/wrapper.go

@@ -88,3 +88,14 @@ func (w wrapper) DeletePrivateKey(ctx context.Context, kid string) error {
 func (w wrapper) ListPrivateKeys(ctx context.Context) []string {
 	return w.wrappedBackend.ListPrivateKeys(ctx)
 }
+
+func (w wrapper) NewPrivateKey(ctx context.Context, namingFunc func(crypto.PublicKey) (string, error)) (crypto.PublicKey, string, error) {
+	publicKey, kid, err := w.wrappedBackend.NewPrivateKey(ctx, namingFunc)
+	if err != nil {
+		return nil, "", err
+	}
+	if err := w.validateKID(kid); err != nil {
+		return nil, "", err
+	}
+	return publicKey, kid, err
+}

crypto/storage/vault/vault.go

@@ -103,6 +103,10 @@ func NewVaultKVStorage(config Config) (spi.Storage, error) {
 	return vaultStorage, nil
 }
 
+func (v vaultKVStorage) NewPrivateKey(ctx context.Context, namingFunc func(crypto.PublicKey) (string, error)) (crypto.PublicKey, string, error) {
+	return spi.GenerateAndStore(ctx, v, namingFunc)
+}
+
 func configureVaultClient(cfg Config) (*vault.Client, error) {
 	vaultConfig := vault.DefaultConfig()
 	vaultConfig.Timeout = cfg.Timeout