search

openethereum / parity-ethereum

The fast, light, and robust client for Ethereum-like networks.
Other
6.83k stars 1.69k forks source link

Implement EIP 1024 encryption methods #9893

Closed topealabi closed 4 years ago

topealabi commented 5 years ago

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]
tomusdrw commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@christianlundkvist @danfinlay

spm32 commented 5 years ago

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

Tbaut commented 5 years ago

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 commented 5 years ago

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

spm32 commented 5 years ago

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

3ach commented 5 years ago

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

topealabi commented 5 years ago

@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 commented 5 years ago

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

topealabi commented 5 years ago

Thanks @Tbaut

tomusdrw commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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!

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

3ach commented 5 years ago

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

gitcoinbot commented 5 years ago

@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!

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

gitcoinbot commented 5 years ago

@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!

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

gitcoinbot commented 5 years ago

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!

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

3ach commented 5 years ago

Sorry! I am still working on this!! If I haven’t made any progress in a couple days I will release it back to the Gitcoin pool

On Wed, Dec 12, 2018 at 10:16 Gitcoin.co Bot notifications@github.com wrote:

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

@zachzundel https://github.com/zachzundel due to inactivity, we have escalated this issue https://gitcoin.co/issue/paritytech/parity-ethereum/9893/1817 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 https://gitcoin.co/issue/paritytech/parity-ethereum/9893/1817?snooze=1 | 3 days https://gitcoin.co/issue/paritytech/parity-ethereum/9893/1817?snooze=3 | 5 days https://gitcoin.co/issue/paritytech/parity-ethereum/9893/1817?snooze=5 | 10 days https://gitcoin.co/issue/paritytech/parity-ethereum/9893/1817?snooze=10 | 100 days https://gitcoin.co/issue/paritytech/parity-ethereum/9893/1817?snooze=100

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/paritytech/parity-ethereum/issues/9893#issuecomment-446668187, or mute the thread https://github.com/notifications/unsubscribe-auth/AC8zTZbYxWBuuS8pmaRkWPyb3IztOa23ks5u4TnagaJpZM4YY4Q7 .

topealabi commented 5 years ago

@zachzundel awesome, let us know how we can help

gitcoinbot commented 5 years ago

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!

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

3ach commented 5 years ago

Pushed a WIP PR. I am still getting up to speed on Rust and the Parity codebase, so I probably made some mistakes and would appreciate a look over.

gitcoinbot commented 5 years ago

@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!

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

gitcoinbot commented 5 years ago

@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!

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

gitcoinbot commented 5 years ago

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!

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

gitcoinbot commented 5 years ago

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!

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

rmshea commented 5 years ago

hey @zachzundel any updates here?

3ach commented 5 years ago

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

shamardy commented 5 years ago

I would like to continue the work on this issue. I already applied to work on this from gitcoin here https://github.com/paritytech/parity-ethereum/issues/9893#issuecomment-440918613.

topealabi commented 5 years ago

Thanks for your efforts @zachzundel

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

gitcoinbot commented 5 years ago

@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!

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

spm32 commented 5 years ago

@shamardy @topealabi how are things going here?

topealabi commented 5 years ago

@shamardy

gitcoinbot commented 5 years ago

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!

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

shamardy commented 5 years ago

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

gitcoinbot commented 5 years ago

@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!

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

gitcoinbot commented 5 years ago

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!

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

hskang9 commented 4 years ago

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 commented 4 years ago

Ok now done with test.

topealabi commented 4 years ago

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

gitcoinbot commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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.