Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Implement EIP 1024 encryption methods #9893

Closed
topealabi opened this issue Nov 12, 2018 · 47 comments · May be fixed by #11284
Closed

Implement EIP 1024 encryption methods #9893

topealabi opened this issue Nov 12, 2018 · 47 comments · May be fixed by #11284
Labels
A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. F8-enhancement 🎊 An additional feature request. M5-binaries 📦 External binaries (ethkey, ethstore, ethvm, etc.) M6-rpcapi 📣 RPC API.
Milestone
2.7

Comments

@topealabi
Copy link

  • Parity Ethereum version: 2.1.4
  • Operating system: all
  • Installation: all
  • Fully synchronized: yes
  • Network: all
  • Restarted: yes

Current Behavior

Geth does not currently provide any encryption methods

Parity currently provides a data encryption method using elliptic curve secp256k1.

Wanted Behavior

A new encryption method that works across different key management softwares. For example a sender could encrypt data with their MetaMask wallet and the receiver could decrypt it with Parity. This new encryption method will use curve25519 as specified in EIP 1024.

EIP 1024 uses a well audited library called nacl. The rust implementation of the necessary nacl functions can be found here

##Definition of Done

You have successfully completed this bounty, when:

  1. A developer can request an accounts encryption public key
  2. A developer can encrypt any arbitrary data with using elliptical curve25519
  3. A developer can decrypt any arbitrary data encrypted with their accounts encryption public key
  4. These methods are compatible with MetaMasks encryption methods listed (here)[https://github.com/MetaMask/eth-sig-util]
@jam10o-new jam10o-new added F8-enhancement 🎊 An additional feature request. M5-binaries 📦 External binaries (ethkey, ethstore, ethvm, etc.) labels Nov 12, 2018
@jam10o-new jam10o-new added this to the 2.3 milestone Nov 12, 2018
@jam10o-new jam10o-new added the M6-rpcapi 📣 RPC API. label Nov 12, 2018
@tomusdrw
Copy link
Collaborator

What's the motivation behind using a different curve? I think currently it's really nice that security-wise, ethereum pretty much relies only on keccak & secp256k1. Using another curve requires additional (potentially unsafe) crypto libraries (or using bindings, which we would really want to avoid) and complicates the case with public keys (a need to communicate them somehow).

@topealabi
Copy link
Author

Hi @tomusdrw this suggestion was made by crypto-experts (I am not a crypto-expert) who suggest that using the same curve for signing and encryption increases the surface area of attack. The nacl library, which is the library suggested in the EIP, is one of the most well audited crypto libraries available. While there is no official rust version of nacl, I know of one developer who has already begun writing the necessary nacl functions in rust.

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 1000.0 DAI (1000.0 USD @ $1.0/DAI) attached to it.

@gitcoinbot
Copy link

gitcoinbot commented Nov 22, 2018 •
edited
Loading

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work has been started.

These users each claimed they can complete the work by 11 months ago.
Please review their action plans below:

1) shamardy has started work.

I would like to work on this issue.

In addition to the 3 required methods for requesting an accounts encryption public key, encrypting and decrypting data. I think there should be a method for the generation of curve25519 keypairs from the Ethereum private key.

Also which namespace do we use for these methods, do we use the eth_ namespace?
2) hskang9 has started work.

I have already implemented eip1024.
https://github.com/hskang9/eip1024-rs/blob/master/src/lib.rs

Learn more on the Gitcoin Issue Details page.

@tomusdrw
Copy link
Collaborator

@topealabi thanks for the details, I wonder however what's the reasoning behind having that implemented in the node itself - it's not part of the protocol, it's strictly something that is used on top.
We've already removed high-level languages compilation from the RPCs, we are planning on removing accounts altogether, I don't really know if such functionality fits (already bloated) nodes APIs.

For the record, I'd be really hesitant of using nacl-mini, the gh profile of the owner looks fishy.

@shamardy:

  1. Indeed generating keypairs would be nice addition, perhaps in parity_accouns namespace or personal_ (under a flag)
  2. eth_ namespace is fine, but please hide it under --experimental-rpcs (perhaps we should extend that in future to be more granular). Would be good to have the proposed methods names in the EIP as well.

@topealabi
Copy link
Author

Haha @tomusdrw I know the developer of nacl-mini in real life, he’s not shady he’s just shy and thinks GitHub is like a social network or something.

If you are still weary we can use a library like ring or rust-crypto for the encryption as long as we can decrypt using another key manger like Metamask. The whole point of this is to be cross-client. Other than that we are happy to take your guidance on where this should live in the Parity stack.

@topealabi
Copy link
Author

@christianlundkvist @danfinlay

@spm32
Copy link

spm32 commented Nov 25, 2018 •
edited
Loading

Hey @topealabi do you have a preference for who works on this? It looks like @shamardy and @zachzundel both commented pretty quickly.

@Tbaut
Copy link
Contributor

Tbaut commented Nov 25, 2018

Hey @ceresstation, you surely meant @zachzundel as @tomusdrw works at Parity and is overlooking this issue.

Please make sure we are carefully aligned on the task/tools before starting any work on this.

@spm32
Copy link

spm32 commented Nov 26, 2018

Yep, thanks for catching that @Tbaut, just updated, @topealabi @tomusdrw I'll leave it to you both to confirm a candidate :)

@spm32
Copy link

spm32 commented Nov 29, 2018

Hey @zachzundel looks like @topealabi accepted you, you're good to go :)

@3ach
Copy link

3ach commented Nov 29, 2018

Hi @topealabi , should I use nacl-mini or another library?

@topealabi
Copy link
Author

@zachzundel I think we should use ring, also the Parity folks have promised to provide guidance with this so perhaps we can create a slack channel on gitcoin for higher throughput communication

@Tbaut
Copy link
Contributor

Tbaut commented Nov 29, 2018

@tomusdrw is currently OOO, hence the non-responsiveness, please wait for his guidance before starting anything.

@topealabi
Copy link
Author

Thanks @Tbaut

@tomusdrw
Copy link
Collaborator

@zachzundel I'm fine with both TBH (main point is to avoid bringing any dynamically linked libs). The encryption part should be considered heavily experimental though, cause the code is not audited or tested in wild.

@kdenhartog
Copy link

kdenhartog commented Nov 30, 2018 •
edited
Loading

Hi @tomusdrw this suggestion was made by crypto-experts (I am not a crypto-expert) who suggest that using the same curve for signing and encryption increases the surface area of attack. The nacl library, which is the library suggested in the EIP, is one of the most well audited crypto libraries available. While there is no official rust version of nacl, I know of one developer who has already begun writing the necessary nacl functions in rust.

We've (Hyperledger IndySDK core dev team) been building similar functionality in rust to this based on sodium-oxide which wraps libsodium through FFI calls. Are you looking for a pure rust implementation of nacl? I haven't found one of these yet.

@gitcoinbot
Copy link

@zachzundel Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@3ach
Copy link

3ach commented Dec 5, 2018

Yes, sorry, I am still working on this. I will plan on having a WIP PR out this weekend.

@gitcoinbot
Copy link

@zachzundel Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

1 similar comment
@gitcoinbot
Copy link

@zachzundel Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

@zachzundel due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@tomaka tomaka mentioned this issue Dec 12, 2018
Closed
@3ach
Copy link

3ach commented Dec 12, 2018 via email

@topealabi
Copy link
Author

@zachzundel awesome, let us know how we can help

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

@zachzundel due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@5chdn 5chdn modified the milestones: 2.3, 2.4 Jan 10, 2019
@rmshea
Copy link

rmshea commented Jan 30, 2019

hey @zachzundel any updates here?

@3ach
Copy link

3ach commented Feb 11, 2019

I am going to relinquish this issue back into the Gitcoin pool. I don't have the time to sort it out.

@shamardy
Copy link
Contributor

shamardy commented Feb 12, 2019 •
edited
Loading

I would like to continue the work on this issue. I already applied to work on this from gitcoin here #9893 (comment).

@topealabi
Copy link
Author

Thanks for your efforts @zachzundel

@shamardy I will work with the Gitcoin team to get you on-boarded

@gitcoinbot
Copy link

@shamardy Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@spm32
Copy link

spm32 commented Feb 17, 2019

@shamardy @topealabi how are things going here?

@topealabi
Copy link
Author

@shamardy

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

@shamardy due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@shamardy
Copy link
Contributor

@ceresstation @topealabi Still working on this issue. Should have a PR ready for review in a week.

@5chdn 5chdn modified the milestones: 2.4, 2.5 Feb 21, 2019
@gitcoinbot
Copy link

@shamardy Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

@shamardy due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@soc1c soc1c modified the milestones: 2.5, 2.6 Apr 2, 2019
@ordian ordian modified the milestones: 2.6, 2.7 Jul 12, 2019
@hskang9
Copy link

hskang9 commented Nov 22, 2019

Hello all, I am currently implementing EIP-1024 here. So far the encryption and generating public key is implemented. Is this bounty still alive?

@hskang9
Copy link

hskang9 commented Nov 23, 2019

Ok now done with test.

@topealabi
Copy link
Author

Nice work @hskang9 , @tomusdrw can you take a look at this?

@hskang9 hskang9 mentioned this issue Nov 24, 2019
Open
@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work for 1000.0 DAI (1000.0 USD @ $1.0/DAI) has been submitted by:

  1. @hskang9

@ceresstation please take a look at the submitted work:

@dvdplm
Copy link
Collaborator

dvdplm commented Nov 25, 2019

@topealabi @ceresstation I am bit hesitant to accept this EIP in the repo; there's the matter of post-bounty maintenance of the submitted code ofc. Furthermore, the EIP itself is not on the roadmap for upcoming HFs.
It is obviously fine to submit a PR as part of the work to develop the EIP into something accepted by all client implementors, but before that happens I don't see this as something we will be able to merge.

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

The funding of 1000.0 DAI (1000.0 USD @ $1.0/DAI) attached to this issue has been approved & issued to @hskang9.

@adria0 adria0 added the A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jul 27, 2020
@adria0 adria0 closed this as completed Jul 27, 2020
@bumblefudge bumblefudge mentioned this issue Oct 23, 2022
Open
2 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. F8-enhancement 🎊 An additional feature request. M5-binaries 📦 External binaries (ethkey, ethstore, ethvm, etc.) M6-rpcapi 📣 RPC API.
Projects
None yet
Milestone
2.7
Development

Successfully merging a pull request may close this issue.

feat: Implement EIP-1024 hskang9/parity-ethereum
17 participants
@dvdplm @tomusdrw @3ach @topealabi @ordian @adria0 @rmshea @spm32 @hskang9 @5chdn @kdenhartog @gitcoinbot @Tbaut @shamardy @jam10o-new @soc1c and others