Skip to content

Commit

Permalink
Merge pull request #969 from ably/fix-ably-concurrent-encryption
Browse files Browse the repository at this point in the history 
fix: create Cipher instance in place, do not store it in `ChannelOptions`
  • Loading branch information
@ttypic
ttypic authored Sep 28, 2023
2 parents a38b026 + 8e84a14 commit 05c986b
Show file tree
Hide file tree
Showing 8 changed files with 147 additions and 170 deletions.
19 changes: 19 additions & 0 deletions lib/src/main/java/io/ably/lib/rest/DeviceDetails.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,9 @@ public boolean equals(Object o) {
thisJson.remove("deviceSecret");
otherJson.remove("deviceSecret");

normalizeRecipientField(thisJson);
normalizeRecipientField(otherJson);

if ((this.metadata == null || this.metadata.entrySet().isEmpty()) && (other.metadata == null || other.metadata.entrySet().isEmpty())) {
// Empty metadata == null metadata.
thisJson.remove("metadata");
Expand Down Expand Up @@ -170,4 +173,20 @@ public DeviceDetails fromJsonElement(JsonElement e) {
public static HttpCore.ResponseHandler<DeviceDetails> httpResponseHandler = new Serialisation.HttpResponseHandler<DeviceDetails>(DeviceDetails.class, fromJsonElement);

public static HttpCore.BodyHandler<DeviceDetails> httpBodyHandler = new Serialisation.HttpBodyHandler<DeviceDetails>(DeviceDetails[].class, fromJsonElement);

/**
* Push recipient can contain some additional field, but `transportType`, `deviceToken`, `registrationToken` only matters for equals
*/
private static void normalizeRecipientField(JsonObject deviceDetailsJson) {
JsonElement push = deviceDetailsJson.get("push");
if (push == null) return;
JsonElement recipient = push.getAsJsonObject().get("recipient");
if (recipient == null) return;
JsonObject normalizedRecipient = JsonUtils.object()
.add("transportType", recipient.getAsJsonObject().get("transportType"))
.add("deviceToken", recipient.getAsJsonObject().get("deviceToken"))
.add("registrationToken", recipient.getAsJsonObject().get("registrationToken"))
.toJson();
push.getAsJsonObject().add("recipient", normalizedRecipient);
}
}
7 changes: 5 additions & 2 deletions lib/src/main/java/io/ably/lib/types/BaseMessage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
import com.google.gson.JsonParseException;
import com.google.gson.JsonPrimitive;
import io.ably.lib.util.Base64Coder;
import io.ably.lib.util.Crypto;
import io.ably.lib.util.Crypto.EncryptingChannelCipher;
import io.ably.lib.util.Crypto.DecryptingChannelCipher;
import io.ably.lib.util.Log;
import io.ably.lib.util.Serialisation;
import org.msgpack.core.MessageFormat;
Expand Down Expand Up @@ -145,7 +147,8 @@ public void decode(ChannelOptions opts, DecodingContext context) throws Message
case "cipher":
if(opts != null && opts.encrypted) {
try {
data = opts.getCipherSet().getDecipher().decrypt((byte[]) data);
DecryptingChannelCipher cipher = Crypto.createChannelDecipher(opts.getCipherParamsOrDefault());
data = cipher.decrypt((byte[]) data);
} catch(AblyException e) {
throw MessageDecodeException.fromDescription(e.errorInfo.message);
}
Expand Down Expand Up @@ -193,7 +196,7 @@ public void encode(ChannelOptions opts) throws AblyException {
}
}
if (opts != null && opts.encrypted) {
EncryptingChannelCipher cipher = opts.getCipherSet().getEncipher();
EncryptingChannelCipher cipher = Crypto.createChannelEncipher(opts.getCipherParamsOrDefault());
data = cipher.encrypt((byte[]) data);
encoding = ((encoding == null) ? "" : encoding + "/") + "cipher+" + cipher.getAlgorithm();
}
Expand Down
68 changes: 12 additions & 56 deletions lib/src/main/java/io/ably/lib/types/ChannelOptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@

import io.ably.lib.util.Base64Coder;
import io.ably.lib.util.Crypto;
import io.ably.lib.util.Crypto.ChannelCipher;
import io.ably.lib.util.Crypto.ChannelCipherSet;
import io.ably.lib.util.Crypto.CipherParams;

/**
* Passes additional properties to a {@link io.ably.lib.rest.Channel} or {@link io.ably.lib.realtime.Channel} object,
Expand All @@ -27,8 +26,6 @@ public class ChannelOptions {
*/
public ChannelMode[] modes;

private ChannelCipherSet cipherSet;

/**
* Requests encryption for this channel when not null,
* and specifies encryption-related parameters (such as algorithm, chaining mode, key length and key).
Expand Down Expand Up @@ -59,58 +56,6 @@ public int getModeFlags() {
return flags;
}

/**
* Returns a wrapper around the cipher set to be used for this channel. This wrapper is only available in this API
* to support customers who may have been using it in their applications with version 1.2.10 or before.
*
* @deprecated Since version 1.2.11, this method (which was only ever intended for internal use within this library
* has been replaced by {@link #getCipherSet()}. It will be removed in the future.
*/
@Deprecated
public ChannelCipher getCipher() throws AblyException {
return new ChannelCipher() {
@Override
public byte[] encrypt(byte[] plaintext) throws AblyException {
return getCipherSet().getEncipher().encrypt(plaintext);
}

@Override
public byte[] decrypt(byte[] ciphertext) throws AblyException {
return getCipherSet().getDecipher().decrypt(ciphertext);
}

@Override
public String getAlgorithm() {
try {
return getCipherSet().getEncipher().getAlgorithm();
} catch (final AblyException e) {
throw new IllegalStateException("Unexpected exception when using legacy crypto cipher interface.", e);
}
}
};
}

/**
* Internal; this method is not intended for use by application developers. It may be changed or removed in future.
*
* Returns the cipher set to be used for encrypting and decrypting data on a channel, given the current state of
* this instance. On the first call to this method a new cipher set instance is created, with subsequent callers to
* this method being returned that same cipher set instance. This method is safe to be called from any thread.
*
* @apiNote Once this method has been called then the cipher set is fixed based on the value of the
* {@link #cipherParams} field at that time. If that field is then mutated, the cipher set will not be updated.
* This is not great API design and we should fix this under https://github.com/ably/ably-java/issues/745
*/
public synchronized ChannelCipherSet getCipherSet() throws AblyException {
if (!encrypted) {
throw new IllegalStateException("ChannelOptions encrypted field value is false.");
}
if (null == cipherSet) {
cipherSet = Crypto.createChannelCipherSet(cipherParams);
}
return cipherSet;
}

/**
* <b>Deprecated. Use withCipherKey(byte[]) instead.</b><br><br>
* Create ChannelOptions from the given cipher key.
Expand Down Expand Up @@ -161,4 +106,15 @@ public static ChannelOptions withCipherKey(byte[] key) throws AblyException {
public static ChannelOptions withCipherKey(String base64Key) throws AblyException {
return withCipherKey(Base64Coder.decode(base64Key));
}

/**
* Internal; returns cipher params or generate default
*/
public synchronized CipherParams getCipherParamsOrDefault() throws AblyException {
CipherParams params = Crypto.checkCipherParams(this.cipherParams);
if (this.cipherParams == null) {
this.cipherParams = params;
}
return params;
}
}
120 changes: 30 additions & 90 deletions lib/src/main/java/io/ably/lib/util/Crypto.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import java.security.SecureRandom;
import java.util.ConcurrentModificationException;
import java.util.Locale;
import java.util.concurrent.Semaphore;

import javax.crypto.BadPaddingException;
import javax.crypto.Cipher;
Expand Down Expand Up @@ -176,22 +175,6 @@ public static byte[] generateRandomKey() {
return generateRandomKey(DEFAULT_KEYLENGTH);
}

/**
* Interface for a ChannelCipher instance that may be associated with a Channel.
*
* The operational methods implemented by channel cipher instances (encrypt and decrypt) are not designed to be
* safe to be called from any thread.
*
* @deprecated Since version 1.2.11, this interface (which was only ever intended for internal use within this
* library) has been replaced by {@link ChannelCipherSet}. It will be removed in the future.
*/
@Deprecated
public interface ChannelCipher {
byte[] encrypt(byte[] plaintext) throws AblyException;
byte[] decrypt(byte[] ciphertext) throws AblyException;
String getAlgorithm();
}

/**
* Internal; a cipher used to encrypt plaintext to ciphertext, for a channel.
*/
Expand Down Expand Up @@ -227,40 +210,30 @@ public interface DecryptingChannelCipher {
}

/**
* Internal; a matching encipher and decipher pair, where both are guaranteed to have been configured with the same
* {@link CipherParams} as each other.
* Internal; get an encrypting cipher instance based on the given channel options.
*/
public interface ChannelCipherSet {
EncryptingChannelCipher getEncipher();
DecryptingChannelCipher getDecipher();
public static EncryptingChannelCipher createChannelEncipher(final CipherParams cipherParams) throws AblyException {
return new EncryptingCBCCipher(cipherParams);
}

/**
* Internal; get an encrypting cipher instance based on the given channel options.
* Internal; get a decrypting cipher instance based on the given channel options.
*/
public static ChannelCipherSet createChannelCipherSet(final Object cipherParams) throws AblyException {
final CipherParams nonNullParams;
if (null == cipherParams)
nonNullParams = Crypto.getDefaultParams();
else if (cipherParams instanceof CipherParams)
nonNullParams = (CipherParams)cipherParams;
else
throw AblyException.fromErrorInfo(new ErrorInfo("ChannelOptions not supported", 400, 40000));

return new ChannelCipherSet() {
private final EncryptingChannelCipher encipher = new EncryptingCBCCipher(nonNullParams);
private final DecryptingChannelCipher decipher = new DecryptingCBCCipher(nonNullParams);

@Override
public EncryptingChannelCipher getEncipher() {
return encipher;
}
public static DecryptingChannelCipher createChannelDecipher(final CipherParams cipherParams) throws AblyException {
return new DecryptingCBCCipher(cipherParams);
}

@Override
public DecryptingChannelCipher getDecipher() {
return decipher;
}
};
/**
* Internal; if `cipherParams` is null returns default params otherwise check if params valid and returns them
*/
public static CipherParams checkCipherParams(final Object cipherParams) throws AblyException {
if (null == cipherParams) {
return Crypto.getDefaultParams();
} else if (cipherParams instanceof CipherParams) {
return (CipherParams) cipherParams;
} else {
throw AblyException.fromErrorInfo(new ErrorInfo("ChannelOptions not supported", 400, 40000));
}
}

/**
Expand All @@ -277,7 +250,6 @@ private static class CBCCipher {
protected final Cipher cipher;
protected final int blockLength;
protected final String algorithm;
private final Semaphore semaphore = new Semaphore(1);

protected CBCCipher(final CipherParams params) throws AblyException {
final String cipherAlgorithm = params.getAlgorithm();
Expand All @@ -293,28 +265,6 @@ protected CBCCipher(final CipherParams params) throws AblyException {
throw AblyException.fromThrowable(e);
}
}

/**
* Subclasses must call this method before performing any work that uses the {@link #cipher} or otherwise
* mutates the state of this instance.
*
* TODO: under https://github.com/ably/ably-java/issues/747 we can then:
* - remove the need for the {@link #releaseOperationalPermit()} method, and
* - make this method return an AutoCloseable implementation that releases the semaphore.
*/
protected void acquireOperationalPermit() {
if (!semaphore.tryAcquire()) {
throw new ConcurrentModificationException("ChannelCipher instances are not designed to be operated from multiple threads simultaneously.");
}
}

/**
* Subclasses must call this method after performing any work that uses the {@link #cipher} or otherwise
* mutates the state of this instance.
*/
protected void releaseOperationalPermit() {
semaphore.release();
}
}

private static class EncryptingCBCCipher extends CBCCipher implements EncryptingChannelCipher {
Expand Down Expand Up @@ -390,23 +340,17 @@ private byte[] getNextIv() {
public byte[] encrypt(byte[] plaintext) {
if (plaintext == null) return null;

acquireOperationalPermit();
try {
final int plaintextLength = plaintext.length;
final int paddedLength = getPaddedLength(plaintextLength);
final byte[] cipherIn = new byte[paddedLength];
final byte[] ciphertext = new byte[paddedLength + blockLength];
final int padding = paddedLength - plaintextLength;
System.arraycopy(plaintext, 0, cipherIn, 0, plaintextLength);
System.arraycopy(pkcs5Padding[padding], 0, cipherIn, plaintextLength, padding);
System.arraycopy(getNextIv(), 0, ciphertext, 0, blockLength);
final byte[] cipherOut = cipher.update(cipherIn);
System.arraycopy(cipherOut, 0, ciphertext, blockLength, paddedLength);
return ciphertext;
} finally {
// TODO: under https://github.com/ably/ably-java/issues/747 we will remove this call.
releaseOperationalPermit();
}
final int plaintextLength = plaintext.length;
final int paddedLength = getPaddedLength(plaintextLength);
final byte[] cipherIn = new byte[paddedLength];
final byte[] ciphertext = new byte[paddedLength + blockLength];
final int padding = paddedLength - plaintextLength;
System.arraycopy(plaintext, 0, cipherIn, 0, plaintextLength);
System.arraycopy(pkcs5Padding[padding], 0, cipherIn, plaintextLength, padding);
System.arraycopy(getNextIv(), 0, ciphertext, 0, blockLength);
final byte[] cipherOut = cipher.update(cipherIn);
System.arraycopy(cipherOut, 0, ciphertext, blockLength, paddedLength);
return ciphertext;
}
}

Expand All @@ -417,17 +361,13 @@ private static class DecryptingCBCCipher extends CBCCipher implements Decrypting

@Override
public byte[] decrypt(byte[] ciphertext) throws AblyException {
if(ciphertext == null) return null;
if (ciphertext == null) return null;

acquireOperationalPermit();
try {
cipher.init(Cipher.DECRYPT_MODE, keySpec, new IvParameterSpec(ciphertext, 0, blockLength));
return cipher.doFinal(ciphertext, blockLength, ciphertext.length - blockLength);
} catch (InvalidAlgorithmParameterException | IllegalBlockSizeException | BadPaddingException | InvalidKeyException e) {
throw AblyException.fromThrowable(e);
} finally {
// TODO: under https://github.com/ably/ably-java/issues/747 we will remove this call.
releaseOperationalPermit();
}
}
}
Expand Down
37 changes: 37 additions & 0 deletions lib/src/test/java/io/ably/lib/rest/DeviceDetailsTest.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package io.ably.lib.rest;

import io.ably.lib.util.JsonUtils;
import org.junit.Test;

import static org.junit.Assert.assertTrue;

public class DeviceDetailsTest {

@Test
public void shouldIgnoreUnrelatedRecipientFields() {
DeviceDetails details = DeviceDetails.fromJsonObject(JsonUtils.object()
.add("id", "testDeviceDetails")
.add("platform", "ios")
.add("formFactor", "phone")
.add("metadata", JsonUtils.object())
.add("push", JsonUtils.object()
.add("recipient", JsonUtils.object()
.add("transportType", "apns")
.add("deviceToken", "foo")
.add("apnsDeviceTokens", JsonUtils.object().add("default", "foo"))))
.toJson());

DeviceDetails otherDetails = DeviceDetails.fromJsonObject(JsonUtils.object()
.add("id", "testDeviceDetails")
.add("platform", "ios")
.add("formFactor", "phone")
.add("metadata", JsonUtils.object())
.add("push", JsonUtils.object()
.add("recipient", JsonUtils.object()
.add("transportType", "apns")
.add("deviceToken", "foo")))
.toJson());

assertTrue("Should ignore `apnsDeviceTokens` field", details.equals(otherDetails));
}
}
Loading

0 comments on commit 05c986b

Please sign in to comment.