New Crypto Spec - Githubissues

ably / ably-java

Java, Android, Clojure and Scala client library SDK for Ably realtime messaging service

https://ably.com/download

Apache License 2.0

86 stars 39 forks source link

New Crypto Spec #105

Open mattheworiordan opened 8 years ago

mattheworiordan commented 8 years ago

Please see https://github.com/ably/docs/pull/89

We need to ensure this library complies with the changes described in the PR

┆Issue is synchronized with this Jira Task by Unito

mattheworiordan commented 7 years ago

@trenouf whilst you are in the Crypto stuff, I know what we don't actually adhere to the Crypto spec correctly as there are missing methods / objects according to the 0.8 IDL.

We have had one customer complain about this already and thought that the Java lib was unstable as a result.

Could you specifically check that ChannelOptions, CipherParams and Crypto match the IDL? If there are changes to be made, I suspect there will be no underlying functionality change fortunately.

trenouf commented 7 years ago

Will do.

mattheworiordan commented 7 years ago

Thanks

trenouf commented 7 years ago

ChannelOptions spec points to the java one as a reference! However the field that the spec thinks should be called cipher is instead called cipherParams, and there is another private field called cipher. That could cause confusion.

CipherParams: Instead of a binary field key, it has a SecretKeySpec field keySpec. I guess we could implement the binary field key with some code to notice when it has changed and update the keySpec. It does not have a mode field. It has a ivSpec public field that is not in the spec.

Crypto: Missing the generateRandomKey static method.

paddybyers commented 7 years ago

Instead of a binary field key, it has a SecretKeySpec field keySpec. I guess we could implement the binary field key with some code to notice when it has changed and update the keySpec.

Is the key supposed to be mutable? That certainly wasn't the original intention.

mattheworiordan commented 7 years ago

From what I can tell:

getDefaultParams does not conform, see http://docs.ably.io/client-lib-development-guide/features/#RSE1b, it should take a Hashmap or language equivalent so that any subset of CipherParams fields that includes a key can be provided. That needs to be fixed.
I agree it should be key not keySpec. On @paddybyers's point I am not sure it needs to be mutable, that remains unspecified in the spec. Why can it not simply be provided as binary and remain as binary as an attribute?
ChannelOptions#withCipherKey is missing, and is in fact used in our documentation, see https://www.ably.io/documentation/realtime/encryption. So that definitely needs fixing.

I think we should definitely get this to conform to the spec given our documentation assumes this functionality exists.

paddybyers commented 7 years ago

I am not sure it needs to be mutable, that remains unspecified in the spec. Why can it not simply be provided as binary and remain as binary as an attribute?

Well the original idea was that concrete subtypes of CipherParams would have the implementation- or platform-specific realisation of the given params, even if they are created using portably-defined arguments.

However, we have a keySpec in a Cipher object, so there's no need for it to be constructed in the CipherParams.

it should take a Hashmap or language equivalent so that any subset of CipherParams fields that includes a key can be provided.

That's not very java-esque but I suppose it would be a Properties.