-
Notifications
You must be signed in to change notification settings - Fork 21
INTPYTHON-527 Add queryable encryption support #319
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
@@ -94,3 +94,11 @@ def test_invalid_credentials(self): | |||
def test_no_scheme(self): | |||
with self.assertRaisesMessage(pymongo.errors.InvalidURI, "Invalid URI scheme"): | |||
parse_uri("cluster0.example.mongodb.net") | |||
|
|||
|
|||
# TODO: This can be moved to `test_features` once transaction support is merged. |
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.
test_features.py
would only be for testing the logic of DatabaseFeatures.supports_queryable_encryption
.
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.
Agreed
try: | ||
import pymongocrypt # noqa: F401 | ||
|
||
has_pymongocrypt = True | ||
except ImportError: | ||
has_pymongocrypt = False |
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.
It's inappropriate to check for this package here. Instead the ImportError should be surfaced to the user if they try to use a feature that requires it. I can imagine a couple of ways to do so, but I think this concern should be deferred until the implementation is ironed 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.
I'll move it to get_auto_encryption_options
def get_auto_encryption_options(crypt_shared_lib_path=None): | ||
key_vault_database_name = "encryption" | ||
key_vault_collection_name = "__keyVault" | ||
key_vault_namespace = f"{key_vault_database_name}.{key_vault_collection_name}" | ||
kms_providers = {"local": {"key": os.urandom(96)}} | ||
return AutoEncryptionOpts( | ||
key_vault_namespace=key_vault_namespace, | ||
kms_providers=kms_providers, | ||
crypt_shared_lib_path=crypt_shared_lib_path, | ||
) |
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.
It's still obvious to me from the design doc to what extent it's appropriate for this library to provide helpers to generate AutoEncryptionOpts
.
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.
From the design doc
Take less steps than manual configuration of QE
We can expand that bullet into a section about configuration and it does not have to be a helper like get_auto_encryption_options
but the tutorial takes this approach and I like it so far.
) | ||
|
||
|
||
def parse_uri(uri, *, auto_encryption_options=None, db_name=None, test=None): |
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'd rather than options=None
than auto_encryption_options=None
so that we're not blowing up the signature of parse_uri()
with every kwarg
of MongoClient
. (If it's even in scope to continually expand parse_uri()
rather than to guide more advanced users toward a dictionary DATABASES
). I think all the changes in this file could be discussed/implemented in a follow up.
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.
To enable this feature in PyMongo and start development in Django, an auto_encryption_options
object has to be passed to MongoClient
in base.py
. As someone who opposed db_name
in parse_uri
I can tell you this is not a casual addition and this implementation is based on your suggestion to merge the options. We could rename, but I'm not convinced yet. I'll shorten it to auto_encryption_opts
to match PyMongo for now.
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 suggesting:
def parse_uri(uri, *, ... options=None)`:
...
if options:
options = {**uri.get("options"), options}
Usage:
parse_uri(uri, options={"auto_encryption_options": ...})
Otherwise, we're headed down the road of adding a new keyword to parse_uri()
for each MongoClient
keyword that the user wants to customize.
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.
Ah! Got it, thanks
|
||
# TODO: This can be moved to `test_features` once transaction support is merged. | ||
class ParseUriOptionsTests(TestCase): | ||
@skipUnlessDBFeature("supports_queryable_encryption") |
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.
Probably we just make installing pymongo[encryption]
a required part of running our test suite. In that case, this test wouldn't need a skip since it doesn't interact with the server.
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.
Not sure, but as you said above we shouldn't check imports in features and so this check is for enterprise_or_atlas
and we'd skip it on anything that is not that (similar to transaction support checks) and that has nothing to do with encryption libraries.
Another WIP
get_auto_encryption_options
to support ephemeral local KMS and allowcrypt_shared_lib_path
to be passed insupports_queryable_encryption
feature checkissubclassof
check forEncryptedModel
to callcreate_encrypted_collection
instead ofcreate_collection
but need to passencrypted_client
toClientEncryption
somewhere in Django.TODO
crypt_shared_lib_path
. I don't like setting an environment variable for this configuration task so I settled on passing toget_auto_encryption_options
in the short term but maybe a Django setting or something else in long term.EncryptedModel
andEncryptedField
(notEncryptedModelField
or EncryptedEmbeddedModelField yet!) implementation to support this:Question
Lastly, I'm not sure why PyMongo is trying to do explicit here or at least that is what it appears to be doing despite the auto config:
Seems like I may have missed passing
kms_providers
somewhere.I ruled out
crypted_shared_lib_path
being the cause. If you configure an invalid/path/to/libmongocrypt.so
you get:So… yeah.