-
Notifications
You must be signed in to change notification settings - Fork 56
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
Implement loading old megolm keys from SSSS #687
Conversation
Kudos, SonarCloud Quality Gate passed! |
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.
Haven't reviewed the code in detail yet but got one thing to begin with, please.
2b6250b
to
87b4810
Compare
I think this is ready from my side. What's "missing" for a complete SSSS implementation is the "other side" of the whole thing - uploading keys to the backup, etc. - I'll do that in a different merge request. One thing I'm not sure about is the verified status of the megolm sessions - thing like the |
b9b1dee
to
4585628
Compare
4585628
to
1f125e8
Compare
Using an iterator instead of repeated hashmap lookups is not only slightly more efficient but also allows to make QOlmInboundSession constructor private again.
cryptoutils: more type safety with compile-time-sized spans
aesCtr256En/Decrypt used FixedBuffer for the target (encrypted and decrypted, respectively) buffer. FixedBuffer uses secure memory, and it is purposefully limited to 64k, because it's meant to be used for secrets, not for payloads (for now at least). This commit reverts to using normal QByteArray to store the result, using asWritableCBytes() to get a span adapter that is more convenient to work with in OpenSSL.
...to work with QUOTIENT_FORCE_NAMESPACED_INCLUDES
1. Instead of templating decryptKey (which effectively emits the same function 4 times), just pass the event type as a parameter. 2. Similarly, just use event type strings instead of full-blown Event derivatives. 3. requestKeyFromDevices() is pretty much all about doing stuff in the Connection context, so move it there.
1. The rename is because it does so much more than just default key calculation. 2. Speaking of the default key, it's now only retrieved once, not 5 times. 3. The default key "event" type definition can be removed now that usage of it becomes one-time access of a single key.
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.
A couple more things discovered while messing with refactoring the code.
The method became rather large and with additional error checking would get even larger - so now the initial preparations concerned with default key and key description retrieval are put out to UnlockData::prepare() - a static factory method returning the loaded information that is used further in the process. That allows to put the piece concerned with making the key out of the passphrase to unlockSSSSWithPassphrase() (formerly not quite correctly named as unlockSSSSFromPassword). More byte_views are introduced, to ensure correct payload sizes as early (preferably at compile time) as possible.
Fixes building on macOS.
Also: fix failing to jump over the result-using code in case of failing to decrypt the session.
This now looks ready from my side, too. Sorry for taking plenty of time to polish the code, I trust it was worth it though. Let me know if you're good to go with this. |
Thanks to @tobias for spotting that.
Some security checks are still missing, also a bunch of refactoring and cleanup
Best enjoyed with https://invent.kde.org/network/neochat/-/merge_requests/1111