Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Split ChannelCipher implementation into encrypt and decrypt specialisms #746

Merged
merged 14 commits into from
Feb 3, 2022
Merged

Split ChannelCipher implementation into encrypt and decrypt specialisms #746

merged 14 commits into from
Feb 3, 2022

Conversation

QuintinWillison
Copy link
Contributor

@QuintinWillison QuintinWillison commented Feb 1, 2022 •
edited
Loading

I've made the cipher get method on ChannelOptions safe to be called from any thread.

I've also added a lightweight protection to each cipher implementation to fail early with a ConcurrentModificationException if the cipher is asked to operate from multiple threads concurrently.

This refactor ended up being a little larger than I had originally anticipated, but I feel that this area of the codebase is now clearer to follow and reason about. There is more I could have tackled but it fell outside of the scope of solving this particular grey area.

Fixes #741.

@QuintinWillison
Split ChannelCipher implementation into encrypt and decrypt specialisms.
cb73f1e 
I've also made the cipher get methods on ChannelOptions safe to be called from any thread.
@QuintinWillison QuintinWillison self-assigned this Feb 1, 2022
@QuintinWillison QuintinWillison mentioned this pull request Feb 1, 2022
Closed
@QuintinWillison
Correct grammar - this is a channel options instance, not a channel i…
939d165 
…nstance.
@QuintinWillison
Merge branch 'main' into feature/channel-cipher-refactor
2d0334b
@QuintinWillison
Add checks to detect attempts to operate a channel cipher from more t…
b8c8bef 
…han one thread.

I could have gone down the route of simplistic addition of locks to the encrypt and decrypt methods, but that would just have been a sticky plaster, probably hiding deeper routed architectural issues elsewhere in this codebase.
By adding this lightweight check, we will see specific evidence that this particular problem is the fault if customers encounter this.
@QuintinWillison
Merge branch 'main' into feature/channel-cipher-refactor
a7ae369
@QuintinWillison
Pass expected cipher params instance, not channel options, when obtai…
73ca5a9 
…ning the cipher from crypto implementation.

A mistake made by me in my refactor.
@QuintinWillison
Add channel options consistency check on getting ciphers.
0b94a36
@QuintinWillison
Merge branch 'main' into feature/channel-cipher-refactor
2717221
@QuintinWillison
Copy link
Contributor Author

I think the failure I'm currently seeing is 'my fault', being something new since the changes I made... however, I suspect that it may be related to a problem we recently saw elsewhere: ably/ably-cocoa#1266

I'll iterate further on this tomorrow.

Current failing run:

-> Test crypto_publish[text_protocol](io.ably.lib.test.rest.RestCryptoTest)

io.ably.lib.test.rest.RestSuite > io.ably.lib.test.rest.RestCryptoTest.crypto_publish[text_protocol] FAILED
    java.lang.AssertionError: expected:<This is a string message payload> but was:<[B@165d1149>
        at org.junit.Assert.fail(Assert.java:88)
        at org.junit.Assert.failNotEquals(Assert.java:834)
        at org.junit.Assert.assertEquals(Assert.java:118)
        at org.junit.Assert.assertEquals(Assert.java:144)
        at io.ably.lib.test.rest.RestCryptoTest.crypto_publish(RestCryptoTest.java:70)
-> Test crypto_publish_key_mismatch[text_protocol](io.ably.lib.test.rest.RestCryptoTest)
-> Test crypto_send_encrypted_unhandled[text_protocol](io.ably.lib.test.rest.RestCryptoTest)
-> Test crypto_publish_256[text_protocol](io.ably.lib.test.rest.RestCryptoTest)

io.ably.lib.test.rest.RestSuite > io.ably.lib.test.rest.RestCryptoTest.crypto_publish_256[text_protocol] FAILED
    java.lang.AssertionError: expected:<This is a string message payload> but was:<[B@168484e3>
        at org.junit.Assert.fail(Assert.java:88)
        at org.junit.Assert.failNotEquals(Assert.java:834)
        at org.junit.Assert.assertEquals(Assert.java:118)
        at org.junit.Assert.assertEquals(Assert.java:144)
        at io.ably.lib.test.rest.RestCryptoTest.crypto_publish_256(RestCryptoTest.java:107)
-> Test crypto_publish_alt[text_protocol](io.ably.lib.test.rest.RestCryptoTest)

io.ably.lib.test.rest.RestSuite > io.ably.lib.test.rest.RestCryptoTest.crypto_publish_alt[text_protocol] FAILED
    java.lang.AssertionError: expected:<This is a string message payload> but was:<[B@184c6913>
        at org.junit.Assert.fail(Assert.java:88)
        at org.junit.Assert.failNotEquals(Assert.java:834)
        at org.junit.Assert.assertEquals(Assert.java:118)
        at org.junit.Assert.assertEquals(Assert.java:144)
        at io.ably.lib.test.rest.RestCryptoTest.crypto_publish_alt(RestCryptoTest.java:146)
-> Test crypto_send_unencrypted[text_protocol](io.ably.lib.test.rest.RestCryptoTest)
-> Test crypto_publish[binary_protocol](io.ably.lib.test.rest.RestCryptoTest)

io.ably.lib.test.rest.RestSuite > io.ably.lib.test.rest.RestCryptoTest.crypto_publish[binary_protocol] FAILED
    java.lang.AssertionError: expected:<This is a string message payload> but was:<[B@3e05ad8e>
        at org.junit.Assert.fail(Assert.java:88)
        at org.junit.Assert.failNotEquals(Assert.java:834)
        at org.junit.Assert.assertEquals(Assert.java:118)
        at org.junit.Assert.assertEquals(Assert.java:144)
        at io.ably.lib.test.rest.RestCryptoTest.crypto_publish(RestCryptoTest.java:70)
-> Test crypto_publish_key_mismatch[binary_protocol](io.ably.lib.test.rest.RestCryptoTest)
-> Test crypto_send_encrypted_unhandled[binary_protocol](io.ably.lib.test.rest.RestCryptoTest)
-> Test crypto_publish_256[binary_protocol](io.ably.lib.test.rest.RestCryptoTest)

io.ably.lib.test.rest.RestSuite > io.ably.lib.test.rest.RestCryptoTest.crypto_publish_256[binary_protocol] FAILED
    java.lang.AssertionError: expected:<This is a string message payload> but was:<[B@3ec592b7>
        at org.junit.Assert.fail(Assert.java:88)
        at org.junit.Assert.failNotEquals(Assert.java:834)
        at org.junit.Assert.assertEquals(Assert.java:118)
        at org.junit.Assert.assertEquals(Assert.java:144)
        at io.ably.lib.test.rest.RestCryptoTest.crypto_publish_256(RestCryptoTest.java:107)
-> Test crypto_publish_alt[binary_protocol](io.ably.lib.test.rest.RestCryptoTest)

io.ably.lib.test.rest.RestSuite > io.ably.lib.test.rest.RestCryptoTest.crypto_publish_alt[binary_protocol] FAILED
    java.lang.AssertionError: expected:<This is a string message payload> but was:<[B@6ab5cbf5>
        at org.junit.Assert.fail(Assert.java:88)
        at org.junit.Assert.failNotEquals(Assert.java:834)
        at org.junit.Assert.assertEquals(Assert.java:118)
        at org.junit.Assert.assertEquals(Assert.java:144)
        at io.ably.lib.test.rest.RestCryptoTest.crypto_publish_alt(RestCryptoTest.java:146)

@QuintinWillison
Return encrypting and decrypting ciphers as a set from the Crypto imp…
0bd270c 
…lementation.

This was the reason for the failing tests, where the tests were using 'default cipher params' which means a randomly generated encryption key (RSE1).
Having split the encipher and decipher apart, I had broken the implicit linkage between the two for that scenario (i.e. from ChannelOptions where cipherParams was null).
@QuintinWillison QuintinWillison marked this pull request as ready for review February 2, 2022 20:23
ikbalkaya
ikbalkaya suggested changes Feb 3, 2022
View reviewed changes
Copy link
Contributor

@ikbalkaya ikbalkaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I was going through code, I wasn't sure whether we should explicitly say that a method will throw ConcurrentModificationException, but I can see that this is actually thrown when another thread tries to acquire access. So it might as well be a good idea to keep it. I am going to request change in order to clarify this.

lib/src/main/java/io/ably/lib/util/Crypto.java Show resolved Hide resolved
@QuintinWillison
Expand method commentaries to make lack of thread safety clearer.
4cf075e
@QuintinWillison
Remove unused method.
f36bd3e 
I had forgotten that I had moved this into the single method that was using it.
ikbalkaya
ikbalkaya approved these changes Feb 3, 2022
View reviewed changes
Copy link
Contributor

@ikbalkaya ikbalkaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

KacperKluka
KacperKluka suggested changes Feb 3, 2022
View reviewed changes
Copy link
Contributor

@KacperKluka KacperKluka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good but we can improve it further 😉 I love the fact that you've applied the interface segregation principle and created multiple smaller interfaces instead of the big one 😁

lib/src/main/java/io/ably/lib/util/Crypto.java Show resolved Hide resolved
lib/src/main/java/io/ably/lib/types/ChannelOptions.java Show resolved Hide resolved
@QuintinWillison QuintinWillison marked this pull request as draft February 3, 2022 12:13
@QuintinWillison QuintinWillison removed the request for review from paddybyers February 3, 2022 12:13
@QuintinWillison
Restore the getCipher Crypto method and the original purpose of the C…
546b713 
…hannelCipher interface, deprecating them.

There might have been customers using this method, as it was technically public API even though it was annotated (in plain text) as 'Internal'.
@QuintinWillison
Solve the public API in the right place!
3391783 
It was ChannelOptions' getCipher() which potentially needed legacy support.
@QuintinWillison QuintinWillison marked this pull request as ready for review February 3, 2022 13:24
KacperKluka
KacperKluka approved these changes Feb 3, 2022
View reviewed changes
Copy link
Contributor

@KacperKluka KacperKluka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

ikbalkaya
ikbalkaya approved these changes Feb 3, 2022
View reviewed changes
Copy link
Contributor

@ikbalkaya ikbalkaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@QuintinWillison QuintinWillison merged commit 58e5a4c into main Feb 3, 2022
@QuintinWillison QuintinWillison deleted the feature/channel-cipher-refactor branch February 3, 2022 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KacperKluka KacperKluka KacperKluka approved these changes

@ikbalkaya ikbalkaya ikbalkaya approved these changes

@paddybyers paddybyers Awaiting requested review from paddybyers

@Peter-Maguire Peter-Maguire Awaiting requested review from Peter-Maguire

Assignees

@QuintinWillison QuintinWillison

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

IllegalStateException in Crypto CBCCipher's decrypt method
3 participants
@QuintinWillison @ikbalkaya @KacperKluka