Skip to content

CDRIVER-4498 return error if masterKey is set, but provider is not set #1259

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 7 commits into from
May 17, 2023

Conversation

kevinAlbs
Copy link
Collaborator

@kevinAlbs kevinAlbs commented May 4, 2023

Summary

  • return error if masterKey is set, but provider is not set for rewrapManyDataKey
  • add prose test specified in DRIVERS-2441

Background & Motivation

The specified API for rewrapManyDataKey does not permit calling with a set masterKey without a provider.

    class ClientEncryption {
        rewrapManyDataKey(filter: Document, opts: RewrapManyDataKeyOpts | null): RewrapManyDataKeyResult;
    }
    class RewrapManyDataKeyOpts {
        provider: String
        masterKey: Optional<Document>
    }

Some driver implementations do not represent RewrapManyDataKeyOpts as a separate type. Instead, the provider and masterKey are both optional arguments to RewrapManyDataKey.

The C driver API permits setting masterKey without provider:

MONGOC_EXPORT (bool)
mongoc_client_encryption_rewrap_many_datakey (
   mongoc_client_encryption_t *client_encryption,
   const bson_t *filter,
   const char *provider,
   const bson_t *master_key,
   mongoc_client_encryption_rewrap_many_datakey_result_t *result,
   bson_error_t *error);

The current behavior of mongoc_client_encryption_rewrap_many_datakey silently ignores the master_key option if provider is NULL.

This may result in unexpected behavior. A user may be attempting to rewrap keys with a new master_key and mistakenly passed a NULL provider. A NULL provider results in rewrapping with the same master_key.

The Java driver had similar behavior, which was fixed in JAVA-4717.

@kevinAlbs kevinAlbs marked this pull request as ready for review May 10, 2023 19:01
@kevinAlbs kevinAlbs requested a review from eramongodb May 10, 2023 19:01
@kevinAlbs kevinAlbs changed the title CDRIVER-4498 add prose test Rewrap Case 2 CDRIVER-4498 return error if masterKey is set, but provider is not set May 10, 2023
Copy link
Contributor

@eramongodb eramongodb left a comment

Choose a reason for hiding this comment

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

Minor suggestions; otherwise, LGTM.

clientEncryption,
NULL /* filter */,
NULL /* kms_provider */,
tmp_bson ("{'foo': 'bar'}"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tmp_bson ("{'foo': 'bar'}"),
tmp_bson ("{}"),

&error);
// Assert an error is returned from the driver suggesting that the
// ``provider`` option is required.
ASSERT (!ok && "expected error, but got success");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ASSERT (!ok && "expected error, but got success");
ASSERT_WITH_MSG (!ok, "expected error, but got success");

@kevinAlbs kevinAlbs merged commit 98995b5 into mongodb:master May 17, 2023
kevinAlbs added a commit that referenced this pull request May 17, 2023
…t set (#1259)

* add CSE Prose Test 16: Case 2

* return error if `provider` is not set when `master_key` is set

* Simplify assertion of provider

Co-authored-by: Ezra Chung <[email protected]>

* update commented test name to match specification

* use empty doc for masterKey to match specification

* use ASSERT_WITH_MSG

* clarify in docs that current master key is used when provider is NULL

---------

Co-authored-by: Ezra Chung <[email protected]>
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.

2 participants