Macro for deriving RandGen for some structs

[realcr]

The code at https://github.com/freedomlayer/offst/blob/master/components/crypto/src/rand.rs is very repetitive. Example:

impl RandGen for Salt {
    fn rand_gen(crypt_rng: &impl CryptoRandom) -> Self {
        let mut res = Self::default();
        crypt_rng.fill(&mut res).unwrap();
        res
    }
}

impl RandGen for InvoiceId {
    fn rand_gen(crypt_rng: &impl CryptoRandom) -> Self {
        let mut res = Self::default();
        crypt_rng.fill(&mut res).unwrap();
        res
    }
}

// ...

We can create a macro that does this automatically, possibly a macro that that can be used like this:
derive_rand_gen!(InvoiceId);

This issue is suitable for beginners with Rust and with the Offst project.

[cy6erlion]

@realcr can I take this one?

[realcr]

Of course! Please send a message if you need any help.

[djb4ai]

@realcr I am very new to Rust, I would like to work on this issue!

[amosonn]

I was going suggest a different approach: a blanket-impl for things implementing the two traits used in the implementation: Default and DerefMut into [u8]. This doesn't work though, because PrivateKey needs a separate implementation.

This raises the question however: should PrivateKey implement those traits at all? Whereas the other structs here are all valid as any collection of bytes (including the default 0-s), PrivateKey isn't.

[realcr]

@lladhibhutall : Sure thing, will be happy to help you out. @amosonn idea looks very interesting, maybe we can try it out.

@amosonn: I couldn't remember myself, so I just checked and this chunk of code compiles:

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn check_private_key() {
        let mut private_key = PrivateKey::default();
        let _a: &mut [u8] = &mut private_key;
    }
}

Does it mean we are good to go with your idea?

[amosonn]

No, I meant it the other way around, that maybe PrivateKey shouldn't implement DerefMut, only Deref.

The boilerplate impls could be replaced with a blanket impl for things which do implement DerefMut with Deref::Target = [u8] (and Default), it makes sense for those things to be filled with random bytes. But PrivateKey shouldn't be filled with random bytes, rather with a valid key, so it needs a custom impl; and this suggests to me that it shouldn't implement DerefMut, so that nobody can write invalid bytes into it.

Those impls of DerefMut are all macro-generated, in the protocol section; PrivateKey is already an oddball there, because it doesn't implement the serde traits. Which might even suggest it could be altogether moved from the protocol, since it isn't serializable, but I didn't look enough into that.

[realcr]

Now I understand. PrivateKey is serializable because you sometimes want to export it to a file.
Maybe going the macro-s way is still a reasonable idea, at least as a first solution, until we think of something better.

[amosonn]

Right, the macro that was present for the others and not for PrivateKey was defining capnp traits, not serde.

Here are the relevant files:
The definitions of the types: https://github.com/freedomlayer/offset/blob/master/components/proto/src/crypto/mod.rs
The definition of the macro: https://github.com/freedomlayer/offset/blob/master/components/common/src/define_fixed_bytes.rs

Having PrivateKey being serializable makes sense. What seems to me problematic (and what also blocks a blanket impl) is that it implement DerefMut, which means it's possible for code to change the content of a PrivateKey and make it invalid. Similar goes for it implementing From<&[u8]> without checking the content - this means you can create a PrivateKey from invalid bytes (which is indeed done in one of the tests). It is best to write the interface so that it is impossible to hold an invalid PrivateKey.

My suggestion for now would be to split at least this DerefMut impl from the define_fixed_bytes! macro into another one, which will be used on the other types but not on PrivateKey. This will enable replacing these repetetive impls for RandGen with a blanket impl. Then later, we can consider whether it is beneficial to remove some other impls from the common macro in order to give PrivateKey an implementation which validates the data.

[realcr]

I began porting the cryptographic library away from ring at PR #300, this might have some interference with solving this issue. Hope to update soon.

@lladhibhutall : Sorry for all the complications. I actually thought that this is a very simple issue, but it turned out that there are turtles all the way down. I hope to update you when this issue becomes something more manageable, or when there is another interesting task to do at the codebase.

[realcr]

So PR #300 is done.
After going again over the code I began to better understand what @amosonn was talking about.
The general idea is that it shouldn't be possible to create or modify things like PublicKey and PrivateKey, except for using the proper ways. For example, PrivateKey should only be created using a random generator, and PublicKey can be derived from a PrivateKey.
There might be other structures like PrivateKey that probably deserve a similar treatment.

This means that traits like DerefMut and Default should not be implemented automatically.
Having this change will require fixes, hopefully minor, across other places in the code.

Possible obstacles I can think of:

• serde serialization
• quickcheck automatic generation

[amosonn]

I'm pretty sure serde doesn't need this DerefMut, and perhaps neither quickcheck; it's also worth noting, that if you only allow valid data inside say `PrivateKey`, then it's good that quickcheck will check exactly that.

…

On Fri, Jun 19, 2020, 14:24 real ***@***.***> wrote: So PR #300 <#300> is done. After going again over the code I began to better understand what @amosonn <https://github.com/amosonn> was talking about. The general idea is that it shouldn't be possible to create or modify things like PublicKey and PrivateKey, except for using the proper ways. For example, PrivateKey should only be created using a random generator, and PublicKey can be derived from a PrivateKey. There might be other structures like PrivateKey that probably deserve a similar treatment. This means that traits like DerefMut and Default should not be implemented automatically. Having this change will require fixes, hopefully minor, across other places in the code. Possible obstacles I can think of: - serde serialization - quickcheck automatic generation — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#231 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAX7HLI6FGKQSLWWSA7EQNTRXNKHZANCNFSM4IEAYXDQ> .