Add EIP-5630: Encryption and Decryption #5630
Conversation
|
A critical exception has occurred: |
|
relevant PRs (for linking purposes):
@TJKoury @kdenhartog @awoie @rdubois-crypto feedback is most welcome 😄 |
trying again
can't figure out how to satisfy HTML proofer
|
Concur with the approach. Using a standardized KDF to deterministically create entropy is a portable solution that is easy to implement / maintain. |
|
EIP numbers are assigned by EIP editors and are generally the PR number. |
There was a problem hiding this comment.
Thank you for taking the time to write this up. From a security perspective the cryptography design seems fine. However, I think I've got some fundamental differences in how to approach this problem compared to the design of this approach. I'd likely be a -1 to the design decisions of this EIP at this point.
EIPS/eip-5605.md
Outdated
| - the HMAC `HMAC–SHA-256–256 with 32 octet or 256 bit keys` (see [SEC 1, § 3.7]), | ||
| - the symmetric encryption scheme `AES–256 in CBC mode` (see [SEC 1, § 3.8]). | ||
|
|
||
| We finally describe a method to derive encryption secret keys deterministically—but pseudorandomly—from signing keys, in such a way that a natural one-to-one relationship obtains between these keys (this latter property is essential, since it allows Ethereum accounts to be used as handles onto encryption/decryption keys, as both the former and current API interfaces do). |
There was a problem hiding this comment.
This turns the Ethereum account into a global identifier in a way that makes it much harder to establish a pairwise identifier relationship. I'd rather see these detached personally since there's not a necessity to achieve the goals of encrypting the messages.
EIPS/eip-5605.md
Outdated
| Thus, on the request: | ||
| ```javascript | ||
| request({ | ||
| method: 'eth_getEncryptionPublicKey', |
There was a problem hiding this comment.
It's not going to be a good idea to overlap the method names here. Furthermore, I'd suggest the namespace used here is wallet_* rather than eth_* as this is more likely to be used by wallets than nodes.
EIPS/eip-5605.md
Outdated
| On the request | ||
| ```javascript | ||
| request({ | ||
| method: 'eth_decrypt', |
There was a problem hiding this comment.
Personally, I don't think this is a great API design any longer. The original purpose of this API design seemed motivated by the need for P2P messaging. However, the more likely case in which this is used is to encrypt data passed by dApps to wallets. I'd rather see the design move more towards an approach of a storage call here as this design only allows one way communication from the dApp to the client. The client is then forced to rely on TLS for any sort of confidentiality guarantees in the response because there's no way for them to use the message layer to encrypt. In other words TLS is already having to do half the work here if this is meant to be a synchronous session based messaging protocol. Given TLS is pervasively deployed across dApps and provides stronger cryptographic guarantees then this design I think it's more useful for us to focus on storage based encryption for dApps (e.g. data at rest) rather than session based encryption (e.g. data in transit) which would be more useful for a P2P communication system.
firnprotocol
changed the title
got it; many thanks for the heads up; renamed. |
|
friendly bump to @lightclient, @axic, @SamWilsn, @Pandapip1; could you please advise if further action is needed on my end to get this validated? many thanks in advance. |
| @@ -0,0 +1,161 @@ | |||
| --- | |||
There was a problem hiding this comment.
@SamWilsn why is EIPW passing on this EIP? It should be throwing at least 5 errors (from what I've counted so far).
There was a problem hiding this comment.
you mean, the Walidator? I had to fix a bunch, but I think i got them all?
There was a problem hiding this comment.
No, there are some issues with this draft that EIP Walidator isn't catching but that need fixing. I don't know why EIPW isn't erroring. CC @SamWilsn
(I'm going to try to point to things that need changing in the meantime).
There was a problem hiding this comment.
k; please do; happy to address them.
There was a problem hiding this comment.
Hi @Pandapip1 @SamWilsn, any updates here? Thanks
|
hi @lightclient, @axic, @SamWilsn, @Pandapip1—could y'all please help me help you? you have cited an (undisclosed) issue, but are not yet giving us the material to edit things to your satisfaction. i will gladly make the edits if you just please tell me what they are. thank you very much in advance. |
|
@firnprotocol please review my changes. |
|
@Pandapip1 many thanks for this. i have looked them over, and approve. looks good with me; feel free to merge. (by the way, I am not 100% sure of the rationale for the changes, i.e., of removing references, but I am ok with them.) |
|
@Pandapip1 shoot, apologies, didn't see the review before pushing. my fault. |
firnprotocol
requested review from
Pandapip1
and removed request for
4441114417342316
|
@Pandapip1 you the man! thanks. |
|
IMHO, HKDF-sha512 is perhaps overkill. HKDF-sha256 is balanced on speed and security. |
|
i actually completely agree; i only did this because it's easier on many implementations: you only need to hash once to implement the HKDF to get 2 keys, for AES and MAC, instead of hashing twice. many implementations actually don't implement a proper HKDF; they only (incorrectly) implement a hash. but open to changing this. btw, the discussion is proceeding here; you should join that. |
* first commit for encryption * add category * add EIP number * try to remove all (non-relative) links * more attempts to comply with scan * more adjustments. * make links verbatim; try to satisfy HTML Proofer * create ethereum magicians page * remove offending links * another attempt at HTML proofer * some kind of bug with HTML proofer trying again * minor stuff * just remove the two references can't figure out how to satisfy HTML proofer * rename file to match PR number * also adjust table * Update eip-5630.md * remove spurious parenthesis * grammar * a few more grammar changes post-edits Co-authored-by: Pandapip1 <[email protected]>
see
eip-5630.md.