Skip to content

[CXX-2546] Create Encrypted Collection #959

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

Conversation

vector-of-bool
Copy link
Contributor

Refer: CXX-2546

This changeset introduces the create encrypted collection convenience API, as outlined in DRIVERS-2312. This is used to create data encryption keys on-the-fly when creating a new encrypted collection. The C++ API is mostly just a wrapper around the underlying C API, with a few convenience overloads for optional parameters.

The error-initiating code may be questionable. It has been a while since I've worked in the C++ codebase.

@vector-of-bool
Copy link
Contributor Author

Seeing failures on some replica_set configurations. Very mysterious $\ldots$

Copy link
Contributor

@kkloberdanz kkloberdanz left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@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.

LGTM with consideration to remove an overload and suggested simplifications.

bsoncxx::document::value& out_options,
const std::string& kms_provider,
const stdx::optional<bsoncxx::document::view>& masterkey);

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there is no use case to call this overload with a nullopt for masterkey.

Consider combining the two overloads:

    /**
     * @brief Create a collection with client-side-encryption enabled, automatically filling any
     * datakeys for encrypted fields.
     *
     * @param db The database in which the collection will be created
     * @param coll_name The name of the new collection
     * @param options The options for creating the collection. @see database::create_collection
     * @param out_options Output parameter to receive the generated collection options.
     * @param kms_provider The KMS provider to use when creating data encryption keys for the
     * collection's encrypted fields
     * @param masterkey If non-null, specify the masterkey to be used when creating data keys in the
     * collection.
     * @return collection A handle to the newly created collection
     */
    collection create_encrypted_collection(
        const database& db,
        const std::string& coll_name,
        const bsoncxx::document::view& options,
        bsoncxx::document::value& out_options,
        const std::string& kms_provider,
        const stdx::optional<bsoncxx::document::view> masterkey = stdx::nullopt);

Or alternatively: make masterkey non-optional in this overload.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is more due to my aversion to defaulted arguments. I don't know if there's precedent in the codebase. Combining them will result in a mostly-equivalent API. I'll combine them for now.

bson_t* opt_mkey_ptr = nullptr;
scoped_bson_t opt_mkey;
if (masterkey) {
bson_init_static(opt_mkey.bson_for_init(), masterkey->data(), masterkey->length());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
bson_init_static(opt_mkey.bson_for_init(), masterkey->data(), masterkey->length());
opt_mkey.init_from_static(*masterkey);

Comment on lines 371 to 372
scoped_bson_t coll_opts;
bson_init_static(coll_opts.bson_for_init(), opts.data(), opts.length());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
scoped_bson_t coll_opts;
bson_init_static(coll_opts.bson_for_init(), opts.data(), opts.length());
scoped_bson_t coll_opts(opts);

@vector-of-bool vector-of-bool merged commit 803386f into mongodb:master Apr 28, 2023
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