Must support specifying / introspecting on primitives

[stouset]

Right now, all the APIs in this library (that I can tell) simply defer to the default implementation in libsodium. This is dangerous, and RbNaCl needs to expose some way to both identify primitives used in an operation and specify that an alternative be used.

For instance, with the crypto_secretbox API, every function is actually a #define to crypto_secretbox_xsalsa20poly1305. What I'm asking for is:

• calls to Crypto::SecretBox#box default to using crypto_secretbox
• calls to Crypto::SecretBox#box indicate what low-level primitive was used
• calls to Crypto::SecretBox#open can specify to use crypto_secretbox_xsalsa20poly1305

(note: Crypto::SecretBox is being used as an example; this also applies to Crypto::Box, Crypto::Auth, et al)

It's important that these features are available immediately. Imagine that tomorrow, XSalsa20Poly1305 is broken. A patch is released to nacl/libsodium to implement crypto_secretbox_aes_256_gcm and to define crypto_secretbox using this primitive. How does a current user of RbNaCl migrate their existing data to a non-broken version?

Even without a complete break, imagine any situation where libsodium changes the default primitive. What happens if a user upgrades libsodium now that the default has changed? RbNacl will start using the new primitive transparently, and users will have a mix of data encrypted with the old and new algorithms, with no way of detecting which ciphertexts were encrypted with which algorithm (other than through trial decryption).

Scenarios like this are exactly why libsodium and nacl make this type of information available to callers. And why RbNaCl must support it as well. RbNaCl doesn't have to make the process transparent, but it must make it possible.

[tarcieri]

Right now, all the APIs in this library (that I can tell) simply defer to the default implementation in libsodium

This is not the case. The API for "default implementations" is provided in NaCl as a set of macros, therefore the default implementations are not a part of NaCl's ABI at all. Since we're using FFI, we have no choice to use the ABI, so for, example, SecretBox, we bind directly to crypto_secretbox_xsalsa20poly1305. You can see that happening here:

https://github.com/cryptosphere/rbnacl/blob/master/lib/rbnacl/nacl.rb#L72

It's important that these features are available immediately. Imagine that tomorrow, XSalsa20Poly1305 is broken. A patch is released to nacl/libsodium to implement crypto_secretbox_aes_256_gcm and to define crypto_secretbox using this primitive. How does a current user of RbNaCl migrate their existing data to a non-broken version?

I agree there should be an API exposed in Ruby to select different algorithms. I can imagine something like SecretBox taking an options hash:

Crypto::SecretBox.new(secret_key, :encoding => :hex, :algorithm => :xsalsa20poly1305)

[stouset]

Ah, you're right about the FFI approach. It wasn't obvious that was the case from the Crypto::SecretBox code itself.

I'd also recommend primitive over algorithm, since it's closer to the typical nomenclature.

[namelessjon]

I'd say specifying the primitive(s) is completely at odds with the design goal of NaCl, which is to remove such choices from the developer. Your hypothetical situation also breaks libsodium. And NaCl itself. Assuming you're using the recommended approach of calling crypto_secretbox or whatever in your C code, you will also suffer from this transparent break problem, since that method also just returns a bunch of bytes.

The fix needs to happen down there, which you noted, but I think it needs to happen there first. I'd rather have the ruby code rely on a standard NaCl or libsodium functionality, such that its possible to talk between C and ruby, or ruby and python, instead of relying on the developer making correct choices.

[namelessjon]

Possibly all the strings returned need an extra byte or two to specify primitive(s)? Though that then probably opens you up to session downgrade attacks.

[namelessjon]

Also, instead of the options hash idea, maybe draw on the C api, maybe go for:

class Crypto::XSalsa20Poly1305Box
end

Crypto::SecretBox = Crypto::XSalsa20Poly1305Box

That way it makes it really obvious you're doing something different when you instantiate a Crypto::AESGCM256Box, instead of 'just' passing in an options hash.

[tarcieri]

Separate classes for each algorithm also makes sense to me, although I'd still be fine with passing something into an options hash

[stouset]

This seems easier:

class Crypto::SecretBox
  def initialize(key, primitive: NaCl::SecretBox::XSalsa20Poly1305)
    ...
  end

  def open(...)
    ...
    self.primitive.open(...)
    ...
  end
end

[stouset]

@namelessjon Specifying primitives does not go against the spirit of NaCl. NaCl explicitly provides a way to specify primitives. My situation doesn't break libsodium or NaCl: they both provide mechanisms to determine which primitive is being used, and to choose which primitive to use.

Like I said, it doesn't have to be seamless. It just has to be possible.

[namelessjon]

@stouset Okay, yes they do via #define, even if you're using the recommended crypto_box style of the API. Sorry for talking shit. I can wish they did so in a more compact manner than a 20-odd byte string (to encourage a standard and compact way of specifying cross-language), but that is something I might file with libsodium.

I should have some time to turn one of these implementation ideas into code later today. Your suggestion is nice, though I don't think restricting this to ruby 2.0 would be useful at this point.

[namelessjon]

There, a first stab at this for the SecretBox class. At the moment, it's 100% transparent from the API user's PoV.
Access to which primitive is being used is via secret_box.primitive.class, which should probably be improved.

[stouset]

Brilliant!

[tarcieri]

I think a better name would be Crypto::SecretBox::XSalsa20Poly1305. This looks nicer and more closely matches the NaCl API.

[stouset]

Agreed with @tarcieri on that. The extra Box on the end is superfluous, and removes an otherwise happy union with the NaCl API.

[namelessjon]

Fair enough. Not emotionally wedded to the name I picked. Will update and continue

[tarcieri]

I think it would make sense to use an options hash here:

94d116c...14c6400#L4R36

And do:

Crypto::SecretBox.new('A' * 32, :primitive => ...)

[namelessjon]

This is when I really wish that ruby 2.0 would transparently support keyword args for methods with default parameters, because then I could just say "Nah, just upgrade to 2.0". Not sure it needs an options hash. SecretBox and/or Box shouldn't pick up any more responsibilities. I'll see what I think when I've moved Box over.

[tarcieri]

I don't think it makes sense that the user needs to specify an encoding in order to specify a primitive type.

Also I'm confused about your concerns with keyword args in Ruby 2.0. I'm pretty sure you can do that, but it'd break compatibility with earlier Rubies.

[stouset]

@tarcieri I'm not actually a big fan of passing the key encoding into constructors like this. It unnecessarily complicates the API, and puts the responsibility of decoding keys into every part of the API.

Keys should be bytes. If you want to encode them as a user of the API, that's your responsibility.

Once the work has been done in RbNaCl to allow for selecting primitives, I'm going to port my extremely-high-level crypto library to work on top of RbNaCl instead of NaCl directly. The approach it takes:

# key objects contain the length (e.g., 256), low-level primitive (e.g.,
# xsals20poly1305), high-level use-case (e.g., crypto_secretbox), and the
# bytes themselves
key = Cryptography::Vault.key                                                                                                                                                                                                                 

# the key object can be encoded as a raw byte stream, Base64, Base32, JSON,
# whatever. Some of the formats (e.g., json, hash, etc.) directly expose the
# components of the key described above
key.to_bytes
key.to_base64
key.to_base32
key.to_hash
key.to_json

# keys can be parsed from their encoded formats
Cryptography::Key.from_bytes(bytes)
Cryptography::Key.from_base64(base64)
...

Point being, that's one of the benefits of representing things like keys as objects rather than raw strings. But for something like RbNaCl, I would try to avoid incorporating this kind of encoding logic.

[tarcieri]

@stouset this is an approach I like because it's rather convenient for people who do want to use the API directly, and a pattern I've seen in numerous other crypto libraries

You are of course free to pass bytes in directly as that's the default. Simply doing the simplest thing possible gives you the behavior you want.

[tarcieri]

@namelessjon I'm concerned the Ciphertext objects break the existing API (which has already been released as 1.0.0)

Per semantic versioning, to do that, we'd have to release version 2.0.0

[namelessjon]

@tarcieri While it does in terms of the concrete type returned, in terms of behaviour, it didn't require any spec changes (they were all for other things). In exchange for that, you get some more functionality in terms of both encoding strings and access to the primitive it corresponds too. Possibly it should duck-type slightly better to String, but actually subclassing String seemed like overkill.

[grantr]

I think object.is_a?(String) is a common idiom, so it would be nice to support that. I haven't thought of any specific cases where that would be useful with rbnacl though.

[namelessjon]

@grantr It is very common, and I suppose for that reason I should probably subclass String in 1.0. Though that would mean ditching the encoding stuff, which could be handy.

[tarcieri]

You could make Ciphertext a SimpleDelegator instead of subclassing String too

[namelessjon]

Never used SimpleDelegator before, so for that reason alone I will at least give it a try. Though wouldn't that leave the same problem as Ciphertext not being a String from the #is_a? PoV

I can also just remove Ciphertext. And either way, it has just occurred to me that the name should probably change anyway, to allow for it to also represent e.g. the output of an auth token, which also might want to report the primivite(s) used to generate it.

[stouset]

@namelessjon I'd just remove Ciphertext. Since the primitive used is already completely determined by the input to the methods, that's good enough. A user simply needs to be able to know which algorithm is being used, and that satisfies it.