CipherParam defaults

[mattheworiordan]

When creating an encrypted channel in Ruby, for example, you can simply do:

channel = channels.get('secret', encrypted: true, cipherParams: { key: 'very secret' })

This does not require one to explicitly get the default cipherParams because the channel itself will fill in the defaults where missing.

However, with ably-js this is not possible, and you have to do the following:

Crypto.getDefaultParams(function(err, params) {
  params.key = 'very secret';
  channel = channels.get('secret', { encrypted:true, cipherParams: params });
});

I think it would make life a lot easier for our customers if we supported something more akin to the Ruby style:

channel = channels.get('secret', { encrypted: true, cipherParams: { key: 'very secret' } });

[tcard]

Plus, it's documented that way: http://docs.ably.io/realtime/channels-messages/#channel-options

[paddybyers]

The reason that it's async is that in general getting crypto-quality random data is async, so in the case that you don't specify a key it needs to be async. I don't know the best way to reconcile that with a sync API in the case that you do specify a key.

[mattheworiordan]

I am not sure why being async or sync is relevant. The Channel object, if necessary, can issue an async request and set the options post the channel being created. Given this will happen quicker than the channel attaching in pretty much all cases, or even we could defer the attach request until that point, it should work just fine no?

[SimonWoolf]

PR: https://github.com/ably/ably-js/pull/236

One thing -- if we're making it easy for people to specify a key directly, there are going to be a bunch of people who don't realise that the key has to be precisely 16 or 32 chars long, and then have things fail silently when it isn't. (People like... me, for the last half hour :( ). For the 0.9 spec, should we consider an easy encryption mode where we hash the provided key to the required length? (Or as a stopgap, throw an exception if given a key that fails this?)

[paddybyers]

throw an exception if given a key that fails this?

we should certainly do this.

we hash the provided key to the required length?

that would require that we implement an identical procedure in each library and I'm not sure that makes things any better. The hash would lose entropy so you'd end up with a weaker key than the one you started with. Everyone knows how to generate a random byte array of a given length.

[SimonWoolf]

I don't know the best way to reconcile that with a sync API in the case that you do specify a key.

@paddybyers to the extent that that's a problem, it's one we already had, since you could already do channels.get("channel", {encrypted: true}). This PR just lets you specify a key in the same options hash, which at least doesn't make things any worse.

[SimonWoolf]

The hash would lose entropy so you'd end up with a weaker key than the one you started with

I don't understand. Hashing won't increase the entropy, sure -- if you take an 8 bit key and hash it to 256 bits, it'll still only have 8 bits of entropy -- but surely it won't significantly reduce it (except in the case that the provided key is > 256 bits, in which case it'll reduce the entropy to 256 bits, which is expected)? Isn't that the whole point of universal hash functions?

Edit: OK, so in the case of a hashing a 256-bit input to a 256-bit output, you do lose slightly under one bit of entropy due to collision probability, so you end up with ~255 bits - http://crypto.stackexchange.com/a/10405. I'm not sure we should worry about that too much.

[mattheworiordan]

if we're making it easy for people to specify a key directly, there are going to be a bunch of people who don't realise that the key has to be precisely 16 or 32 chars long

Am I missing something, using a key length other than 16 or 32 seems to work just fine for me, https://jsbin.ably.io/odabuj/3/edit?

[SimonWoolf]

Am I missing something, using a key length other than 16 or 32 seems to work just fine for me, https://jsbin.ably.io/odabuj/3/edit?

So: currently, getDefaultParams, which takes a key as the first parameter, calculates the keyLength. But in your example you call getDefaultParams with no key, so it generates a default key with a keylength of 128, and sets the keyLength to 128. You then override that with 'ASDA'. CBCCipher then gives that straight to CryptoJS. I'm guessing CryptoJS pads it or something to bring it up to the next largest supported keylength.

See https://jsbin.ably.io/odabuj/5/ for a fixed (i.e. broken) jsbin.

This is pretty suboptimal all round :/

[mattheworiordan]

This is pretty suboptimal all round :/

Agreed, could you guys discuss on Monday to work out a "friendlier" API for Crypto as it feels like it's fraught with potential traps for people using this API.

[mattheworiordan]

Also, while we are at it, we should definitely review the Java API too because it does not seem to adhere to the spec in this regard and wonder why not. For example, see the attributes of a Java CipherParams object, and the expected attributes (based on the spec) of a JS CipherParams object

[mattheworiordan]

Sorry, one last note, I think for simplicity we should always be allowing a string key and ideally keys that are not just 128 bit or 256 bits. I realise this is not necessarily ideal as we may truncate the provided key or pad the key as needed, but I think it's important to keep the API simple for examples like:

var options = { encrypted: true, cipherParams: { key: 'A__SECRET__KEY__' } };
var channel = realtime.channels.get('channelName', options);

[paddybyers]

I think for simplicity we should always be allowing a string key and ideally keys that are not just 128 bit or 256 bits.

We have algorithms that take 128 or 256 bit keys. We don't have other algorithms to choose from. So you're perhaps talking about convenience methods for creating a key, but we can't describe an arbitrary length string as a key.

[SimonWoolf]

I think for simplicity we should always be allowing a string key and ideally keys that are not just 128 bit or 256 bits. I realise this is not necessarily ideal as we may truncate the provided key or pad the key as needed,

If we want to let users use effectively a password rather than key, then instead of trying to hammer the one into the other by padding or truncating them to a required bit-length, we should probably be using a key derivation function, that's what they're for. PBKDF2, bcrypt, or scrypt, whichever's supported by all the crypto libraries the client libs are using.

Probably with a different property (ie user specifies either {key: ...}, which is used as-is and must be 128 or 256 bits, or {password: ...}, which is passed through PBKDF2 and can be a string of any length) - we shouldn't conflate the two

[mattheworiordan]

We have algorithms that take 128 or 256 bit keys. We don't have other algorithms to choose from.

I do appreciate that and understand the technical constraints we have imposed by the algorithms.

So you're perhaps talking about convenience methods for creating a key

Yes, that is where I am headed.

For example, in order to perhaps not change the API and still require that the key matches the key length of the algorithm, could we not provide a helper function to generate a SHA 128 or 256 from the provided password? This builds on @SimonWoolf's suggestion of course.

For example, this generates a SHA 256 (a 32 byte array in hex) (we support SHA256 already in the lib)

echo -n "matt's silly password" | shasum -a 256
2ec1143552bc7f682fd62323d94e7e2842c1cac2e6ad687fd0ba3588f885196d

So perhaps we offer an API like this:

key = Ably.Realtime.Crypto.deriveKey("matt's silly password");
channel = realtime.channels.get({ encryption: true, cipherParams: { key: key });

// or a shorthand version
channel = realtime.channels.get({ encryption: true, cipherParams: { deriveKey: "matt's silly password" });

Which does make me wonder why we even need to specify encryption: true. Surely we should simply state that if cipherParams is provided, then encryption is assumed in all cases? Why the additional param?

[paddybyers]

Moved to private discussion.

[SimonWoolf]

Fixed by #240 per ably/docs#89