Adjust Crypto, CipherParams and ChannelOptions to spec. #231
Conversation
|
After a conversation with @paddybyers and @mattheworiordan, it seems like the iOS library is doing this pretty wrong. The IV is supposed to be randomly generated in every message (it's currently being set when instantiating the channel only), and it's communicated as the first block in every encrypted message data. Java already does this, could take a look there. This PR can be merged for now, but the IV param will be gone in a followup. |
|
Sure, looks like it can be merged. Equally, not difficult to add another commit onto this PR to address |
tcard
force-pushed
the
fix-crypto-api
branch
from
38c1029 to
d68fc8d
Compare
|
@mattheworiordan Done. Turns out it was actually doing that already, so I've only added a test in the end. I also checked with another library to confirm that the implementations are compatible. |
tcard
force-pushed
the
fix-crypto-api
branch
2 times, most recently
from
a4382cd to
a3c4208
Compare
|
LGTM |
| @@ -1297,6 +1299,66 @@ class RealtimeClientChannel: QuickSpec { | |||
|
|
|||
| } | |||
|
|
|||
| context("crypto") { | |||
| it("if configured for encryption, channels encrypt and decrypt messages' data") { | |||
There was a problem hiding this comment.
I like this test, however I am concerned that at no point are we actually checking that the message is encrypted i.e. if you removed the cipher options completely this test would still pass. A simple way to make sure this test is actually checking for encrypted messages is to add another subscriber that has encryption disabled and check that the messages are received with an encrypted payload i.e. data should be different and encoding should indicate it's an encrypted payload
There was a problem hiding this comment.
Right now that wouldn't work, because that behavior is not working properly (see #211). Can I just intercept the raw ProtocolMessage and check that the data is encoded and the encoding is set there?
There was a problem hiding this comment.
Ok sure intercept is fine
|
API is about to change, discussion at ably/docs#89 (comment). Once that's settled I make the changes here. |
tcard
force-pushed
the
fix-crypto-api
branch
from
e4e0667 to
a53c194
Compare
|
@tcard this looks good, but can you confirm that the |
@mattheworiordan Fixed at 2d6e9b7. I realized along the way that the IV was being chained between messages when encrypting, I thought I checked this already but apparently I only did so for decryption, which was right. So I had to fix that. Now CipherParams only has an IV for testing. If set, |
|
LGTM |
1 similar comment
|
LGTM |
When decrypting, IV is being extracted from the beginning of the message's data blob. When encrypting, the IV should be random, and there's no point in taking it as an argument except for testing. I've added and end-to-end test that sends and receives encrypted messages. I've also run it with ably/ably-js as sender and receiver and it works.
tcard
force-pushed
the
fix-crypto-api
branch
from
2d6e9b7 to
c377cae
Compare
Adjust Crypto, CipherParams and ChannelOptions to spec.
#211 (comment)
I've kept the IV param in Crypto.getDefaultParams for the reasons mentioned in the discussion.