Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add pickling logic to pk_encryption #173
Add pickling logic to pk_encryption #173
Changes from 1 commit
ca73bad
fba38d2
7cc7129
87724f4
5e7d40f
f5eca51
0057971
6132e38
7b9b030
5b299ee
99d073f
4900fee
723c1b2
6fb1507
5bf62e8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll go ahead and remove the
to_libolm_pickle
function here 👍There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that would be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be annotated as a secret, described here: https://github.com/matrix-org/matrix-pickle/?tab=readme-ov-file#encoding-and-decoding-secrets.