-
Notifications
You must be signed in to change notification settings - Fork 495
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
Client Encryption: Fix decryption for 'order by' query results #1369
Conversation
Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/EncryptionTests.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/EncryptionTests.cs
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Query/v3Query/CosmosQueryClientCore.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/src/Query/v3Query/CosmosQueryClientCore.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/EncryptionTests.cs
Show resolved
Hide resolved
@@ -181,5 +194,34 @@ public override CosmosElement GetCosmsoElementContinuationToken() | |||
{ | |||
return this.cosmosQueryExecutionContext.GetCosmosElementContinuationToken(); | |||
} | |||
|
|||
private async Task<List<CosmosElement>> GetDecryptedElementResponseAsync( |
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.
At this point all the encryption and decryption seems to happen outside of the core SDK logic. In fact, it seems a user could implement this feature without any internal access, since it's encrypting the document before write and decrypting after read. If that's the case couldn't we just have an EncryptedContainer
class that composes / wraps Container
and just wraps all the methods appropriately? That core SDK logic will not need to know about 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.
The container is not encrypted, only some fields in some items are - given this, should users use Container when they think the item they are reading doesn't have encrypted properties? The answer is no, since customers may not even know whether the item they are reading has encrypted properties - given this, we should not force customers to use one more set of classes.
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 @bchong95 is talking about something like this. Where the EncryptedContainer
is just an internal implementation class that is never directly exposed to users. All the encryption logic is completely external to the SDK. It's just like if an external users created the package.
public sealed class EncryptionDatabaseCore : Database
{
private Database database;
public override Container GetContainer(string id){ return new EncryptionContainerCore();}
}
public sealed class EncryptionContainerCore : Container
{
private Container container;
public override ItemResponse<T> CreateItemAsync<T>(){}
.....
}
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.
Thanks Jake, Brandon & Abhijit! I've setup some time with you guys & Kiran to finalize the internal implementation details so as to be on the same page and avoid back & forth.
Since this is internal implementation details we are talking about, maybe I can work on refactoring the code as a follow up if we decide to go down that route? In the meanwhile, can we get this fix in to unblock Office team 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.
Resolved offline. @anujtoshniwal will follow-up with future PR.
Co-Authored-By: j82w <j82w@users.noreply.github.com>
Description
For 'order by' queries, the response from the server is wrapped in the form of {orderByItems, payload}. In this case, the payload contains the document properties (including encryption property ("_ei")) as opposed to normal queries which had the properties as part of the response itself. As a result, EncryptionProcessor wasn't able to find encryption property and hence didn't decrypt accordingly for orderBy queries. The fix is to move the decryption logic from CosmosQueryClientCore.cs to QueryIterator.cs where the response is already ordered and unwrapped.
Type of change
Please delete options that are not relevant.