matrix-org / matrix-rust-sdk

Matrix Client-Server SDK for Rust
Apache License 2.0
1.19k stars 239 forks source link

Remove the default serializer in the matrix-sdk-store-encryption crate #718

Open poljar opened 2 years ago

poljar commented 2 years ago

The default serialization format for encrypted values in the store-encryption crate is JSON using serde_json. Since the ciphertext and nonce are byte vectors and JSON a textual encoding this ends up using more storage than what would be needed.

People can already use any other serialization format that is supported by Serde so let's just remove the default one.

poljar commented 7 months ago

This PR will deprecate the methods in question: https://github.com/matrix-org/matrix-rust-sdk/pull/3086. After that, they can be removed in the release after.

richvdh commented 2 months ago

It's worth noting that there are two serialization steps in encryption, and this issue isn't entirely clear whether we're talking about both, or just one (and if so, which).

First of all we have to serialize the input data from rust. encrypt_value , encrypt_value_typed, and encrypt_value_base64_typed serialize the data as JSON. encrypt_value_base64_data and encrypt_value_data take the serialized data.

Then we do the encryption, which gives us an EncryptedValue struct.

Then, possibly, we, do a second round of serialization, on the EncryptedValue. Here, encrypt_value serializes the EncryptedValue as JSON. encrypt_value_typed and encrypt_value_data return the raw EncryptedValue for serialization by the caller. encrypt_value_base64_typed and encrypt_value_base64_data base64-encode the values inside the EncryptedValue to give an EncryptedValueBase64. (The latter is not strictly a serialization, but it's convenient to consider together here.)

In summary:

Method Pre-encryption serialization Post-encryption conversion
encrypt_value JSON JSON
~encrypt_value_typed~ ~JSON~ ~None~
~encrypt_value_base64_typed~ ~JSON~ ~EncryptedValueBase64~
encrypt_value_base64_data None EncryptedValueBase64
encrypt_value_data None None

[edited: *_typed have been removed]

So, I'm not sure if this issue is talking about just removing encrypt_value (and its counterpart decrypt_value), or also the *_typed methods? I note that #3086 did deprecate en/decrypt_value_typed.

poljar commented 2 months ago

So, I'm not sure if this issue is talking about just removing encrypt_value (and its counterpart decrypt_value), or also the *_typed methods?

Ideally, the matrix-sdk-store-encryption crate would not depend on the serde_json crate anymore. This would mean that only encrypt_value_data() would be left, though that could be a bit hard to pull off due to backwards compatibility needs.

A weaker, but still worthy, goal would be to remove any post-encryption conversion.

richvdh commented 2 months ago

Ok thanks. #3638 has now removed the *_typed variants; I've updated the table above accordingly.

en/decrypt_value and en/decrypt_value_base64_data are only used in the indexeddb cryptostore impl, so we could at least hoist them up to there to contain the mess.