Skip to content
This repository has been archived by the owner on Aug 30, 2022. It is now read-only.

wrap libsodium #392

Merged
merged 1 commit into from Apr 27, 2020
Merged

wrap libsodium #392

merged 1 commit into from Apr 27, 2020

Conversation

little-dude
Copy link
Contributor

@little-dude little-dude commented Apr 24, 2020

We'd like to implement some traits on libsodium types, but this is
prevented by the orphan rule. To work around this, this commit
introduce newtypes around the libsodium crypto primitives that we use
in our codebase.

Copy link
Contributor

@janpetschexain janpetschexain left a comment

Choose a reason for hiding this comment

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

i like the wrapping, it provides us with a good abstraction. regarding some of the naming i would prefer to make it more consistent, the things related to soidumoxide::crypto::box_:: are named with an indication but the things related to sodiumoxide::crypto::sign:: are named with an additional Sign. i would suggest the following naming:
EncrPublicKey and SignPublicKey
EncrSecretKey and SignSecretKey
generate_encr_key_pair() and generate_sign_key_pair()
derive_encr_key_pair_from_seed() and derive_sign_key_pair_from_seed()
etc

rust/src/crypto.rs Outdated Show resolved Hide resolved
rust/src/crypto.rs Outdated Show resolved Hide resolved
@little-dude
Copy link
Contributor Author

little-dude commented Apr 24, 2020

re: naming

I hesitate, but I didn't really like abbreviating Encr, and I felt like Encryption was a bit long so ultimately I decided not to choose and go without a prefix :p

Now that I think about it more, I think I prefer the full Encryption version. What do you think about:

PublicEncryptionKey
SecretEncryptionKey
PublicSigningKey
SecretSigningKey

I think moving the Signing/Encryption part to the middle makes it sound more natural in english.

Just to list all the alternatives:

# current (1)
PublicKey, SecretKey
SignPublicKey, SignSecretKey

# @jan suggestion (2)
EncrPublicKey, EncrSecretKey
SignPublicKey, SignSecretKey

# my suggestion (3)
PublicEncryptionKey, SecretEncryptionKey
PublicSigningKey, SecretSigningKey

# another suggestion like (2) but with full prefix instead of abbreviations (4)
EncryptionPublicKey, EncryptionSecretKey
SigningPublicKey, SigningSecretKey

@janpetschexain
Copy link
Contributor

i would go with (3) or (4), slightly favoring (3).

@finiteprods
Copy link
Contributor

i actually quite like (2) as Encr and Sign are both 4 chars, so line up nicely (as do Public and Secret). i'll throw out another suggestion, which is compromise of (2) and (3)

# suggestion (5)
PublicEncryptKey, SecretEncryptKey
PublicSigningKey, SecretSigningKey

i don't mind whether it's Public/Secret prefix (as above) or Encrypt/Signing

@janpetschexain
Copy link
Contributor

i actually quite like (2) as Encr and Sign are both 4 chars, so line up nicely (as do Public and Secret). i'll throw out another suggestion, which is compromise of (2) and (3)

# suggestion (5)
PublicEncryptKey, SecretEncryptKey
PublicSigningKey, SecretSigningKey

i don't mind whether it's Public/Secret prefix (as above) or Encrypt/Signing

that's another good suggestion. having equally long names was my original intention behind Encr.

we're tackling one of the hardest problems in computer science, no easy solution :D
https://hackernoon.com/the-art-of-naming-variables-52f44de00aad

@Robert-Steiner
Copy link
Contributor

I like (2) and (5)

@little-dude
Copy link
Contributor Author

rebased. I like (5) to and it seems to make everyone happy so I'm gonna go with that. I don't understand what having the same length for the type names is important though 😄

@janpetschexain
Copy link
Contributor

rebased. I like (5) to and it seems to make everyone happy so I'm gonna go with that. I don't understand what having the same length for the type names is important though 😄

the answer to that is OCD ^^

@little-dude
Copy link
Contributor Author

@janpetschexain I addressed all your comments, and updated the tests. This is ready for another round of review.

@little-dude
Copy link
Contributor Author

I think I'd prefer to remove the use crypto::* and move the type aliases to lib.rs actually. I'll do that tonight.

rust/src/crypto.rs Outdated Show resolved Hide resolved
rust/src/crypto.rs Outdated Show resolved Hide resolved
rust/src/crypto.rs Outdated Show resolved Hide resolved
rust/src/crypto.rs Outdated Show resolved Hide resolved
rust/src/crypto.rs Outdated Show resolved Hide resolved
rust/src/crypto.rs Outdated Show resolved Hide resolved
rust/src/crypto.rs Outdated Show resolved Hide resolved
rust/src/crypto.rs Outdated Show resolved Hide resolved
rust/src/crypto.rs Outdated Show resolved Hide resolved
rust/src/crypto.rs Outdated Show resolved Hide resolved
rust/src/crypto.rs Outdated Show resolved Hide resolved
rust/src/crypto.rs Outdated Show resolved Hide resolved
rust/src/crypto.rs Outdated Show resolved Hide resolved
@little-dude
Copy link
Contributor Author

I've addressed all the comments except #392 (comment)

I also:

  • moved the type aliases back to lib.rs
  • split the crypto module into two files: crypt/sign.rs and crypto/encrypt.rs
  • removed the use crypto::* that I had put in all the modules. That created tons of errors, and I went through all of them, replacing the generic crypto::* types by the appropriate alias. In the tests there were places where it was unclear what was required though, especially here: 3cd671b#diff-7047ef3ec6efbf0f4f070fc70c1eb404R300

Copy link
Contributor

@janpetschexain janpetschexain left a comment

Choose a reason for hiding this comment

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

i left some remarks, but it really looks good so far! i know it's tedious work. also sorry for the sometimes unclear types in the tests, i often took the lazy way and just mocked keys where ever they are just collected but not used, this makes type inference not easy.

rust/src/message/sum.rs Outdated Show resolved Hide resolved
rust/src/message/sum.rs Outdated Show resolved Hide resolved
rust/src/message/sum.rs Outdated Show resolved Hide resolved
rust/src/message/sum.rs Outdated Show resolved Hide resolved
rust/src/message/sum.rs Outdated Show resolved Hide resolved
rust/src/message/sum.rs Outdated Show resolved Hide resolved
rust/src/participant.rs Outdated Show resolved Hide resolved
rust/src/crypto/encrypt.rs Outdated Show resolved Hide resolved
@little-dude
Copy link
Contributor Author

also sorry for the sometimes unclear types in the tests, i often took the lazy way and just mocked keys where ever they are just collected but not used, this makes type inference not easy.

Ah no problem at all! And thank you for the thorough reviews, this PR is definitely looking much better than the initial version.

I've addressed your last round of comment, so I'll wait for @finiteprods feedback and then squash everything.

rust/src/lib.rs Outdated Show resolved Hide resolved
We'd like to implement some traits on libsodium types, but this is
prevented by the orphan rule. To work around this, this commit
introduce newtypes around the libsodium crypto primitives that we use
in our codebase.
@little-dude little-dude merged commit f0b1608 into pet Apr 27, 2020
@little-dude little-dude deleted the crypto-wrapper branch April 27, 2020 06:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants