ACP/ThresholdMessageKit Work #74

derekpierre · 2023-08-17T14:04:55Z

Type of PR:

Bugfix
Feature
Documentation
Other

Required reviews:

1
2
3

What this does:

High-level idea of the changes introduced in this PR.
List relevant API changes (if any), as well as related PRs and issues.

Issues fixed/closed:
Depends on nucypher/ferveo#156
Related to nucypher/nucypher#3194

Why it's needed:

Explain how this PR fits in the greater context of the NuCypher Network.
E.g., if this PR address a nucypher/productdev issue, let reviewers know!

Notes for reviewers:

What should reviewers focus on?
Is there a particular commit/function/section of your PR that requires more attention from reviewers?

…ldDecryptionRequest. Requires changes to DkgPublicKey in ferveo.

codecov-commenter · 2023-08-17T14:08:43Z

Codecov Report

Merging #74 (ef2568d) into main (d1a9cad) will increase coverage by 2.16%.
The diff coverage is 39.75%.

@@            Coverage Diff             @@
##             main      #74      +/-   ##
==========================================
+ Coverage   21.22%   23.38%   +2.16%     
==========================================
  Files          16       18       +2     
  Lines        3176     3540     +364     
==========================================
+ Hits          674      828     +154     
- Misses       2502     2712     +210

Files Changed	Coverage Δ
nucypher-core-python/src/lib.rs	`0.00% <0.00%> (ø)`
nucypher-core-wasm/src/lib.rs	`0.00% <0.00%> (ø)`
nucypher-core/src/conditions.rs	`53.84% <ø> (ø)`
nucypher-core/src/threshold_message_kit.rs	`80.70% <80.70%> (ø)`
nucypher-core/src/access_control.rs	`87.06% <87.06%> (ø)`
nucypher-core/src/dkg.rs	`89.80% <100.00%> (+0.14%)`	⬆️

…espectively.

…olicy to "acp"; also rename accessor method.

…thon module.

fjarri · 2023-08-18T22:45:18Z

nucypher-core/src/versioning.rs

+    ///
+    /// NOTE: Underlying struct members for types that must remain backwards compatible should
+    /// also remain backwards compatible as well.
+    fn must_remain_backwards_compatible() -> (bool, u16) {


I understand that is the solution for smooth switching from development to the production version?

Not sure how I feel about it. Seems kind of brittle. Maybe just a different type would be the way to go. You'd have to do some additional work when switching to production, but that would solidify the commitment, so to speak. The new type may still save/check the last development major version in the serialized form, to allow the usage of objects created during development, but I'm not sure how much the convenience here outweighs the safety. Perhaps it is better to force the devs to start the production objects from a clean slate.

Let's muse more about this in #75.

fjarri · 2023-08-18T22:46:21Z

I think it might be better to extract the permanent-support objects into a separate PR, because it's pretty important and shouldn't be mixed up with the rest of the stuff that's going on here.

…ing use/exposure of key encapsulation i.e. use of CiphertextHader, and rework of Ciphertext.

derekpierre · 2023-08-20T00:48:12Z

I think it might be better to extract the permanent-support objects into a separate PR, because it's pretty important and shouldn't be mixed up with the rest of the stuff that's going on here.

Removed the functionality from this PR and moved to #75.

…ta object which works around to chicken and egg issue for obtaining the aad to use when encrypting data, but which is needed for the AccessControlPolicy after the ciphertext is signed. Update bindings/tests.

… can be returned from the same call. Added python/wasm bindings.

…xt object purely to get the CiphertextHeader. Add decrypt_with_shared_secret to ThresholdMessageKit so that to decrypt data, the ciphertext does not need to be returned to python/js from Rust and then passed back in to Rust to decrypt. Keep it in Rust and just do the decryption internally.

…ignature#trailing-optional-arguments.

piotr-roslaniec · 2023-08-23T09:56:33Z

nucypher-core-python/nucypher_core/__init__.pyi

+@final
+class AuthenticatedData:
+
+    def __init__(self, public_key: DkgPublicKey, conditions: Optional[Conditions]):


@derekpierre I just recalled your comment on Optional<&Condition> in DKG structures. Are conditions supposed to be optional here? Or is it just downstream of that optionality in the DKG structures?

It is downstream.

This is something that came to me last night, but it pervades many types in nucypher-core (ThresholdDecryptionRequest, ... etc.) and I didn't have time to fix it across all of them. I think we should fix it in a separate PR. Basically for DKG types Conditions should not be Optional but should be specified. Hence I filed #76 .

nucypher-core-wasm/src/lib.rs

piotr-roslaniec · 2023-08-23T10:24:10Z

nucypher-core-wasm/src/lib.rs

+//
+#[wasm_bindgen]
+#[derive(PartialEq, Eq, Debug, derive_more::From, derive_more::AsRef)]
+pub struct ThresholdMessageKit(nucypher_core::ThresholdMessageKit);


I've noticed that in some places in nucypher-core, we're using { backend: InnerType } and (InnerType) in others. Perhaps we could settle on one way to define structs. It's not relevant to this PR, just a general comment on style

nucypher-core-wasm/src/lib.rs

piotr-roslaniec · 2023-08-23T10:27:24Z

nucypher-core-wasm/tests/wasm.rs

@@ -699,14 +699,23 @@ fn threshold_decryption_request() {
    let context: JsValue = Some(Context::new("{'user': 'context'}")).into();

    let dkg_pk = DkgPublicKey::random();
+
+    let auth_data =
+        AuthenticatedData::new(&dkg_pk, &conditions_js.unchecked_into::<OptionConditions>())


Using unchecked_into is type-unsafe, but it's fine here because it's a test

piotr-roslaniec · 2023-08-23T10:31:36Z

nucypher-core/src/access_control.rs

+    pub fn aad(&self) -> Box<[u8]> {
+        let public_key_bytes = self.public_key.to_bytes().unwrap();
+        let condition_bytes = self.conditions.as_ref().unwrap().as_ref().as_bytes();
+        let mut result = Vec::with_capacity(public_key_bytes.len() + condition_bytes.len());
+        result.extend(public_key_bytes);
+        result.extend(condition_bytes);
+        result.into_boxed_slice()
+    }


I'm tempted to suggest this functional approach

Suggested change

pub fn aad(&self) -> Box<[u8]> {

let public_key_bytes = self.public_key.to_bytes().unwrap();

let condition_bytes = self.conditions.as_ref().unwrap().as_ref().as_bytes();

let mut result = Vec::with_capacity(public_key_bytes.len() + condition_bytes.len());

result.extend(public_key_bytes);

result.extend(condition_bytes);

result.into_boxed_slice()

}

pub fn aad(&self) -> Result<Box<[u8]>, Error> {

Ok([

self.public_key.to_bytes()?,

self.conditions.as_ref().map(|c| c.to_bytes()).unwrap_or_else(|| Ok(vec![]))?,

]

.concat()

.into_boxed_slice())

}

Is there a good reason to unwrap() (and hence, panic) here? I take that we should always expect conditions to be present here (per discussion on optionality in DKG).

@piotr-roslaniec Can you take care of this change for me - either as part of this PR or a follow-up 🙏 ?

Sure, we can address any non-critical changes in a follow-up PR. I can take care of that once I test this PR with nucypher-ts.

nucypher-core/src/access_control.rs

piotr-roslaniec · 2023-08-23T10:43:05Z

nucypher-core/src/access_control.rs

+    }
+}
+
+impl Eq for AuthenticatedData {}


Can we #[derive(Eq)] on AuthenticatedData instead? I assume there is a reason for deriving Eq in this particular way.

I may have mistakenly thought that you have to do this if you implemented PartialEq. Was that mistaken? In any case, it's not a blocker right...?

#[derive(Eq)] is an automatic way to impl Eq ..., so these are equivalent. I was just curious if there was a reason to specifically implement an empty impl Eq {} instead of deriving.

It's not a blocker, we can address it later.

While working on #77 I realized that automatic derivation of Eq is not possible since DkgPublicKey from ferveo doesn't implement it either.

So it makes sense to provide an implementation stub impl Eq {} as a workaround. We need Eq do implement ProtocolObjectInner for AccessControlPolicy. Tracking this here: nucypher/ferveo#157

nucypher-core/src/threshold_message_kit.rs

piotr-roslaniec

Looks like a solid piece of work.

I have some comments, mostly nitpicks. AFAIK this is functionally correct.

I'm not approving yet because I want to test these changes in nucypher-ts.

jMyles

Just did a first read. Looks good on first read. :-)

piotr-roslaniec

Successfully tested in nucypher/taco-web#273

derekpierre added 2 commits August 17, 2023 09:59

Initial code and tests for AccessControlPolicy and its use by Thresho…

30aab6c

…ldDecryptionRequest. Requires changes to DkgPublicKey in ferveo.

Initial code for ThresholdMessageKit and tests.

2af5242

derekpierre added 4 commits August 17, 2023 13:48

Add aad() function to AccessControlPolicy.

3bab4c3

Rename ThresholdMessageKit kem/dem ciphertext to header and payload r…

07b4339

…espectively.

Rename ThresholdMessageKit underlying member for the access control p…

32cc6c8

…olicy to "acp"; also rename accessor method.

Add AccessControlPolicy and ThresholdMessageKit to nucypher_core py…

da87ad0

…thon module.

This was referenced Aug 17, 2023

AccessControlPolicy (ACP) and ThresholdMessageKit nucypher/nucypher#3194

Merged

Changes needed for ACP, ThresholdMessageKit work in nucypher-core nucypher/ferveo#156

Merged

fjarri reviewed Aug 18, 2023

View reviewed changes

Update nucypher-core to incorporate latest changes from ferveo regard…

a5cfd82

…ing use/exposure of key encapsulation i.e. use of CiphertextHader, and rework of Ciphertext.

derekpierre force-pushed the acp branch from 96f0a35 to a5cfd82 Compare August 20, 2023 00:31

derekpierre mentioned this pull request Aug 21, 2023

Max size for decryption request ciphertext nucypher/nucypher#3176

Closed

Modify AccessControlPolicy to be instantiated with an AuthenticatedDa…

71312cf

…ta object which works around to chicken and egg issue for obtaining the aad to use when encrypting data, but which is needed for the AccessControlPolicy after the ciphertext is signed. Update bindings/tests.

derekpierre mentioned this pull request Aug 22, 2023

Incorporate latest ACP/ThresholdMessageKit objects from nucypher-core nucypher/taco-web#269

Closed

derekpierre added 2 commits August 22, 2023 16:08

Add encrypt_for_dkg function so that Ciphertext and AuthenticatedData…

1136efe

… can be returned from the same call. Added python/wasm bindings.

derekpierre force-pushed the acp branch from e8602c6 to 5ed307f Compare August 22, 2023 20:08

Update nucypher-core changelog with latest changes.

2ba7f5d

derekpierre requested review from fjarri, jMyles, KPrasch, cygnusv, theref, manumonti and piotr-roslaniec August 22, 2023 20:32

derekpierre changed the title ~~[WIP] ACP/ThresholdMessageKit Work~~ ACP/ThresholdMessageKit Work Aug 22, 2023

derekpierre marked this pull request as ready for review August 22, 2023 20:32

Fix mypy build error. Related to this https://pyo3.rs/main/function/s…

89031a0

…ignature#trailing-optional-arguments.