update for libmongocrypt 1.5.2 #1037

kevinAlbs · 2022-07-29T19:27:30Z

Summary

Resolve GODRIVER-2511:

Add CSFLE prose test 16.
Update tests to use libmongocrypt 1.5.2.

Resolve GODRIVER-2513:

Return error if libmongocrypt < 1.5.2 is detected in RewrapManyDataKey.
Retract Go 1.10.0.

Resolve GODRIVER-2509:

Resync RewrapManyDataKey specification tests to mongodb/specifications@10b4a41.

Resolve GODRIVER-2512:

Resync fle2-InsertFind-Unindexed.json file to DRIVERS-2407 update expected error in fle2-InsertFind-Unindexed specifications#1286.

Background & Motivation

libmongocrypt 1.5.2 includes a fix to a severe bug introduced in libmongocrypt 1.5.0: MONGOCRYPT-464.

Accounts for changes made in MONGOCRYPT-437

benjirewis · 2022-08-01T20:34:19Z

data/client-side-encryption/legacy/fle2-InsertFind-Unindexed.yml

+          # After: "can only execute encrypted equality queries with an encrypted equality index"
+          # Use a small common substring.
+          errorContains: "encrypt"


Suggested change

errorContains: "encrypt"

errorContains: "encrypt"

[opt] also missing newline in the spec PR.

The .yml file is generated from a template: https://github.com/mongodb/specifications/blob/07260166cb48ef6d9455661ad5f1166a87bc7291/source/client-side-encryption/etc/generate-test.py#L614

I will leave this as-is for now.

benjirewis · 2022-08-01T20:37:00Z

mongo/client_encryption.go

+	// libmongocrypt versions 1.5.0 and 1.5.1 have a severe bug in RewrapManyDataKey.
+	// Check if the version string starts with 1.5.0 or 1.5.1. This accounts for pre-release versions, like 1.5.0-rc0.
+	libmongocryptVersion := mongocrypt.Version()
+	if strings.Index(libmongocryptVersion, "1.5.0") == 0 || strings.Index(libmongocryptVersion, "1.5.1") == 0 {


[nit] I'd probably use strings.HasPrefix for clarity.

benjirewis · 2022-08-01T20:47:28Z

mongo/integration/client_side_encryption_prose_test.go

+					mt.Run(fmt.Sprintf("%s to %s", srcProvider, dstProvider), func(mt *mtest.T) {
+						var err error
+						// Drop the collection ``keyvault.datakeys``.
+						{


Hmm is the point of these empty blocks just to emphasize what the steps of this test are? Or are you using them for some scope logic that I'm not understanding 🧑‍🔧 . If it's just for emphasizing the steps, I'm not sure it's necessary.

Yes, it is to emphasize what code accomplishes the step. It is not necessary.

benjirewis · 2022-08-01T20:49:33Z

mongo/integration/client_side_encryption_prose_test.go

+							if val, ok := dataKeyMap[dstProvider]; ok {
+								rwOpts.SetMasterKey(val)
+							}
+							res, err := clientEncryption2.RewrapManyDataKey(context.Background(), bson.D{{}}, rwOpts)


Suggested change

res, err := clientEncryption2.RewrapManyDataKey(context.Background(), bson.D{{}}, rwOpts)

res, err := clientEncryption2.RewrapManyDataKey(context.Background(), bson.D{}, rwOpts)

Should this just be bson.D{} if you want an empty filter?

benjirewis

LGTM mod my small comments/questions.

matthewdale

Looks good with a few questions. 👍

matthewdale · 2022-08-01T23:37:55Z

mongo/doc.go

@@ -110,7 +110,7 @@
 // - Go Driver v1.2.0 requires libmongocrypt v1.0.0 or higher
 // - Go Driver v1.5.0 requires libmongocrypt v1.1.0 or higher
 // - Go Driver v1.8.0 requires libmongocrypt v1.3.0 or higher
-// - Go Driver v1.10.0 requires libmongocrypt v1.5.0 or higher
+// - Go Driver v1.10.0 requires libmongocrypt v1.5.2 or higher


Optional: This is slightly odd messaging and conflates compatibility requirements with libmongocrypt version retraction. My understanding is that libmongocrypt v1.5.0 and v1.5.1 contain serious bugs that can cause data corruption, but are technically build-compatible with Go Driver v1.10.0. Consider adding a note that describes why libmongocrypt v1.5.0/v1.5.1 shouldn't be used.

I have updated to say v1.5.0 is required but added a note to say 1.5.2 is required to avoid the severe bug in RewrapManyDataKey.

matthewdale · 2022-08-01T23:38:48Z

mongo/client_encryption.go

+	libmongocryptVersion := mongocrypt.Version()
+	if strings.Index(libmongocryptVersion, "1.5.0") == 0 || strings.Index(libmongocryptVersion, "1.5.1") == 0 {
+		return nil, fmt.Errorf("RewrapManyDataKey requires libmongocrypt 1.5.2 or newer. Detected version: %v", libmongocryptVersion)
+	}


These updates will only apply to users who update to Go Driver v1.10.1+ and could be missed by users who are using Go Driver v1.10.0. Are there other ways we're messaging to Go Driver users that they shouldn't use libmongocrypt v1.5.0/v1.5.1 (e.g. retraction notices on the libmongocrypt download page)?

Good question. I updated the libmongocrypt release notes and changelog to add a warning. There is no libmongocrypt downloads page. Go users are advised to download libmongocrypt from Homebrew, the Linux packages we maintain, or the Windows download URL.

One option is to update the Go driver 1.10.0 release notes to add a notice like the following. I have asked in the team channel.

Sounds good.

kevinAlbs · 2022-08-02T01:23:57Z

Updated to retract v1.10.0.

matthewdale

Looks good 👍

matthewdale · 2022-08-02T01:30:39Z

mongo/doc.go

+// - Go Driver v1.10.0 requires libmongocrypt v1.5.0 or higher.
+// There is a severe bug when calling RewrapManyDataKey with libmongocrypt versions less than 1.5.2.
+// This bug may result in data corruption.
+// Please use libmongocrypt 1.5.2 or higher when calling RewrapManyDataKey.


Great godoc format improvement!

matthewdale · 2022-08-02T01:31:58Z

mongo/client_encryption.go

+	libmongocryptVersion := mongocrypt.Version()
+	if strings.Index(libmongocryptVersion, "1.5.0") == 0 || strings.Index(libmongocryptVersion, "1.5.1") == 0 {
+		return nil, fmt.Errorf("RewrapManyDataKey requires libmongocrypt 1.5.2 or newer. Detected version: %v", libmongocryptVersion)
+	}


Sounds good.

Resolve GODRIVER-2511: * Add CSFLE prose test 16. * Update tests to use libmongocrypt 1.5.2. Resolve GODRIVER-2513: * Return error if libmongocrypt < 1.5.2 is detected in RewrapManyDataKey. * Retract Go driver release v1.10.0. Resolve GODRIVER-2509: * Resync RewrapManyDataKey specification tests to mongodb/specifications@10b4a41. * Resolve GODRIVER-2512: Resolve GODRIVER-2512: - Resync fle2-InsertFind-Unindexed.json

add CSFLE prose test 16

05226d6

kevinAlbs mentioned this pull request Jul 29, 2022

Error if RewrapManyDataKey is called with libmongocrypt < 1.5.2 mongodb/specifications#1284

Closed

5 tasks

kevinAlbs added 5 commits July 30, 2022 14:09

sync tests to mongodb/specifications@10b4a41

2719c33

pin libmongocrypt test dependency to 1.5.2

143047c

update download URLs for libmongocrypt

83f73b3

Accounts for changes made in MONGOCRYPT-437

document requirement of 1.5.2

da89e92

return error if libmongocrypt < 1.5.2 is detected in RewrapManyDataKey

cc643a5

kevinAlbs force-pushed the D2403 branch from 25abb61 to cc643a5 Compare July 30, 2022 18:12

do not cd into libmongocrypt

3a374dd

kevinAlbs changed the title ~~DRIVERS-2403 POC~~ GODRIVER-2510 update for libmongocrypt 1.5.2 Jul 30, 2022

kevinAlbs added 4 commits July 30, 2022 14:22

add LIBMONGOCRYPT_TAG

79932a5

remove stutter

3385d6d

DRIVERS-2407 resync fle2-InsertFind-Unindexed

1c9b34c

run go fmt

5004181

kevinAlbs mentioned this pull request Jul 30, 2022

DRIVERS-2407 update expected error in fle2-InsertFind-Unindexed mongodb/specifications#1286

Merged

5 tasks

kevinAlbs changed the title ~~GODRIVER-2510 update for libmongocrypt 1.5.2~~ update for libmongocrypt 1.5.2 Jul 30, 2022

This was referenced Jul 30, 2022

DRIVERS-2406 add Rewrap prose test mongodb/specifications#1287

Merged

DRIVERS-2408 error if RewrapManyDataKey is called with libmongocrypt < 1.5.2 mongodb/specifications#1288

Merged

kevinAlbs marked this pull request as ready for review July 30, 2022 20:05

kevinAlbs requested review from benjirewis and matthewdale July 30, 2022 20:05

benjirewis reviewed Aug 1, 2022

View reviewed changes

benjirewis approved these changes Aug 1, 2022

View reviewed changes

matthewdale approved these changes Aug 1, 2022

View reviewed changes

kevinAlbs added 5 commits August 1, 2022 21:09

use strings.HasPrefix

8b6cace

fix godoc formatting

7c10825

Note why libmongocrypt 1.5.2 is needed

68c96f3

remove unnecessary braces

f6922ae

retract v1.10.0

feed4b6

kevinAlbs requested review from matthewdale and benjirewis August 2, 2022 01:24

matthewdale approved these changes Aug 2, 2022

View reviewed changes

kevinAlbs removed the request for review from benjirewis August 2, 2022 02:06

kevinAlbs merged commit 82a6eeb into mongodb:master Aug 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update for libmongocrypt 1.5.2 #1037

update for libmongocrypt 1.5.2 #1037

kevinAlbs commented Jul 29, 2022 •

edited

benjirewis Aug 1, 2022

kevinAlbs Aug 2, 2022

benjirewis Aug 1, 2022

kevinAlbs Aug 2, 2022

benjirewis Aug 1, 2022

kevinAlbs Aug 2, 2022

benjirewis Aug 1, 2022

kevinAlbs Aug 2, 2022

benjirewis left a comment

matthewdale left a comment

matthewdale Aug 1, 2022

kevinAlbs Aug 2, 2022

matthewdale Aug 1, 2022

kevinAlbs Aug 2, 2022

matthewdale Aug 2, 2022

kevinAlbs commented Aug 2, 2022

matthewdale left a comment

matthewdale Aug 2, 2022

matthewdale Aug 2, 2022

	res, err := clientEncryption2.RewrapManyDataKey(context.Background(), bson.D{{}}, rwOpts)
	res, err := clientEncryption2.RewrapManyDataKey(context.Background(), bson.D{}, rwOpts)

update for libmongocrypt 1.5.2 #1037

update for libmongocrypt 1.5.2 #1037

Conversation

kevinAlbs commented Jul 29, 2022 • edited

Summary

Background & Motivation

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjirewis left a comment

Choose a reason for hiding this comment

matthewdale 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

kevinAlbs commented Aug 2, 2022

matthewdale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinAlbs commented Jul 29, 2022 •

edited