RUST-1384 Implement auto encryption #717

abr-egn · 2022-08-04T15:52:12Z

This implements support for auto encryption and decryption; the large majority of the complexity is in state_machine.rs; the changes needed to the main executor were pretty minor.

This is the companion PR to mongodb/libmongocrypt-rust#11 (and its ill-fated predecessor mongodb/libmongocrypt-rust#9).

Cargo.toml

abr-egn · 2022-08-04T15:54:50Z

src/client/csfle/state_machine.rs

+        // can return a helpful error if things get into a broken state rather than panicing.
+        let mut ctx = Ok(ctx);
+        loop {
+            let state = result_ref(&ctx)?.state()?;


result_ref and result_mut are needed because as_ref/as_mut on Results also applies to the error type, which causes an error when using ? because the types don't match the return type.

abr-egn · 2022-08-04T15:57:14Z

src/client/csfle/state_machine.rs

+                    while let Some(kms_ctx) = scope.next_kms_ctx() {
+                        kms_ctxen.push(Ok(kms_ctx));
+                    }
+                    stream::iter(kms_ctxen)


Just using spawn for concurrency here wouldn't work because that requires a 'static bound; it's a little odd that the non-static equivalent is only found when operating on streams, but 🤷

I think the stream versoin works basically the same as FuturesUnordered: the current thread polls all the futures, rather than having potentially many threads doing it in parallel.

Ah, there it is. Yup, it's polling rather than parallel, which seems fine for this because these futures are going to be IO-bound anyway.

I'm going to stick with using the stream method rather than FuturesUnordered directly because the semantics of try_for_each_concurrent are very handy here.

abr-egn · 2022-08-04T15:59:18Z

src/client/executor.rs

+        csfle: &'a super::csfle::ClientState,
+        command: &'a RawDocument,
+        target_db: &'a str,
+    ) -> BoxFuture<'a, Result<RawDocumentBuf>> {


This needs to be boxed because it's technically recursive (run_mongocrypt_ctx can execute operations).

abr-egn · 2022-08-04T16:02:34Z

src/operation/mod.rs

 pub(crate) use run_command::RunCommand;
 pub(crate) use update::Update;

 const SERVER_4_2_0_WIRE_VERSION: i32 = 8;

 /// A trait modeling the behavior of a server side operation.
+///
+/// No methods in this trait should have default behaviors to ensure that wrapper operations
+/// replicate all behavior.  Default behavior is provided by the `OperationDefault` trait.
 pub(crate) trait Operation {


The Trait / TraitWithDefaults pattern introduced here solves a gripe I've had for a while, which is that when implementing something that delegates most trait logic (e.g. raw_output::RawOutput) you won't get any help from the compiler if new trait methods are added that have defaults, which in turn could cause nasty logic bugs downstream. It's definitely a little awkward and not worth using for most traits, but Operation is critical enough that it seems worth using here.

kmahar

looks good overall! just a couple comments re: spec details

kmahar · 2022-08-09T19:00:58Z

src/operation/insert/mod.rs

@@ -49,7 +54,7 @@ impl<'a, T> Insert<'a, T> {
    }
 }

-impl<'a, T: Serialize> Operation for Insert<'a, T> {
+impl<'a, T: Serialize> OperationWithDefaults for Insert<'a, T> {


I noticed the spec talks about some differences in batch splitting behavior when automatic encryption is being used - it sounds like we might need to use 2MiB rather than max_bson_obj_size as the split boundary below?

cc @isabelatkinson - I think we'll need to keep the differing size limit in mind with the new bulk spec and API

Ah, you're right - I'd initially read that as applying only to the bulk write API, but it also applies here. Updated.

I filed RUST-1438 so we remember to do the same for bulk writes when the time comes

kmahar · 2022-08-09T20:08:36Z

src/client/csfle/state_machine.rs

+                    let db = self.database(db.as_ref().ok_or_else(|| {
+                        Error::internal("db required for NeedMongoCollinfo state")
+                    })?);
+                    let mut cursor = db.list_collections(filter, None).await?;


I think this operation is actually supposed to use the metadata client to avoid potential deadlocks, as described in this section of the spec. I see the integration guide mentions using the encrypted MongoClient, but I think we maybe forgot to update that back when SPEC-1768 was done (if so, it would be good to make a PR to update it for future readers).

Good catch, fixed. I'll send a PR for the integration guide.

patrickfreed

Looks good! I just have some clarifying questions

src/client/csfle/state_machine.rs

patrickfreed · 2022-08-12T21:27:31Z

src/client/csfle/state_machine.rs

+                    while let Some(kms_ctx) = scope.next_kms_ctx() {
+                        kms_ctxen.push(Ok(kms_ctx));
+                    }
+                    stream::iter(kms_ctxen)


I think the stream versoin works basically the same as FuturesUnordered: the current thread polls all the futures, rather than having potentially many threads doing it in parallel.

src/client/csfle/state_machine.rs

src/client/executor.rs

patrickfreed · 2022-08-12T21:49:42Z

src/operation/raw_output.rs

+
+use super::Operation;
+
+pub(crate) struct RawOutput<Op>(pub(crate) Op);


Can we add a docstring indicating this type's purpose? If I'm understanding it right, it forwards all implementation to the underlying op except that it performs no validation or parsing of the response, is that right?

Yup, that's correct - added a comment.

I think the commit for this didn't actually get pushed.

Whoops, fixed.

src/client/csfle/state_machine.rs

patrickfreed

LGTM! (bar a commit from a previous comment, no need for me to re-review)

patrickfreed · 2022-08-16T14:06:05Z

src/operation/raw_output.rs

+
+use super::Operation;
+
+pub(crate) struct RawOutput<Op>(pub(crate) Op);


I think the commit for this didn't actually get pushed.

patrickfreed · 2022-08-16T14:08:26Z

Oh and it looks like the clippy task failed.

abr-egn · 2022-08-16T14:17:29Z

Oh and it looks like the clippy task failed.

Looks like those failures are in src/gridfs.rs, unrelated to this PR (@sanav33).

isabelatkinson · 2022-08-16T19:20:55Z

src/operation/mod.rs

@@ -398,3 +391,140 @@ macro_rules! remove_empty_write_concern {
 }

 pub(crate) use remove_empty_write_concern;
+
+// A mirror of the `Operation` trait, with default behavior where appropriate.  Should only be
+// implemented by leaf operation types.


what do you mean by leaf operation types here?

Derp, my inner terminology leaking out. Updated the comment to clarify.

isabelatkinson · 2022-08-16T19:27:49Z

src/operation/run_command/test.rs

@@ -20,7 +22,8 @@ fn build() {
        command
            .body
            .get("hello")
-            .and_then(crate::bson_util::get_int),
+            .unwrap()


I think we should just remove this rather than updating

Good call, done. Sorry that I keep forgetting that!

isabelatkinson

looks like the format task failed but lgtm otherwise!

abr-egn commented Aug 4, 2022

View reviewed changes

abr-egn requested review from patrickfreed, kmahar and isabelatkinson August 4, 2022 16:05

abr-egn marked this pull request as ready for review August 4, 2022 16:05

kmahar reviewed Aug 9, 2022

View reviewed changes

kmahar mentioned this pull request Aug 10, 2022

RUST-569 Client Side Field Level Encryption Support #520

Closed

kmahar approved these changes Aug 10, 2022

View reviewed changes

abr-egn added 22 commits August 11, 2022 13:27

api outline

9d05d23

wip

5bd3324

shift operation defaults to mirror trait

56c4160

needmongocollinfo

1349cff

wip

d1ccd79

raw runcommand

1709fd3

local boxed run_mongocrypt_ctx

c7d02d9

dedicated ctx thread wip

99f2c18

dedicated ctx thread scaffolding

21f0189

cleanup

e118f52

catchup

36c75db

tidy error handling

f586c44

needmongokeys

7244004

start to rejigger to new ctx

2646bed

progress

edeaab4

select runtimes for ctx spawning

04a2fac

needkms

d0f974b

fill back in previous states

ed668cc

placeholder for needkmscredentials

9c98ecb

auto decrypt

8a0cd31

rustfmt

54362a6

put crypto finalization on a thread pool

edf0924

abr-egn added 6 commits August 11, 2022 13:27

format and clippy

bf78b11

fix comment

15006f1

point back to main mongocrypt repo

e1b9ad8

move thread pool to client state

293cfb9

review updates

d7acff9

fix tests and include csfle in feature testing

6bbd8c1

abr-egn force-pushed the RUST-1384/auto-encryption branch from 115e6fd to 6bbd8c1 Compare August 11, 2022 17:27

abr-egn added 2 commits August 12, 2022 14:25

actually use tls_options

859967a

fix mongocrypt dep

0dedc5b

patrickfreed reviewed Aug 12, 2022

View reviewed changes

abr-egn added 2 commits August 15, 2022 11:00

use advance/current

8169c6a

comment rawoutput

40cffb9

patrickfreed approved these changes Aug 16, 2022

View reviewed changes

isabelatkinson reviewed Aug 16, 2022

View reviewed changes

abr-egn added 2 commits August 17, 2022 11:38

review updates

d270393

fix test imports

614bd78

isabelatkinson approved these changes Aug 17, 2022

View reviewed changes

fix rustfmt

0b007b5

abr-egn merged commit 38ba60b into mongodb:main Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUST-1384 Implement auto encryption #717

RUST-1384 Implement auto encryption #717

abr-egn commented Aug 4, 2022

abr-egn Aug 4, 2022

abr-egn Aug 4, 2022

patrickfreed Aug 12, 2022

abr-egn Aug 15, 2022

abr-egn Aug 4, 2022

abr-egn Aug 4, 2022 •

edited

Loading

kmahar left a comment

kmahar Aug 9, 2022

abr-egn Aug 10, 2022

kmahar Aug 10, 2022

kmahar Aug 9, 2022

abr-egn Aug 10, 2022

patrickfreed left a comment

patrickfreed Aug 12, 2022

patrickfreed Aug 12, 2022

abr-egn Aug 15, 2022

patrickfreed Aug 16, 2022

abr-egn Aug 16, 2022

patrickfreed left a comment

patrickfreed Aug 16, 2022

patrickfreed commented Aug 16, 2022

abr-egn commented Aug 16, 2022

isabelatkinson Aug 16, 2022

abr-egn Aug 17, 2022

isabelatkinson Aug 16, 2022

abr-egn Aug 17, 2022

isabelatkinson left a comment


		use super::Operation;

		pub(crate) struct RawOutput<Op>(pub(crate) Op);

RUST-1384 Implement auto encryption #717

RUST-1384 Implement auto encryption #717

Conversation

abr-egn commented Aug 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abr-egn Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

kmahar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickfreed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickfreed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickfreed commented Aug 16, 2022

abr-egn commented Aug 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isabelatkinson left a comment

Choose a reason for hiding this comment

abr-egn Aug 4, 2022 •

edited

Loading