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

Added ECIES encryption #13832

Closed
wants to merge 6 commits into from
Closed

Added ECIES encryption #13832

wants to merge 6 commits into from

Conversation

arkpar
Copy link
Member

@arkpar arkpar commented Apr 5, 2023

This implements simple encryption scheme that uses x25519 key exchange and AEAD. Required for https://github.com/paritytech/labs/issues/3 and #13893

@arkpar arkpar mentioned this pull request Apr 12, 2023
3 tasks
@stale
Copy link

stale bot commented May 5, 2023

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A3-stale label May 5, 2023
@arkpar arkpar removed the A3-stale label May 5, 2023
@arkpar arkpar requested review from bkchr and cheme May 8, 2023 10:53
@arkpar
Copy link
Member Author

arkpar commented May 8, 2023

@burdges would appreciate your review as well

@arkpar arkpar added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels May 8, 2023
@arkpar arkpar marked this pull request as ready for review May 8, 2023 10:57
@burdges
Copy link

burdges commented May 8, 2023

I cannot see https://github.com/paritytech/labs/issues/3 btw but afaik a conventional ECIES mode would not be suitable for mixnets if that's what this references. It'd employ the same pieces of course, but they're glued together slightly differently.

If you want encrypted information stored in the chain then ECIES works fine, but if nodes hold the keys then I guess you want the secrecy to be only temporary?

@arkpar
Copy link
Member Author

arkpar commented May 8, 2023

@burges It is intended to work offchain. See https://github.com/paritytech/substrate/tree/master/primitives/statement-store#readme, which is basically same description as the labs issue.

Copy link
Contributor

@cheme cheme left a comment

Choose a reason for hiding this comment

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

looks good except I got a serious interrogation (from my understanding of eceis only): I would not use a random nonce but kdf derived data and use it in aad.

@@ -114,6 +121,13 @@ std = [
"futures/thread-pool",
"libsecp256k1/std",
"dyn-clonable",

"ed25519-dalek",
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit ~ with this ed25519 story, but guess here we needed ed25519-dalek (in addition to already included zebra) to be able to use x25 dalek ? (remember something like this on mixnet).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, ed25519-zebra provide a limitied API for signinig/verification only. We need ECDH exchange here.

fn aes_encrypt(key: &[u8; AES_KEY_LEN], nonce: &[u8], plaintext: &[u8]) -> Result<Vec<u8>, Error> {
let enc = aes_gcm::Aes256Gcm::new(key.into());

enc.encrypt(nonce.into(), aes_gcm::aead::Payload { msg: plaintext, aad: b"" })
Copy link
Contributor

Choose a reason for hiding this comment

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

should we use aad to some additional kdf content?

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I understand aad is not that useful as long as we use a new unique ephemeral key for each encryption.

primitives/core/src/ecies.rs Show resolved Hide resolved
let ephemeral_pk = x25519_dalek::PublicKey::from(&ephemeral_sk);

let mut shared_secret = ephemeral_sk.diffie_hellman(pk).to_bytes().to_vec();
shared_secret.extend_from_slice(ephemeral_pk.as_bytes());
Copy link
Contributor

Choose a reason for hiding this comment

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

could be ephemeral_pk then shared_secret. I don t think it matter (and found it describe in the order you use).

primitives/core/src/ecies.rs Show resolved Hide resolved
@arkpar
Copy link
Member Author

arkpar commented Jun 17, 2023

@burdges Could you please take a look at @cheme's comments. Does it make sense to specify aad here?

@burdges
Copy link

burdges commented Jun 17, 2023

I'm confused why substrate needs a non-transport encryption. Do you want a host call for this?

If you do not want a host call, then folks should use the AEAD crates directly, not this interface. AeadInPlace maybe preferable sometimes for example.

If you do want a host call, then you should make them future proof. Imho, this means:

  1. symmetric and asymmetric host calls should be separate, not integrated, i.e. no ECIES.
  2. symmetric should be in-place methods, which likely breaks substrate, but we know this area of substrate needs fixing.

I'm dubious about the use cases list in https://github.com/paritytech/substrate/tree/master/primitives/statement-store#readme too. I doubt ECIES fits all those, like maybe not revealing the secret key matters somewhere? As for your question, associated data likely becomes important somewhere.

Copy link
Contributor

@cheme cheme left a comment

Choose a reason for hiding this comment

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

I misunderstood the use of aad, looks good this way.

@arkpar
Copy link
Member Author

arkpar commented Jun 20, 2023

I'm confused why substrate needs a non-transport encryption. Do you want a host call for this?

It is intended to be used in statement-store, so not a direct host call.

If you do not want a host call, then folks should use the AEAD crates directly, not this interface. AeadInPlace maybe preferable sometimes for example.

Makes senes. I'm going to remove it from primitives and move it under statement-store

@arkpar arkpar closed this Jun 20, 2023
@burdges
Copy link

burdges commented Jun 20, 2023

We should hammer down more concretely how one uses the statement store in those applications imho, like how would state channels use this?

Also, some questions:

  1. Does polkadot itself have any uses for the statement store? Or it's purely for parachains?
  2. If a parachain uses the statement store, then how does the parachain benefit from using this statement store code, instead of rolling their own.
  3. If the statement store aims for wallet integration then what makes sense for projects who do not fit the statement store?

I suppose the "fee payment by another party" use makes more sense, in that you provide the transaction that pays the fee, and transfer the secret key inside the statement store. In principle, soft key derivation works as here too, and seems simpler, but not all schemes support soft key derivation. Soft key derivation breaks BLS for example, but really accounts should never use BLS anyways. Btw, Ed25519 has soft key derivation if you do things Tor's way, not Cardano's way.

I'll give some concerning examples:

I'm dubious "ring signature aggregation" makes sense. In say a zkAMM or MimbleWimble, you'd send ring-like signatures on some Pedersen commitments directly to a collator, but also reveal the opening, so then the collator makes a block which hides your opening, but produces their own zk proof using the opening, like they create the UTXO or aggregate transactions.

An account provides encrypted metadata for its future self, like say an opening of a pedersen commitment in ome zk chain. We could store this data on-chain, but doing so maybe larger. We could store this off-chain, but then we could lose user data more easily than the whole chain dying. In between, we'd want some erasure coded statement store, but likely with the signatures paying some on-chain fee, and reconstruction when nodes disappear.

A sender provides a signed transaction with an encrypted component, which contains a transaction they claim satisfies some properties. There are penalties if these claims are false, so decryption must happen on-chain. Also, a receiver should not provide their secret key, only the key exchange result. If the AEAD's MAC fails, the chain cannot tell if the the sender lied about the encryption, or if the receiver lied about the key exchange. Instead, the receiver should supply a DLEQ proof of doing the key exchange correctly, which works exactly like the DLEQ proof in sr25519 or VEd25519. Btw, you need slightly more if you want this scheme to be composable with VRFs.

An MEV defense in w3f/Grants-Program#1660 and elsewhere has parachains insert encrypted tx into a block, and then decrypt the transaction in the future. Aside from inner and outer signature, their scheme cannot fit the statement store:

  • In aura, their key exchange should be IBE. In sassafras, they could use ed25519/x25519 but..
  • They need a threshold key encapsulation of several key exchanges in both aura and sassafras.
  • An AEAD must be run on-chain. It's likely this is fast enough, but maybe they'll request a host call.

Anyways, we should understand the actual use cases for the statement store, but avoid making exaggerated claims.

@arkpar arkpar deleted the arkpar/ecies branch June 21, 2023 07:10
@arkpar
Copy link
Member Author

arkpar commented Jun 21, 2023

We should hammer down more concretely how one uses the statement store in those applications imho, like how would state channels use this?

This one is for @gavofyork, as he wrote the original spec. There's going to be a statement store PSP where we can discuss this further.

Does polkadot itself have any uses for the statement store? Or it's purely for parachains?

This is for parachains and other substrate-based chains as far as I understand.

If a parachain uses the statement store, then how does the parachain benefit from using this statement store code, instead of rolling their own.

Statement store provides things like the network protocol, database storage with indexing. Implementing all of that on top of substrate is not a trivial task. We also provide a standard runtime event for submitting a statement. There's going to be an integration with the mixnet for network level anonymity.

If the statement store aims for wallet integration then what makes sense for projects who do not fit the statement store?

Wallet integration will probably still need some customization for each use case anyway. The statement store itself is designed to be generic enough. You can put anything into the data field, including, for example, a custom signature scheme or encryption.

@burdges
Copy link

burdges commented Jun 21, 2023

Alright cool, so this is really more about message transport, and these messages arriving at where they should go, which maybe everywhere.

As an aside, I really know little about it but https://openprivacy.ca/work/cwtch/ uses https://docs.rs/fuzzytags/0.6.0/fuzzytags/ and you might find that interesting. It's not really privacy preserving, but it's one way to keep messages form going to every node. It might find your real applications here better than it fits cwtch.. or maybe cwtch itself could just be used somehow. It might otoh not work very well since if you've like 10 accounts when you only want to talk to one RPC node, and if cwtch is sending all those accounts messages to different RPC nodes because they have different keys then that sucks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
Development

Successfully merging this pull request may close these issues.

3 participants