Skip to content
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

Merged
merged 5 commits into from
May 22, 2020

Conversation

yeastplume
Copy link
Member

@yeastplume yeastplume commented May 19, 2020

Integration slatepack encryption, changes are:

  • Creates a public key by retrieving it from the tor derivation path at index 0 and converting to an x25519 keypair
  • Enc/decryption of the slate payload using said keypairs via the age library
  • Required age headers are embedded in the payload rather than split out into the slatepack struct, as discussed in the RFC
  • An optional number of recipient entries can be included as a slatepack field
  • Tests to ensure future slatepack versions can still be read.

Copy link
Member

@j01tz j01tz left a 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).

@yeastplume yeastplume changed the title [WIP] Slatepack - Pt 2 - Encryption Slatepack - Pt 2 - Encryption May 21, 2020
Copy link
Member

@j01tz j01tz left a 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

@yeastplume
Copy link
Member Author

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)

👍 Will handle this as the sole focus of the next PR.

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

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.

@yeastplume yeastplume merged commit 2769436 into mimblewimble:master May 22, 2020
@yeastplume yeastplume deleted the slatepack_pt2 branch July 13, 2020 10:20
antiochp pushed a commit to antiochp/grin-wallet that referenced this pull request Aug 7, 2020
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants