-
Notifications
You must be signed in to change notification settings - Fork 137
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
Slatepack - Pt 2 - Encryption #411
Conversation
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 stepped through the crypto line by line but wanted to generally comment after reviewing this in context of the points you raised in the RFC about splitting out the header
. If we can live with adding either a new recipients
field in the metadata or bringing back the header
when we want slatepack to support more complex multiparty builds, I agree that just keeping the age format is probably the most sane way to proceed from here.
Long term we may end up needing something more comprehensive but simple and easy is better here and we don't need complex support yet (at least until we are ready to support multisig natively in the wallet).
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 haven't had a chance to manually run the tests myself but here are my brief notes on the updates (and maybe some things I missed on the first one).
To quickly restate two features missing in armor.rs
for my own benefit:
- multipart messages to support edge case for large slates
- newline formatting support
For the Slatepack
struct I think we want sender
and recipients
to be ed25519 pub keys and not x25519 pub keys because we plan to use them to map to onion v3 addresses for routing primarily- when this fails we will map to an x25519 for fallback to encrypted ascii armor. (ed25519->x25519 is clean, x25519->ed25519 is complicated)
Unfortunately I don't think we can get away with just adding recipients
field to support future multiparty use- this will require some additional work. We will also need a way to MAC this data to prevent it from being undetectably MITM'd. This will require some review because it wouldn't be safe to use the same file key
used in our payloads age MAC to generate the MAC for our recipients
so we would need to do some further reasoning (this shows some the advantage of managing our own header outside of age- only tracking one MAC/message key for the entire message).
Otherwise this should be good to merge to get testing because it is isolated from other parts (nice design choice) and prevents us from having another chonker of a PR like compact slates :)
Edit: just found-
2019-06-12: added a nonce to the HKDF payload key derivation, making the file key reusable
So I guess we can reuse file key for the MAC after all which saves us a fair amount of trouble
👍 Will handle this as the sole focus of the next PR.
All fine, and this can be specced out later. Most of the time spent here was ensuring that optional fields can be added and that reading a binary slatepack with more fields than we recognize doesn't automatically choke. |
* recreate PR from mimblewimble#400 * first tests with slate encryption * simplify slatepack model to contain encryption header in payload, and add de/ser tests * update tests and confirm slatepack encryption working * remove recipient list, add version check warning
Integration slatepack encryption, changes are: