-
Notifications
You must be signed in to change notification settings - Fork 28
INTPYTHON-527 Add Queryable Encryption support #329
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
Open
aclark4life
wants to merge
32
commits into
mongodb:main
Choose a base branch
from
aclark4life:INTPYTHON-527
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
32 commits
Select commit
Hold shift + click to select a range
36bd1e5
INTPYTHON-527 Add Queryable Encryption support
aclark4life 70c946b
encrypted fields map != encrypted fields
aclark4life cb3512c
Use dot separator
aclark4life 46ca9dc
Refactor tests
aclark4life 87b1dc9
Add embedding
aclark4life d39f3b3
Review feedback
aclark4life 3f8b5c2
remove EncryptedEmbeddedModel
timgraham 441c584
PatientRecord shouldn't be encrypted
timgraham c9cc301
fix linting of kms_provider() docstring line length
timgraham 332decb
Code review fixes
aclark4life 0317fba
Use EncryptionTestCase instead of TestCase
aclark4life e3da2f6
make atlas tests use encryption settings
timgraham fe790c7
try shared library
timgraham 2270058
try ubuntu 22.04 just to be sure
timgraham e38b496
Code review fixes
aclark4life 36c6bfd
Update QE guide with complete Python tutorial
aclark4life 86486eb
update version added to 5.2.2
aclark4life 59865f7
Add tests for EncryptedFieldMixin
aclark4life fc87e9f
Remove confusing paragraph about crypt shared
aclark4life 75873e9
Target 5.2.3 for release and require MongoDB 8
aclark4life 324c959
try Mongo 8.0.15 on CI
timgraham 13ed19a
Misc updates
aclark4life b23c4f2
Misc updates
aclark4life a0cd197
Kill the helper
aclark4life 65b96b2
Misc updates
aclark4life 80881fa
Add assertEncrypted to verify field data is binary
aclark4life 25e7da1
Fails on CI only
aclark4life ff84902
Remove create_data_keys until use case manifests
aclark4life 23f20a4
Add partial index on keyAltNames for uniqueness
aclark4life 01ec095
Code review updates
aclark4life 8a3ccf5
Code review updates
aclark4life 7b0956d
Add EncryptedArrayField + test
aclark4life File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 lose this helpful message:
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 think that logic was flawed. Updated to check for existing keys or create if not found.
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.
Then
showencryptedfieldsmap
isn't read-only and may have a side effect of creating keys?What is still missing for me is the "why" of setting
encrypted_fields_map
inAutoEncryptionOpts
. Based on past discussion in this PR, I recall it has something to do with security, but I don't think this is explained in this PR's documention or in the design doc.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've removed
--create-data-keys
so now we're just talking aboutshowencryptedfieldsmap
and yes, since it calls_get_encrypted_fields
it will create the data keys if they aren't found in the key vault.That's in the spec:
I don't fully understand that explanation so I haven't gone too far in documenting it other than to say "recommended".
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 think it's necessary to understand how it works so we can document the proper workflow. For example, it feels to me like the sequence of actions might be: deploy your app, migrate, run showencryptedfieldsmap, put the result in your settings. (i.e. you should not deploy with
encrypted_fields_map
already set, because those keyIds won't be used as I described in #329 (comment)). (It's possible I'm wrong or that I made a mistake, but we cannot ship a feature with uncertainties like this.)Uh oh!
There was an error while loading. Please reload this page.
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 don't mean that we don't understand what we are building, I mean I'm not sure I understand how "a malicious server advertising a false encryptedFields" happens. In any event, mostly agree on the flow. I'm picturing:
showencryptedfieldsmap
and setencrypted_fields_map
inauto_encryption_opts
At first I was picturing a scenario in which the second deployment wasn't needed, but I think
client_encryption.create_data_key
makes that very difficult if not impossible, thus invalidating the "single deployment" use case.