MAID-2957: design crypto primitives #17

povilasb · 2018-07-31T10:27:47Z

Add proposal on core structure changes.

ustulation · 2018-07-31T11:35:46Z

design/crypto_primitives.rs

+    pub fn from_bytes(public_key: &[u8]) -> Self
+
+    /// For anyone who wants to store public key.
+    pub fn as_bytes(&self) -> &[u8]


These set of functions i think should return an array instead of a slice as it allows for users to consume it as arrays rather than as heap allocated storage. With slices, the length is expressed only at runtime forcing the collection into a heap allocated container (like Vec for e.g.)

There should perhaps be a contant:

const PUBLIC_ENCRYPT_KEY_BYTES: usize = <n>; // different for mock and non-mock ofc; // also is `_` favoured for constants ? ... pub fn raw(&self) -> [u8; PUBLIC_ENCRYPT_KEY_BYTES];

Now the consumption can be as:

let consume = enc_pk.raw();

ustulation · 2018-07-31T11:36:39Z

design/crypto_primitives.rs

+    // pub fn new() -> Self
+
+    /// Construct public key from bytes. Useful when it was serialized before.
+    pub fn from_bytes(public_key: &[u8]) -> Self


These set of functions i think should return an array instead of a slice as it allows for users to consume it as arrays rather than as heap allocated storage. Explained further, below.

ustulation · 2018-07-31T11:39:09Z

design/crypto_primitives.rs

+//! `PublicSignKey` and `SecretSignKey`.
+
+// no signing key
+pub struct PublicEncryptKey {


for sake of completeness can we mention what traits it needs implemented (either via deriving or manual roll out) ?
Same for other types where applicable

ustulation · 2018-07-31T11:42:51Z

design/crypto_primitives.rs

+    /// Construct secret key from given bytes. Useful when secret key was serialized before.
+    // Note that we don't need to provide public key, seems like it's derived from secret key:
+    // https://docs.rs/rust_sodium/0.9.0/rust_sodium/crypto/box_/curve25519xsalsa20poly130/struct.SecretKey.html#method.public_key
+    pub fn from_bytes(secret_key: &[u8]) -> Self


Does it happen for any array of bytes or does it need to be a part of valid RSA keypair ? Also is it valid for both RSA and EC algos ? If the inner working is via prime factorisation, does every number have a valid corresponding number to pair with ?

Otherwise we might want to return a result instead.

If the inner working is via prime factorisation, does every number have a valid corresponding number to pair with

definitely not, will make this method return Result :)

ustulation · 2018-07-31T11:46:29Z

design/crypto_primitives.rs

+
+
+//! Types that remain intact as in https://github.com/maidsafe/safe_crypto/blob/f214410/src/lib.rs:
+//! SharedSecretKey


from the definition in the commit hash, there's no facility to obtain raw() like other secret keys. Better to add that too.

ustulation · 2018-07-31T11:46:56Z

design/crypto_primitives.rs

+//! Types that remain intact as in https://github.com/maidsafe/safe_crypto/blob/f214410/src/lib.rs:
+//! SharedSecretKey
+//! Signature
+//! SymmetricKey


just like above for shared secret, there's no facility to obtain raw() like other secret keys. Better to add that too.

ustulation · 2018-07-31T11:47:39Z

design/crypto_primitives.rs

+//! cases, we don't need signing key at all, e.g. in Crust. So this document proposes to split
+//! `PublicKeys` and `SecretKeys` into corresponding types: `PublicEncryptKey, `SecretEncryptKey`,
+//! `PublicSignKey` and `SecretSignKey`.
+


The document misses the password hashing primitives. Pls add those as well, for both mock and non mock

ustulation · 2018-08-01T15:27:01Z

design/crypto_primitives.rs

+}
+
+impl Signature {
+    pub fn from_bytes(key: [u8; SIGNATURE_BYTES]) -> Result<Self, Error>


when would we ever error out here ?

ustulation · 2018-08-01T15:27:07Z

design/crypto_primitives.rs

+}
+
+impl SymmetricKey {
+    pub fn from_bytes(key: [u8; SYMMETRIC_KEY_BYTES]) -> Result<Self, Error>


when would we ever error out here ?

ustulation · 2018-08-01T15:34:07Z

design/crypto_primitives.rs

+impl PublicSignKey {
+    pub fn verify_detached(&self, signature: &Signature, data: &[u8]) -> bool {
+
+    pub fn from_bytes(public_key: [u8; PUBLIC_SIGN_KEY_BYTES]) -> Result<Self, Error>


In context of asymmetric keys, when would we ever error out here ? Given a secret we could know the public key in some standards i think, not sure that if given a public key can you actually infer anything, including if it's a part of valid keypair ? Is this the zero-knowledge proof thingy wherein I can prove that this is the product of two big primes without revealing the primes, or something ?

if the above point with zero knowledge proof etc. is valid, is this extensible to all algos (like RSA/ECDH etc) ?

My knowledge is sparse here - might need confirming from others if the API is fine.
Similarly in other asymmetric keys' from_bytes() functions here.

You're right, I don't see ways to validate bytes as a key. I've looked into a couple of ciphers (chacha20 and salsa20) from rust-crypto crate and it doesn't do any validation, just constructs structure from given bytes.

ustulation · 2018-08-02T11:55:02Z

design/crypto_primitives.rs

+}
+
+/// Derives encryption key from plaintext password.
+pub fn key_from_password(password: &[u8], salt: &[u8]) -> Result<SymmetricKey, Error>;


Key derivations usually will have a CPU and Memory complexity parameters associated - to thwart brute force attacks even for simple passwords. E.g. if deriving the correct key takes x CPU cycles == X secs and y amount of memory, then that makes brute forcing practically useless unless you run them in parallel over several machines, but anyway it's significantly decreased the ability to do it easily. If you are looking in scrypt or any other password hashing don't they have a way to specify those parameters ? How does it compare to pwhash from rust_sodium for instance ?

Might be a useful feature we don't want to miss as our goal is not so much as to store (or encourage to store) the password in plaintext and protect it (via hashing) than to actually deter the mentioned attacks by making it computationally very dear to try permutations.

I think the idea was to remove these configuration details, at least in a first place and only expose them if and when needed (based on Fraser's comment)

would guess that we don't need this KeyDeriveParams struct at all, at least for now. It'd be easier to just pick what we think are the best default values to initialise our chosen PBKDF and hide that inside key_from_password(). We can always add this extra flexibility to the safe_crypto API if there's a demand for it? Harder to remove it from the API later on.

Yes that's already covered and actioned according to @Fraser999 's review below ? (the cpu and mem limit parameters have been removed outdating his comment)

pierrechevalier83 · 2018-08-03T15:34:36Z

design/crypto_primitives.rs

+    pub fn sensitive() -> Self
+
+    /// Use custom CPU and memory parameters for key derivation.
+    // Not sure if we want this variantion, cause different implementations have different


s/variantion/variation/

pierrechevalier83

I've reviewed your change. Nothing jumped out to me, but I really don't know a lot about crypto, so I'm afraid my review isn't that valuable...

Fraser999 · 2018-08-03T09:38:16Z

design/crypto_primitives.rs

+    pub enum Error {
+        /// Occurs when serialisation or deserialisation fails.
+        Serialisation(e: SerialisationError) {
+            description("error serialising or deserialising message")


description() has been soft deprecated, so I think it should be replaced by display() in each variant here.

Fraser999 · 2018-08-03T12:22:56Z

design/crypto_primitives.rs

+    // Not sure if we want this variantion, cause different implementations have different
+    // parameters. E.g. scrypt uses N, r, p parameters:
+    // https://github.com/DaGenix/rust-crypto/blob/master/src/scrypt.rs#L163
+    pub fn new(cpu_limit: usize, mem_limit: usize) -> Self


I would guess that we don't need this KeyDeriveParams struct at all, at least for now. It'd be easier to just pick what we think are the best default values to initialise our chosen PBKDF and hide that inside key_from_password(). We can always add this extra flexibility to the safe_crypto API if there's a demand for it? Harder to remove it from the API later on.

However, if we do keep this struct, we'd maybe need to have new() return a Result since the scrypt function you linked to has several constraints on the values it'll accept. It asserts, and since we don't like that style of error handling, we'd need to check in our wrapper for such invalid values and error out instead of panicking.

If you think/know that we always use the same default params for key derivation, then I agree that we don't need those params at least yet :)

Fraser999 · 2018-08-03T15:23:36Z

design/crypto_primitives.rs

+}
+
+/// Derives encryption key from plaintext password.
+pub fn key_from_password(password: &[u8], salt: &[u8], params: &KeyDeriveParams) -> Result<SymmetricKey, Error>;


If we don't pass params here and use suitable defaults, can this function fail?

Mmm, I believe so. Say we still use rust_sodium whose docs say:

The function returns Ok(key) on success and Err(()) if the computation didn't complete, usually because the operating system refused to allocate the amount of requested memory.

https://docs.rs/rust_sodium/0.9.0/rust_sodium/crypto/pwhash/scryptsalsa208sha256/fn.derive_key.html

Sounds like with secure/slow key derivation algorithms this might be a common trait when one has low memory resources.

pierrechevalier83 · 2018-08-07T13:39:35Z

design/crypto_primitives.rs

+/// Store secret key in `Arc` to discourage cloning it. Makes it more secure by not duplicating
+/// encryption keys in memory.
+/// Use `encrypt_key_pair()` to generate public and private keys pair.
+#[derive(Debug, PartialEq, Eq, Clone)]


From what I understood, we will need to also be Serialize and Deserialize as it's needed for self_authentication (we want to store some encrypted secrets on the network).
Applies for SecretEncryptKey (line 87) and SecretEncryptKeyInner (line 92)

Do we really want to use Serialize/Deserialize as they add some metadata or as_bytes()/from_bytes() instead? E.g. in case of signatures we want to attach just a signature without serde metadata, hence we use as_bytes(). Not sure about secret key serialization though.

I don't see a strong argument against as_bytes(), from_bytes(), but maybe @ustulation has more insight on this?

Me and Spandan discusses that we should implement Serialize/Deserialize anyway. Say we have a struct that wraps public and secret key and we want it to be serializable. If SecretKey was not serializable, we should implement Serialize manually.

pierrechevalier83 · 2018-08-07T13:47:25Z

design/crypto_primitives.rs

+}
+
+impl SecretSignKey {
+    pub fn new() -> Self


I don't understand this constructor:
since it takes no input, I'm assuming it generates the public/secret key pair, but then it would discard the public key and only store the secret key. How do I advertise my identity in these conditions?

If I understand it right, it should probably take a SecretSignKey as input so that I can first generate my key pair with sign_key_pair and then use this class to expose functionality of the private key while holding on my public key to share it with whoever I want.

There's no need for new() function as wrappers are 1-to-1. So free functions gen_sign/encrypt_keypair() should be sufficient. For construction from raw, there's facility below (from_bytes())

(Had to retype this as it was being displayed twice and deleting one deleted both responses)

Good spotting, that's an accidental leftover from refactor, will remove it :)

pierrechevalier83 · 2018-08-07T13:49:33Z

design/crypto_primitives.rs

+}
+
+impl PublicSignKey {
+    pub fn verify_detached(&self, signature: &Signature, data: &[u8]) -> bool {


Should there be a constsructor that takes a PublicKey?

there's from_bytes() below. Or did you mean something else ?

pierrechevalier83

Here is my review now I've got a better idea of what things represent :)

ustulation · 2018-08-07T14:26:38Z

design/crypto_primitives.rs

+}
+
+impl PublicSignKey {
+    pub fn verify_detached(&self, signature: &Signature, data: &[u8]) -> bool {


there's from_bytes() below. Or did you mean something else ?

feat/design: add description

c010d58

ustulation suggested changes Jul 31, 2018

View reviewed changes

ustulation suggested changes Aug 1, 2018

View reviewed changes

ustulation suggested changes Aug 2, 2018

View reviewed changes

chore/appveyor: skip builds when only design/ dir is changed

3fa4c50

pierrechevalier83 reviewed Aug 3, 2018

View reviewed changes

Fraser999 reviewed Aug 3, 2018

View reviewed changes

pierrechevalier83 reviewed Aug 7, 2018

View reviewed changes

ustulation suggested changes Aug 7, 2018

View reviewed changes

design/primitives: add crypto primitives design

25e3b65

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAID-2957: design crypto primitives #17

MAID-2957: design crypto primitives #17

povilasb commented Jul 31, 2018

ustulation Jul 31, 2018

ustulation Jul 31, 2018

ustulation Jul 31, 2018

ustulation Jul 31, 2018

povilasb Aug 1, 2018 •

edited

Loading

ustulation Jul 31, 2018

ustulation Jul 31, 2018

ustulation Jul 31, 2018

ustulation Aug 1, 2018

ustulation Aug 1, 2018

ustulation Aug 1, 2018

povilasb Aug 2, 2018

ustulation Aug 2, 2018

pierrechevalier83 Aug 7, 2018

ustulation Aug 7, 2018

pierrechevalier83 Aug 3, 2018

pierrechevalier83 left a comment

Fraser999 Aug 3, 2018

Fraser999 Aug 3, 2018

povilasb Aug 6, 2018

Fraser999 Aug 3, 2018

povilasb Aug 6, 2018

pierrechevalier83 Aug 7, 2018

povilasb Aug 8, 2018

pierrechevalier83 Aug 8, 2018

povilasb Aug 9, 2018

pierrechevalier83 Aug 7, 2018 •

edited

Loading

ustulation Aug 7, 2018

povilasb Aug 8, 2018

pierrechevalier83 Aug 7, 2018

ustulation Aug 7, 2018

pierrechevalier83 left a comment

ustulation Aug 7, 2018



		//! Types that remain intact as in https://github.com/maidsafe/safe_crypto/blob/f214410/src/lib.rs:
		//! SharedSecretKey

MAID-2957: design crypto primitives #17

Are you sure you want to change the base?

MAID-2957: design crypto primitives #17

Conversation

povilasb commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

povilasb Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierrechevalier83 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierrechevalier83 Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierrechevalier83 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

povilasb Aug 1, 2018 •

edited

Loading

pierrechevalier83 Aug 7, 2018 •

edited

Loading