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-530 Implement FLE2IndexedEqualityEncryptedValueV2 #594

Closed
wants to merge 5 commits into from

Conversation

sgolemon
Copy link
Contributor

No description provided.

src/mc-fle2-payload-iev-v2.c Outdated Show resolved Hide resolved
src/mc-fle2-payload-iev-v2.c Outdated Show resolved Hide resolved
src/mc-fle2-payload-iev-v2.c Outdated Show resolved Hide resolved
src/mc-fle2-payload-iev-v2.c Outdated Show resolved Hide resolved
src/mc-fle2-payload-iev-v2.c Outdated Show resolved Hide resolved
src/mc-fle2-payload-iev-v2.c Outdated Show resolved Hide resolved
src/mc-fle2-payload-iev-v2.c Outdated Show resolved Hide resolved
src/mc-fle2-payload-iev-v2.c Show resolved Hide resolved
*/

typedef struct _mc_FLE2IndexedEqualityEncryptedValueV2_t
mc_FLE2IndexedEqualityEncryptedValueV2_t;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make this API also agnostic to whether the indexed value is equality vs range, as it is in v1? I think the only function that will need to distinguish them will be in parse(); the rest of the decrypt logic is the same for both equality and range.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was planning, but I've added some extra prep for 531 to make that more obvious.

@@ -106,6 +106,7 @@ set (MONGOCRYPT_SOURCES
src/mc-fle2-find-equality-payload.c
src/mc-fle2-find-equality-payload-v2.c
src/mc-fle2-payload-iev.c
src/mc-fle2-payload-iev-v2.c
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
src/mc-fle2-payload-iev-v2.c
src/mc-fle2-payload-ieev-v2.c

The V1 struct in mc-fle2-payload-iev.c uses the same struct mc_FLE2IndexedEncryptedValue_t to represent both payloads FLE2IndexedEqualityEncryptedValue and FLE2IndexedRangeEncryptedPayload.

The new mc_FLE2IndexedEqualityEncryptedValueV2_t only represents FLE2IndexedEqualityEncryptedValueV2.

Suggest renaming related files to include extra "e" for equality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My plan is to repeat that style of having unified accessors, I just haven't gotten to MC-531 yet. I've re-organized the code to make that more obvious.

&reader, &iev->ServerEncryptedValue, SEV_len, status));

// Ignore Metadata block.
BSON_ASSERT (mc_reader_get_remaining_length (&reader) == kMetadataLen);
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
BSON_ASSERT (mc_reader_get_remaining_length (&reader) == kMetadataLen);
if (mc_reader_get_remaining_length (&reader) != kMetadataLen) {
CLIENT_ERR ("Expected remaining payload length %" PRIu64 ", got %" PRIu64,
kMetadataLen,
mc_reader_get_remaining_length (&reader));
return false;
}

Return an error instead of assert since this is parsing user data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason that it asserts rather than erroring is because we've explicitly calculated out position to be at this location when we created SEV_len. If the remaining length is even NOT kMetadataLen at this point then the reader implementation has failed in some spectacular way.

* limitations under the License.
*/

#ifndef MONGOCRYPT_INDEXED_ENCRYPTED_VALUE_PRIVATE_V2_H
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
#ifndef MONGOCRYPT_INDEXED_ENCRYPTED_VALUE_PRIVATE_V2_H
#ifndef MONGOCRYPT_INDEXED_EQUALITY_ENCRYPTED_VALUE_PRIVATE_V2_H

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Negative, see above.

};

#define kMetadataLen 96U // encCount(32) + tag(32) + encZeros(32)
#define kMinServerEncryptedValueLen 17U // IV(16) + EncrtyptCTR(1byte)
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 kMinServerEncryptedValueLen 17U // IV(16) + EncrtyptCTR(1byte)
#define kMinServerEncryptedValueLen 17U // IV(16) + EncryptCTR(1byte)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

* }
*
* ServerEncryptedValue :=
* EncryptCTR(ServerEncryptionToken, ClientEncryptedValue)
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
* EncryptCTR(ServerEncryptionToken, ClientEncryptedValue)
* EncryptCTR(ServerEncryptionToken, K_KeyId || ClientEncryptedValue)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@sgolemon sgolemon requested a review from erwee March 16, 2023 21:47
test/test-mc-fle2-payload-iev-v2.c Outdated Show resolved Hide resolved
@sgolemon
Copy link
Contributor Author

Merged as 528102f

@sgolemon sgolemon closed this Mar 17, 2023
@sgolemon sgolemon deleted the MC-530 branch March 17, 2023 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants