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

Add EIP-5630: Encryption and Decryption #5630

Merged
merged 23 commits into from
Sep 16, 2022
Merged

Add EIP-5630: Encryption and Decryption #5630

merged 23 commits into from
Sep 16, 2022

Conversation

firnprotocol
Copy link
Contributor

@firnprotocol firnprotocol commented Sep 8, 2022

see eip-5630.md.

@github-actions github-actions bot added the s-draft This EIP is a Draft label Sep 8, 2022
@eth-bot
Copy link
Collaborator

eth-bot commented Sep 8, 2022

A critical exception has occurred:
Message: pr 5630 is already merged; quitting
(cc @alita-moore, @mryalamanchi)

@github-actions github-actions bot added the t-erc label Sep 8, 2022
@github-actions github-actions bot added the c-new Creates a brand new proposal label Sep 8, 2022
@firnprotocol
Copy link
Contributor Author

@firnprotocol firnprotocol dismissed a stale review via 50c8b0c September 8, 2022 23:28
@TJKoury
Copy link

TJKoury commented Sep 9, 2022

Concur with the approach. Using a standardized KDF to deterministically create entropy is a portable solution that is easy to implement / maintain.

@abcoathup
Copy link
Contributor

EIP numbers are assigned by EIP editors and are generally the PR number.
https://eips.ethereum.org/EIPS/eip-1#eip-editor-responsibilities

Copy link
Contributor

@kdenhartog kdenhartog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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',
Copy link
Contributor

@kdenhartog kdenhartog Sep 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 firnprotocol changed the title Add EIP-5605: Encryption and Decryption Add EIP-5630: Encryption and Decryption Sep 9, 2022
@firnprotocol
Copy link
Contributor Author

@abcoathup

EIP numbers are assigned by EIP editors and are generally the PR number.
https://eips.ethereum.org/EIPS/eip-1#eip-editor-responsibilities

got it; many thanks for the heads up; renamed.

@firnprotocol
Copy link
Contributor Author

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 @@
---
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SamWilsn why is EIPW passing on this EIP? It should be throwing at least 5 errors (from what I've counted so far).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean, the Walidator? I had to fix a bunch, but I think i got them all?

Copy link
Member

@Pandapip1 Pandapip1 Sep 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

k; please do; happy to address them.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Pandapip1 @SamWilsn, any updates here? Thanks

@firnprotocol
Copy link
Contributor Author

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.

@Pandapip1
Copy link
Member

@firnprotocol please review my changes.

@firnprotocol
Copy link
Contributor Author

@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.)

@firnprotocol firnprotocol dismissed a stale review via 5fb65cf September 15, 2022 15:07
@firnprotocol
Copy link
Contributor Author

@Pandapip1 shoot, apologies, didn't see the review before pushing. my fault.

@firnprotocol firnprotocol requested review from Pandapip1 and removed request for 4441114417342316 September 15, 2022 15:58
Copy link
Member

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The language is a bit wonky, but this can be fixed while the EIP is in draft.

@eth-bot eth-bot enabled auto-merge (squash) September 16, 2022 15:55
@eth-bot eth-bot merged commit 543d386 into ethereum:master Sep 16, 2022
@firnprotocol
Copy link
Contributor Author

@Pandapip1 you the man! thanks.

@kigawas
Copy link

kigawas commented Nov 6, 2022

IMHO, HKDF-sha512 is perhaps overkill. HKDF-sha256 is balanced on speed and security.

@firnprotocol
Copy link
Contributor Author

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.

nachomazzara pushed a commit to nachomazzara/EIPs that referenced this pull request Jan 13, 2023
* 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 <45835846+Pandapip1@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-new Creates a brand new proposal s-draft This EIP is a Draft t-erc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants