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

Place all classes in modules describing their primitives #74

Merged
merged 2 commits into from
Oct 2, 2013

Conversation

tarcieri
Copy link
Contributor

cc @namelessjon @stouset

This change places all primitives into their own module. I have to say that by
doing this, the organization of the source code seems much improved to me.

The original API is preserved (with some changes) by aliasing shorthand class
names to their algorithm-specific versions.

The following name changes have been made:

  • RbNaCl::Box => RbNaCl::Curve25519XSalsa20Poly1305::Box
    • RbNaCl::PrivateKey => RbNaCl::Curve25519XSalsa20Poly1305::PrivateKey
    • RbNaCl::PublicKey => RbNaCl::Curve25519XSalsa20Poly1305::PublicKey
  • RbNaCl::SecretBox => RbNaCl::XSalsa20Poly1305::SecretBox
  • RbNaCl::SigningKey => RbNaCl::Ed25519::SigningKey
  • RbNaCl::VerifyKey => RbNaCl::Ed25519::VerifyKey
  • RbNaCl::Point => RbNaCl::Curve25519::Point
  • RbNaCl::Auth::OneTime => RbNaCl::Poly1305::OneTimeAuth
  • RbNaCl::Hash::Blake2b => RbNaCl::Blake2b::Hash
  • RbNaCl::HMAC::SHA256 => RbNaCl::SHA256::HMAC
  • RbNaCl::HMAC::SHA512256 => RbNaCl::SHA512256::HMAC

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) when pulling 26f2e20 on add-primitive-names into 420b87d on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) when pulling 26f2e20 on add-primitive-names into 420b87d on master.

@namelessjon
Copy link
Contributor

Thinking about this some more, I think this is the 'wrong way around'.

The point of libsodium/NaCl is in the high level constructs it provides. I think that's actually more useful than the specific algorithmic choices it provides. The developer says "I want to exchange messages securely when I have public key pairs" and libsodium says "Here's a crypto_box, that will do it (coincidentally, it uses Curve25519, XSalsa20, Poly1305)". The organisation you propose reintroduces a focus on low level implementation details (the precise algorithms) and not on the high level constructs. I know you provide the aliases, but ss it stands, I have to know Curve25519XSalsa20Poly1305 provides a secret box, and if I want to change the algorithm I also need to know Nistp256AES256GCM is also a valid box combination.

A construct focused layout would be better.

@tarcieri
Copy link
Contributor Author

What you're proposing, organizing everything by the construct, is what I originally set out to do. However, when I actually sat down and started doing it I realized something:

What we're actually exposing are APIs which should be more or less identical for the given constructs provided they're used with the appropriate types. Thus what the end user ends up working with should be a Box class, not a Curve25519XSalsa20Poly1305.

Some of the algorithms (Curve25519 and Ed25519) have multiple classes we need to deal with and group in a logical manner.

Hence:

RbNaCl::Curve25519XSalsa20Poly1305::Box

The class you're working with is named Box and exposes the Box API. It has two associated classes: public and private keys, which are named PublicKey and PrivateKey.

We could, perhaps, go for a more NaCl like structure that does both. I'd imagine it would look like this (call this Alternative #1):

  • RbNaCl::Box => RbNaCl::Boxes::Curve25519XSalsa20Poly1305::Box
    • RbNaCl::PrivateKey => RbNaCl::Boxes::Curve25519XSalsa20Poly1305::PrivateKey
    • RbNaCl::PublicKey => RbNaCl::Boxes::Curve25519XSalsa20Poly1305::PublicKey
  • RbNaCl::SecretBox => RbNaCl::SecretBoxes::XSalsa20Poly1305::SecretBox
  • RbNaCl::SigningKey => RbNaCl::Signatures::Ed25519::SigningKey
  • RbNaCl::VerifyKey => RbNaCl::Signatures::Ed25519::VerifyKey
  • RbNaCl::Point => RbNaCl::DH::Curve25519::Point
  • RbNaCl::Auth::OneTime => RbNaCl::OneTimeAuth::Poly1305::OneTimeAuth
  • RbNaCl::Hash::Blake2b => RbNaCl::Hashes::Blake2b::Hash
  • RbNaCl::HMAC::SHA256 => RbNaCl::HMACs::SHA256::HMAC
  • RbNaCl::HMAC::SHA512256 => RbNaCl::HMACs::SHA512256::HMAC

Or we could do something more like what I had originally planned (call this Alternative #2):

  • RbNaCl::Box => RbNaCl::Boxes::Curve25519XSalsa20Poly1305
    • RbNaCl::PrivateKey => RbNaCl::Boxes::Curve25519XSalsa20Poly1305::PrivateKey
    • RbNaCl::PublicKey => RbNaCl::Boxes::Curve25519XSalsa20Poly1305::PublicKey
  • RbNaCl::SecretBox => RbNaCl::SecretBoxes::XSalsa20Poly1305
  • RbNaCl::SigningKey => RbNaCl::Signatures::Ed25519::SigningKey
  • RbNaCl::VerifyKey => RbNaCl::Signatures::Ed25519::VerifyKey
  • RbNaCl::Point => RbNaCl::DH::Curve25519::Point
  • RbNaCl::Auth::OneTime => RbNaCl::OneTimeAuth::Poly1305
  • RbNaCl::Hash::Blake2b => RbNaCl::Hashes::Blake2b
  • RbNaCl::HMAC::SHA256 => RbNaCl::HMACs::SHA256
  • RbNaCl::HMAC::SHA512256 => RbNaCl::HMACs::SHA512256

Or do you have a better suggestion?

All in all, I think I would prefer the class names should reflect the constructs (e.g. ::Box) and not the algorithms (e.g. ::Curve25519XSalsa20Poly1305) and doing both seems overly verbose and hard-to-read to me (e.g. ::Curve25519XSalsa20Poly1305Box) and that was the major motivation for this particular PR.

@tarcieri
Copy link
Contributor Author

@namelessjon any thoughts here?

@stouset
Copy link

stouset commented Sep 16, 2013

Alternative 2 is more or less what I'm doing in Sodium, although I'm honestly a fan of

  • RbNaCl::HMAC
    • RbNaCl::HMAC::SHA256
    • RbNaCl::HMAC::SHA512

et al (e.g., no need for plurals and separate namespaces) but that's mostly just an issue of taste.

@tarcieri
Copy link
Contributor Author

@stouset RbNaCl::HMAC::* look better to me too

This change places all primitives into their own module. I have to say that by
doing this, the organization of the source code seems much improved to me.

The original API is preserved (with some changes) by aliasing shorthand class
names to their algorithm-specific versions.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.15%) when pulling a381190 on add-primitive-names into 580d2cc on master.

@tarcieri
Copy link
Contributor Author

tarcieri commented Oct 1, 2013

@namelessjon @stouset have a look at a381190

This places all primitives into modules/directories according to the construct name, not the underlying algorithm. I used the mapping I suggested before with the tweaks suggested by @stouset.

I also decided to use RbNaCl::Points::Curve25519 in case @CodesInChaos's suggestion to add Ed25519 scalar multiplication makes it into libsodium, in which case it would be nice to have RbNaCl::Points::Ed25519 (and methods for converting to Curve25519 points).

I also used RbNaCl::Authenticators::Poly1305 since RbNaCl::OneTimeAuth::Poly1305 would collide the class alias and module name. Not the greatest solution but I'm not sure of a better one.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) when pulling a381190 on add-primitive-names into 580d2cc on master.

@tarcieri
Copy link
Contributor Author

tarcieri commented Oct 1, 2013

Noted by @CodesInChaos: perhaps RbNaCl::Point should be RbNaCl::GroupElement

@tarcieri
Copy link
Contributor Author

tarcieri commented Oct 2, 2013

Per @CodesInChaos's suggestion I've renamed RbNaCl::Point to RbNaCl::GroupElement. Curve25519 is available through RbNaCl::GroupElements::Curve25519. Conceivably we could support RbNaCl::GroupElements::Ed25519 as well.

@namelessjon look good now?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) when pulling 38faa1a on add-primitive-names into 580d2cc on master.

@namelessjon
Copy link
Contributor

Looks good.

namelessjon added a commit that referenced this pull request Oct 2, 2013
Place all classes in modules describing their primitives
@namelessjon namelessjon merged commit 1e20022 into master Oct 2, 2013
@namelessjon namelessjon deleted the add-primitive-names branch October 2, 2013 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants