Add pickling logic to pk_encryption #173

devonh · 2024-08-23T00:46:19Z

This is an attempt to add pickling to the new pk_encryption work. #171
I am not very familiar with this codebase, but I took my best attempt at implementing the functionality based on how the legacy libolm Account pickling works.

The tests are passing against the olm_rs impl which gives me some confidence in it.

This patch introduces support for the libolm PkEncryption/PkDecryption concepts, ensuring bug-for-bug compatibility with libolm. Notably, the libolm implementation has a known flaw that leaves ciphertext unauthenticated, as documented in the Matrix spec [1]. To address this, the feature is gated behind a feature flag to better inform users of this issue. [1]: https://spec.matrix.org/v1.11/client-server-api/#backup-algorithm-mmegolm_backupv1curve25519-aes-sha2 Changelog: Add support for the libolm PkEncryption feature. This allows Matrix clients to implement the [m.megolm_backup.v1.curve25519-aes-sha2](https://spec.matrix.org/v1.11/client-server-api/#backup-algorithm-mmegolm_backupv1curve25519-aes-sha2) room key backup algorithm. Please note that this algorithm contains a critical flaw and should only be used for compatibility reasons.

poljar

Left some nits, and a question if one of the methods is even needed.

poljar · 2024-09-02T14:41:42Z

src/pk_encryption.rs

+    ///     PicklingMode::Encrypted { key: [0u8; 32].to_vec() },
+    /// ).expect("We should be able to unpickle our exported PkDecryption");
+    /// ```
+    pub fn to_libolm_pickle(&self, pickle_key: &[u8]) -> Result<String, crate::LibolmPickleError> {


Err, does anybody use that? We just need the seed for a Curve25519, AFAIK even in libolm days, Element Web didn't use this to store the key.

Persisting the public key is unnecessary, and then there's the whole key re-use trap this might propagate.

This is currently how Matrix Content Scanner saves it's keys. I have no objections to saving them in a different format going forward. I kept this in out of convenience to minimize changes in the content scanner.

I'll go ahead and remove the to_libolm_pickle function here 👍

Thanks, that would be great.

Hmm I'm not sure if removing this entirely will work. The Matrix Content Scanner backend relies on saving it's key locally in pickled form.

I could change this to be pickle and create a pickled PkDecryption which only contains the secret and omits the public key. Either way we will need something that allows us to create a new key and save it somewhere.

@poljar Is there a better way to go about this?

Those would allow us to save the key yes. We would lose the additional encryption we get with the current approach though. (Since the libolm pickle is encrypted with an additional key)

I'm not sure if the encrypted pickle adds much, since the key for it is stored in the Content Scanner's config file.

Thoughts?

Oh, you don't have encryption for storage. Let me sleep on that since it's quite late where I'm at.

Fuck it, let's keep it then. Somebody else might depend on this as well and then I'll have this conversation again if we don't keep it.

For the scanner itself it seems fine to me to just ignore the pickled key and create a new one, it would just break on update for a very small number of reqs that fetched the public key before the update, and it will just work on retry I believe (new public key will be fetch and used).

I'm not sure if the encrypted pickle adds much, since the key for it is stored in the Content Scanner's config file.

Agreed, it seems fairly useless to me.

But poljar point applies, now that it's done here we may as well merge it if others need it, it's not a lot of code.

Do we need to plug the pickle logic in the content scanner or just remove it altogether (and generate a new key on first start) is another story :)

src/pk_encryption.rs

poljar · 2024-09-05T14:32:58Z

src/pk_encryption.rs

+struct PkDecryptionPickle {
+    version: u32,
+    public_curve25519_key: [u8; 32],
+    private_curve25519_key: Box<[u8; 32]>,


This should be annotated as a secret, described here: https://github.com/matrix-org/matrix-pickle/?tab=readme-ov-file#encoding-and-decoding-secrets.

poljar

Still found a small nit in the docs, though after that I think this is fine. We still need to wait for #171 to land though.

src/pk_encryption.rs

Co-authored-by: Damir Jelić <poljar@termina.org.uk>

poljar · 2024-09-11T09:41:11Z

Oh my, this got auto-closed because the base branch got auto-deleted. Could you reopen it with a different base?

devonh · 2024-09-11T14:54:59Z

Oh my, this got auto-closed because the base branch got auto-deleted. Could you reopen it with a different base?

haha yeah no problem. I'll open it against main now

poljar · 2024-09-11T15:02:36Z

Oh my, this got auto-closed because the base branch got auto-deleted. Could you reopen it with a different base?

haha yeah no problem. I'll open it against main now

Thanks.

devonh · 2024-09-11T15:50:19Z

Oh my, this got auto-closed because the base branch got auto-deleted. Could you reopen it with a different base?

haha yeah no problem. I'll open it against main now

Thanks.

Hmm, it seems like I might not have enough permissions to be able to reopen a PR.
Are you able to reopen it?

Otherwise I have to create a whole new PR.

poljar · 2024-09-11T16:54:37Z

I am not able to reopen it either 😅. I can't reopen it because the base branch was deleted and I can't change the base branch...

devonh · 2024-09-11T17:24:28Z

Haha what a pickle :P
I can just create a new PR then

poljar and others added 4 commits August 15, 2024 16:12

WIP PK decryption support

ca73bad

fixup! WIP PK decryption support

fba38d2

fixup! WIP PK decryption support

7cc7129

Add pickling logic to pk_encryption

87724f4

devonh requested review from dkasak and poljar as code owners August 23, 2024 00:46

poljar force-pushed the poljar/pk-dekurcina branch 2 times, most recently from 6f97915 to c231c72 Compare September 2, 2024 14:15

poljar added 3 commits September 3, 2024 13:03

ci: Enable all features for our clippy run

f5eca51

Constify some more functions

0057971

poljar force-pushed the poljar/pk-dekurcina branch from d50e302 to 0057971 Compare September 3, 2024 11:03

devonh added 5 commits September 3, 2024 12:03

Merge branch 'poljar/pk-dekurcina' into devon/pk-pickle

6132e38

Fix style issues

7b9b030

Add better error messages to pickle tests

5b299ee

Add docs for additional pickle functions

99d073f

Remove unnecessary docs

4900fee

poljar requested changes Sep 4, 2024

View reviewed changes

Derive public key from private key

723c1b2

poljar reviewed Sep 5, 2024

View reviewed changes

Make private key secret

6fb1507

poljar approved these changes Sep 6, 2024

View reviewed changes

src/pk_encryption.rs Outdated Show resolved Hide resolved

Update src/pk_encryption.rs

5bf62e8

Co-authored-by: Damir Jelić <poljar@termina.org.uk>

poljar force-pushed the poljar/pk-dekurcina branch 3 times, most recently from 805e657 to 484317b Compare September 11, 2024 09:34

poljar deleted the branch matrix-org:poljar/pk-dekurcina September 11, 2024 09:39

poljar closed this Sep 11, 2024

devonh mentioned this pull request Sep 11, 2024

Add pickling logic to pk_encryption #180

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pickling logic to pk_encryption #173

Add pickling logic to pk_encryption #173

devonh commented Aug 23, 2024 •

edited

Loading

poljar left a comment

poljar Sep 2, 2024

devonh Sep 4, 2024

devonh Sep 5, 2024

poljar Sep 5, 2024

devonh Sep 5, 2024

devonh Sep 5, 2024

poljar Sep 5, 2024

poljar Sep 6, 2024

MatMaul Sep 6, 2024

MatMaul Sep 6, 2024 •

edited

Loading

poljar Sep 5, 2024

poljar left a comment

poljar commented Sep 11, 2024

devonh commented Sep 11, 2024

poljar commented Sep 11, 2024

devonh commented Sep 11, 2024

poljar commented Sep 11, 2024

devonh commented Sep 11, 2024

Add pickling logic to pk_encryption #173

Add pickling logic to pk_encryption #173

Conversation

devonh commented Aug 23, 2024 • edited Loading

poljar left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatMaul Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poljar left a comment

Choose a reason for hiding this comment

poljar commented Sep 11, 2024

devonh commented Sep 11, 2024

poljar commented Sep 11, 2024

devonh commented Sep 11, 2024

poljar commented Sep 11, 2024

devonh commented Sep 11, 2024

devonh commented Aug 23, 2024 •

edited

Loading

MatMaul Sep 6, 2024 •

edited

Loading