feat: add encryption capabilities #102
Conversation
mirceanis
force-pushed
the
101-add-encryption
branch
from
b9970fb to
b4e09ae
Compare
mirceanis
force-pushed
the
101-add-encryption
branch
from
b4e09ae to
fca207d
Compare
| * If we decide to add additional dependencies, this class along with the `nacl` utility could be replaced with `eth-sig-util`. | ||
| * @see https://github.com/ethereum/EIPs/pull/1098 | ||
| * @see https://github.com/MetaMask/eth-sig-util/blob/main/src/encryption.ts | ||
| * @see https://nacl.cr.yp.to/box.html |
There was a problem hiding this comment.
I think the biggest concern of copying or adding a new library is that it requires another round of vetting/auditing.
So if we do add this now, it means that the package is frozen until we get it audited.
Lets discuss this. I'm thinking we can go to the snaps team to figure out how we can get this audited.
- Potentially keep this as a separate branch, so it keeps the main branch unblocked in case we need to to other changes.
There was a problem hiding this comment.
The alternative is to use eth-sig-util directly.
It will make this PR significantly smaller, but also make the snap bigger and a bit harder to update it in the extension/mobile, as we'll probably have to rebuild the lavamoat policies.
There was a problem hiding this comment.
True...
However funny thing is that we already have @metamask/eth-sig-util installed as a peer dependency (see yarn.lock) 😄
There was a problem hiding this comment.
And wow, since this is already used as a peer dependency... I think auditing won't be too much of a problem?
However you may need to confirm this.
There was a problem hiding this comment.
If that's the case, I'm happy to use that directly instead of reinventing the wheel :)
The only reason I did it this way was to avoid inflating the preinstalled snap bundle and lavamoat approval friction
There was a problem hiding this comment.
actually, it seems eth-sig-util is designed for node so I get warnings when using it in a snap :(
mirceanis
force-pushed
the
101-add-encryption
branch
from
b3678b7 to
949b10d
Compare
src/entropy-keys.ts
Outdated
| */ | ||
| async function getEncryptionSecretKey(): Promise<Uint8Array> { | ||
| const privateEntropy = await getPrivateEntropyKey(); | ||
| return sha256(concatBytes([privateEntropy, utf8ToBytes(staticSalt)])); |
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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:
- SRP -> snap entropy (I don't know where to find this info yet, but I suspect this is not standard BIP44)
- snap entropy -> encryption secret (
sha256(entropy | 'metamask:snaps:encryption'))
There was a problem hiding this comment.
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 with staticSalt as salt, as that generates a completely different key too.
There was a problem hiding this comment.
SRP -> snap entropy (I don't know where to find this info yet)
There was a problem hiding this comment.
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_getEntropywithstaticSaltas salt, as that generates a completely different key too.
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.
I updated the call to snap_getEntropy to achieve proper separation of concerns.
Thanks again for spotting this @Mrtenz !
This enforces total key independence between signing and encryption
|
@metamaskbot publish-preview |
What's new
This preinstalled snap allows an instance of MetaMask to sign messages using an SRP derived key.
We really need an SRP bound encryption system too, to be able to pair devices properly.
This PR introduces an ERC1024 compatible encryption system, to be compatible1 with the existing
eth_getEncryptionPublicKey/eth_decryptimplementation in MetaMask.The encryption secret key is derived from the SRP-bound entropy using a static salt, to avoid key reuse.
The encryption layer is implemented using the existing
@noble/ciphersdependency, since that was already in use.The tests for the encryption layer represent the vast majority of additions to this PR.
But, theoretically, the ERC1024 implementation can be replaced with
@metamask/eth-sig-util, however, that library is designed for node and throws some warnings when used in a snap.Fixes #101
Footnotes
The compatibility is on the data format, not on the actual public key, so messages encrypted for the snap will not be decryptable by any of the MM accounts, nor vice-versa. ↩