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
feat: add encryption capabilities #102
base: main
Are you sure you want to change the base?
feat: add encryption capabilities #102
Changes from 5 commits
fca207d
949b10d
03ff7a8
4060d7d
be45046
cb05551
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.
Does EIP-1024 describe that the entropy should be concatenated with the salt and hashed? Otherwise you could use the salt property of
snap_getEntropy
instead.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.
ERC1024 doesn't specify how the encryption key-pairs are generated from a given entropy, but rather which curve should be used (x25519), the message formats and algorithms.
I deliberately added a salt here to avoid key reuse.
The raw entropy is being used as a private key for signing but since the encryption key will be used for ECDH it's best to avoid reuse.
ethereum/EIPs#1098
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.
Since the entropy provided to the snap is not easily replicable to some other context, I think it's less important here to use it directly as a secret key and more important to adhere to best practices, like avoiding key reuse.
If we ever needed to document the whole key generation process for this we would include these 2 steps:
sha256(entropy | 'metamask:snaps:encryption')
)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.
Sorry, maybe I'm not understanding it properly, but how does it avoid key-reuse when using a static salt? It would be the same as calling
snap_getEntropy
withstaticSalt
as salt, as that generates a completely different key too.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.
SIP-6: https://metamask.github.io/SIPs/SIPS/sip-6
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.
What I meant was that from the same entropy we need to derive 2 secret keys. One for encryption and one for signing. The point of avoiding key reuse is so that by some means the encryption key gets compromised, you don't automatically leak the signing key. This should be vice-versa, but sadly we are already using the unsalted entropy directly as the signing key.
You are, of course, right when stating that we can provide the salt directly to the
snap_getEntropy
call. I wasn't aware it accepts a salt parameter, so thank you for insisting on 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.
I updated the call to snap_getEntropy to achieve proper separation of concerns.
Thanks again for spotting this @Mrtenz !