Store encryption settings #1460

Anderas · 2023-02-07T12:19:07Z

Resolves #1578

Add encryption settings to the crypto store that are currently still persisted in legacy client database. These include:

room encryption algorithm
settings to block unverified devices per room and globally

This is achieved by a new RoomSettings struct as well as generic get/set_custom_value for global flags.

Note that the higher-level non-crypto rust-sdk already stores these values as state events, which is now duplicated by the crypto store. This should be fully migrated to the crypto store because these settings are critical for the cryptographic functionality (e.g. no API to reset room encryption) and are used by clients that only integrate crypto-sdk.

poljar · 2023-02-21T09:45:32Z

crates/matrix-sdk-crypto/src/store/mod.rs

@@ -920,6 +922,15 @@ pub trait CryptoStore: AsyncTraitDeps {
    /// * `request_id` - The unique request id that identifies this outgoing key
    /// request.
    async fn delete_outgoing_secret_requests(&self, request_id: &TransactionId) -> Result<()>;
+
+    /// TODO: docs
+    async fn load_encryption_settings(


One thing I mentioned already, but it is still my main concern with this patch is that this is expanding the CryptoStore trait. I prefer to avoid this if possible, especially since this functionality is not something the matrix-sdk crate will use.

The idea here is that we might want to reuse the existing get/set value methods the crypto stores have. This would also allow people to store other custom things.

Yea that makes sense, I'll rework it as a separate commit so we can compare it

The idea here is that we might want to reuse the existing get/set value methods the crypto stores have. This would also allow people to store other custom things.

To be clear, we would still need to expose something like async fn set_value(&self, key: &str, value: &impl Serialize) -> Result<()>; (and getter equivalent) in the CryptoStore trait, at which point the compiler becomes unhappy about the use of generics when trying to make an object from the trait. Is this what you are suggesting?

Yep, the state store already has such an interface, though it puts the responsibility of serializing onto the caller:

matrix-rust-sdk/crates/matrix-sdk-sled/src/state_store.rs

Line 1245 in 7e64c15

async fn get_custom_value(&self, key: &[u8]) -> Result<Option<Vec<u8>>> {

Of course the interface of the CryptoStore needs to be exposed on the OlmMachine where you can put the generic impl Serialize argument if you prefer the Rust side to take care of the serialization.

Anderas · 2023-02-24T16:57:48Z

@poljar I have restructured this PR to only expose generic get/set_custom_value, similar to the API exposed in state store. I have then added another pair of get/set_value methods using generics into the Store struct, making it easier to use. Encryption works as expected (stretching single boolean into a massive cipertext)

If a viable approach, I'd still need to extract the raw-string keys into some kind of enum or so to ensure type safety

poljar · 2023-02-27T11:12:18Z

crates/matrix-sdk-crypto/src/machine.rs

@@ -506,6 +506,42 @@ impl OlmMachine {
        self.session_manager.get_missing_sessions(users).await
    }

+    /// TODO: docs
+    pub async fn get_encryption_settings(


Could we either keep those just in the bindings or if you disagree, let's expose the Store struct and let users use those methods directly on the store.

Of course we need to make sure we don't expose something we don't want.

poljar · 2023-02-27T11:13:25Z

@poljar I have restructured this PR to only expose generic get/set_custom_value, similar to the API exposed in state store. I have then added another pair of get/set_value methods using generics into the Store struct, making it easier to use. Encryption works as expected (stretching single boolean into a massive cipertext)

If a viable approach, I'd still need to extract the raw-string keys into some kind of enum or so to ensure type safety

Yup, this seems nicer than extending the CryptoStore trait with value specific setters and getters.

Anderas · 2023-03-01T11:23:03Z

After some discussions I decided to create a new table for room settings after all. This is because room settings are sufficiently important concept to warrant the expansion of the CryptoStore trait, and should be used by all clients (legacy as well as rust-sdk).

I have also decided to not change the share_room_key method (by removing EncryptionSettings) and instead let the clients continue passing them as a parameter. This is because at least history_visibility is still stored on the client side, and either way that change can be dealt with in a separate PR.

Anderas · 2023-03-01T11:24:35Z

bindings/matrix-sdk-crypto-ffi/src/machine.rs

+        let settings = self
+            .runtime
+            .block_on(self.inner.store().get_room_settings(&room_id))?
+            .map(|v| v.try_into().unwrap());


How can I flatten the possible error from try_into and return as CryptoStoreError?

You could change the error type that the TryFrom implementation uses from a string to a serde_json error, which supports custom error strings and then do something like:

let settings = self .runtime .block_on(self.inner.store().get_room_settings(&room_id))? .map(|v| v.try_into()) .transpose()?;

crates/matrix-sdk-crypto/src/store/mod.rs

Anderas · 2023-03-01T11:27:08Z

crates/matrix-sdk-crypto/src/store/mod.rs

+
+    fn serialize_value(&self, value: &impl Serialize) -> Result<Vec<u8>> {
+        let serialized =
+            rmp_serde::to_vec_named(value).map_err(|x| CryptoStoreError::Backend(x.into()))?;


The mapped error should ideally be CryptoStoreError::Serialization, but this is typed to serde_json instead of rmp_serde and I dont think adding another Serialization case is a good approach. Can we make that serialization error more generic?

Yeah, error handling in the stores is messy... I have an idea on how it may be fixable, will post a separate PR.

codecov · 2023-03-07T11:44:43Z

Codecov Report

Patch coverage: 71.54% and project coverage change: -0.06 ⚠️

Comparison is base (0f7d839) 75.49% compared to head (d226738) 75.44%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1460      +/-   ##
==========================================
- Coverage   75.49%   75.44%   -0.06%     
==========================================
  Files         137      137              
  Lines       14788    14883      +95     
==========================================
+ Hits        11164    11228      +64     
- Misses       3624     3655      +31

Impacted Files	Coverage Δ
crates/matrix-sdk-crypto/src/machine.rs	`75.58% <0.00%> (-0.40%)`	⬇️
crates/matrix-sdk-crypto/src/store/memorystore.rs	`79.04% <0.00%> (-6.52%)`	⬇️
crates/matrix-sdk-crypto/src/store/traits.rs	`62.71% <0.00%> (-7.10%)`	⬇️
crates/matrix-sdk-crypto/src/store/mod.rs	`68.25% <60.00%> (-4.63%)`	⬇️
crates/matrix-sdk-sled/src/crypto_store.rs	`89.36% <90.90%> (+0.07%)`	⬆️
crates/matrix-sdk-sqlite/src/crypto_store.rs	`93.01% <97.43%> (+0.38%)`	⬆️
...s/matrix-sdk-crypto/src/store/integration_tests.rs	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

poljar

I think this looks fine, as long as the unwrap is gone and some more methods are made pub(crate)

poljar · 2023-03-07T13:14:56Z

bindings/matrix-sdk-crypto-ffi/src/machine.rs

+        let settings = self
+            .runtime
+            .block_on(self.inner.store().get_room_settings(&room_id))?
+            .map(|v| v.try_into().unwrap());


You could change the error type that the TryFrom implementation uses from a string to a serde_json error, which supports custom error strings and then do something like:

let settings = self .runtime .block_on(self.inner.store().get_room_settings(&room_id))? .map(|v| v.try_into()) .transpose()?;

crates/matrix-sdk-crypto/src/store/mod.rs

Anderas · 2023-03-10T11:05:58Z

@poljar this should now be ready for review. the recent uniffi changes caused some conflicts but all should be resolved against main now

poljar

Alright, I think this looks good. There's a missing newline and a merge conflict, after this is resolved we can merge.

poljar · 2023-03-14T09:42:28Z

bindings/matrix-sdk-crypto-ffi/src/olm.udl

+dictionary RoomSettings {
+    EventEncryptionAlgorithm algorithm;
+    boolean only_allow_trusted_devices;
+};


Missing newline:

Suggested change

};

};

poljar reviewed Feb 21, 2023

View reviewed changes

Anderas force-pushed the andy/enhance_store branch 2 times, most recently from 363e744 to 022028f Compare February 24, 2023 16:52

poljar reviewed Feb 27, 2023

View reviewed changes

Anderas force-pushed the andy/enhance_store branch from 92a4b9c to c5ba87a Compare March 1, 2023 11:19

Anderas marked this pull request as ready for review March 1, 2023 11:20

Anderas commented Mar 1, 2023

View reviewed changes

Anderas force-pushed the andy/enhance_store branch from c5ba87a to cfb48c3 Compare March 1, 2023 11:24

Anderas commented Mar 1, 2023

View reviewed changes

crates/matrix-sdk-crypto/src/store/mod.rs Show resolved Hide resolved

Anderas commented Mar 1, 2023

View reviewed changes

Anderas requested a review from poljar March 1, 2023 11:28

Anderas force-pushed the andy/enhance_store branch 4 times, most recently from 1a33c75 to 869e0b4 Compare March 7, 2023 11:29

Anderas force-pushed the andy/enhance_store branch from 869e0b4 to f565a3a Compare March 7, 2023 11:47

poljar reviewed Mar 7, 2023

View reviewed changes

Anderas force-pushed the andy/enhance_store branch 2 times, most recently from 115b83b to f3bc1e8 Compare March 7, 2023 16:17

Anderas requested a review from poljar March 7, 2023 16:17

Anderas force-pushed the andy/enhance_store branch 3 times, most recently from 4b95afe to 39c4126 Compare March 10, 2023 10:27

This was referenced Mar 10, 2023

Fully deprecate MXCryptoStore matrix-org/matrix-ios-sdk#1735

Merged

Integrate room encryption store element-hq/element-ios#7419

Closed

poljar approved these changes Mar 14, 2023

View reviewed changes

Anderas added 11 commits March 14, 2023 10:27

Store encryption settings

b4b111f

Generic key-value API

7e7c916

Constrain new api to Store struct

33b348c

Introduce RoomSettings

283137f

Add room settings migration

51b3bf4

Missing stores

8f13023

Constrain store methods

98ca774

Indexdb migration

1177bdd

Fix issues after rebase

5bf9182

Update js tests

0d082f6

Missing migration method in UDL

404b95a

Anderas force-pushed the andy/enhance_store branch from 73853f0 to 404b95a Compare March 14, 2023 10:28

More pub(crate) methods

d226738

Anderas force-pushed the andy/enhance_store branch from 9f01e06 to d226738 Compare March 14, 2023 10:43

poljar merged commit 4041c87 into main Mar 14, 2023

poljar deleted the andy/enhance_store branch March 14, 2023 11:36

BillCarsonFr mentioned this pull request Mar 20, 2023

Store room encryption algorithm in the crypto store #1578

Closed

BillCarsonFr mentioned this pull request May 26, 2023

Extract common crypto interface for all flavors element-hq/element-android#8470

Merged

15 tasks

poljar mentioned this pull request Feb 2, 2024

Have the crypto crate handle some subset of state events #1325

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store encryption settings #1460

Store encryption settings #1460

Anderas commented Feb 7, 2023 •

edited

Loading

poljar Feb 21, 2023

Anderas Feb 22, 2023

Anderas Feb 22, 2023 •

edited

Loading

poljar Feb 22, 2023

Anderas commented Feb 24, 2023 •

edited

Loading

poljar Feb 27, 2023

poljar commented Feb 27, 2023

Anderas commented Mar 1, 2023 •

edited

Loading

Anderas Mar 1, 2023

poljar Mar 7, 2023

Anderas Mar 1, 2023

jplatte Mar 2, 2023

codecov bot commented Mar 7, 2023 •

edited

Loading

poljar left a comment

poljar Mar 7, 2023

Anderas commented Mar 10, 2023

poljar left a comment

poljar Mar 14, 2023

Store encryption settings #1460

Store encryption settings #1460

Conversation

Anderas commented Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anderas Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anderas commented Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

poljar commented Feb 27, 2023

Anderas commented Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 7, 2023 • edited Loading

Codecov Report

poljar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anderas commented Mar 10, 2023

poljar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anderas commented Feb 7, 2023 •

edited

Loading

Anderas Feb 22, 2023 •

edited

Loading

Anderas commented Feb 24, 2023 •

edited

Loading

Anderas commented Mar 1, 2023 •

edited

Loading

codecov bot commented Mar 7, 2023 •

edited

Loading