- Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-3053 Key Management API #958
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
Test failures are being addressed in PYTHON-3286 and PYTHON-3319. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few final comments.
test/unified_format.py Outdated
| ||
| ||
binary_types = (Binary, bytes) | ||
binary_types = (Binary, bytes, uuid.UUID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little concerned this might lead to masking bugs where we expect to be sending (or returning) Binary but actually end up with UUID. Can we remove this now that we use uuid_representation=unspecified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying it out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[2022/06/29 21:48:53.819] FAIL: test_create_data_key_with_AWS_KMS_provider (test.test_encryption.TestUnifiedCreateDataKey) [2022/06/29 21:48:53.819] ---------------------------------------------------------------------- [2022/06/29 21:48:53.819] Traceback (most recent call last): ... [2022/06/29 21:48:53.819] self.test.assertIsInstance(value, permissible_types) [2022/06/29 21:48:53.819] AssertionError: UUID('55c9396c-0d87-4128-93b0-095b806818cb') is not an instance of (<class 'bson.binary.Binary'>, <class 'bytes'>)
We should fix these errors by making _KEY_VAULT_OPTS use unspecified UUID representation (the default) instead of STANDARD. The "data_key _id must be a UUID" check will need to be updated to check for Binary with UUID_SUBTYPE too.
Note that _DATA_KEY_OPTS should still keep using uuid_representation=STANDARD
otherwise we may break b/c for encoding a schema_map that contains UUIDs. _KEY_VAULT_OPTS is only used internally so we are free to change it as we please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
I updated the pymongocrypt dependency to point to the commit merging mongodb/libmongocrypt#375 |
pymongo/encryption.py Outdated
if isinstance(data_key_id, Binary): | ||
if data_key_id.subtype != UUID_SUBTYPE: | ||
raise TypeError("data_key _id must have a UUID subtype") | ||
elif not isinstance(data_key_id, uuid.UUID): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should only accept Binary, never UUID here. Let's also update the return below:
- return Binary(data_key_id.bytes, subtype=UUID_SUBTYPE) + return data_key_id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Depends on mongodb/libmongocrypt#375