CipherParams conform to latest spec (separate algorithm and keyLength)

[SimonWoolf]

Per http://docs.ably.io/client-lib-development-guide/features/. (Also makes it align better with cipher params options on the fixtures, which all have separate algorithm and keylength entries. (though annoyingly they use keylength rather than keyLength, so aren't quite valid CipherParams objects.))

@paddybyers what's the logic behind cipherParams being required to be an instance of a CipherParams object (with no behaviour) when eg tokenParams can be a plain object?

Also I'm a bit puzzled by the error thrown when it isn't a CipherParams -- "ChannelOptions not supported". It's true that we don't yet support passing channelOptions when getting a channel (you need to call setOptions), but isn't that independent of what kind of object cipherParams should be?

Edit: also, for some reason cipherParams created in a test using new Crypto.CipherParams are failing the params instanceof CipherParams test in nodejs. (Though working as expected in a browser).

[paddybyers]

I think the spec is missing the point a bit of the CipherParams type, because the idea is that the params are opaque - each different algorithm and mode might require a different set of params. We can specify reasonably portably the params that you'd get in a CipherParams for CBC, say, but even then the contents of the CipherParams object won't be comparable: in JS you'll have a Buffer for the key, whereas in Java you'll have a different KeySpec type.

So this is why we've got getDefaultParams() - ie all CipherParams instances are supplied by the library, except when there is extra algorithm/mode-specific knowledge shared between the client app and the library in a given language.

So we're confusing the portability of the arguments you might supply to get a CipherParams instance - and we only define the mode string and key length arguments portably, and define the additional arguments portably for one specific mode.

I agree that the fixtures should have keyLength instead of keylength.

[paddybyers]

I'm fine with merging this PR but in the light of the comment above do you think anything needs to change in the spec?

[SimonWoolf]

In that case, possible spec tweak:

(TZ1) params to configure encryption for a channel, see Java CipherParams class as a reference
(TZ2) The attributes of CipherParams consist of anything necessary to implement the supported algorithms (eg key and iv), in addition to the following standardised attributes:
    (TZ2a) algorithm string – Default is 'aes'. Optionally specify the algorithm to use for encryption, currently only AES is supported
    (TZ2b) keyLength integer – Default is 128. Optionally specify the key length of the cipher, typically 128 or 256
    (TZ2c) mode string – Default is 'cbc'. Optionally specify cipher mode, currently only CBC is supported
(TZ3) Crypto#getDefaultParams function
    (TZ3a) returns a complete CipherParams instance with randomly generated key and iv
    (TZ3b) takes an optional key parameter, returning a CipherParams instance created with that key and with a keyLength calculated from that key

Still not very happy with the CipherParams atm -- https://github.com/ably/ably-js/blob/master/browser/lib/util/crypto.js#L100 says people may instance a CipherParams directly and populate it themselves, but that's currently broken in node (params instanceof CipherParams is returning false, even though params.constructor is indeed CipherParams). Should we possibly make that a ducktyped check and change the error message to something more useful, something like 79c11e2 ?

[paddybyers]

params instanceof CipherParams is returning false, even though params.constructor is indeed CipherParams

Yes, I think that's a consequence of how we're aggregating the different contexts into the node lib.

I think that whole instanceof thing is unnecessary because the Cipher constructor should be the thing that decides whether or not the given params is ok. Also the ChannelOptions not supported thing is broken. I think the whole check should be removed.

Re your spec changes: keylength defaults are algorithm-specific. 128 is the default for AES. It's not true that keyLength is meaningful for all algorithms, but it's nearly always meaningful so I'm ok to keep it as a standard property.

[SimonWoolf]

I think the whole check should be removed.

Cool, added that to the PR.

[paddybyers]

Thanks.

Will you raise a docs issue for that spec question?

[SimonWoolf]

Will you raise a docs issue for that spec question?

ably/docs#43