Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

serde support #67

Merged
merged 1 commit into from
Sep 12, 2021
Merged

serde support #67

merged 1 commit into from
Sep 12, 2021

Conversation

fjarri
Copy link
Contributor

@fjarri fjarri commented Sep 1, 2021

  • adds Serialize/Deserialize implementations for PublicKey, Signature, Capsule, KeyFrag, CapsuleFrag
  • for human-readable formats, PublicKey is serialized as hex, and the rest in base64-encoded form

Note: all the serializations simply use the bytestring produced by to_array() for binary formats, and the b64 encoding of it for human-readable formats; this helps preserve compatibility with pyumbral (as opposed to implementing Serialize for CurveScalar and CurvePoint, and deriving the rest).

For now, not including any serde support for Verified* objects - they won't be used as nested fields in any protocol objects, so the existing serialization support is enough. Same for the secret objects (SecretKey, SecretKeyFactory).

For future: we could expose the binary/b64 mechanic to help serialize ciphertexts too. Either as a wrapper class, or returning a Ciphertext object that would implement serde traits. Not in this PR.

@fjarri fjarri added the API Related to public API label Sep 1, 2021
@fjarri fjarri added this to the v1.0.0 milestone Sep 1, 2021
@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2021

Codecov Report

Merging #67 (3e5e1e1) into master (e90188c) will increase coverage by 0.74%.
The diff coverage is 90.32%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #67      +/-   ##
==========================================
+ Coverage   81.61%   82.36%   +0.74%     
==========================================
  Files          13       14       +1     
  Lines         990     1083      +93     
==========================================
+ Hits          808      892      +84     
- Misses        182      191       +9     
Impacted Files Coverage Δ
umbral-pre/src/serde.rs 80.00% <80.00%> (ø)
umbral-pre/src/capsule.rs 90.15% <100.00%> (+0.80%) ⬆️
umbral-pre/src/capsule_frag.rs 85.81% <100.00%> (+1.02%) ⬆️
umbral-pre/src/key_frag.rs 79.31% <100.00%> (+1.07%) ⬆️
umbral-pre/src/keys.rs 71.25% <100.00%> (+3.47%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e90188c...3e5e1e1. Read the comment docs.

T: SerializableToArray,
S: Serializer,
{
if serializer.is_human_readable() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this is_human_readable defined? What does it mean?
If the serializer for something "is human readable", we are returning base64?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's defined by the implementers of specific formats (see docs), e.g. JSON, BSON etc. They decide whether it's supposed to be "human-readable" or not.

If the serializer for something "is human readable", we are returning base64?

Yep. Although I want to make it a hex representation in case of public keys, the same as it currently works in nucypher.

@piotr-roslaniec
Copy link
Contributor

piotr-roslaniec commented Sep 9, 2021

My 2 cents:

  • Having Deserialize for Verified* methods could be used to create a new implementation of a re-encryption scheme. IMHO not an immediate concern.
  • Serialization for SecretKey* could be useful for handling keys created in "airtight" environments, such as MetaMask snaps. I'm giving a cautious +1 here.
  • Exposing serialization Ciphertext would be useful for storing message kits. Currently, in nucypher-ts I'm just using raw bytes. +1 Actually not sure about this one

@fjarri
Copy link
Contributor Author

fjarri commented Sep 9, 2021

Having Deserialize for Verified* methods could be used to create a new implementation of a re-encryption scheme. IMHO not an immediate concern.

This may be useful in nucypher-ts if it implements verified kfrag/cfrag caching, and those frags are embedded in larger structs. The latter may not be necessary if some kind of key-value database is used.

Serialization for SecretKey* could be useful for handling keys created in "airtight" environments, such as MetaMask snaps. I'm giving a cautious +1 here.

They can be (de)serialized right now, just not with serde. Is that enough for Metamask?

serde support is needed if we want to nest these secret objects as a part of larger serialized structs... which we might, but it will provide more opportunities for the secret data to be leaked.

Exposing serialization Ciphertext would be useful for storing message kits. Currently, in nucypher-ts I'm just using raw bytes. +1. Actually not sure about this one

My rationale was that in order to make the distinction between b64/byte representation for ciphertexts in structs that contain them (e.g. message kits), you'd have to write a custom serializer yourself. As opposed to having a Ciphertext object that already has that implemented, and also implements AsRef<[u8]>, which is all we really need from it.

@piotr-roslaniec
Copy link
Contributor

This may be useful in nucypher-ts if it implements verified kfrag/cfrag caching, and those frags are embedded in larger structs. The latter may not be necessary if some kind of key-value database is used.

Not sure I get it. IMHO in either case, If we use a key-value database for caching (like localStorage in the browser) we still need to serialize and deserialize (specifically to string)

They can be (de)serialized right now, just not with serde. Is that enough for Metamask?

Right, it already has toBytes method. Yes, that's enough.

serde support is needed if we want to nest these secret objects as a part of larger serialized structs... which we might, but it will provide more opportunities for the secret data to be leaked.

I share this concern.

My rationale was that in order to make the distinction between b64/byte representation for ciphertexts in structs that contain them (e.g. message kits), you'd have to write a custom serializer yourself. As opposed to having a Ciphertext object that already has that implemented, and also implements AsRef<[u8]>, which is all we really need from it.

I see. I would weigh more on the side of the former as it leaves more flexibility to the user.

@fjarri
Copy link
Contributor Author

fjarri commented Sep 10, 2021

Not sure I get it. IMHO in either case, If we use a key-value database for caching (like localStorage in the browser) we still need to serialize and deserialize (specifically to string)

Hm, I realize now that I confused two usage scenarios here. This PR is important only for the applications that use Umbral as a Rust library; for JS or Python applications there will be no change whatsoever.

So, the question is whether some Rust application would want to:

  1. Have a struct with one of the fields being VerifiedKeyFrag or VerifiedCapsuleFrag, and
  2. Derive serialization for such a struct with serde

For example, if we write a Bob in Rust (or some part of Bob), and want to be able to cache retrieved cfrags, we will have to deal with this problem. One may be able to get away with using some key-value storage and deserializing verified cfrags manually with from_verified_bytes(), but it is possible that more complicated scenarios are possible where serde is needed.

@fjarri fjarri marked this pull request as ready for review September 12, 2021 21:30
@fjarri
Copy link
Contributor Author

fjarri commented Sep 12, 2021

Merging in the minimal version for now, to keep things moving. If @vepkenez needs more stuff, I'll add it in another PR.

@fjarri fjarri merged commit 3d3adf3 into nucypher:master Sep 12, 2021
@fjarri fjarri deleted the serde branch September 12, 2021 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Related to public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants