Skip to content

DRIVERS-2017 Add clientEncryption entity to unified test format #1188

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 36 commits into from
Apr 26, 2022

Conversation

eramongodb
Copy link
Contributor

@eramongodb eramongodb commented Apr 20, 2022

This PR is a part of DRIVERS-2017. It is being reviewed separately to unblock FLE 2 work.

Add schema-1.8.json

As this PR introduces new entities to the JSON schema, the minor version of the schema version must be bumped.

Add schema for encryptedClient entity

The schema for the new encryptedClient entity primarily depends on the schema for clientEncryptionOpts and kmsProviders.

The schema for clientEncryptionOpts is nearly the same as described in the Client Side Encryption spec, except that the tlsOptions field is not supported in unified test files (although Drivers can configure the field internally as needed). This may be changed in the future if unified tests that require explicitly configuring the tlsOptions field are needed.

The schema for kmsProviders is nearly the same as described in the Client Side Encryption spec, except that KMS credential fields are permitted to omitted (even if required by spec) and are permitted to be empty objects {} to indicate the field must be loaded by the Driver.

This kmsProviders schema permits the test runner to validate several conditions. Using the aws KMS provider as an example:

  • No KMS provider is configured: kmsProviders: {}. This may (intentionally) lead to an error during ClientEncryption object creation.
  • AWS KMS provider is configured without credentials: kmsProviders: { aws: {} }. This may (intentionally) lead to requests for KMS credential during encryptedClient operations.
  • AWS KMS provider is configured with incomplete credentials: kmsProviders: { aws: { accessKeyId: "abc" } }. This may (intentionally) lead to an error during ClientEncryption object creation.
  • AWS KMS provider is configured with credentials loaded by the Driver: kmsProviders: { aws: { accessKeyId: { $$placeholder: {} }, secretAccessKey: { $$placeholder: {} } } }. This is expected to be the most common case.
  • AWS KMS provider is configured with explicit credentials in the test file: kmsProviders: { aws: { accessKeyId: "abc", secretAccessKey: "def" } }. This may (intentionally) lead to KMS-related errors during encryptedClient operations.

Drivers are only expected to load KMS credentials from the environment if they are required by a given test (e.g. with aws: { accessKeyId: {} }. This is to permit unified test runners to run non-CSE unified tests without having to configure their environment or configuration file with CSE-related settings.

Add unified test format tests for clientEncryptionOpts

These tests validate the schema described above. encryptedClient operations are deliberately not included in these tests as they are still being drafted.

Add spec wording for unified test format for Client Side Encryption

The wording in the Unified Test Format spec hopefully captures the behavior and rationale given above in a manner suitable for Drivers specification.

Code Review Updates

  • Renamed encryptedClient entity to clientEncryption for consistency with CSE spec.
  • Introduced $$placeholder as dedicated syntax to represent placeholder values in unified test format.

@eramongodb eramongodb requested a review from kevinAlbs April 20, 2022 18:48
@eramongodb eramongodb requested a review from a team as a code owner April 20, 2022 18:48
@eramongodb eramongodb self-assigned this Apr 20, 2022
@eramongodb eramongodb requested review from jmikola and removed request for a team April 20, 2022 18:48
Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a small hiccup in the Go driver. ClientEncryption.Close() disconnects the key vault client. The workaround was to track which clients were used as key vault clients.

All tests passed on Go POC with a fix to the local key material. Nice work. LGTM!

@eramongodb eramongodb requested a review from a team as a code owner April 25, 2022 15:23
@eramongodb eramongodb requested a review from kevinAlbs April 25, 2022 15:24
@eramongodb eramongodb requested a review from jmikola April 25, 2022 15:24
@jmikola
Copy link
Member

jmikola commented Apr 26, 2022

Note: #959 is also bumping the schema version for CSOT so this may conflict if that PR is merged first.

@eramongodb eramongodb requested a review from kevinAlbs April 26, 2022 16:19
@eramongodb eramongodb changed the title DRIVERS-2017 Add encryptedClient entity to unified test format DRIVERS-2017 Add clientEncryption entity to unified test format Apr 26, 2022
Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a bunch of suggestions for consistency with existing filenames within the invalid directory. The suggestions were made on the description field but also apply to the filenames. Doing so and listing all of the suggested filenames highlighted some redundant and missing tests.

If all suggestions are made, I think we'll end up with the following new invalid tests, which I think cover enough surface area:

entity-clientEncryption-additionalProperties
entity-clientEncryption-id-required
entity-clientEncryption-id-type
entity-clientEncryption-clientEncryptionOpts-required (NEW)
entity-clientEncryption-clientEncryptionOpts-type
clientEncryptionOpts-additionalProperties
clientEncryptionOpts-keyVaultClient-required
clientEncryptionOpts-keyVaultClient-type
clientEncryptionOpts-keyVaultNamespace-required
clientEncryptionOpts-keyVaultNamespace-type
clientEncryptionOpts-kmsProviders-required
clientEncryptionOpts-kmsProviders-type
clientEncryptionOpts-tlsOptions_not_supported
clientEncryptionOpts-kmsProviders-aws-additionalProperties
clientEncryptionOpts-kmsProviders-aws-type
clientEncryptionOpts-kmsProviders-azure-additionalProperties
clientEncryptionOpts-kmsProviders-azure-type
clientEncryptionOpts-kmsProviders-gcp-additionalProperties
clientEncryptionOpts-kmsProviders-gcp-type
clientEncryptionOpts-kmsProviders-kmip-additionalProperties
clientEncryptionOpts-kmsProviders-kmip-type
clientEncryptionOpts-kmsProviders-local-additionalProperties
clientEncryptionOpts-kmsProviders-local-type

The logic here is that the filenames correspond to the JSON schema definitions. clientEncryptionOpts is used as a root name, as previously done for collectionOrDatabaseOptions, expectedCommandEvent, and other definitions within the schema.

Since clientEncryption only exists within the entity definition, it's lumped under entity. That's consistent with the entity-client-* test files.


Syntax::

{ field: { $$placeholder: {} } }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the value is important, and would say as much below. I'd propose using 1 to simplicity, which is consistent with examples in the server manual (e.g. some commands disregard the value for a command name).

Suggested change
{ field: { $$placeholder: {} } }
{ field: { $$placeholder: 1 } }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I defaulted to {} for ease of schema definition (just a "const" value) and to avoid possible misinterpretations of the numeric value as having any special meaning. However, if : 1 is already an established pattern, I will conform to it. I will also add a clause indicating that test runner need not validate the value of a $$placeholder field.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted. I see this complicated the stringOrPlaceholder definition, which wasn't my intention -- but it's done now so I think we can leave it in place.

If any other context in the schema needs to use $$placeholder in the future, it will hopefully be extracted into a separate definition (out of stringOrPlaceholder).

@eramongodb eramongodb requested a review from jmikola April 26, 2022 19:24
@eramongodb eramongodb merged commit e91f0ec into mongodb:master Apr 26, 2022
@eramongodb eramongodb deleted the client-encryption-entity branch April 26, 2022 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants