Primitive specification #46

Merged
merged 14 commits into from Mar 14, 2013

4 participants

@namelessjon

Okay, I'm making this into a full blown pull request, so that comments on the implementation etc. can be made inline.

As I see it, things that need doing before this is merged:

  • Decide on whether or not to have Ciphertext (or whatever it ends up being named)
  • Support swapping out of the PublicKey and PrivateKey mechanisms.
  • What else?

This fixes #43

namelessjon added some commits Mar 13, 2013
@namelessjon namelessjon Expose access to XSalsa20Poly1305 primitive
This is to start to fix #43

Explicitly expose access to the XSalsa20Poly1305 primitive from the
library.  The primitive class handles all of the input checking, and the
encryption methods.  At the moment, the code has been more or less just
copied across.  Eventually, it might make sense to factor this out into
the SecretBox module.

Also, it's possible that one or other of the method calls might return
something other than a string, augmented with additional information and
methods.
04eb156
@namelessjon namelessjon Rename SecretBox primitive
Now more closely mimics C API
3d068b9
@namelessjon namelessjon Return a Ciphertext object
This behaves approximately like a string, at least for many operations
that a ciphertext is likely to have performed on it.  It also supports
encoding to any encoding recognised by RbNaCl
8e38448
@namelessjon namelessjon Allow runtime interrogation of the primitive
Should be a little nicer to work with in a multi-primitive setup
f619ade
@namelessjon namelessjon Provide method for querying # of nonce bytes
This allows the RandomNonceBox to funciton in an indeterminate primivite
world.
34d1de4
@namelessjon namelessjon Note that we're returning a Ciphertext a6da4ba
@namelessjon namelessjon Ooops, fix RandomNonceBox e1ac68a
@namelessjon namelessjon Support specification of Box primitive
This obviously defaults to Curve25519XSalsa20Poly1305, which is also the
only primitive currently implemented.

Still need to add the necessary support for changing out the Key, too.
ed3d9d8
@namelessjon namelessjon Kill un-needed module 96fa188
@coveralls

Coverage remained the same when pulling 96fa188 on primitive_specification into 9be368b on master.

View Details

@namelessjon namelessjon RandomNonceBox now reports primitive used too
Also, specs updated to use TestVectors
658be94
@tarcieri
Cryptosphere member

I'd still probably be in favor of combining the latter two arguments into an options hash. I also think it might make sense for the ciphers to register themselves as symbols the same way Crypto::Encodings do, so the API could be:

Crypto::SecretBox.new(key, :primitive => :xsalsa20poly1305)

That just seems a lot more like idiomatic Ruby to me, versus:

Crypto::SecretBox.new(key, :raw, Crypto::SecretBox::XSalsaPoly1305)
@namelessjon namelessjon Remove Ciphertext
It's breaking the 1.0 API.  That's the price of rushing for 1.0, I
guess.
3652ca3
@namelessjon

They might get a hash interface after I also allow for swapping out of the Keys.

I think classes are fine though. (1) you have the visual check of Crypto::SecretBox needing to match. (2) It should be a rare enough operation that I don't think it needs the sugar.

@namelessjon

I'm 100% fine with not providing sugar for operations that should be rare and somewhat painful, so that their use is not encouraged, and is very obvious on a quick scan through code.

@tarcieri
Cryptosphere member

@namelessjon I buy that, and we can switch the API later if it does become commonly used anyway.

@coveralls

Coverage decreased (-5.18%) when pulling 3652ca3 on primitive_specification into 9be368b on master.

View Details

@namelessjon

That's enough for today anyway. I'll hopefully finish this off tomorrow.

@coveralls

Coverage remained the same when pulling a25bd5d on primitive_specification into 9be368b on master.

View Details

@tarcieri
Cryptosphere member

Cool

@tarcieri
Cryptosphere member

Here's what I don't like about this approach:

If library authors want raw access to particular primitives, why pass in a parameter? Just use the class! If @stouset wants to use Crypto::SecretBox::XSalsa20Poly1305, why not just use that class instead of passing that class in as a parameter to Crypto::SecretBox.new?

I think this makes more sense:

  • Simply rename Crypto::SecretBox => Crypto::SecretBox::XSalsa20Poly1305 and Crypto::Box => Crypto::Box::Curve25519XSalsa20Poly1305
  • Have Crypto::SecretBox and Crypto::Box simply be modules that forward calls to new to the respective primitive-specific classes.

You've also removed Crypto::NaCl::NONCEBYTES. I'd say that breaks the public API to an extent, even if that wasn't documented (we should have documented constants for people to reference). This isn't just a hypothetical problem. I was already using these constants in Keyspace:

https://github.com/livingsocial/keyspace/blob/master/lib/keyspace/capability.rb#L18

My suggestion would be to define these constants under their respective classes, e.g.:

  • Crypto::SecretBox::NONCEBYTES
  • Crypto::SecretBox::XSalsa20Poly1305::NONCEBYTES

I would probably suggest keeping all the constants that are already defined under Crypto::NaCl too, just for backwards compatibility's sake. We can flag them for removal in a hypothetical RbNaCl 2.0.

@namelessjon

On the split: I like the split API, because it allows you to have the conceptually clean, almost direct NaCl bindings in the explicitly named classes. These just do the cryptography operations, are very insistent on what they have passed to them, etc. This then leaves Box and SecretBox to present a slightly friendlier API, which takes encoded keys, and potentially provides encoded output.

Yes, I removed NONCEBYTES. However, I realised it probably shouldn't be accessed as a generic constant anyway. Different crypto_box constructions can (and in the tentative design on http://nacl.cr.yp.to do) have different nonce lengths. So you really have to check the crypto_box you've instantiated to be safe. That's why they now have a #nonce_bytes method. The constant could be added back, but it needs a big health warning slapped on it due to it not being cross-box generic. Ditto for the other generically named constants. They're all (potentially) subject to change when a new crypto_box comes along, so they need to be specified per box and the generic ones not used. (Note that things like KEYBYTES are, although those should perhaps also reflect down onto the generic box too)

@tarcieri
Cryptosphere member

On the split: I like the split API, because it allows you to have the conceptually clean, almost direct NaCl bindings in the explicitly named classes.

Well sure, I was saying I like that API too. What I'm saying is why would we ever tell people to do this:

secret_box = Crypto::SecretBox.new(secret_key, :raw, Crypto::SecretBox::XSalsaPoly1305)

instead of this:

secret_box = Crypto::SecretBox::XSalsaPoly1305.new(secret_key)

It's just ugly and convoluted...

RE: nonce lengths (and key lengths!) methods would be fine. We definitely need some API for this though.

Just the way these changes are heading, it will break Keyspace at the very least, which doesn't make me very happy about this being a "1.x" release. We either need to go to version "2.0" with these changes, or argue that the things which are breaking were never officially part of the public API (which seems silly because I was already using them in a project)

@namelessjon

Relying on undocumented features is surely the road to ruin ;) A quick check of the documentation didn't reveal anywhere where the nonce length constant is explicitly used, and its not documented in the source either. I don't think we should be held to undocumented methods/constants being part of the API -- I don't want to have to keep all the direct NaCl binding methods attached to the module unchanged, for example, as they're not intended for general use. The fact that we neglected to provide an official API for the nonce length, on the other hand, was a 1.0.0 bug.

On the split class, I'll think a little more. Its is ugly in some ways, but it does allow some things to be split out more cleanly. And again, it's a "only do this if you're very sure" feature, not one for general use.

I was avoiding any changes that required changing documentation (the Ciphertext being an obvious example, and that was rolled back). A better discussion on what the API was would have been good before 1.0, but it didn't happen. The generically named constants, if now officially part of the API, need to be documented with a deprecation warning.

@tarcieri
Cryptosphere member

I think the generically named constants are fine, and don't need to be deprecated.

What I was really suggesting was something along the lines of:

module Crypto
  module SecretBox
    class XSalsaPoly1305
      include SecretBox # if we really care about is_a? working
      ...
    end

    extend Forwardable
    def_delegator :XSalsaPoly1305, :new
  end
end

This will ensure SecretBox continues to work for those who want to use the short names, while also making the algorithm-specific classes accessible.

@stouset

@tarcieri Your approach mandates that the specific implementations be strictly bound to the public API.

I'm not saying that's a dealbreaker, but it is something that needs to be considered. With the parameterized approach, the "parent" SecretBox class can wrap all sorts of generic functionality (for instance, right now, it decodes keys) without the child classes needing to be aware of it. If the implementation-specific classes are intended to be part of the public API, they will also need to perform this work. This can be handled cleanly with inheritance and sprinkled calls to super, but again, it's important to at least point out.

Other than that, I don't have a problem with it. It would be nice to have some way of getting the "string" name of the primitive used, and being able to convert it back and forth between class and string. That way, a library on top of this gem can store 'xsalsa20poly1305', and do something like

Crypto::SecretBox.implementation('xsalsa20poly1305') # => Crypto::SecretBox::XSalsa20Poly1305
@tarcieri
Cryptosphere member

We already have Crypto::Encoder#[] which is quite similar. Classes can easily register themselves as symbols:

https://github.com/cryptosphere/rbnacl/blob/master/lib/rbnacl/encoder.rb#L28

@stouset

That really seems like abusing what that class was originally intended for.

I'm also, as stated earlier, not really a fan of the encoding stuff. That seems like a concern that's external to the library: what you want to do with your bag of bytes is your own call.

@tarcieri
Cryptosphere member

Well yeah, I'm not really a fan of that either, just throwing it out there as a suggestion ;) I'd personally just like for Crypto::SecretBox to seamlessly thunk to Crypto::SecretBox::XSalsa20Poly1305 in the same way the existing C macros do. As for libraries like Krypt and your library that are building bindings to specific algorithms, they can use Crypto::SecretBox::XSalsa20Poly1305. Seems good to me?

I know you're not a fan of the encoding API. It's there as a convenience for people who would like to use RbNaCl directly, and it came in rather handy for allowing the test vectors to be hex. Apparently you prefer an asston of differently-named methods that more or less do the same thing with different formats ;)

FWIW I'm not using the encoding API in my project Keyspace. I have my own capability format that combines multiple keys and I work with the byte API directly. Again, the encoding API completely stays out of your way if you do the simplest thing possible.

@stouset

Opening another issue for the Encoding stuff so we can at least talk about it in a separate area. :)

@tarcieri
Cryptosphere member

My vote would probably be merging this, then I can send up a proposed change in a PR

@namelessjon

I'll re-add in the generic constants, and document the ones people should be using (with the warning re: only appropriate for the default Box construction, might break your assumptions otherwise)

As I said earlier, I like this implementation, "clunky" as it is, since it separates the crypto from the sugar, in a way that also makes it easier to implement new primitives and doesn't change class vs module nature of a constant in the public api ;)

@tarcieri
Cryptosphere member

@namelessjon could leave it as a class, but I personally think it should be a module to use the Forwardable approach.

Code is easier to talk about than handwaving, so I'd say go ahead and merge this, and I can follow up with some suggestions in PR form.

@namelessjon

Just fixing up those constants ;)

namelessjon added some commits Mar 14, 2013
@namelessjon namelessjon Restore NONCEBYTES constant
Though it still isn't documented, in the hope people use the helpful
methods provided.
2d16346
@namelessjon namelessjon Remove last traces of ill-fated Ciphertext class
Now it lives only in the history
8c025d1
@coveralls

Coverage remained the same when pulling 8c025d1 on primitive_specification into 9be368b on master.

View Details

@namelessjon

Right, merging. We can bikeshed around the API some more before I code up support for swapping out our public/private key primitives too.

@namelessjon namelessjon merged commit 83a122e into master Mar 14, 2013

1 check passed

Details default The Travis build passed
@tarcieri
Cryptosphere member

Keyspace tests still pass... woohoo!

@tarcieri tarcieri deleted the primitive_specification branch Mar 15, 2013
@tarcieri tarcieri commented on the diff Mar 15, 2013
lib/rbnacl/box.rb
@@ -74,11 +77,31 @@ class Box
# @raise [Crypto::LengthError] on invalid keys
#
# @return [Crypto::Box] The new Box, ready to use
- def initialize(public_key, private_key, encoding = :raw)
- @public_key = Encoder[encoding].decode(public_key) if public_key
- @private_key = Encoder[encoding].decode(private_key) if private_key
- Util.check_length(@public_key, PublicKey::BYTES, "Public key")
- Util.check_length(@private_key, PrivateKey::BYTES, "Private key")
+ def initialize(public_key, private_key, encoding = :raw, primitive = DEFAULT_PRIMITIVE)
+ public_key = Encoder[encoding].decode(public_key) if public_key
+ private_key = Encoder[encoding].decode(private_key) if private_key
+ @primitive = primitive.new(public_key, private_key)
+ end
+
+ # returns the defaul primitive for the Box class
@tarcieri
Cryptosphere member
tarcieri added a line comment Mar 15, 2013

Uhoh, typo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@namelessjon namelessjon added a commit that referenced this pull request Apr 8, 2013
@namelessjon namelessjon Revert "Merge pull request #46 from cryptosphere/primitive_specificat…
…ion"

This branch probably shouldn't have been merged at the time, as it
combined two seperate problems: that providing information on which
primitives were used, as well as how to specify them.  It also cleaned
up some specs.  So it's going away

This reverts commit 83a122e, reversing
changes made to 9be368b.
9afa235
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment