[CSHARP-4820] C# driver doesn't work with IReadOnlyCollection<s> types Created: 25/Oct/23 Updated: 05/Feb/24 |
|
| Status: | In Code Review |
| Project: | C# Driver |
| Component/s: | Serialization |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Bug | Priority: | Minor - P4 |
| Reporter: | Ladan Nekuii | Assignee: | Robert Stam |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Attachments: |
|
| Case: | (copied to CRM) |
| Documentation Changes Summary: | 1. What would you like to communicate to the user about this feature? |
| Description |
SummaryC# driver doesn't work with IReadOnlyCollection<s> types. Customer is implementing CSFLE with C# driver, It is working with fields and generic IEnumerable<> types but recently a new feature tried to use IReadOnlyCollection<s> which should have been encrypted, but is not. They think the issue is somewhere in the C# driver as they are not getting called back into PropertyEncrypterBsonSerializer. Adding a .ToList() to the update operation ( after the Select()) fixes the issue. Please provide the version of the driver. If applicable, please provide the MongoDB server version and topology (standalone, replica set, or sharded cluster).MongoDB 7.0.2 (Replica Set) How to ReproduceSteps to reproduce. If possible, please include a Short, Self Contained, Correct (Compilable), Example. Here is the sample code customer is using:
For exemple, a document that has the following properties will only encrypt the IEnumerable<s> type: __
|
| Comments |
| Comment by Robert Stam [ 02/Feb/24 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I think we can set aside how the serialization is configured, as that's not part of the issue. The underlying issue is that a value is being serialized using the wrong serializer. Looks like there are two pieces working together to result in this outcome. The first piece is that the compiler is inferring the "wrong" type for `<TField>`in the following line of code:
It is inferring the type of `<TField>` to be `IEnumerable<string>` instead of `IReadOnlyCollection<string>`. The second piece is that the driver is trying to be helpful by making every possible attempt to serialize the value, preferably using the serializer configured for the `ExternalInviteeEmails` field. Using the serializer configured for the field requires converting the supplied value to the type of the field, but that's not possible in this case. A value of type `IEnumerable<string>` cannot be converted to `IReadOnlyCollection<string>`. Therefore the driver gives up on using the configured serializer and looks up one in the serializer registry instead. Normally this works fine. But in your case it was important that we use the original serializer. I am creating a pull request that changes the driver to throw an exception if the supplied value cannot be converted to the type of the field. In your scenario your code would still compile without the call to `ToList()`, but you will now get a runtime exception. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Ladan Nekuii [ 22/Jan/24 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Thanks Robert. Please let me know if you need any other information. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Robert Stam [ 16/Jan/24 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Thank you. I will be back in the office tomorrow and will take another look. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Ladan Nekuii [ 16/Jan/24 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Here is the update: If I look at your github I can see the try/catch statement in the same file linkname. Here is the stack trace:
Here is a screenshot of his debugger with locals if that helps:
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Robert Stam [ 11/Jan/24 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Perhaps a simple repro would be to use string fields and a "dummy" encrypting serializer that does something to that string (perhaps reverses the characters?). It seems like this is probably a question about how to configure serialization so that the correct encrypting serializer gets used for a field. In order to troubleshoot that we don't need to actually encrypt the values, we just need to do something to the string so that we can verify that the values stored in the database have been "encrypted" as expected. It's also not clear to me why the exception would be swallowed. I might need to see more repro code (enough to get the same exception myself). Do you need to encrypt any kind of value? If so I assume that the encrypted values would be stored in the database as byte arrays? | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Robert Stam [ 11/Jan/24 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
> Which is swallowed by the driver here in FieldValueSerializerHelper > {{try { convertedValue = (TTo)Convert.ChangeType(value, toType); return true; }catch { }}} I don't see any such line in FieldValueSerializerHelper.cs. There is such a line in a different file. A full stack trace would help determine how we reached that line. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Ladan Nekuii [ 10/Jan/24 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
>If the goal is to do field level encryption perhaps a better approach would be to use the built-in support for field level encryption. We cannot use the CSFLE provided by Mongo because originally this product was built with EventStore as a primary data store. MongoDB was only used as a readmodel data store where the data can be thrown away and rebuilt from the event sourced streams. Because EventStore is immutable we cannot change the encryption keys. This mongo serializer technique handles our encryption that is the same used for EventStore. The company is making a lot of efforts to get a SOC2 certification and having this loophole where we could fail to encrypt data is a little bit worrisome. >I need a better repro. And maybe a better explanation of exactly what is trying to be accomplished and how, and why you think that it is the way to do it. I'll try to rework the sample I provided so it is more clear to you, but the behavior is a silent exception swallow, resulting in the client code thinking the encryption worked and thus serializing the unencrypted bytes as is. Converting the input field from IReadOnlyCollection to an IList fixes the problem, but a developer could accidentally fall into this trap in the future. For the repro case. Nevermind the error you got when calling await this.ThisWillNotCallTheSerializer(); What is important is that the serializer is being called as you can see in the callstack. This is where we would perform our encryption of the field decorated with the SensitiveInformationPropertyAttribute. What is important is the other function. Just remove the above call in the test and execute it. With proper exception settings you should trap an exception like this:
Which is swallowed by the driver here in FieldValueSerializerHelper {{try { convertedValue = (TTo)Convert.ChangeType(value, toType); return true; } catch { }}} This code then gets presumably branched out of the serializer in the else statement here:
Let me know if this helps, or not. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Robert Stam [ 02/Jan/24 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I've also found more recent documentation on client side field level encryption here: https://www.mongodb.com/docs/drivers/csharp/current/fundamentals/encrypt-fields/ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Robert Stam [ 02/Jan/24 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
If the goal is to do field level encryption perhaps a better approach would be to use the built-in support for field level encryption: See: https://mongodb.github.io/mongo-csharp-driver/2.18/reference/driver/crud/client_side_encryption/ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Robert Stam [ 02/Jan/24 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I need a better repro. And maybe a better explanation of exactly what is trying to be accomplished and how, and why you think that it is the way to do it. When I run the repro provided above I simply get this exception:
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Ladan Nekuii [ 02/Jan/24 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Thanks for the update. In their case it silently fails and the codepath continues. The end result on their end is that the caller thinks that their encryption succeeded and writes the document in clear text. Please let me know if you need any other information. Thanks. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Robert Stam [ 26/Dec/23 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
In my attempts to reproduce this I get an exception from the PropertyEncrypterBsonSerializer methods. The Deserialize and Serialize methods call the base methods which in turn throw exceptions. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Robert Stam [ 26/Dec/23 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Thank you for the code to reproduce this with. Can you also let me know exactly how it is failing for you? Is it throwing an exception? | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Ladan Nekuii [ 21/Dec/23 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hi robert@mongodb.com, Here is a reproduction test using xunit:
Please let me know if you need anything else. Thanks. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by PM Bot [ 15/Nov/23 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There hasn't been any recent activity on this ticket, so we're resolving it. Thanks for reaching out! Please feel free to reopen this ticket if you're still experiencing the issue, and add a comment if you're able to provide more information. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Carl Tremblay [ 07/Nov/23 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
We're in the middle of something, I will take a look in the near future. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by PM Bot [ 07/Nov/23 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hi ladan.nekuii@mongodb.com! CSHARP-4820 is awaiting your response. If this is still an issue for you, please open Jira to review the latest status and provide your feedback. Thanks! | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Robert Stam [ 30/Oct/23 ] | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I reformatted the code in the description to make it readable but discovered that it ends abruptly and some of the code is missing. Can you take a look and provide the missing code? |