Add pickling logic to pk_encryption #173
Conversation
poljar
force-pushed
the
poljar/pk-dekurcina
branch
2 times, most recently
from
6f97915 to
c231c72
Compare
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
force-pushed
the
poljar/pk-dekurcina
branch
from
d50e302 to
0057971
Compare
| /// 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> { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'll go ahead and remove the to_libolm_pickle function here 👍
There was a problem hiding this comment.
Thanks, that would be great.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Oh, you don't have encryption for storage. Let me sleep on that since it's quite late where I'm at.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 :)
| struct PkDecryptionPickle { | ||
| version: u32, | ||
| public_curve25519_key: [u8; 32], | ||
| private_curve25519_key: Box<[u8; 32]>, |
There was a problem hiding this comment.
This should be annotated as a secret, described here: https://github.com/matrix-org/matrix-pickle/?tab=readme-ov-file#encoding-and-decoding-secrets.
Co-authored-by: Damir Jelić <[email protected]>
poljar
force-pushed
the
poljar/pk-dekurcina
branch
3 times, most recently
from
805e657 to
484317b
Compare
|
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 |
Thanks. |
Hmm, it seems like I might not have enough permissions to be able to reopen a PR. Otherwise I have to create a whole new PR. |
|
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... |
|
Haha what a pickle :P |
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
Accountpickling works.The tests are passing against the
olm_rsimpl which gives me some confidence in it.