[Encryption] Set master key when creating an encrypted collection #2780
Conversation
| if (! is_array($options['kmsProviders'] ?? null) || count($options['kmsProviders']) === 0) { | ||
| throw new InvalidArgumentException('The "kmsProviders" encryption option is required and must be a non-empty.'); | ||
| } | ||
|
|
||
| if (! isset($options['kmsProvider']) && count($options['kmsProviders']) > 1) { | ||
| throw new InvalidArgumentException('The "kmsProvider" encryption option is required when multiple KMS providers are specified.'); | ||
| } | ||
|
|
||
| $options['kmsProvider'] ??= array_key_first($options['kmsProviders']); | ||
|
|
||
| if (! array_key_exists($options['kmsProvider'], $options['kmsProviders'])) { | ||
| throw new InvalidArgumentException(sprintf('The "kmsProvider" encryption option "%s" is not defined in the "kmsProviders" option.', $options['kmsProvider'])); | ||
| } |
There was a problem hiding this comment.
My suggestion is to only expose a kmsProvider option, which we then normalise into a single kmsProviders element when passing the provider list to the client. This single provider would only have a type (e.g. local), which we can use as the name for the provider wherever needed. This would make the configuration simpler and still allow us to add support for multiple KMS providers in future.
| // @todo Throw en exception if multiple KMS providers are defined. This is not supported yet and would require a setting for the KMS provider to use when creating a new collection | ||
| if (! isset($options['kmsProviders']) || ! is_array($options['kmsProviders']) || count($options['kmsProviders']) < 1) { | ||
| throw new InvalidArgumentException('The "kmsProviders" option is required.'); | ||
| if ($options['kmsProvider'] !== 'local' && ! isset($options['masterKey'])) { |
There was a problem hiding this comment.
Suggestion: rename the option defaultMasterKey, as we'll eventually allow setting a different master key in encryption config.
| * kmsProvider?: KmsProvider, | ||
| * defaultMasterKey?: array<string, mixed>|null, | ||
| * autoEncryption?: array<string, mixed>, |
There was a problem hiding this comment.
I extracted the kmsProvider and defaultMasterKey from the autoEncryption as they are not part of the driver options.
|
|
||
| class ConfigurationTest extends BaseTestCase |
There was a problem hiding this comment.
The parent class was opening a connection to the mongodb server, which is not necessary for validating the configuration.
| * | ||
| * @return array{keyVaultClient?: Client|Manager, keyVaultNamespace: string, kmsProviders: array<string, mixed>, tlsOptions?: array<string, mixed>} | ||
| */ | ||
| public function getClientEncryptionOptions(): array |
There was a problem hiding this comment.
The Configuration class is now responsible for creating the option arrays.
GromNaN
merged commit 64cb384
into
doctrine:feature/queryable-encryption
The master key is required for all KMS exception "local" (doc).
The
masterKeyandkmsProviderconfigurations as set independently from the otherautoEncryptionoptions so that we can validate and transform them.A single
kmsProvideris allowed, and used as default when an encrypted collection is created.