Skip to content
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

MONGOCRYPT-756 Convert FLE2EncryptionPlaceholder to FLE2InsertUpdatePayloadV2 for text search indexed fields #939

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

erwee
Copy link
Collaborator

@erwee erwee commented Jan 10, 2025

No description provided.

@erwee erwee requested a review from marksg07 January 10, 2025 00:41
@erwee erwee marked this pull request as ready for review January 14, 2025 19:48
@erwee erwee requested a review from kevinAlbs January 14, 2025 19:49
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved these tests to test-mc-fle2-encryption-placehoder.c


#define RAW_STRING(...) #__VA_ARGS__

static void _test_FLE2EncryptionPlaceholder_parse(_mongocrypt_tester_t *tester) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

taken from test-mongocrypt-ctx-encrypt.c

mongocrypt_status_destroy(status);
}

static void _test_FLE2EncryptionPlaceholder_range_parse(_mongocrypt_tester_t *tester) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

taken from test-mongocrypt-ctx-encrypt.c

src/mc-fle2-encryption-placeholder-private.h Show resolved Hide resolved
* https://github.com/mongodb/mongo/blob/master/src/mongo/crypto/fle_field_schema.idl
* for the representation in the MongoDB server. */
typedef struct {
// v is the value to encrypt.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is always a string, can we parse it into char * and uint32_t len? MONGOCRYPT-755 assumes this.

Copy link
Collaborator Author

@erwee erwee Jan 21, 2025

Choose a reason for hiding this comment

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

It's simpler to keep it a bson_iter_t of type string, because when we encrypt it, we encrypt the bson element (which includes the 4 byte length + null terminator), not the raw string.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait, when you say "we encrypt the bson element" - do you mean that the string passed into StrEncode is not the raw string, but the BSON element including the length + null terminator?

Copy link
Collaborator Author

@erwee erwee Jan 23, 2025

Choose a reason for hiding this comment

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

No, I mean when we do the AES-CBC encrypt & derive the ESC/EDC/Server value-derived tokens, we pass in the (length + string + nullbyte). For StrEncode, we pass in just the string. When we derive the tokens for each substring/suffix/prefix from StrEncode later, we'll have to wrap those substrings so that they include the length + null-byte in the token derivations.

src/mc-fle2-insert-update-payload-v2.c Outdated Show resolved Hide resolved
src/mc-fle2-insert-update-payload-v2.c Outdated Show resolved Hide resolved
src/mongocrypt-marking.c Outdated Show resolved Hide resolved
src/mongocrypt-marking.c Show resolved Hide resolved
src/mongocrypt-marking.c Outdated Show resolved Hide resolved
test/test-mc-fle2-encryption-placeholder.c Outdated Show resolved Hide resolved
@erwee erwee requested a review from marksg07 January 21, 2025 22:31
index++; \
} \
if (index == UINT32_MAX && array->len > UINT32_MAX) { \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if I understand -- Why are we appending one extra element in this case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

bc the loop above it is not inclusive of index at UINT32_MAX if the array length exceeds that. highly unlikely, but this is the same logic for range edge tokens in mc_FLE2InsertUpdatePayloadV2_serializeForRange().

for (size_t i = 0; i < array->len; i++) { \
mc_Text##Type##TokenSet_t ts = _mc_array_index(array, mc_Text##Type##TokenSet_t, i); \
uint32_t limit = (array->len > UINT32_MAX) ? UINT32_MAX : (uint32_t)array->len; \
while (index < limit) { \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit:

Suggested change
while (index < limit) { \
for (; index < limit; index++) { \

And remove the increment at the end of the loop.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

src/mongocrypt-marking.c Show resolved Hide resolved
Copy link
Collaborator

@marksg07 marksg07 left a comment

Choose a reason for hiding this comment

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

Nit + question about logic

@erwee erwee requested a review from marksg07 January 23, 2025 02:32
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.

LGTM with addition of public docs to note this is experimental.

src/mongocrypt.h Outdated
Comment on lines 1537 to 1539
#define MONGOCRYPT_QUERY_TYPE_SUBSTRINGPREVIEW_STR "substringPreview"
#define MONGOCRYPT_QUERY_TYPE_SUFFIXPREVIEW_STR "suffixPreview"
#define MONGOCRYPT_QUERY_TYPE_PREFIXPREVIEW_STR "prefixPreview"
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
#define MONGOCRYPT_QUERY_TYPE_SUBSTRINGPREVIEW_STR "substringPreview"
#define MONGOCRYPT_QUERY_TYPE_SUFFIXPREVIEW_STR "suffixPreview"
#define MONGOCRYPT_QUERY_TYPE_PREFIXPREVIEW_STR "prefixPreview"
/// NOTE: "substringPreview" is experimental and may be removed in a future non-major release.
#define MONGOCRYPT_QUERY_TYPE_SUBSTRINGPREVIEW_STR "substringPreview"
/// NOTE: "suffixPreview" is experimental and may be removed in a future non-major release.
#define MONGOCRYPT_QUERY_TYPE_SUFFIXPREVIEW_STR "suffixPreview"
/// NOTE: "prefixPreview" is experimental and may be removed in a future non-major release.
#define MONGOCRYPT_QUERY_TYPE_PREFIXPREVIEW_STR "prefixPreview"

Document public API as unstable to permit removal in a non-major release.

* E?CText<T>DerivedFromDataTokenAndContentionFactorToken = HMAC(HMAC(E?CText<T>Token, v) cf)
*
* E?C = EDC|ESC
* n = 1 for EDC, 2 for ESC, 3 for ECC
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
* n = 1 for EDC, 2 for ESC, 3 for ECC
* n = 1 for EDC, 2 for ESC

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