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

Encryption support for the statement store #14440

Merged
merged 8 commits into from
Jul 17, 2023
Merged

Conversation

arkpar
Copy link
Member

@arkpar arkpar commented Jun 22, 2023

This implements simple encryption scheme that uses ed25519 key exchange and AEAD for the statement store. See https://github.com/paritytech/labs/issues/3 and #13893

cumulus companion: paritytech/cumulus#2876

@arkpar arkpar requested review from cheme and bkchr June 22, 2023 08:53
@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. labels Jun 22, 2023
@arkpar arkpar changed the title Added ECIES encryption Encryption support for the statement store Jun 22, 2023
@arkpar arkpar added the D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. label Jun 22, 2023
key_type: KeyTypeId,
public: &ed25519::Public,
f: &mut dyn FnMut(&ed25519::Pair),
) -> std::result::Result<bool, TraitError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember doing a similar function in a branch.

One possible issue is that this is maybe not appropriate for a future hardware keystore, so a specialized function may be more suitable, but at this point I don't know if it is worth doing (given this would mean putting eceis in keystore and I think it was a point that was not suitable from previous PR).

Since it is explicitely a Statement store key that would not be possible to run on HSM, it may not be such an issue.

HexDisplay::from(&statement.hash())
),
},
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can have with_ed25519_key use a function that return an error as param, and potentially have posted clear report this in its list for results.

@@ -196,6 +196,21 @@ impl Keystore for LocalKeystore {
self.sign::<ed25519::Pair>(key_type, public, msg)
}

fn with_ed25519_key(
Copy link
Member

Choose a reason for hiding this comment

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

We already provide key_pair in the local key store. You can directly use this?

@@ -233,6 +233,7 @@ pub fn new_partial(
&config.data_path,
Default::default(),
client.clone(),
keystore_container.keystore(),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
keystore_container.keystore(),
keystore_container.local_keystore(),

@arkpar arkpar requested a review from athei as a code owner July 3, 2023 12:38
@arkpar arkpar requested review from a team July 3, 2023 12:38
@arkpar arkpar requested a review from koute as a code owner July 3, 2023 12:38
@arkpar
Copy link
Member Author

arkpar commented Jul 3, 2023

@bkchr Changed to use local keystore.
There are some additional changes introduced with cargo +nightly fmt. They seem harmless.

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

No idea about the crypto, but looks good.

let hkdf = hkdf::Hkdf::<sha2::Sha256>::new(None, shared_secret);
let mut aes_key = [0u8; AES_KEY_LEN];
hkdf.expand(b"", &mut aes_key)
.expect("There's always enough data for derivation.");
Copy link
Member

Choose a reason for hiding this comment

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

qed? :P

@arkpar
Copy link
Member Author

arkpar commented Jul 17, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 8463ca3 into master Jul 17, 2023
4 checks passed
@paritytech-processbot paritytech-processbot bot deleted the arkpar/ecies branch July 17, 2023 18:41
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* Added ECIES encryption

* tweaks

* fmt

* Make clippy happy

* Use local keystore

* qed
Ank4n pushed a commit that referenced this pull request Jul 22, 2023
* Added ECIES encryption

* tweaks

* fmt

* Make clippy happy

* Use local keystore

* qed
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
None yet
Development

Successfully merging this pull request may close these issues.

3 participants