You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by sc...@apache.org on 2018/11/21 15:15:34 UTC
svn commit: r1847118 - in /tomcat/trunk:
java/org/apache/catalina/tribes/group/interceptors/
test/org/apache/catalina/tribes/group/interceptors/
Author: schultz
Date: Wed Nov 21 15:15:34 2018
New Revision: 1847118
URL: http://svn.apache.org/viewvc?rev=1847118&view=rev
Log:
Use random IVs for encryption.
Support cipher block modes other than CBC.
Expand unit tests.
Modified:
tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/EncryptInterceptor.java
tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/LocalStrings.properties
tomcat/trunk/test/org/apache/catalina/tribes/group/interceptors/TestEncryptInterceptor.java
Modified: tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/EncryptInterceptor.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/EncryptInterceptor.java?rev=1847118&r1=1847117&r2=1847118&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/EncryptInterceptor.java (original)
+++ tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/EncryptInterceptor.java Wed Nov 21 15:15:34 2018
@@ -17,6 +17,8 @@
package org.apache.catalina.tribes.group.interceptors;
import java.security.GeneralSecurityException;
+import java.security.InvalidAlgorithmParameterException;
+import java.security.InvalidKeyException;
import java.security.SecureRandom;
import javax.crypto.BadPaddingException;
@@ -59,6 +61,7 @@ public class EncryptInterceptor extends
private String encryptionAlgorithm = DEFAULT_ENCRYPTION_ALGORITHM;
private byte[] encryptionKeyBytes;
private String encryptionKeyString;
+ private SecretKeySpec secretKey;
private Cipher encryptionCipher;
private Cipher decryptionCipher;
@@ -92,7 +95,7 @@ public class EncryptInterceptor extends
XByteBuffer xbb = msg.getMessage();
// Completely replace the message
- xbb.setLength(0);
+ xbb.clear();
xbb.append(bytes[0], 0, bytes[0].length);
xbb.append(bytes[1], 0, bytes[1].length);
@@ -104,6 +107,12 @@ public class EncryptInterceptor extends
} catch (BadPaddingException bpe) {
log.error(sm.getString("encryptInterceptor.encrypt.failed"));
throw new ChannelException(bpe);
+ } catch (InvalidKeyException ike) {
+ log.error(sm.getString("encryptInterceptor.encrypt.failed"));
+ throw new ChannelException(ike);
+ } catch (InvalidAlgorithmParameterException iape) {
+ log.error(sm.getString("encryptInterceptor.encrypt.failed"));
+ throw new ChannelException(iape);
}
}
@@ -114,25 +123,21 @@ public class EncryptInterceptor extends
data = decrypt(data);
- // Remove the decrypted IV/nonce block from the front of the message
- int blockSize = getDecryptionCipher().getBlockSize();
- int trimmedSize = data.length - blockSize;
- if(trimmedSize < 0) {
- log.error(sm.getString("encryptInterceptor.decrypt.error.short-message"));
- throw new IllegalStateException(sm.getString("encryptInterceptor.decrypt.error.short-message"));
- }
-
XByteBuffer xbb = msg.getMessage();
// Completely replace the message with the decrypted one
- xbb.setLength(0);
- xbb.append(data, blockSize, data.length - blockSize);
+ xbb.clear();
+ xbb.append(data, 0, data.length);
super.messageReceived(msg);
} catch (IllegalBlockSizeException ibse) {
log.error(sm.getString("encryptInterceptor.decrypt.failed"), ibse);
} catch (BadPaddingException bpe) {
log.error(sm.getString("encryptInterceptor.decrypt.failed"), bpe);
+ } catch (InvalidKeyException ike) {
+ log.error(sm.getString("encryptInterceptor.decrypt.failed"), ike);
+ } catch (InvalidAlgorithmParameterException iape) {
+ log.error(sm.getString("encryptInterceptor.decrypt.failed"), iape);
}
}
@@ -262,55 +267,51 @@ public class EncryptInterceptor extends
String algorithm = getEncryptionAlgorithm();
- String mode = getAlgorithmMode(algorithm);
-
- if(!"CBC".equalsIgnoreCase(mode))
- throw new IllegalArgumentException(sm.getString("encryptInterceptor.algorithm.requires-cbc-mode", mode));
-
- Cipher cipher;
-
- String providerName = getProviderName();
- if(null == providerName) {
- cipher = Cipher.getInstance(algorithm);
- } else {
- cipher = Cipher.getInstance(algorithm, getProviderName());
- }
-
- byte[] iv = new byte[cipher.getBlockSize()];
+ String algorithmName;
+ String algorithmMode;
- // Always use a random IV For cipher setup.
- // The recipient doesn't need the (matching) IV because we will always
- // pre-pad messages with the IV as a nonce.
- new SecureRandom().nextBytes(iv);
-
- IvParameterSpec IV = new IvParameterSpec(iv);
-
- // If this is a cipher transform of the form ALGO/MODE/PAD,
+ // We need to break-apart the algorithm name e.g. AES/CBC/PKCS5Padding
// take just the algorithm part.
int pos = algorithm.indexOf('/');
- String bareAlgorithm;
if(pos >= 0) {
- bareAlgorithm = algorithm.substring(0, pos);
+ algorithmName = algorithm.substring(0, pos);
+ int pos2 = algorithm.indexOf('/', pos+1);
+
+ if(pos2 >= 0) {
+ algorithmMode = algorithm.substring(pos + 1, pos2);
+ } else {
+ algorithmMode = "CBC";
+ }
} else {
- bareAlgorithm = algorithm;
+ algorithmName = algorithm;
+ algorithmMode = "CBC";
}
- SecretKeySpec encryptionKey = new SecretKeySpec(getEncryptionKey(), bareAlgorithm);
+ // Note: ECB is not an appropriate mode for secure communications.
+ if(!("CBC".equalsIgnoreCase(algorithmMode)
+ || "OFB".equalsIgnoreCase(algorithmMode)
+ || "CFB".equalsIgnoreCase(algorithmMode)))
+ throw new IllegalArgumentException(sm.getString("encryptInterceptor.algorithm.unsupported-mode", algorithmMode));
- cipher.init(Cipher.ENCRYPT_MODE, encryptionKey, IV);
-
- encryptionCipher = cipher;
+ setSecretKey(new SecretKeySpec(getEncryptionKeyInternal(), algorithmName));
+ String providerName = getProviderName();
if(null == providerName) {
- cipher = Cipher.getInstance(algorithm);
+ encryptionCipher = Cipher.getInstance(algorithm);
+ decryptionCipher = Cipher.getInstance(algorithm);
} else {
- cipher = Cipher.getInstance(algorithm, getProviderName());
+ encryptionCipher = Cipher.getInstance(algorithm, getProviderName());
+ decryptionCipher = Cipher.getInstance(algorithm, getProviderName());
}
+ }
- cipher.init(Cipher.DECRYPT_MODE, encryptionKey, new IvParameterSpec(iv));
+ private void setSecretKey(SecretKeySpec secretKey) {
+ this.secretKey = secretKey;
+ }
- decryptionCipher = cipher;
+ private SecretKeySpec getSecretKey() {
+ return secretKey;
}
private Cipher getEncryptionCipher() {
@@ -321,20 +322,9 @@ public class EncryptInterceptor extends
return decryptionCipher;
}
- private static String getAlgorithmMode(String algorithm) {
- int start = algorithm.indexOf('/');
- if(start < 0)
- throw new IllegalArgumentException(sm.getString("encryptInterceptor.algorithm.required"));
- int end = algorithm.indexOf('/', start + 1);
- if(end < 0)
- throw new IllegalArgumentException(sm.getString("encryptInterceptor.algorithm.required"));
-
- return algorithm.substring(start + 1, end);
- }
-
/**
* Encrypts the input <code>bytes</code> into two separate byte arrays:
- * one for the initial block (which will be the encrypted random IV)
+ * one for the random initialization vector (IV) used for this message,
* and the second one containing the actual encrypted payload.
*
* This method returns a pair of byte arrays instead of a single
@@ -343,21 +333,32 @@ public class EncryptInterceptor extends
*
* @param bytes The data to encrypt.
*
- * @return The encrypted IV block in [0] and the encrypted data in [1].
+ * @return The IV in [0] and the encrypted data in [1].
*
* @throws IllegalBlockSizeException If the input data is not a multiple of
* the block size and no padding has been requested (for block
* ciphers) or if the input data cannot be encrypted
* @throws BadPaddingException Declared but should not occur during encryption
+ * @throws InvalidAlgorithmParameterException If the algorithm is invalid
+ * @throws InvalidKeyException If the key is invalid
*/
- private byte[][] encrypt(byte[] bytes) throws IllegalBlockSizeException, BadPaddingException {
+ private byte[][] encrypt(byte[] bytes) throws IllegalBlockSizeException, BadPaddingException, InvalidKeyException, InvalidAlgorithmParameterException {
Cipher cipher = getEncryptionCipher();
- // Adding the IV to the beginning of the encrypted data
- byte[] iv = cipher.getIV();
+ byte[] iv = new byte[cipher.getBlockSize()];
+
+ // Always use a random IV For cipher setup.
+ // The recipient doesn't need the (matching) IV because we will always
+ // pre-pad messages with the IV as a nonce.
+ new SecureRandom().nextBytes(iv);
+
+ IvParameterSpec IV = new IvParameterSpec(iv);
+ cipher.init(Cipher.ENCRYPT_MODE, getSecretKey(), IV);
+
+ // Prepend the IV to the beginning of the encrypted data
byte[][] data = new byte[2][];
- data[0] = cipher.update(iv, 0, iv.length);
+ data[0] = iv;
data[1] = cipher.doFinal(bytes);
return data;
@@ -373,10 +374,20 @@ public class EncryptInterceptor extends
* @throws IllegalBlockSizeException If the input data cannot be encrypted
* @throws BadPaddingException If the decrypted data does not include the
* expected number of padding bytes
- *
+ * @throws InvalidAlgorithmParameterException If the algorithm is invalid
+ * @throws InvalidKeyException If the key is invalid
*/
- private byte[] decrypt(byte[] bytes) throws IllegalBlockSizeException, BadPaddingException {
- return getDecryptionCipher().doFinal(bytes);
+ private byte[] decrypt(byte[] bytes) throws IllegalBlockSizeException, BadPaddingException, InvalidKeyException, InvalidAlgorithmParameterException {
+ Cipher cipher = getDecryptionCipher();
+
+ int blockSize = cipher.getBlockSize();
+
+ // Use first-block of incoming data as IV
+ IvParameterSpec IV = new IvParameterSpec(bytes, 0, blockSize);
+ cipher.init(Cipher.DECRYPT_MODE, getSecretKey(), IV);
+
+ // Decrypt remainder of the message.
+ return cipher.doFinal(bytes, blockSize, bytes.length - blockSize);
}
Modified: tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/LocalStrings.properties
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/LocalStrings.properties?rev=1847118&r1=1847117&r2=1847118&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/LocalStrings.properties [UTF-8] (original)
+++ tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/LocalStrings.properties [UTF-8] Wed Nov 21 15:15:34 2018
@@ -17,7 +17,7 @@ domainFilterInterceptor.member.refused=M
domainFilterInterceptor.message.refused=Received message from cluster[{0}] was refused.
encryptInterceptor.algorithm.required=Encryption algorithm is required, fully-specified e.g. AES/CBC/PKCS5Padding
-encryptInterceptor.algorithm.requires-cbc-mode=EncryptInterceptor only supports CBC cipher modes, not [{0}]
+encryptInterceptor.algorithm.unsupported-mode=EncryptInterceptor does not support block cipher mode [{0}]
encryptInterceptor.decrypt.error.short-message=Failed to decrypt message: premature end-of-message
encryptInterceptor.decrypt.failed=Failed to decrypt message
encryptInterceptor.encrypt.failed=Failed to encrypt message
Modified: tomcat/trunk/test/org/apache/catalina/tribes/group/interceptors/TestEncryptInterceptor.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/catalina/tribes/group/interceptors/TestEncryptInterceptor.java?rev=1847118&r1=1847117&r2=1847118&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/catalina/tribes/group/interceptors/TestEncryptInterceptor.java (original)
+++ tomcat/trunk/test/org/apache/catalina/tribes/group/interceptors/TestEncryptInterceptor.java Wed Nov 21 15:15:34 2018
@@ -18,8 +18,14 @@ package org.apache.catalina.tribes.group
import org.junit.Assert;
import org.junit.Before;
+import org.junit.FixMethodOrder;
import org.junit.Ignore;
import org.junit.Test;
+import org.junit.runners.MethodSorters;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
import org.apache.catalina.tribes.Channel;
import org.apache.catalina.tribes.ChannelException;
@@ -38,8 +44,9 @@ import org.apache.catalina.tribes.io.XBy
* though the interceptor actually operates on byte arrays. This is done
* for readability for the tests and their outputs.
*/
+@FixMethodOrder(MethodSorters.NAME_ASCENDING)
public class TestEncryptInterceptor {
- private static final String encryptionKey128 = "cafebabedeadbeefcafebabedeadbeef";
+ private static final String encryptionKey128 = "cafebabedeadbeefbeefcafecafebabe";
private static final String encryptionKey192 = "cafebabedeadbeefbeefcafecafebabedeadbeefbeefcafe";
private static final String encryptionKey256 = "cafebabedeadbeefcafebabedeadbeefcafebabedeadbeefcafebabedeadbeef";
@@ -95,7 +102,7 @@ public class TestEncryptInterceptor {
}
@Test
- @Ignore // Too big for default settings. Breaks Gump, Eclipse, ...
+ @Ignore("Too big for default settings. Breaks Gump, Eclipse, ...")
public void testHugePayload() throws Exception {
src.start(Channel.SND_TX_SEQ);
dest.start(Channel.SND_TX_SEQ);
@@ -157,7 +164,7 @@ public class TestEncryptInterceptor {
bytes = roundTrip(bytes, src, dest);
- return new String(((ValueCaptureInterceptor)dest.getPrevious()).getValue(), "UTF-8");
+ return new String(bytes, "UTF-8");
}
/**
@@ -171,6 +178,123 @@ public class TestEncryptInterceptor {
return ((ValueCaptureInterceptor)dest.getPrevious()).getValue();
}
+ @Test
+ @Ignore("ECB mode isn't because it's insecure")
+ public void testECB() throws Exception {
+ src.setEncryptionAlgorithm("AES/ECB/PKCS5Padding");
+ src.start(Channel.SND_TX_SEQ);
+ dest.setEncryptionAlgorithm("AES/ECB/PKCS5Padding");
+ dest.start(Channel.SND_TX_SEQ);
+
+ String testInput = "The quick brown fox jumps over the lazy dog.";
+
+ Assert.assertEquals("Failed in ECB mode",
+ testInput,
+ roundTrip(testInput, src, dest));
+ }
+
+ @Test
+ public void testOFB() throws Exception {
+ src.setEncryptionAlgorithm("AES/OFB/PKCS5Padding");
+ src.start(Channel.SND_TX_SEQ);
+ dest.setEncryptionAlgorithm("AES/OFB/PKCS5Padding");
+ dest.start(Channel.SND_TX_SEQ);
+
+ String testInput = "The quick brown fox jumps over the lazy dog.";
+
+ Assert.assertEquals("Failed in OFB mode",
+ testInput,
+ roundTrip(testInput, src, dest));
+ }
+
+ @Test
+ public void testCFB() throws Exception {
+ src.setEncryptionAlgorithm("AES/CFB/PKCS5Padding");
+ src.start(Channel.SND_TX_SEQ);
+ dest.setEncryptionAlgorithm("AES/CFB/PKCS5Padding");
+ dest.start(Channel.SND_TX_SEQ);
+
+ String testInput = "The quick brown fox jumps over the lazy dog.";
+
+ Assert.assertEquals("Failed in CFB mode",
+ testInput,
+ roundTrip(testInput, src, dest));
+ }
+
+ @Test
+ @Ignore("GCM mode is unsupported because it requires a custom initialization vector")
+ public void testGCM() throws Exception {
+ src.setEncryptionAlgorithm("AES/GCM/PKCS5Padding");
+ src.start(Channel.SND_TX_SEQ);
+ dest.setEncryptionAlgorithm("AES/GCM/PKCS5Padding");
+ dest.start(Channel.SND_TX_SEQ);
+
+ String testInput = "The quick brown fox jumps over the lazy dog.";
+
+ Assert.assertEquals("Failed in GCM mode",
+ testInput,
+ roundTrip(testInput, src, dest));
+ }
+
+ @Test
+ public void testViaFile() throws Exception {
+ src.start(Channel.SND_TX_SEQ);
+ src.setNext(new ValueCaptureInterceptor());
+
+ String testInput = "The quick brown fox jumps over the lazy dog.";
+
+ ChannelData msg = new ChannelData(false);
+ msg.setMessage(new XByteBuffer(testInput.getBytes("UTF-8"), false));
+ src.sendMessage(null, msg, null);
+
+ byte[] bytes = ((ValueCaptureInterceptor)src.getNext()).getValue();
+
+ try (FileOutputStream out = new FileOutputStream("message.bin")) {
+ out.write(bytes);
+ }
+
+ dest.start(Channel.SND_TX_SEQ);
+
+ bytes = new byte[8192];
+ int read;
+
+ try (FileInputStream in = new FileInputStream("message.bin")) {
+ read = in.read(bytes);
+ }
+
+ msg = new ChannelData(false);
+ XByteBuffer xbb = new XByteBuffer(read, false);
+ xbb.append(bytes, 0, read);
+ msg.setMessage(xbb);
+
+ dest.messageReceived(msg);
+ }
+
+ @Test
+ public void testPickup() throws Exception {
+ File file = new File("message.bin");
+ if(!file.exists()) {
+ System.err.println("File message.bin does not exist. Skipping test.");
+ return;
+ }
+
+ dest.start(Channel.SND_TX_SEQ);
+
+ byte[] bytes = new byte[8192];
+ int read;
+
+ try (FileInputStream in = new FileInputStream("message.bin")) {
+ read = in.read(bytes);
+ }
+
+ ChannelData msg = new ChannelData(false);
+ XByteBuffer xbb = new XByteBuffer(read, false);
+ xbb.append(bytes, 0, read);
+ msg.setMessage(xbb);
+
+ dest.messageReceived(msg);
+ }
+
/**
* Interceptor that delivers directly to a destination.
*/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1847118 - in /tomcat/trunk:
java/org/apache/catalina/tribes/group/interceptors/
test/org/apache/catalina/tribes/group/interceptors/
Posted by Christopher Schultz <ch...@christopherschultz.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Mark,
On 11/21/18 11:36, Mark Thomas wrote:
> On 21/11/2018 15:37, Mark Thomas wrote:
>> On 21/11/2018 15:29, Christopher Schultz wrote:
>>> All,
>>>
>>> With this last patch, I'm ready for a back-port to tc8.5.x, but
>>> I'm waiting for a user who is trying to get this working on
>>> tc9.0 to be successful.
>>>
>>> If anyone else can confirm that this is all working in a real
>>> cluster (dev/test is okay) then I'll go ahead and back-port,
>>> assuming there is some kind of configuration error in that
>>> particular user's case.
>>
>> I'll fire up my 4 node test cluster and let you know. It may take
>> me a while - there are usually a bunch of OS updates waiting for
>> me when I start it up.
>
> I'm seeing lots of errors.
>
> I think the problem is that the interceptor is using one Cipher for
> all members but nodes don't send the same messages to every member
> so the members get out of sync and decryption starts failing.
Each message is encrypted separately and has no effect on the other
messages it might emit. So it shouldn't matter which nodes get which
messages. There is no synchronization between them.
As long as everything is single-threaded, there shouldn't be any problem
s.
- -chris
>>> On 11/21/18 10:15, schultz@apache.org wrote:
>>>> Author: schultz Date: Wed Nov 21 15:15:34 2018 New Revision:
>>>> 1847118
>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1847118&view=rev Log:
>>>> Use random IVs for encryption. Support cipher block modes
>>>> other than CBC. Expand unit tests.
>>>
>>>
>>>> Modified:
>>>> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/Enc
ryp
>>>
>>>>
tInterceptor.java
>>>
>>>
>>> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/Loca
lStr
>>>
>>>
ings.properties
>>>> tomcat/trunk/test/org/apache/catalina/tribes/group/interceptors/Tes
tEn
>>>
>>>>
cryptInterceptor.java
>>>
>>>> Modified:
>>>> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/Enc
ryp
>>>
>>>>
tInterceptor.java
>>>
>>>
>>> URL:
>>> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/t
ribe
>>>
>>>
s/group/interceptors/EncryptInterceptor.java?rev=1847118&r1=1847117&r2=1
>>> 847118&view=diff
>>>> ===================================================================
===
>>>
>>>>
========
>>>
>>>
>>> ---
>>> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/Encr
yptI
>>>
>>>
nterceptor.java
>>> (original)
>>>> +++
>>>> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/Enc
ryp
>>>
>>>>
tInterceptor.java
>>>> Wed Nov 21 15:15:34 2018 @@ -17,6 +17,8 @@ package
>>>> org.apache.catalina.tribes.group.interceptors;
>>>
>>>> import java.security.GeneralSecurityException; +import
>>>> java.security.InvalidAlgorithmParameterException; +import
>>>> java.security.InvalidKeyException; import
>>>> java.security.SecureRandom;
>>>
>>>> import javax.crypto.BadPaddingException; @@ -59,6 +61,7 @@
>>>> public class EncryptInterceptor extends private String
>>>> encryptionAlgorithm = DEFAULT_ENCRYPTION_ALGORITHM; private
>>>> byte[] encryptionKeyBytes; private String
>>>> encryptionKeyString; + private SecretKeySpec secretKey;
>>>
>>>> private Cipher encryptionCipher; private Cipher
>>>> decryptionCipher; @@ -92,7 +95,7 @@ public class
>>>> EncryptInterceptor extends XByteBuffer xbb =
>>>> msg.getMessage();
>>>
>>>> // Completely replace the message -
>>>> xbb.setLength(0); + xbb.clear(); xbb.append(bytes[0], 0,
>>>> bytes[0].length); xbb.append(bytes[1], 0, bytes[1].length);
>>>
>>>> @@ -104,6 +107,12 @@ public class EncryptInterceptor extends
>>>> } catch (BadPaddingException bpe) {
>>>> log.error(sm.getString("encryptInterceptor.encrypt.failed"));
>>>> throw new ChannelException(bpe); + } catch
>>>> (InvalidKeyException ike) { +
>>>> log.error(sm.getString("encryptInterceptor.encrypt.failed"));
>>>> + throw new ChannelException(ike); + } catch
>>>> (InvalidAlgorithmParameterException iape) { +
>>>> log.error(sm.getString("encryptInterceptor.encrypt.failed"));
>>>> + throw new ChannelException(iape); } }
>>>
>>>> @@ -114,25 +123,21 @@ public class EncryptInterceptor
>>>> extends
>>>
>>>> data = decrypt(data);
>>>
>>>> - // Remove the decrypted IV/nonce block from the
>>>> front of the message - int blockSize =
>>>> getDecryptionCipher().getBlockSize(); - int
>>>> trimmedSize = data.length - blockSize; -
>>>> if(trimmedSize < 0) { -
>>>> log.error(sm.getString("encryptInterceptor.decrypt.error.short-mess
age
>>>
>>>>
"));
>>>
>>>
>>> - throw new
>>> IllegalStateException(sm.getString("encryptInterceptor.decrypt.error
.sho
>>>
>>>
rt-message"));
>>>> - } - XByteBuffer xbb = msg.getMessage();
>>>
>>>> // Completely replace the message with the decrypted one -
>>>> xbb.setLength(0); - xbb.append(data, blockSize,
>>>> data.length - blockSize); + xbb.clear(); +
>>>> xbb.append(data, 0, data.length);
>>>
>>>> super.messageReceived(msg); } catch
>>>> (IllegalBlockSizeException ibse) {
>>>> log.error(sm.getString("encryptInterceptor.decrypt.failed"),
>>>> ibse); } catch (BadPaddingException bpe) {
>>>> log.error(sm.getString("encryptInterceptor.decrypt.failed"),
>>>> bpe); + } catch (InvalidKeyException ike) { +
>>>> log.error(sm.getString("encryptInterceptor.decrypt.failed"),
>>>> ike); + } catch (InvalidAlgorithmParameterException
>>>> iape) { +
>>>> log.error(sm.getString("encryptInterceptor.decrypt.failed"),
>>>> iape); } }
>>>
>>>> @@ -262,55 +267,51 @@ public class EncryptInterceptor
>>>> extends
>>>
>>>> String algorithm = getEncryptionAlgorithm();
>>>
>>>> - String mode = getAlgorithmMode(algorithm); - -
>>>> if(!"CBC".equalsIgnoreCase(mode)) - throw new
>>>> IllegalArgumentException(sm.getString("encryptInterceptor.algorithm
.re
>>>
>>>>
quires-cbc-mode",
>>>> mode)); - - Cipher cipher; - - String
>>>> providerName = getProviderName(); - if(null ==
>>>> providerName) { - cipher = Cipher.getInstance(algorithm); -
>>>> } else { - cipher = Cipher.getInstance(algorithm,
>>>> getProviderName()); - } - - byte[] iv = new
>>>> byte[cipher.getBlockSize()]; + String algorithmName; +
>>>> String algorithmMode;
>>>
>>>> - // Always use a random IV For cipher setup. -
>>>> // The recipient doesn't need the (matching) IV because we
>>>> will always - // pre-pad messages with the IV as a
>>>> nonce. - new SecureRandom().nextBytes(iv); - -
>>>> IvParameterSpec IV = new IvParameterSpec(iv); - - //
>>>> If this is a cipher transform of the form ALGO/MODE/PAD, +
>>>> // We need to break-apart the algorithm name e.g.
>>>> AES/CBC/PKCS5Padding // take just the algorithm part. int pos
>>>> = algorithm.indexOf('/');
>>>
>>>> - String bareAlgorithm; if(pos >= 0) { - bareAlgorithm
>>>> = algorithm.substring(0, pos); + algorithmName =
>>>> algorithm.substring(0, pos); + int pos2 =
>>>> algorithm.indexOf('/', pos+1); + + if(pos2 >= 0) {
>>>> + algorithmMode = algorithm.substring(pos + 1, pos2); +
>>>> } else { + algorithmMode = "CBC"; +
>>>> } } else { - bareAlgorithm = algorithm; +
>>>> algorithmName = algorithm; + algorithmMode =
>>>> "CBC"; }
>>>
>>>> - SecretKeySpec encryptionKey = new
>>>> SecretKeySpec(getEncryptionKey(), bareAlgorithm); + //
>>>> Note: ECB is not an appropriate mode for secure
>>>> communications. + if(!("CBC".equalsIgnoreCase(algorithmMode)
>>>> + || "OFB".equalsIgnoreCase(algorithmMode) +
>>>> || "CFB".equalsIgnoreCase(algorithmMode))) + throw
>>>> new
>>>> IllegalArgumentException(sm.getString("encryptInterceptor.algorithm
.un
>>>
>>>>
supported-mode",
>>>> algorithmMode));
>>>
>>>> - cipher.init(Cipher.ENCRYPT_MODE, encryptionKey, IV);
>>>> - - encryptionCipher = cipher; + setSecretKey(new
>>>> SecretKeySpec(getEncryptionKeyInternal(), algorithmName));
>>>
>>>> + String providerName = getProviderName(); if(null ==
>>>> providerName) { - cipher =
>>>> Cipher.getInstance(algorithm); + encryptionCipher
>>>> = Cipher.getInstance(algorithm); +
>>>> decryptionCipher = Cipher.getInstance(algorithm); } else { -
>>>> cipher = Cipher.getInstance(algorithm, getProviderName()); +
>>>> encryptionCipher = Cipher.getInstance(algorithm,
>>>> getProviderName()); + decryptionCipher =
>>>> Cipher.getInstance(algorithm, getProviderName()); } + }
>>>
>>>> - cipher.init(Cipher.DECRYPT_MODE, encryptionKey, new
>>>> IvParameterSpec(iv)); + private void
>>>> setSecretKey(SecretKeySpec secretKey) { +
>>>> this.secretKey = secretKey; + }
>>>
>>>> - decryptionCipher = cipher; + private
>>>> SecretKeySpec getSecretKey() { + return secretKey; }
>>>
>>>> private Cipher getEncryptionCipher() { @@ -321,20 +322,9 @@
>>>> public class EncryptInterceptor extends return
>>>> decryptionCipher; }
>>>
>>>> - private static String getAlgorithmMode(String algorithm)
>>>> { - int start = algorithm.indexOf('/'); - if(start <
>>>> 0) - throw new
>>>> IllegalArgumentException(sm.getString("encryptInterceptor.algorithm
.re
>>>
>>>>
quired"));
>>>
>>>
>>> - int end = algorithm.indexOf('/', start + 1);
>>>> - if(end < 0) - throw new
>>>> IllegalArgumentException(sm.getString("encryptInterceptor.algorithm
.re
>>>
>>>>
quired"));
>>>
>>>
>>> -
>>>> - return algorithm.substring(start + 1, end); - } -
>>>> /** * Encrypts the input <code>bytes</code> into two separate
>>>> byte arrays: - * one for the initial block (which will be
>>>> the encrypted random IV) + * one for the random
>>>> initialization vector (IV) used for this message, * and the
>>>> second one containing the actual encrypted payload. * * This
>>>> method returns a pair of byte arrays instead of a single @@
>>>> -343,21 +333,32 @@ public class EncryptInterceptor extends *
>>>> * @param bytes The data to encrypt. * - * @return The
>>>> encrypted IV block in [0] and the encrypted data in [1]. +
>>>> * @return The IV in [0] and the encrypted data in [1]. * *
>>>> @throws IllegalBlockSizeException If the input data is not a
>>>> multiple of * the block size and no padding has
>>>> been requested (for block * ciphers) or if the
>>>> input data cannot be encrypted * @throws BadPaddingException
>>>> Declared but should not occur during encryption + *
>>>> @throws InvalidAlgorithmParameterException If the algorithm
>>>> is invalid + * @throws InvalidKeyException If the key is
>>>> invalid */ - private byte[][] encrypt(byte[] bytes) throws
>>>> IllegalBlockSizeException, BadPaddingException { + private
>>>> byte[][] encrypt(byte[] bytes) throws
>>>> IllegalBlockSizeException, BadPaddingException,
>>>> InvalidKeyException, InvalidAlgorithmParameterException {
>>>> Cipher cipher = getEncryptionCipher();
>>>
>>>> - // Adding the IV to the beginning of the encrypted
>>>> data - byte[] iv = cipher.getIV(); + byte[] iv = new
>>>> byte[cipher.getBlockSize()]; + + // Always use a
>>>> random IV For cipher setup. + // The recipient doesn't
>>>> need the (matching) IV because we will always + //
>>>> pre-pad messages with the IV as a nonce. + new
>>>> SecureRandom().nextBytes(iv); + + IvParameterSpec IV =
>>>> new IvParameterSpec(iv);
>>>
>>>> + cipher.init(Cipher.ENCRYPT_MODE, getSecretKey(),
>>>> IV); + + // Prepend the IV to the beginning of the encrypted
>>>> data byte[][] data = new byte[2][]; - data[0] =
>>>> cipher.update(iv, 0, iv.length); + data[0] = iv;
>>>> data[1] = cipher.doFinal(bytes);
>>>
>>>> return data; @@ -373,10 +374,20 @@ public class
>>>> EncryptInterceptor extends * @throws
>>>> IllegalBlockSizeException If the input data cannot be
>>>> encrypted * @throws BadPaddingException If the decrypted data
>>>> does not include the * expected number of
>>>> padding bytes - * + * @throws
>>>> InvalidAlgorithmParameterException If the algorithm is
>>>> invalid + * @throws InvalidKeyException If the key is
>>>> invalid */ - private byte[] decrypt(byte[] bytes) throws
>>>> IllegalBlockSizeException, BadPaddingException { -
>>>> return getDecryptionCipher().doFinal(bytes); + private
>>>> byte[] decrypt(byte[] bytes) throws
>>>> IllegalBlockSizeException, BadPaddingException,
>>>> InvalidKeyException, InvalidAlgorithmParameterException { +
>>>> Cipher cipher = getDecryptionCipher(); + + int
>>>> blockSize = cipher.getBlockSize(); + + // Use
>>>> first-block of incoming data as IV + IvParameterSpec
>>>> IV = new IvParameterSpec(bytes, 0, blockSize); +
>>>> cipher.init(Cipher.DECRYPT_MODE, getSecretKey(), IV); + +
>>>> // Decrypt remainder of the message. + return
>>>> cipher.doFinal(bytes, blockSize, bytes.length - blockSize);
>>>> }
>>>
>>>
>>>
>>>> Modified:
>>>> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/Loc
alS
>>>
>>>>
trings.properties
>>>
>>>
>>> URL:
>>> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/t
ribe
>>>
>>>
s/group/interceptors/LocalStrings.properties?rev=1847118&r1=1847117&r2=1
>>> 847118&view=diff
>>>> ===================================================================
===
>>>
>>>>
========
>>>
>>>
>>> ---
>>> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/Loca
lStr
>>>
>>>
ings.properties
>>> [UTF-8] (original)
>>>> +++
>>>> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/Loc
alS
>>>
>>>>
trings.properties
>>>> [UTF-8] Wed Nov 21 15:15:34 2018 @@ -17,7 +17,7 @@
>>>> domainFilterInterceptor.member.refused=M
>>>> domainFilterInterceptor.message.refused=Received message
>>>> from cluster[{0}] was refused.
>>>
>>>> encryptInterceptor.algorithm.required=Encryption algorithm
>>>> is required, fully-specified e.g. AES/CBC/PKCS5Padding
>>>> -encryptInterceptor.algorithm.requires-cbc-mode=EncryptInterceptor
>>>>
>>>>
only supports CBC cipher modes, not [{0}]
>>>> +encryptInterceptor.algorithm.unsupported-mode=EncryptInterceptor
>>>>
>>>>
does not support block cipher mode [{0}]
>>>> encryptInterceptor.decrypt.error.short-message=Failed to
>>>> decrypt message: premature end-of-message
>>>> encryptInterceptor.decrypt.failed=Failed to decrypt message
>>>> encryptInterceptor.encrypt.failed=Failed to encrypt message
>>>
>>>> Modified:
>>>> tomcat/trunk/test/org/apache/catalina/tribes/group/interceptors/Tes
tEn
>>>
>>>>
cryptInterceptor.java
>>>
>>>
>>> URL:
>>> http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/catalina/t
ribe
>>>
>>>
s/group/interceptors/TestEncryptInterceptor.java?rev=1847118&r1=1847117&
>>> r2=1847118&view=diff
>>>> ===================================================================
===
>>>
>>>>
========
>>>
>>>
>>> ---
>>> tomcat/trunk/test/org/apache/catalina/tribes/group/interceptors/Test
Encr
>>>
>>>
yptInterceptor.java
>>> (original)
>>>> +++
>>>> tomcat/trunk/test/org/apache/catalina/tribes/group/interceptors/Tes
tEn
>>>
>>>>
cryptInterceptor.java
>>>> Wed Nov 21 15:15:34 2018 @@ -18,8 +18,14 @@ package
>>>> org.apache.catalina.tribes.group
>>>
>>>> import org.junit.Assert; import org.junit.Before; +import
>>>> org.junit.FixMethodOrder; import org.junit.Ignore; import
>>>> org.junit.Test; +import org.junit.runners.MethodSorters; +
>>>> +import java.io.File; +import java.io.FileInputStream;
>>>> +import java.io.FileOutputStream;
>>>
>>>> import org.apache.catalina.tribes.Channel; import
>>>> org.apache.catalina.tribes.ChannelException; @@ -38,8 +44,9
>>>> @@ import org.apache.catalina.tribes.io.XBy * though the
>>>> interceptor actually operates on byte arrays. This is done *
>>>> for readability for the tests and their outputs. */
>>>> +@FixMethodOrder(MethodSorters.NAME_ASCENDING) public class
>>>> TestEncryptInterceptor { - private static final String
>>>> encryptionKey128 = "cafebabedeadbeefcafebabedeadbeef"; +
>>>> private static final String encryptionKey128 =
>>>> "cafebabedeadbeefbeefcafecafebabe"; private static final
>>>> String encryptionKey192 =
>>>> "cafebabedeadbeefbeefcafecafebabedeadbeefbeefcafe"; private
>>>> static final String encryptionKey256 =
>>>> "cafebabedeadbeefcafebabedeadbeefcafebabedeadbeefcafebabedeadbeef";
>>>
>>>>
>>>>
@@ -95,7 +102,7 @@ public class TestEncryptInterceptor { }
>>>
>>>> @Test - @Ignore // Too big for default settings. Breaks
>>>> Gump, Eclipse, ... + @Ignore("Too big for default
>>>> settings. Breaks Gump, Eclipse, ...") public void
>>>> testHugePayload() throws Exception {
>>>> src.start(Channel.SND_TX_SEQ);
>>>> dest.start(Channel.SND_TX_SEQ); @@ -157,7 +164,7 @@ public
>>>> class TestEncryptInterceptor {
>>>
>>>> bytes = roundTrip(bytes, src, dest);
>>>
>>>> - return new
>>>> String(((ValueCaptureInterceptor)dest.getPrevious()).getValue(),
>>>>
>>>>
"UTF-8"); + return new String(bytes, "UTF-8"); }
>>>
>>>> /** @@ -171,6 +178,123 @@ public class TestEncryptInterceptor
>>>> { return
>>>> ((ValueCaptureInterceptor)dest.getPrevious()).getValue(); }
>>>
>>>> + @Test + @Ignore("ECB mode isn't because it's
>>>> insecure") + public void testECB() throws Exception { +
>>>> src.setEncryptionAlgorithm("AES/ECB/PKCS5Padding"); +
>>>> src.start(Channel.SND_TX_SEQ); +
>>>> dest.setEncryptionAlgorithm("AES/ECB/PKCS5Padding"); +
>>>> dest.start(Channel.SND_TX_SEQ); + + String testInput =
>>>> "The quick brown fox jumps over the lazy dog."; + +
>>>> Assert.assertEquals("Failed in ECB mode", + testInput, +
>>>> roundTrip(testInput, src, dest)); + } + + @Test +
>>>> public void testOFB() throws Exception { +
>>>> src.setEncryptionAlgorithm("AES/OFB/PKCS5Padding"); +
>>>> src.start(Channel.SND_TX_SEQ); +
>>>> dest.setEncryptionAlgorithm("AES/OFB/PKCS5Padding"); +
>>>> dest.start(Channel.SND_TX_SEQ); + + String testInput =
>>>> "The quick brown fox jumps over the lazy dog."; + +
>>>> Assert.assertEquals("Failed in OFB mode", + testInput, +
>>>> roundTrip(testInput, src, dest)); + } + + @Test +
>>>> public void testCFB() throws Exception { +
>>>> src.setEncryptionAlgorithm("AES/CFB/PKCS5Padding"); +
>>>> src.start(Channel.SND_TX_SEQ); +
>>>> dest.setEncryptionAlgorithm("AES/CFB/PKCS5Padding"); +
>>>> dest.start(Channel.SND_TX_SEQ); + + String testInput =
>>>> "The quick brown fox jumps over the lazy dog."; + +
>>>> Assert.assertEquals("Failed in CFB mode", + testInput, +
>>>> roundTrip(testInput, src, dest)); + } + + @Test +
>>>> @Ignore("GCM mode is unsupported because it requires a custom
>>>> initialization vector") + public void testGCM() throws
>>>> Exception { +
>>>> src.setEncryptionAlgorithm("AES/GCM/PKCS5Padding"); +
>>>> src.start(Channel.SND_TX_SEQ); +
>>>> dest.setEncryptionAlgorithm("AES/GCM/PKCS5Padding"); +
>>>> dest.start(Channel.SND_TX_SEQ); + + String testInput =
>>>> "The quick brown fox jumps over the lazy dog."; + +
>>>> Assert.assertEquals("Failed in GCM mode", + testInput, +
>>>> roundTrip(testInput, src, dest)); + } + + @Test +
>>>> public void testViaFile() throws Exception { +
>>>> src.start(Channel.SND_TX_SEQ); + src.setNext(new
>>>> ValueCaptureInterceptor()); + + String testInput =
>>>> "The quick brown fox jumps over the lazy dog."; + +
>>>> ChannelData msg = new ChannelData(false); +
>>>> msg.setMessage(new XByteBuffer(testInput.getBytes("UTF-8"),
>>>> false)); + src.sendMessage(null, msg, null); + +
>>>> byte[] bytes =
>>>> ((ValueCaptureInterceptor)src.getNext()).getValue(); + +
>>>> try (FileOutputStream out = new
>>>> FileOutputStream("message.bin")) { + out.write(bytes); +
>>>> } + + dest.start(Channel.SND_TX_SEQ); + + bytes = new
>>>> byte[8192]; + int read; + + try
>>>> (FileInputStream in = new FileInputStream("message.bin")) { +
>>>> read = in.read(bytes); + } + + msg = new
>>>> ChannelData(false); + XByteBuffer xbb = new
>>>> XByteBuffer(read, false); + xbb.append(bytes, 0,
>>>> read); + msg.setMessage(xbb); + +
>>>> dest.messageReceived(msg); + } + + @Test + public
>>>> void testPickup() throws Exception { + File file = new
>>>> File("message.bin"); + if(!file.exists()) { +
>>>> System.err.println("File message.bin does not exist. Skipping
>>>> test."); + return; + } + +
>>>> dest.start(Channel.SND_TX_SEQ); + + byte[] bytes =
>>>> new byte[8192]; + int read; + + try
>>>> (FileInputStream in = new FileInputStream("message.bin")) { +
>>>> read = in.read(bytes); + } + + ChannelData msg
>>>> = new ChannelData(false); + XByteBuffer xbb = new
>>>> XByteBuffer(read, false); + xbb.append(bytes, 0,
>>>> read); + msg.setMessage(xbb); + +
>>>> dest.messageReceived(msg); + } + /** * Interceptor that
>>>> delivers directly to a destination. */
>>>
>>>
>>>
>>>> -------------------------------------------------------------------
- --
>>>
>>>
>>>
>>>>
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>>>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>>
>>>
>>> --------------------------------------------------------------------
- -
>>>
>>>
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>>
>>
>>
>> ---------------------------------------------------------------------
>>
>>
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>
>
>
> ---------------------------------------------------------------------
>
>
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
-----BEGIN PGP SIGNATURE-----
Comment: Using GnuPG with Thunderbird - https://www.enigmail.net/
iQIzBAEBCAAdFiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAlv1tUYACgkQHPApP6U8
pFhFrg/+IovJYEMq6uaim8BXBUSIoW2B21o0mCEEf8dgPybcWpbXM7KttiM+TKbr
fSDTyflRwii2ew7ZN8cO6xmiXGooM3y9UGZDRWyaa1YU+9yoW/6AUgkFmd00agYC
J0ppxXKnjM+I+c8fkU/WUg0oskhD/XUU3EZbOr/EGK2hhSZE7RfYPyNZ9AVWIQl/
XpZ5Od8Q0YYx1cL+KFzWGxLENOBCsmEqUIDTMIfCRvk7ARs8PEzH5NG+tIBNV1Hd
tGKE11MkYmIFFoW/F6THWMm/NDRpwQi7a30obRMbIoUxM+xCb8LPpwaPnZwLqY55
rYpWuslcVcJACnT2QWLtjdhZPpJtm+OWkZZwcSV+U4HgJXU5fayBzKCJ3SB4WrFo
gk1KApawyTISOYXCghhraxdp45fE2e/wttVqf1NmHjxKSjJkdwH5SLdh3WJI6bxJ
6aIBb5J69pyYWBxOdY+JFJ6d1tgA9b1YB10IhS2vzFimlu+bbYdhaMC/hdrx5W4J
e1CBXpu/6+o9yH3ea4drbxTOC/rwPIvvypOAR+m2I2aAF34b3Z/bzcefAGUf7EUK
VEupnl9RiHPQ9HaLSj6ar+h415++OsCq9EXQwDDnn0Um/DSCzqhVMDGbLM2G8aMV
A319vR5pcA9DVkLuzlDvqCSRKBg72pmLOSK0IuaO1HVlR9CkTuk=
=dBC2
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1847118 - in /tomcat/trunk:
java/org/apache/catalina/tribes/group/interceptors/
test/org/apache/catalina/tribes/group/interceptors/
Posted by Christopher Schultz <ch...@christopherschultz.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
All,
On 11/22/18 17:32, Christopher Schultz wrote:
> Mark,
>
> On 11/22/18 16:43, Mark Thomas wrote:
>> On 22/11/2018 19:17, Christopher Schultz wrote:
>>> Mark,
>>>
>>> On 11/22/18 05:21, Mark Thomas wrote:
>>>> On 21/11/2018 22:39, Christopher Schultz wrote:
>>>>> Mark,
>>>>>
>>>> <snip/>
>>>
>>>>>> I thought you were using CBC so a missing block (a
>>>>>> message being one or more blocks) means that the next
>>>>>> message can't be decrypted.
>>>>>
>>>>> CBC *is* being used, but the cipher is reset after each
>>>>> message, and a new IV is being randomly generated for that
>>>>> purpose. There is no state-carryover between messages. At
>>>>> least, there shouldn't be.
>>>
>>>> Ah. Thanks for the explanation. I should have looked at the
>>>> code. That should all work then.
>>>
>>>> I'll try and find some time today to figure out what is
>>>> causing the error messages I am seeing.
>>>
>>> Thanks, I'd appreciate a second set of eyes.
>>>
>>> I can't seem to find any problems with it. The only "problems"
>>> I ended up finding were poorly-written tests :)
>
>> syncs on encrypt() and decrypt() seem to have done the trick.
>> That was just a quick hack to confirm a suspicion - it isn't the
>> right long term fix.
>
>> If we want this to be performant under load I'd lean towards
>> using a Queue for encryption ciphers and another for decryption
>> ciphers along the lines of the way SessionIdGeneratorBase
>> handles SecureRandom.
>
>> We should probably handle SecureRandom the same way.
>
>> I'll start working on a patch.
>
> Hmm... I was under the impression that the message-sending
> operations were single-threaded (and similar with the receiving
> operations).
>
> I've read that calling Cipher.init() is expensive, but since it's
> being done for every outgoing (and incoming) message, perhaps there
> is no reason to re-use Cipher objects. I'd be interested to see a
> micro-benchmark showing whether all that complexity (Cipher object
> pool) is really worth it. It probably isn't, given that the code
> without that complexity would be super clear and compact.
Okay, I did a1M rounds with:
a) Cipher.getInstance("AES").init(Cipher.ENCRYPT_MODE, key, IV)
b) existingCipher.init(Cipher.ENCRYPT_MODE, key, IV)
And I got these results:
New Cipher took 31485 ms
Only init took 307 ms
I ran my benchmark a bunch of times and got very similar results.
So, it looks like maintaining a pool of Cipher objects is really
beneficial.
Did you end up doing any work on this yet, or shall I take a stab at it?
- -chris
-----BEGIN PGP SIGNATURE-----
Comment: Using GnuPG with Thunderbird - https://www.enigmail.net/
iQIzBAEBCAAdFiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAlv3MhsACgkQHPApP6U8
pFgD0g//V15YGjhwL0P75Rbk6r0q0W7KfwoOuNsi08AK+mQfZ1WKUnqDSIXKcNvC
G6VcQ4QGuwjXMxD0GI7WrBbnhtYtwgteX19AgoHdGxw1cPDRePJyt2e3MNKHj/7W
TIfgTJYXqPbJTDuBGRha2Y9UVqze7vgi21pRVcXtwlEQez9JJdzsWuufMXCH6RAs
a9BpfbpU48JJlyf8a84vKHT3ccuscsa1rIxQ+l2ldJk1Gf4Y/Rl41dDJyEVEOqaN
j2Zb/Jin3DXapDja9SFOwMO5Do9gbEi9/qDXGTgp53YQgTRX2wyVpbFD6JRmqs2z
czFa6zFf0D5rbw2/M4bPmIezyuQp7pydPUsHzMYwrrISfLGMQJbBZAjEtMC0jWPf
fqVGX/RHU2k1B2x9eFVKPZ8HUKbuum/9iZGK3vIrachncx7OyDQGZLAMsHQBWeU4
pJXnzQV6L83VPMriBymcDKINeVmA/lugUtQq90YpxRlicv1gFsP4gopQWBXL2bAI
uwsbOWYFRkYWMb6Rg+h+e2T6L1uNE5vQZtLN369xOcHnjZFNgJSrPCViwxsCayc6
dQByJH8EYkP96xBisNCzB8rL27J9E4EDeoXdumpr7XKn8BIaMtkrHedft17WUuiO
v/ptPuV7+FBEhj8sqetlxObYrMBmJMyl2JNrJgeJBDzq+ddIujs=
=po3H
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1847118 - in /tomcat/trunk:
java/org/apache/catalina/tribes/group/interceptors/
test/org/apache/catalina/tribes/group/interceptors/
Posted by Christopher Schultz <ch...@christopherschultz.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Mark,
On 11/22/18 18:24, Mark Thomas wrote:
>
>
> On 22/11/2018 23:05, Christopher Schultz wrote:
>> On 11/22/18 17:52, Mark Thomas wrote:
>>> On 22/11/2018 22:32, Christopher Schultz wrote:
>>>> On 11/22/18 16:43, Mark Thomas wrote:
>
> <snip/>
>
>>>>> syncs on encrypt() and decrypt() seem to have done the
>>>>> trick. That was just a quick hack to confirm a suspicion -
>>>>> it isn't the right long term fix.
>>>>>
>>>>> If we want this to be performant under load I'd lean
>>>>> towards using a Queue for encryption ciphers and another
>>>>> for decryption ciphers along the lines of the way
>>>>> SessionIdGeneratorBase handles SecureRandom.
>>>>>
>>>>> We should probably handle SecureRandom the same way.
>>>>>
>>>>> I'll start working on a patch.
>>>>
>>>> Hmm... I was under the impression that the message-sending
>>>> operations were single-threaded (and similar with the
>>>> receiving operations).
>>>
>>> There are locks in other Interceptors which are consistent
>>> with them being multi-threaded.
>>>
>>>> I've read that calling Cipher.init() is expensive, but since
>>>> it's being done for every outgoing (and incoming) message,
>>>> perhaps there is no reason to re-use Cipher objects. I'd be
>>>> interested to see a micro-benchmark showing whether all that
>>>> complexity (Cipher object pool) is really worth it. It
>>>> probably isn't, given that the code without that complexity
>>>> would be super clear and compact.
>>>
>>> Good point. I'll try that. It will depend on how expensive
>>> creating the object is.
>>>
>>> Even with the pools the code is pretty clean but I take the
>>> point that it would be (even) cleaner without.
>>>
>>> I have a patch ready to go but I'll do some work on the
>>> benchmark first.
>>
>> I have a patch below. Passes all my unit-tests, but I don't have
>> any multi-threaded ones written at this point.
>>
>> I'd appreciate a review before I commit. I'm going to change the
>> cipher-pool-size to be configurable via an XML attribute, etc.
>
> The patch is hard to read. Can you upload it to people.a.o (or
> maybe we should create a BZ issue for this).
>
> My benchmark shows similar results to yours. Pooling is certainly
> worth while.
>
> I have a patch too. Available here:
> http://people.apache.org/~markt/patches/20181122-encryption-intercepto
r-concurrency-v1.patch
>
>
>
> It does a little more than just add pooling.
>
> It uses the same principle as elsewhere in Tomcat - that the pools
> grows as needed and doesn't shrink so no need for additional
> configuration options.
>
> I expect we'll pull bits from both patches but I'm going to call it
> a day now and come back to this tomorrow.
I took a slightly different approach, using a fixed-size
ArrayBlockingQueue. I suppose we could use a LinkedBlockingQueue... it
that fits-in better with how Tomcat does things in other scenarios,
that's fine. My code defaulted to using a queue the size of the number
of CPUs, since the encryption code is CPU-bound so a huge pool isn't
really going to help even with thousands of request-processing threads.
In terms of the Queue API... is there a big difference between using
take/put versus poll/offer? I can't think of any case where we'd want
to time-out waiting for an item (either checking-out or checking-in).
For the stop() method... does it matter what "kind" of stop is being
processed? We only call initCiphers() on certain kinds of "start"
events, so we should probably tear everything down under the same
conditions.
I have a multi-threaded unit test that showed me that XButeBuffer is
also not thread-safe :)
- -chris
-----BEGIN PGP SIGNATURE-----
Comment: Using GnuPG with Thunderbird - https://www.enigmail.net/
iQIzBAEBCAAdFiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAlv3QywACgkQHPApP6U8
pFgBbg//fX1G6zLFMlsBs9UT6anKzz7vgNt/0z6ZEIJgFQ9zJa3AL+Xbp8FmwIQH
lZy7aqn+uRPRhRYobsHA24Xyz98z6PzQYbOgch6TGzCyWPJ+h0IGNZVHnvTeOGfO
xstiv/G5Y4GCRkdCvEz1TXnArZOoKp7H3Q9ZvOEqj/AShqycEtWYMx4a3Jt44JbC
1nc79P496Gpska3lYhBJSyhWs9IBVWpfKucCSEThEqcZbZrtDw9hpsCNK2gIYYup
IxtMHZmIgwEtNM8luS6rsbD6Pad4fgbysCeiAfM1wfOmjOaaUU+6JCnv1zAEidGW
TuofTCUvMFotVI/A3tAjxzXFVMi8kP329tFJBzzvmR2ka2D2SanGQIabJG+f3cWQ
7wJiV94cUGOhGpI3tYc6EmS98e9VxR24qv9wHV6gPu5dW5pDNQxjjDHALhnR4Uqq
nIYk5L25iG+YYVQejWt30+vlnkLIPmic95nqx2ODNPN+f9g/21mQiAC2RGIMy415
QUKbt7BFv4P8ejSmcwFoy3rw02kJ2bfBU31rR741Tg6F3oeKed6GRc9N/W7XdaNy
UPSoMel5QH5bLHJUXtbLRk05D/C1n55LQe500gvQQ1pUcaROPjbHg4et0y/eiBg8
5GMlN5WlFh4IAhZVrDCzibfqFeBIcX4pTS8rPqBJzhZcS4etgVs=
=uRYu
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1847118 - in /tomcat/trunk:
java/org/apache/catalina/tribes/group/interceptors/
test/org/apache/catalina/tribes/group/interceptors/
Posted by Mark Thomas <ma...@apache.org>.
On 22/11/2018 23:05, Christopher Schultz wrote:
> On 11/22/18 17:52, Mark Thomas wrote:
>> On 22/11/2018 22:32, Christopher Schultz wrote:
>>> On 11/22/18 16:43, Mark Thomas wrote:
<snip/>
>>>> syncs on encrypt() and decrypt() seem to have done the trick.
>>>> That was just a quick hack to confirm a suspicion - it isn't
>>>> the right long term fix.
>>>>
>>>> If we want this to be performant under load I'd lean towards
>>>> using a Queue for encryption ciphers and another for decryption
>>>> ciphers along the lines of the way SessionIdGeneratorBase
>>>> handles SecureRandom.
>>>>
>>>> We should probably handle SecureRandom the same way.
>>>>
>>>> I'll start working on a patch.
>>>
>>> Hmm... I was under the impression that the message-sending
>>> operations were single-threaded (and similar with the receiving
>>> operations).
>>
>> There are locks in other Interceptors which are consistent with
>> them being multi-threaded.
>>
>>> I've read that calling Cipher.init() is expensive, but since
>>> it's being done for every outgoing (and incoming) message,
>>> perhaps there is no reason to re-use Cipher objects. I'd be
>>> interested to see a micro-benchmark showing whether all that
>>> complexity (Cipher object pool) is really worth it. It probably
>>> isn't, given that the code without that complexity would be super
>>> clear and compact.
>>
>> Good point. I'll try that. It will depend on how expensive creating
>> the object is.
>>
>> Even with the pools the code is pretty clean but I take the point
>> that it would be (even) cleaner without.
>>
>> I have a patch ready to go but I'll do some work on the benchmark
>> first.
>
> I have a patch below. Passes all my unit-tests, but I don't have any
> multi-threaded ones written at this point.
>
> I'd appreciate a review before I commit. I'm going to change the
> cipher-pool-size to be configurable via an XML attribute, etc.
The patch is hard to read. Can you upload it to people.a.o (or maybe we
should create a BZ issue for this).
My benchmark shows similar results to yours. Pooling is certainly worth
while.
I have a patch too. Available here:
http://people.apache.org/~markt/patches/20181122-encryption-interceptor-concurrency-v1.patch
It does a little more than just add pooling.
It uses the same principle as elsewhere in Tomcat - that the pools grows
as needed and doesn't shrink so no need for additional configuration
options.
I expect we'll pull bits from both patches but I'm going to call it a
day now and come back to this tomorrow.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1847118 - in /tomcat/trunk:
java/org/apache/catalina/tribes/group/interceptors/
test/org/apache/catalina/tribes/group/interceptors/
Posted by Christopher Schultz <ch...@christopherschultz.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Mark,
On 11/22/18 17:52, Mark Thomas wrote:
> On 22/11/2018 22:32, Christopher Schultz wrote:
>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
>>
>> Mark,
>>
>> On 11/22/18 16:43, Mark Thomas wrote:
>>> On 22/11/2018 19:17, Christopher Schultz wrote:
>>>> Mark,
>>>>
>>>> On 11/22/18 05:21, Mark Thomas wrote:
>>>>> On 21/11/2018 22:39, Christopher Schultz wrote:
>>>>>> Mark,
>>>>>>
>>>>> <snip/>
>>>>
>>>>>>> I thought you were using CBC so a missing block (a
>>>>>>> message being one or more blocks) means that the next
>>>>>>> message can't be decrypted.
>>>>>>
>>>>>> CBC *is* being used, but the cipher is reset after each
>>>>>> message, and a new IV is being randomly generated for
>>>>>> that purpose. There is no state-carryover between
>>>>>> messages. At least, there shouldn't be.
>>>>
>>>>> Ah. Thanks for the explanation. I should have looked at
>>>>> the code. That should all work then.
>>>>
>>>>> I'll try and find some time today to figure out what is
>>>>> causing the error messages I am seeing.
>>>>
>>>> Thanks, I'd appreciate a second set of eyes.
>>>>
>>>> I can't seem to find any problems with it. The only
>>>> "problems" I ended up finding were poorly-written tests :)
>>>
>>> syncs on encrypt() and decrypt() seem to have done the trick.
>>> That was just a quick hack to confirm a suspicion - it isn't
>>> the right long term fix.
>>>
>>> If we want this to be performant under load I'd lean towards
>>> using a Queue for encryption ciphers and another for decryption
>>> ciphers along the lines of the way SessionIdGeneratorBase
>>> handles SecureRandom.
>>>
>>> We should probably handle SecureRandom the same way.
>>>
>>> I'll start working on a patch.
>>
>> Hmm... I was under the impression that the message-sending
>> operations were single-threaded (and similar with the receiving
>> operations).
>
> There are locks in other Interceptors which are consistent with
> them being multi-threaded.
>
>> I've read that calling Cipher.init() is expensive, but since
>> it's being done for every outgoing (and incoming) message,
>> perhaps there is no reason to re-use Cipher objects. I'd be
>> interested to see a micro-benchmark showing whether all that
>> complexity (Cipher object pool) is really worth it. It probably
>> isn't, given that the code without that complexity would be super
>> clear and compact.
>
> Good point. I'll try that. It will depend on how expensive creating
> the object is.
>
> Even with the pools the code is pretty clean but I take the point
> that it would be (even) cleaner without.
>
> I have a patch ready to go but I'll do some work on the benchmark
> first.
I have a patch below. Passes all my unit-tests, but I don't have any
multi-threaded ones written at this point.
I'd appreciate a review before I commit. I'm going to change the
cipher-pool-size to be configurable via an XML attribute, etc.
- -chris
=== CUT ===
> ### Eclipse Workspace Patch 1.0 #P tomcat-trunk Index:
> java/org/apache/catalina/tribes/group/interceptors/EncryptInterceptor.java
>
>
===================================================================
> ---
> java/org/apache/catalina/tribes/group/interceptors/EncryptInterceptor.java
> (revision 1847118) +++
> java/org/apache/catalina/tribes/group/interceptors/EncryptInterceptor.java
> (working copy) @@ -20,7 +20,7 @@ import
> java.security.InvalidAlgorithmParameterException; import
> java.security.InvalidKeyException; import
> java.security.SecureRandom; - +import
> java.util.concurrent.ArrayBlockingQueue; import
> javax.crypto.BadPaddingException; import javax.crypto.Cipher;
> import javax.crypto.IllegalBlockSizeException; @@ -63,8 +63,7 @@
> private String encryptionKeyString; private SecretKeySpec
> secretKey;
>
> - private Cipher encryptionCipher; - private Cipher
> decryptionCipher; + private ArrayBlockingQueue<Cipher>
> cipherQueue;
>
> public EncryptInterceptor() { } @@ -113,6 +112,9 @@ } catch
> (InvalidAlgorithmParameterException iape) {
> log.error(sm.getString("encryptInterceptor.encrypt.failed")); throw
> new ChannelException(iape); + } catch (InterruptedException
> ie) { +
> log.error(sm.getString("encryptInterceptor.cipher-borrow.failed"),
> ie); + throw new ChannelException(ie); } }
>
> @@ -138,6 +140,8 @@
> log.error(sm.getString("encryptInterceptor.decrypt.failed"), ike);
> } catch (InvalidAlgorithmParameterException iape) {
> log.error(sm.getString("encryptInterceptor.decrypt.failed"),
> iape); + } catch (InterruptedException ie) { +
> log.error(sm.getString("encryptInterceptor.cipher-borrow.failed"),
> ie); } }
>
> @@ -297,12 +301,15 @@ setSecretKey(new
> SecretKeySpec(getEncryptionKeyInternal(), algorithmName));
>
> String providerName = getProviderName(); - if(null ==
> providerName) { - encryptionCipher =
> Cipher.getInstance(algorithm); - decryptionCipher =
> Cipher.getInstance(algorithm); - } else { -
> encryptionCipher = Cipher.getInstance(algorithm,
> getProviderName()); - decryptionCipher =
> Cipher.getInstance(algorithm, getProviderName()); + + int
> queueSize = 10; // TODO: Parameter + cipherQueue = new
> ArrayBlockingQueue<>(queueSize); + for(int i=0; i<queueSize;
> ++i) { + if(null == providerName) { +
> cipherQueue.add(Cipher.getInstance(algorithm)); + } else
> { + cipherQueue.add(Cipher.getInstance(algorithm,
> getProviderName())); + } } }
>
> @@ -314,12 +321,20 @@ return secretKey; }
>
> - private Cipher getEncryptionCipher() { - return
> encryptionCipher; + private Cipher getEncryptionCipher() throws
> InterruptedException { + return cipherQueue.take(); + }
> + + private void returnEncryptionCipher(Cipher cipher) throws
> InterruptedException { + cipherQueue.put(cipher); }
>
> - private Cipher getDecryptionCipher() { - return
> decryptionCipher; + private Cipher getDecryptionCipher() throws
> InterruptedException { + return cipherQueue.take(); + }
> + + private void returnDecryptionCipher(Cipher cipher) throws
> InterruptedException { + cipherQueue.put(cipher); }
>
> /** @@ -341,27 +356,35 @@ * @throws BadPaddingException Declared
> but should not occur during encryption * @throws
> InvalidAlgorithmParameterException If the algorithm is invalid *
> @throws InvalidKeyException If the key is invalid + * @throws
> InterruptedException If there is a problem obtaining or returning a
> pooled Cipher object */ - private byte[][] encrypt(byte[] bytes)
> throws IllegalBlockSizeException, BadPaddingException,
> InvalidKeyException, InvalidAlgorithmParameterException { -
> Cipher cipher = getEncryptionCipher(); + private byte[][]
> encrypt(byte[] bytes) throws IllegalBlockSizeException,
> BadPaddingException, InvalidKeyException,
> InvalidAlgorithmParameterException, InterruptedException { +
> Cipher cipher = null; + + try { + cipher =
> getEncryptionCipher();
>
> - byte[] iv = new byte[cipher.getBlockSize()]; +
> byte[] iv = new byte[cipher.getBlockSize()];
>
> - // Always use a random IV For cipher setup. - //
> The recipient doesn't need the (matching) IV because we will
> always - // pre-pad messages with the IV as a nonce. -
> new SecureRandom().nextBytes(iv); + // Always use a
> random IV For cipher setup. + // The recipient doesn't
> need the (matching) IV because we will always + //
> pre-pad messages with the IV as a nonce. + new
> SecureRandom().nextBytes(iv);
>
> - IvParameterSpec IV = new IvParameterSpec(iv); +
> IvParameterSpec IV = new IvParameterSpec(iv);
>
> - cipher.init(Cipher.ENCRYPT_MODE, getSecretKey(), IV); +
> cipher.init(Cipher.ENCRYPT_MODE, getSecretKey(), IV);
>
> - // Prepend the IV to the beginning of the encrypted data -
> byte[][] data = new byte[2][]; - data[0] = iv; -
> data[1] = cipher.doFinal(bytes); + // Prepend the IV to
> the beginning of the encrypted data + byte[][] data =
> new byte[2][]; + data[0] = iv; + data[1] =
> cipher.doFinal(bytes);
>
> - return data; + return data; + } finally
> { + if(null != cipher) +
> returnEncryptionCipher(cipher); + } }
>
> /** @@ -376,18 +399,26 @@ * expected number of padding
> bytes * @throws InvalidAlgorithmParameterException If the algorithm
> is invalid * @throws InvalidKeyException If the key is invalid +
> * @throws InterruptedException If there is a problem obtaining or
> returning a pooled Cipher object */ - private byte[]
> decrypt(byte[] bytes) throws IllegalBlockSizeException,
> BadPaddingException, InvalidKeyException,
> InvalidAlgorithmParameterException { - Cipher cipher =
> getDecryptionCipher(); + private byte[] decrypt(byte[] bytes)
> throws IllegalBlockSizeException, BadPaddingException,
> InvalidKeyException, InvalidAlgorithmParameterException,
> InterruptedException { + Cipher cipher = null;
>
> - int blockSize = cipher.getBlockSize(); + try { +
> cipher = getDecryptionCipher();
>
> - // Use first-block of incoming data as IV -
> IvParameterSpec IV = new IvParameterSpec(bytes, 0, blockSize); -
> cipher.init(Cipher.DECRYPT_MODE, getSecretKey(), IV); +
> int blockSize = cipher.getBlockSize();
>
> - // Decrypt remainder of the message. - return
> cipher.doFinal(bytes, blockSize, bytes.length - blockSize); +
> // Use first-block of incoming data as IV +
> IvParameterSpec IV = new IvParameterSpec(bytes, 0, blockSize); +
> cipher.init(Cipher.DECRYPT_MODE, getSecretKey(), IV); + +
> // Decrypt remainder of the message. + return
> cipher.doFinal(bytes, blockSize, bytes.length - blockSize); +
> } finally { + if(null != cipher) +
> returnDecryptionCipher(cipher); + } }
>
>
-----BEGIN PGP SIGNATURE-----
Comment: Using GnuPG with Thunderbird - https://www.enigmail.net/
iQIzBAEBCAAdFiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAlv3NiwACgkQHPApP6U8
pFjUgxAAlLmx1gMAez+x/jKWl/VbrW6SXbg6jnBSbC0rHXgde1mq/EyHd4KioE0S
fseYt672OSdxIk+yrN0ZZjWfjofkkc32pZm2ap2n23DjTD8yujgGP2ttCvEvK+zr
CQVhBw3Xgfn5UJeMedTlT50ciJOGDko7W3H7fIvsIuElWd3qULEs0T8rFUktKHlB
EgxPEDcum0YX+jyq8W6LNuYma9IQFZQGjLLcorjqaEvtNRbXC08NwReLWSUW/Qeq
EnXPv7jPxXuoN769uB9Jk1HvWxlMeFU0C6rLw5M+/NiQmj4F1sSYtaptAggHwmiI
rcDbN+NNatTOTInsP/pVpjMB3Yzv8S9c5gBLuflCQiSOggFgjGPE5ncXeRZlkZcX
J+31AXR66OM5n7hsdveLWJByQUIie/HVF7KWGnPE4Z9W+qVOD14T5JN8lRnAyYY3
LdvzQV5kkw3ANhjWiATZ9iBCp24RIUqbKpntY/tikBrHAkYOHfceIDbAYRlgciYT
iCG5Jlxlgymg+0VRm1eED70aSvWBUqOn+zB5EPa1zGkbW13/6TIE33armni/PI2L
3uerYAWOsIdann8y1TOgCjIBrYtj2OhnwtncWD7J5Tnl5pTXqMBu13OnrzzDQP1X
uYLXYhHytiEkK8eO2OKOy3ceIDa7dxdoamFEYDK2OG86EX7+KcE=
=s8xU
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1847118 - in /tomcat/trunk:
java/org/apache/catalina/tribes/group/interceptors/
test/org/apache/catalina/tribes/group/interceptors/
Posted by Mark Thomas <ma...@apache.org>.
On 22/11/2018 22:32, Christopher Schultz wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
>
> Mark,
>
> On 11/22/18 16:43, Mark Thomas wrote:
>> On 22/11/2018 19:17, Christopher Schultz wrote:
>>> Mark,
>>>
>>> On 11/22/18 05:21, Mark Thomas wrote:
>>>> On 21/11/2018 22:39, Christopher Schultz wrote:
>>>>> Mark,
>>>>>
>>>> <snip/>
>>>
>>>>>> I thought you were using CBC so a missing block (a message
>>>>>> being one or more blocks) means that the next message can't
>>>>>> be decrypted.
>>>>>
>>>>> CBC *is* being used, but the cipher is reset after each
>>>>> message, and a new IV is being randomly generated for that
>>>>> purpose. There is no state-carryover between messages. At
>>>>> least, there shouldn't be.
>>>
>>>> Ah. Thanks for the explanation. I should have looked at the
>>>> code. That should all work then.
>>>
>>>> I'll try and find some time today to figure out what is
>>>> causing the error messages I am seeing.
>>>
>>> Thanks, I'd appreciate a second set of eyes.
>>>
>>> I can't seem to find any problems with it. The only "problems" I
>>> ended up finding were poorly-written tests :)
>>
>> syncs on encrypt() and decrypt() seem to have done the trick. That
>> was just a quick hack to confirm a suspicion - it isn't the right
>> long term fix.
>>
>> If we want this to be performant under load I'd lean towards using
>> a Queue for encryption ciphers and another for decryption ciphers
>> along the lines of the way SessionIdGeneratorBase handles
>> SecureRandom.
>>
>> We should probably handle SecureRandom the same way.
>>
>> I'll start working on a patch.
>
> Hmm... I was under the impression that the message-sending operations
> were single-threaded (and similar with the receiving operations).
There are locks in other Interceptors which are consistent with them
being multi-threaded.
> I've read that calling Cipher.init() is expensive, but since it's
> being done for every outgoing (and incoming) message, perhaps there is
> no reason to re-use Cipher objects. I'd be interested to see a
> micro-benchmark showing whether all that complexity (Cipher object
> pool) is really worth it. It probably isn't, given that the code
> without that complexity would be super clear and compact.
Good point. I'll try that. It will depend on how expensive creating the
object is.
Even with the pools the code is pretty clean but I take the point that
it would be (even) cleaner without.
I have a patch ready to go but I'll do some work on the benchmark first.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1847118 - in /tomcat/trunk:
java/org/apache/catalina/tribes/group/interceptors/
test/org/apache/catalina/tribes/group/interceptors/
Posted by Christopher Schultz <ch...@christopherschultz.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Mark,
On 11/22/18 16:43, Mark Thomas wrote:
> On 22/11/2018 19:17, Christopher Schultz wrote:
>> Mark,
>>
>> On 11/22/18 05:21, Mark Thomas wrote:
>>> On 21/11/2018 22:39, Christopher Schultz wrote:
>>>> Mark,
>>>>
>>> <snip/>
>>
>>>>> I thought you were using CBC so a missing block (a message
>>>>> being one or more blocks) means that the next message can't
>>>>> be decrypted.
>>>>
>>>> CBC *is* being used, but the cipher is reset after each
>>>> message, and a new IV is being randomly generated for that
>>>> purpose. There is no state-carryover between messages. At
>>>> least, there shouldn't be.
>>
>>> Ah. Thanks for the explanation. I should have looked at the
>>> code. That should all work then.
>>
>>> I'll try and find some time today to figure out what is
>>> causing the error messages I am seeing.
>>
>> Thanks, I'd appreciate a second set of eyes.
>>
>> I can't seem to find any problems with it. The only "problems" I
>> ended up finding were poorly-written tests :)
>
> syncs on encrypt() and decrypt() seem to have done the trick. That
> was just a quick hack to confirm a suspicion - it isn't the right
> long term fix.
>
> If we want this to be performant under load I'd lean towards using
> a Queue for encryption ciphers and another for decryption ciphers
> along the lines of the way SessionIdGeneratorBase handles
> SecureRandom.
>
> We should probably handle SecureRandom the same way.
>
> I'll start working on a patch.
Hmm... I was under the impression that the message-sending operations
were single-threaded (and similar with the receiving operations).
I've read that calling Cipher.init() is expensive, but since it's
being done for every outgoing (and incoming) message, perhaps there is
no reason to re-use Cipher objects. I'd be interested to see a
micro-benchmark showing whether all that complexity (Cipher object
pool) is really worth it. It probably isn't, given that the code
without that complexity would be super clear and compact.
- -chris
-----BEGIN PGP SIGNATURE-----
Comment: Using GnuPG with Thunderbird - https://www.enigmail.net/
iQIzBAEBCAAdFiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAlv3LnoACgkQHPApP6U8
pFg2ZA/+N1tUfYqTja/rEpgrf3FeM9PSukLit78qK16bFXRjyB7RbkiwaBj696VW
hhOvO5/5FeRPIJWHBPbAL6pwiMND3vGjhZHjCM9HOjoTF1cAL75s+0PUQNYy184O
71T3ozvcxy0TQ/cUKZb0eYOGfleeZWmQ7SZsrozNtGgT9QDSptGLsXi4a+8B5VfJ
nbtpOAibFyCSYguLuBHjCdBzY1xAQXEZnvPOpEfZyYl40aTjEn7o8GmbdqOtcu1t
BrATqi0Dtju5PqPHsnAgdG9PDbw6KyDC+qcCaJ7ljF8arfiGXrc84T5X398ZWEGq
0dzLJeAe4gCfriBDY7EKl62bwVQFQZAOXxA4tvYaSS6kUI+Y1tWxm7pq28qdUXfS
QEKxV+mwglxkhjRdBbiuKW+7TJV6vX81G7hNud6kaaEIh+ycoIXGJfLgir4Q7PKP
uL8CQtQfsTd17lX7nBvfSuV+9/eWLOGPBjcpUrFQzDruH0OYE99rM9SikGlQlS1h
UfKdYuI2H1JxRxMC5Etd9gEDFtiBbencnjMUv295xu4N01UvqklKniHzoFMRwWV/
Z0oGHvAboE40EeGiW1ybiofLteO1ZwYJ83pq1Ma4muN+swvkJqVz7IiQswasKPwP
+HMv9o47IQbEQVyfsHyT+NMFOTgfB1FZWxU3D666Hl+QVREJbyA=
=/Di8
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1847118 - in /tomcat/trunk:
java/org/apache/catalina/tribes/group/interceptors/
test/org/apache/catalina/tribes/group/interceptors/
Posted by Mark Thomas <ma...@apache.org>.
On 22/11/2018 19:17, Christopher Schultz wrote:
> Mark,
>
> On 11/22/18 05:21, Mark Thomas wrote:
>> On 21/11/2018 22:39, Christopher Schultz wrote:
>>> Mark,
>>>
>> <snip/>
>
>>>> I thought you were using CBC so a missing block (a message
>>>> being one or more blocks) means that the next message can't be
>>>> decrypted.
>>>
>>> CBC *is* being used, but the cipher is reset after each message,
>>> and a new IV is being randomly generated for that purpose. There
>>> is no state-carryover between messages. At least, there shouldn't
>>> be.
>
>> Ah. Thanks for the explanation. I should have looked at the code.
>> That should all work then.
>
>> I'll try and find some time today to figure out what is causing
>> the error messages I am seeing.
>
> Thanks, I'd appreciate a second set of eyes.
>
> I can't seem to find any problems with it. The only "problems" I ended
> up finding were poorly-written tests :)
syncs on encrypt() and decrypt() seem to have done the trick. That was
just a quick hack to confirm a suspicion - it isn't the right long term fix.
If we want this to be performant under load I'd lean towards using a
Queue for encryption ciphers and another for decryption ciphers along
the lines of the way SessionIdGeneratorBase handles SecureRandom.
We should probably handle SecureRandom the same way.
I'll start working on a patch.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1847118 - in /tomcat/trunk:
java/org/apache/catalina/tribes/group/interceptors/
test/org/apache/catalina/tribes/group/interceptors/
Posted by Christopher Schultz <ch...@christopherschultz.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Mark,
On 11/22/18 05:21, Mark Thomas wrote:
> On 21/11/2018 22:39, Christopher Schultz wrote:
>> Mark,
>>
> <snip/>
>
>>> I thought you were using CBC so a missing block (a message
>>> being one or more blocks) means that the next message can't be
>>> decrypted.
>>
>> CBC *is* being used, but the cipher is reset after each message,
>> and a new IV is being randomly generated for that purpose. There
>> is no state-carryover between messages. At least, there shouldn't
>> be.
>
> Ah. Thanks for the explanation. I should have looked at the code.
> That should all work then.
>
> I'll try and find some time today to figure out what is causing
> the error messages I am seeing.
Thanks, I'd appreciate a second set of eyes.
I can't seem to find any problems with it. The only "problems" I ended
up finding were poorly-written tests :)
- -chris
-----BEGIN PGP SIGNATURE-----
Comment: Using GnuPG with Thunderbird - https://www.enigmail.net/
iQIzBAEBCAAdFiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAlv3ALEACgkQHPApP6U8
pFgN3w/+JLgVNXCkg+zMNHmqxG4nJeTmByt1QQtJIYWOWMrg7cQYgj4RO+MucGsb
HiapvPQijnP8Po4jh3Xp2gcQ8ZxWPEX/OJTMFItNsP9Bb4/WCYqSy63N8tFo30Zb
7QZ3ZvLY95d8yJOtt0I95d0lgEd00RR++yB8xC/3Km6GvnnX88jwErL1DEWG7K1C
xgX9QQLUrn7PMlko/+gffmlu6yEDuG4UDgis3QvTBeLFDF5OfdBLTAGqsWUwXijr
YqhitGH9LHQtDBY9jiT4k3b/OwTbOpg4KEVGUM5LoseyDDdqhbyYTJW1KoBE0QD2
LPVmac13stfkk9Zu5kpQzPyyo8XyP+3ZJs4Rhgbq/2AmiQ63z9RiGx0WGx3nwDvB
htSxCbvIf3+fWD1d43LIy8ahR2uDS6Ni2bXn1AfQGZyr4nMcsv56DVnBdPldQ7X6
PcEvZTy9/3zx54nBNl3CWe0m2d300ys04piC2eeS0VyICdVhxfQnTJV4w17/lkoJ
7G3QVz+zE5qckLjeroGhhZsU8JJUgL/+fcff6sja7K8wNmFvRzAJYulE+nc//u1y
QnesXMs3tM6+1010guR36Ilaxb1mJAMFvm1ikbUl8MIdm6UmADiA5YwPuSuJwq66
nWCzXCUMZJ20zhaoY86naKczxZgv+g3bnpDKHj4M5BnzgHWQ6wM=
=YD4u
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1847118 - in /tomcat/trunk:
java/org/apache/catalina/tribes/group/interceptors/
test/org/apache/catalina/tribes/group/interceptors/
Posted by Mark Thomas <ma...@apache.org>.
On 21/11/2018 22:39, Christopher Schultz wrote:
> Mark,
>
<snip/>
>> I thought you were using CBC so a missing block (a message being
>> one or more blocks) means that the next message can't be
>> decrypted.
>
> CBC *is* being used, but the cipher is reset after each message, and a
> new IV is being randomly generated for that purpose. There is no
> state-carryover between messages. At least, there shouldn't be.
Ah. Thanks for the explanation. I should have looked at the code. That
should all work then.
I'll try and find some time today to figure out what is causing the
error messages I am seeing.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1847118 - in /tomcat/trunk:
java/org/apache/catalina/tribes/group/interceptors/
test/org/apache/catalina/tribes/group/interceptors/
Posted by Christopher Schultz <ch...@christopherschultz.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Mark,
On 11/21/18 15:11, Mark Thomas wrote:
> On 21/11/2018 19:43, Christopher Schultz wrote:
>> Mark,
>>
>> On 11/21/18 11:51, Mark Thomas wrote:
>>> On 21/11/2018 16:36, Mark Thomas wrote:
>>>> On 21/11/2018 15:37, Mark Thomas wrote:
>>>>> On 21/11/2018 15:29, Christopher Schultz wrote:
>>>>>> All,
>>>>>>
>>>>>> With this last patch, I'm ready for a back-port to
>>>>>> tc8.5.x, but I'm waiting for a user who is trying to get
>>>>>> this working on tc9.0 to be successful.
>>>>>>
>>>>>> If anyone else can confirm that this is all working in a
>>>>>> real cluster (dev/test is okay) then I'll go ahead and
>>>>>> back-port, assuming there is some kind of configuration
>>>>>> error in that particular user's case.
>>>>>
>>>>> I'll fire up my 4 node test cluster and let you know. It
>>>>> may take me a while - there are usually a bunch of OS
>>>>> updates waiting for me when I start it up.
>>>>
>>>> I'm seeing lots of errors.
>>>>
>>>> I think the problem is that the interceptor is using one
>>>> Cipher for all members but nodes don't send the same messages
>>>> to every member so the members get out of sync and decryption
>>>> starts failing.
>>
>>> Oh, and to add to the 'fun' messages may be processed out of
>>> order.
>>
>> That should also be okay, since messages aren't related to each
>> other.
>>
>> But it might be a problem with trying to prevent replay attacks.
>
> I thought you were using CBC so a missing block (a message being
> one or more blocks) means that the next message can't be
> decrypted.
CBC *is* being used, but the cipher is reset after each message, and a
new IV is being randomly generated for that purpose. There is no
state-carryover between messages. At least, there shouldn't be.
- -chris
-----BEGIN PGP SIGNATURE-----
Comment: Using GnuPG with Thunderbird - https://www.enigmail.net/
iQIzBAEBCAAdFiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAlv13pMACgkQHPApP6U8
pFjIog/9EWiyD2bo4ur6z5wdkMw1a3ZLwizItbHd6frnsfHzWFmpmlRo73rNdpiq
kbRoC+eo2lm8r0yJHgVzldsovRx5wVoAie48tZuGudY20K/3GZ3YWWWwUTEeVFIZ
0xelerItAcKm2JCpUdH5J/j2FVoPzjUsxVezgSg1lc3Su2dEhyjMcska4gXlzUeV
wwNekNlKMzjXGWJe9PzetIpmCw4Pu3XZDsboGr2pxyzayP+YpeaN2LxXsGaR+RKq
B8jEpLRtj3TjjMy9LZPUJANXDOpqwSy8ajPpcZrlj70ULRaR3ByFg73AEG3R447Y
GxBIa4bFs66b+eE3crrt3RaxEv3vcOwVpEuKweDx2vIligFAFKYbRmLcoXGtE3DK
3uvlJVycQ+D8YJ2uWeY+KpgdOp55vQSj1Y6TsiPF26QLk+9pUi8WHE10AOopmljf
KITNVkW9nTsy9QLW7sGts5CUiTrqG/XS5xu442qs+VIVO9NJF1YzAxSW6NmMyIa8
08VR41d6Z424l3Fhy1y0OnixlJ2EjGGwGoyWqSToJMSWsvmhHJh6BNRMHIPlURvl
QFbeC896WsBMgtj2f05907powLTs9XB2Dl/jgB17lfQb5JOxFE5DpqEWehYWx9Mn
Byqk8xDzhm+Z+kkl31iG+2sUHYUUhsIehoPvRq5K+mhJbDrTv34=
=3FAJ
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1847118 - in /tomcat/trunk:
java/org/apache/catalina/tribes/group/interceptors/
test/org/apache/catalina/tribes/group/interceptors/
Posted by Mark Thomas <ma...@apache.org>.
On 21/11/2018 19:43, Christopher Schultz wrote:
> Mark,
>
> On 11/21/18 11:51, Mark Thomas wrote:
>> On 21/11/2018 16:36, Mark Thomas wrote:
>>> On 21/11/2018 15:37, Mark Thomas wrote:
>>>> On 21/11/2018 15:29, Christopher Schultz wrote:
>>>>> All,
>>>>>
>>>>> With this last patch, I'm ready for a back-port to tc8.5.x,
>>>>> but I'm waiting for a user who is trying to get this working
>>>>> on tc9.0 to be successful.
>>>>>
>>>>> If anyone else can confirm that this is all working in a real
>>>>> cluster (dev/test is okay) then I'll go ahead and back-port,
>>>>> assuming there is some kind of configuration error in that
>>>>> particular user's case.
>>>>
>>>> I'll fire up my 4 node test cluster and let you know. It may
>>>> take me a while - there are usually a bunch of OS updates
>>>> waiting for me when I start it up.
>>>
>>> I'm seeing lots of errors.
>>>
>>> I think the problem is that the interceptor is using one Cipher
>>> for all members but nodes don't send the same messages to every
>>> member so the members get out of sync and decryption starts
>>> failing.
>
>> Oh, and to add to the 'fun' messages may be processed out of
>> order.
>
> That should also be okay, since messages aren't related to each other.
>
> But it might be a problem with trying to prevent replay attacks.
I thought you were using CBC so a missing block (a message being one or
more blocks) means that the next message can't be decrypted.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1847118 - in /tomcat/trunk:
java/org/apache/catalina/tribes/group/interceptors/
test/org/apache/catalina/tribes/group/interceptors/
Posted by Christopher Schultz <ch...@christopherschultz.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Mark,
On 11/21/18 11:51, Mark Thomas wrote:
> On 21/11/2018 16:36, Mark Thomas wrote:
>> On 21/11/2018 15:37, Mark Thomas wrote:
>>> On 21/11/2018 15:29, Christopher Schultz wrote:
>>>> All,
>>>>
>>>> With this last patch, I'm ready for a back-port to tc8.5.x,
>>>> but I'm waiting for a user who is trying to get this working
>>>> on tc9.0 to be successful.
>>>>
>>>> If anyone else can confirm that this is all working in a real
>>>> cluster (dev/test is okay) then I'll go ahead and back-port,
>>>> assuming there is some kind of configuration error in that
>>>> particular user's case.
>>>
>>> I'll fire up my 4 node test cluster and let you know. It may
>>> take me a while - there are usually a bunch of OS updates
>>> waiting for me when I start it up.
>>
>> I'm seeing lots of errors.
>>
>> I think the problem is that the interceptor is using one Cipher
>> for all members but nodes don't send the same messages to every
>> member so the members get out of sync and decryption starts
>> failing.
>
> Oh, and to add to the 'fun' messages may be processed out of
> order.
That should also be okay, since messages aren't related to each other.
But it might be a problem with trying to prevent replay attacks.
- -chris
-----BEGIN PGP SIGNATURE-----
Comment: Using GnuPG with Thunderbird - https://www.enigmail.net/
iQIzBAEBCAAdFiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAlv1tXAACgkQHPApP6U8
pFhK0xAAn6LzQE+005qETnCIP1/BjUxllOkiqe6MX07GWxd/MuxOOcpQZWBgJhQB
/LdoAApulNvQw8Wv77XgTFv2g5C7vc3bNYX+4D5jPtwq494RmOJz/Vj/2H6d0eCX
Elj+0tWjAJoNBBU9L2n5tgGuA6N8ePnF7hCGdKV5hNSTjRrQFQ2cUGEWxRT0lJto
4V+THAt9J2MimvlDbmYM8q6b4v5re+C0lsHWn3eGzuYAv7bdXk8mY51Xu7dU+SGV
ReyIpy7yfkeh4WZoJMowBtEO51AdVy8A30mNOvikpgcZWQufdYRaIANodJGeoK5X
o4XxMA4+AYC0zUPXwAjy9XND/S6eLHI2V7CP2zqeWeEgxeTEKbEBUSu7dQsO6IRH
sf+rpR0h0Z90szha87i7Ky5MBIC5Jlw7RwJE9gnWfZP/h5sWRatpPTCfLxs9r46E
q5KhRw8OhXqGR5qD92TKF3AewyQ1ciY/AhmRGORe0Z3s7CGc1t8BolUXWeiM+y//
L37duwJ+BvaLxzP/qZ3lmit0wMCHS5J2EzJcMRd8811o7ujZTFZfMRD9jays8JaT
mRm5MJLvMr4WG4xjQSr85Iy9sbxR+J9olYOWB3gp+71KwXqP0NHEIwcFuRkdw7GT
QbULu2r6FuxjiC4e4oukry6eaGD24kiJ8oWah9fe0Pdg0pGl1Mc=
=MfIG
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1847118 - in /tomcat/trunk:
java/org/apache/catalina/tribes/group/interceptors/
test/org/apache/catalina/tribes/group/interceptors/
Posted by Mark Thomas <ma...@apache.org>.
On 21/11/2018 16:36, Mark Thomas wrote:
> On 21/11/2018 15:37, Mark Thomas wrote:
>> On 21/11/2018 15:29, Christopher Schultz wrote:
>>> All,
>>>
>>> With this last patch, I'm ready for a back-port to tc8.5.x, but I'm
>>> waiting for a user who is trying to get this working on tc9.0 to be
>>> successful.
>>>
>>> If anyone else can confirm that this is all working in a real cluster
>>> (dev/test is okay) then I'll go ahead and back-port, assuming there is
>>> some kind of configuration error in that particular user's case.
>>
>> I'll fire up my 4 node test cluster and let you know. It may take me a
>> while - there are usually a bunch of OS updates waiting for me when I
>> start it up.
>
> I'm seeing lots of errors.
>
> I think the problem is that the interceptor is using one Cipher for all
> members but nodes don't send the same messages to every member so the
> members get out of sync and decryption starts failing.
Oh, and to add to the 'fun' messages may be processed out of order.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1847118 - in /tomcat/trunk:
java/org/apache/catalina/tribes/group/interceptors/
test/org/apache/catalina/tribes/group/interceptors/
Posted by Mark Thomas <ma...@apache.org>.
On 21/11/2018 15:37, Mark Thomas wrote:
> On 21/11/2018 15:29, Christopher Schultz wrote:
>> All,
>>
>> With this last patch, I'm ready for a back-port to tc8.5.x, but I'm
>> waiting for a user who is trying to get this working on tc9.0 to be
>> successful.
>>
>> If anyone else can confirm that this is all working in a real cluster
>> (dev/test is okay) then I'll go ahead and back-port, assuming there is
>> some kind of configuration error in that particular user's case.
>
> I'll fire up my 4 node test cluster and let you know. It may take me a
> while - there are usually a bunch of OS updates waiting for me when I
> start it up.
I'm seeing lots of errors.
I think the problem is that the interceptor is using one Cipher for all
members but nodes don't send the same messages to every member so the
members get out of sync and decryption starts failing.
Mark
>
> Mark
>
>
>>
>> -chris
>>
>> On 11/21/18 10:15, schultz@apache.org wrote:
>>> Author: schultz Date: Wed Nov 21 15:15:34 2018 New Revision:
>>> 1847118
>>
>>> URL: http://svn.apache.org/viewvc?rev=1847118&view=rev Log: Use
>>> random IVs for encryption. Support cipher block modes other than
>>> CBC. Expand unit tests.
>>
>>
>>> Modified:
>>> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/Encryp
>> tInterceptor.java
>>
>>
>> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/LocalStr
>> ings.properties
>>> tomcat/trunk/test/org/apache/catalina/tribes/group/interceptors/TestEn
>> cryptInterceptor.java
>>
>>> Modified:
>>> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/Encryp
>> tInterceptor.java
>>
>>
>> URL:
>> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/tribe
>> s/group/interceptors/EncryptInterceptor.java?rev=1847118&r1=1847117&r2=1
>> 847118&view=diff
>>> ======================================================================
>> ========
>>
>>
>> ---
>> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/EncryptI
>> nterceptor.java
>> (original)
>>> +++
>>> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/Encryp
>> tInterceptor.java
>>> Wed Nov 21 15:15:34 2018 @@ -17,6 +17,8 @@ package
>>> org.apache.catalina.tribes.group.interceptors;
>>
>>> import java.security.GeneralSecurityException; +import
>>> java.security.InvalidAlgorithmParameterException; +import
>>> java.security.InvalidKeyException; import
>>> java.security.SecureRandom;
>>
>>> import javax.crypto.BadPaddingException; @@ -59,6 +61,7 @@ public
>>> class EncryptInterceptor extends private String encryptionAlgorithm
>>> = DEFAULT_ENCRYPTION_ALGORITHM; private byte[] encryptionKeyBytes;
>>> private String encryptionKeyString; + private SecretKeySpec
>>> secretKey;
>>
>>> private Cipher encryptionCipher; private Cipher decryptionCipher;
>>> @@ -92,7 +95,7 @@ public class EncryptInterceptor extends
>>> XByteBuffer xbb = msg.getMessage();
>>
>>> // Completely replace the message - xbb.setLength(0); +
>>> xbb.clear(); xbb.append(bytes[0], 0, bytes[0].length);
>>> xbb.append(bytes[1], 0, bytes[1].length);
>>
>>> @@ -104,6 +107,12 @@ public class EncryptInterceptor extends }
>>> catch (BadPaddingException bpe) {
>>> log.error(sm.getString("encryptInterceptor.encrypt.failed")); throw
>>> new ChannelException(bpe); + } catch (InvalidKeyException
>>> ike) { +
>>> log.error(sm.getString("encryptInterceptor.encrypt.failed")); +
>>> throw new ChannelException(ike); + } catch
>>> (InvalidAlgorithmParameterException iape) { +
>>> log.error(sm.getString("encryptInterceptor.encrypt.failed")); +
>>> throw new ChannelException(iape); } }
>>
>>> @@ -114,25 +123,21 @@ public class EncryptInterceptor extends
>>
>>> data = decrypt(data);
>>
>>> - // Remove the decrypted IV/nonce block from the front
>>> of the message - int blockSize =
>>> getDecryptionCipher().getBlockSize(); - int trimmedSize
>>> = data.length - blockSize; - if(trimmedSize < 0) { -
>>> log.error(sm.getString("encryptInterceptor.decrypt.error.short-message
>> "));
>>
>>
>> - throw new
>> IllegalStateException(sm.getString("encryptInterceptor.decrypt.error.sho
>> rt-message"));
>>> - } - XByteBuffer xbb = msg.getMessage();
>>
>>> // Completely replace the message with the decrypted one -
>>> xbb.setLength(0); - xbb.append(data, blockSize,
>>> data.length - blockSize); + xbb.clear(); +
>>> xbb.append(data, 0, data.length);
>>
>>> super.messageReceived(msg); } catch (IllegalBlockSizeException
>>> ibse) {
>>> log.error(sm.getString("encryptInterceptor.decrypt.failed"),
>>> ibse); } catch (BadPaddingException bpe) {
>>> log.error(sm.getString("encryptInterceptor.decrypt.failed"), bpe);
>>> + } catch (InvalidKeyException ike) { +
>>> log.error(sm.getString("encryptInterceptor.decrypt.failed"), ike);
>>> + } catch (InvalidAlgorithmParameterException iape) { +
>>> log.error(sm.getString("encryptInterceptor.decrypt.failed"),
>>> iape); } }
>>
>>> @@ -262,55 +267,51 @@ public class EncryptInterceptor extends
>>
>>> String algorithm = getEncryptionAlgorithm();
>>
>>> - String mode = getAlgorithmMode(algorithm); - -
>>> if(!"CBC".equalsIgnoreCase(mode)) - throw new
>>> IllegalArgumentException(sm.getString("encryptInterceptor.algorithm.re
>> quires-cbc-mode",
>>> mode)); - - Cipher cipher; - - String providerName =
>>> getProviderName(); - if(null == providerName) { -
>>> cipher = Cipher.getInstance(algorithm); - } else { -
>>> cipher = Cipher.getInstance(algorithm, getProviderName()); -
>>> } - - byte[] iv = new byte[cipher.getBlockSize()]; +
>>> String algorithmName; + String algorithmMode;
>>
>>> - // Always use a random IV For cipher setup. - //
>>> The recipient doesn't need the (matching) IV because we will
>>> always - // pre-pad messages with the IV as a nonce. -
>>> new SecureRandom().nextBytes(iv); - - IvParameterSpec IV =
>>> new IvParameterSpec(iv); - - // If this is a cipher
>>> transform of the form ALGO/MODE/PAD, + // We need to
>>> break-apart the algorithm name e.g. AES/CBC/PKCS5Padding // take
>>> just the algorithm part. int pos = algorithm.indexOf('/');
>>
>>> - String bareAlgorithm; if(pos >= 0) { -
>>> bareAlgorithm = algorithm.substring(0, pos); +
>>> algorithmName = algorithm.substring(0, pos); + int pos2
>>> = algorithm.indexOf('/', pos+1); + + if(pos2 >= 0) { +
>>> algorithmMode = algorithm.substring(pos + 1, pos2); + }
>>> else { + algorithmMode = "CBC"; + } }
>>> else { - bareAlgorithm = algorithm; +
>>> algorithmName = algorithm; + algorithmMode = "CBC"; }
>>
>>> - SecretKeySpec encryptionKey = new
>>> SecretKeySpec(getEncryptionKey(), bareAlgorithm); + // Note:
>>> ECB is not an appropriate mode for secure communications. +
>>> if(!("CBC".equalsIgnoreCase(algorithmMode) + ||
>>> "OFB".equalsIgnoreCase(algorithmMode) + ||
>>> "CFB".equalsIgnoreCase(algorithmMode))) + throw new
>>> IllegalArgumentException(sm.getString("encryptInterceptor.algorithm.un
>> supported-mode",
>>> algorithmMode));
>>
>>> - cipher.init(Cipher.ENCRYPT_MODE, encryptionKey, IV); - -
>>> encryptionCipher = cipher; + setSecretKey(new
>>> SecretKeySpec(getEncryptionKeyInternal(), algorithmName));
>>
>>> + String providerName = getProviderName(); if(null ==
>>> providerName) { - cipher =
>>> Cipher.getInstance(algorithm); + encryptionCipher =
>>> Cipher.getInstance(algorithm); + decryptionCipher =
>>> Cipher.getInstance(algorithm); } else { - cipher =
>>> Cipher.getInstance(algorithm, getProviderName()); +
>>> encryptionCipher = Cipher.getInstance(algorithm,
>>> getProviderName()); + decryptionCipher =
>>> Cipher.getInstance(algorithm, getProviderName()); } + }
>>
>>> - cipher.init(Cipher.DECRYPT_MODE, encryptionKey, new
>>> IvParameterSpec(iv)); + private void setSecretKey(SecretKeySpec
>>> secretKey) { + this.secretKey = secretKey; + }
>>
>>> - decryptionCipher = cipher; + private SecretKeySpec
>>> getSecretKey() { + return secretKey; }
>>
>>> private Cipher getEncryptionCipher() { @@ -321,20 +322,9 @@ public
>>> class EncryptInterceptor extends return decryptionCipher; }
>>
>>> - private static String getAlgorithmMode(String algorithm) { -
>>> int start = algorithm.indexOf('/'); - if(start < 0) -
>>> throw new
>>> IllegalArgumentException(sm.getString("encryptInterceptor.algorithm.re
>> quired"));
>>
>>
>> - int end = algorithm.indexOf('/', start + 1);
>>> - if(end < 0) - throw new
>>> IllegalArgumentException(sm.getString("encryptInterceptor.algorithm.re
>> quired"));
>>
>>
>> -
>>> - return algorithm.substring(start + 1, end); - } - /** *
>>> Encrypts the input <code>bytes</code> into two separate byte
>>> arrays: - * one for the initial block (which will be the
>>> encrypted random IV) + * one for the random initialization
>>> vector (IV) used for this message, * and the second one containing
>>> the actual encrypted payload. * * This method returns a pair of
>>> byte arrays instead of a single @@ -343,21 +333,32 @@ public class
>>> EncryptInterceptor extends * * @param bytes The data to encrypt. *
>>> - * @return The encrypted IV block in [0] and the encrypted
>>> data in [1]. + * @return The IV in [0] and the encrypted data
>>> in [1]. * * @throws IllegalBlockSizeException If the input data is
>>> not a multiple of * the block size and no padding has
>>> been requested (for block * ciphers) or if the input
>>> data cannot be encrypted * @throws BadPaddingException Declared but
>>> should not occur during encryption + * @throws
>>> InvalidAlgorithmParameterException If the algorithm is invalid +
>>> * @throws InvalidKeyException If the key is invalid */ - private
>>> byte[][] encrypt(byte[] bytes) throws IllegalBlockSizeException,
>>> BadPaddingException { + private byte[][] encrypt(byte[] bytes)
>>> throws IllegalBlockSizeException, BadPaddingException,
>>> InvalidKeyException, InvalidAlgorithmParameterException { Cipher
>>> cipher = getEncryptionCipher();
>>
>>> - // Adding the IV to the beginning of the encrypted data -
>>> byte[] iv = cipher.getIV(); + byte[] iv = new
>>> byte[cipher.getBlockSize()]; + + // Always use a random IV
>>> For cipher setup. + // The recipient doesn't need the
>>> (matching) IV because we will always + // pre-pad messages
>>> with the IV as a nonce. + new SecureRandom().nextBytes(iv);
>>> + + IvParameterSpec IV = new IvParameterSpec(iv);
>>
>>> + cipher.init(Cipher.ENCRYPT_MODE, getSecretKey(), IV); + +
>>> // Prepend the IV to the beginning of the encrypted data byte[][]
>>> data = new byte[2][]; - data[0] = cipher.update(iv, 0,
>>> iv.length); + data[0] = iv; data[1] =
>>> cipher.doFinal(bytes);
>>
>>> return data; @@ -373,10 +374,20 @@ public class EncryptInterceptor
>>> extends * @throws IllegalBlockSizeException If the input data
>>> cannot be encrypted * @throws BadPaddingException If the decrypted
>>> data does not include the * expected number of padding
>>> bytes - * + * @throws InvalidAlgorithmParameterException If
>>> the algorithm is invalid + * @throws InvalidKeyException If the
>>> key is invalid */ - private byte[] decrypt(byte[] bytes) throws
>>> IllegalBlockSizeException, BadPaddingException { - return
>>> getDecryptionCipher().doFinal(bytes); + private byte[]
>>> decrypt(byte[] bytes) throws IllegalBlockSizeException,
>>> BadPaddingException, InvalidKeyException,
>>> InvalidAlgorithmParameterException { + Cipher cipher =
>>> getDecryptionCipher(); + + int blockSize =
>>> cipher.getBlockSize(); + + // Use first-block of incoming
>>> data as IV + IvParameterSpec IV = new IvParameterSpec(bytes,
>>> 0, blockSize); + cipher.init(Cipher.DECRYPT_MODE,
>>> getSecretKey(), IV); + + // Decrypt remainder of the
>>> message. + return cipher.doFinal(bytes, blockSize,
>>> bytes.length - blockSize); }
>>
>>
>>
>>> Modified:
>>> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/LocalS
>> trings.properties
>>
>>
>> URL:
>> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/tribe
>> s/group/interceptors/LocalStrings.properties?rev=1847118&r1=1847117&r2=1
>> 847118&view=diff
>>> ======================================================================
>> ========
>>
>>
>> ---
>> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/LocalStr
>> ings.properties
>> [UTF-8] (original)
>>> +++
>>> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/LocalS
>> trings.properties
>>> [UTF-8] Wed Nov 21 15:15:34 2018 @@ -17,7 +17,7 @@
>>> domainFilterInterceptor.member.refused=M
>>> domainFilterInterceptor.message.refused=Received message from
>>> cluster[{0}] was refused.
>>
>>> encryptInterceptor.algorithm.required=Encryption algorithm is
>>> required, fully-specified e.g. AES/CBC/PKCS5Padding
>>> -encryptInterceptor.algorithm.requires-cbc-mode=EncryptInterceptor
>>> only supports CBC cipher modes, not [{0}]
>>> +encryptInterceptor.algorithm.unsupported-mode=EncryptInterceptor
>>> does not support block cipher mode [{0}]
>>> encryptInterceptor.decrypt.error.short-message=Failed to decrypt
>>> message: premature end-of-message
>>> encryptInterceptor.decrypt.failed=Failed to decrypt message
>>> encryptInterceptor.encrypt.failed=Failed to encrypt message
>>
>>> Modified:
>>> tomcat/trunk/test/org/apache/catalina/tribes/group/interceptors/TestEn
>> cryptInterceptor.java
>>
>>
>> URL:
>> http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/catalina/tribe
>> s/group/interceptors/TestEncryptInterceptor.java?rev=1847118&r1=1847117&
>> r2=1847118&view=diff
>>> ======================================================================
>> ========
>>
>>
>> ---
>> tomcat/trunk/test/org/apache/catalina/tribes/group/interceptors/TestEncr
>> yptInterceptor.java
>> (original)
>>> +++
>>> tomcat/trunk/test/org/apache/catalina/tribes/group/interceptors/TestEn
>> cryptInterceptor.java
>>> Wed Nov 21 15:15:34 2018 @@ -18,8 +18,14 @@ package
>>> org.apache.catalina.tribes.group
>>
>>> import org.junit.Assert; import org.junit.Before; +import
>>> org.junit.FixMethodOrder; import org.junit.Ignore; import
>>> org.junit.Test; +import org.junit.runners.MethodSorters; + +import
>>> java.io.File; +import java.io.FileInputStream; +import
>>> java.io.FileOutputStream;
>>
>>> import org.apache.catalina.tribes.Channel; import
>>> org.apache.catalina.tribes.ChannelException; @@ -38,8 +44,9 @@
>>> import org.apache.catalina.tribes.io.XBy * though the interceptor
>>> actually operates on byte arrays. This is done * for readability
>>> for the tests and their outputs. */
>>> +@FixMethodOrder(MethodSorters.NAME_ASCENDING) public class
>>> TestEncryptInterceptor { - private static final String
>>> encryptionKey128 = "cafebabedeadbeefcafebabedeadbeef"; + private
>>> static final String encryptionKey128 =
>>> "cafebabedeadbeefbeefcafecafebabe"; private static final String
>>> encryptionKey192 =
>>> "cafebabedeadbeefbeefcafecafebabedeadbeefbeefcafe"; private static
>>> final String encryptionKey256 =
>>> "cafebabedeadbeefcafebabedeadbeefcafebabedeadbeefcafebabedeadbeef";
>>
>>> @@ -95,7 +102,7 @@ public class TestEncryptInterceptor { }
>>
>>> @Test - @Ignore // Too big for default settings. Breaks Gump,
>>> Eclipse, ... + @Ignore("Too big for default settings. Breaks
>>> Gump, Eclipse, ...") public void testHugePayload() throws Exception
>>> { src.start(Channel.SND_TX_SEQ); dest.start(Channel.SND_TX_SEQ); @@
>>> -157,7 +164,7 @@ public class TestEncryptInterceptor {
>>
>>> bytes = roundTrip(bytes, src, dest);
>>
>>> - return new
>>> String(((ValueCaptureInterceptor)dest.getPrevious()).getValue(),
>>> "UTF-8"); + return new String(bytes, "UTF-8"); }
>>
>>> /** @@ -171,6 +178,123 @@ public class TestEncryptInterceptor {
>>> return ((ValueCaptureInterceptor)dest.getPrevious()).getValue(); }
>>
>>> + @Test + @Ignore("ECB mode isn't because it's insecure") +
>>> public void testECB() throws Exception { +
>>> src.setEncryptionAlgorithm("AES/ECB/PKCS5Padding"); +
>>> src.start(Channel.SND_TX_SEQ); +
>>> dest.setEncryptionAlgorithm("AES/ECB/PKCS5Padding"); +
>>> dest.start(Channel.SND_TX_SEQ); + + String testInput = "The
>>> quick brown fox jumps over the lazy dog."; + +
>>> Assert.assertEquals("Failed in ECB mode", +
>>> testInput, + roundTrip(testInput, src, dest));
>>> + } + + @Test + public void testOFB() throws Exception { +
>>> src.setEncryptionAlgorithm("AES/OFB/PKCS5Padding"); +
>>> src.start(Channel.SND_TX_SEQ); +
>>> dest.setEncryptionAlgorithm("AES/OFB/PKCS5Padding"); +
>>> dest.start(Channel.SND_TX_SEQ); + + String testInput = "The
>>> quick brown fox jumps over the lazy dog."; + +
>>> Assert.assertEquals("Failed in OFB mode", +
>>> testInput, + roundTrip(testInput, src, dest));
>>> + } + + @Test + public void testCFB() throws Exception { +
>>> src.setEncryptionAlgorithm("AES/CFB/PKCS5Padding"); +
>>> src.start(Channel.SND_TX_SEQ); +
>>> dest.setEncryptionAlgorithm("AES/CFB/PKCS5Padding"); +
>>> dest.start(Channel.SND_TX_SEQ); + + String testInput = "The
>>> quick brown fox jumps over the lazy dog."; + +
>>> Assert.assertEquals("Failed in CFB mode", +
>>> testInput, + roundTrip(testInput, src, dest));
>>> + } + + @Test + @Ignore("GCM mode is unsupported because
>>> it requires a custom initialization vector") + public void
>>> testGCM() throws Exception { +
>>> src.setEncryptionAlgorithm("AES/GCM/PKCS5Padding"); +
>>> src.start(Channel.SND_TX_SEQ); +
>>> dest.setEncryptionAlgorithm("AES/GCM/PKCS5Padding"); +
>>> dest.start(Channel.SND_TX_SEQ); + + String testInput = "The
>>> quick brown fox jumps over the lazy dog."; + +
>>> Assert.assertEquals("Failed in GCM mode", +
>>> testInput, + roundTrip(testInput, src, dest));
>>> + } + + @Test + public void testViaFile() throws Exception
>>> { + src.start(Channel.SND_TX_SEQ); + src.setNext(new
>>> ValueCaptureInterceptor()); + + String testInput = "The
>>> quick brown fox jumps over the lazy dog."; + + ChannelData
>>> msg = new ChannelData(false); + msg.setMessage(new
>>> XByteBuffer(testInput.getBytes("UTF-8"), false)); +
>>> src.sendMessage(null, msg, null); + + byte[] bytes =
>>> ((ValueCaptureInterceptor)src.getNext()).getValue(); + + try
>>> (FileOutputStream out = new FileOutputStream("message.bin")) { +
>>> out.write(bytes); + } + +
>>> dest.start(Channel.SND_TX_SEQ); + + bytes = new byte[8192];
>>> + int read; + + try (FileInputStream in = new
>>> FileInputStream("message.bin")) { + read =
>>> in.read(bytes); + } + + msg = new
>>> ChannelData(false); + XByteBuffer xbb = new
>>> XByteBuffer(read, false); + xbb.append(bytes, 0, read); +
>>> msg.setMessage(xbb); + + dest.messageReceived(msg); + }
>>> + + @Test + public void testPickup() throws Exception { +
>>> File file = new File("message.bin"); + if(!file.exists()) {
>>> + System.err.println("File message.bin does not exist.
>>> Skipping test."); + return; + } + +
>>> dest.start(Channel.SND_TX_SEQ); + + byte[] bytes = new
>>> byte[8192]; + int read; + + try (FileInputStream in =
>>> new FileInputStream("message.bin")) { + read =
>>> in.read(bytes); + } + + ChannelData msg = new
>>> ChannelData(false); + XByteBuffer xbb = new
>>> XByteBuffer(read, false); + xbb.append(bytes, 0, read); +
>>> msg.setMessage(xbb); + + dest.messageReceived(msg); + }
>>> + /** * Interceptor that delivers directly to a destination. */
>>
>>
>>
>>> ---------------------------------------------------------------------
>>
>>
>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1847118 - in /tomcat/trunk:
java/org/apache/catalina/tribes/group/interceptors/
test/org/apache/catalina/tribes/group/interceptors/
Posted by Mark Thomas <ma...@apache.org>.
On 21/11/2018 15:29, Christopher Schultz wrote:
> All,
>
> With this last patch, I'm ready for a back-port to tc8.5.x, but I'm
> waiting for a user who is trying to get this working on tc9.0 to be
> successful.
>
> If anyone else can confirm that this is all working in a real cluster
> (dev/test is okay) then I'll go ahead and back-port, assuming there is
> some kind of configuration error in that particular user's case.
I'll fire up my 4 node test cluster and let you know. It may take me a
while - there are usually a bunch of OS updates waiting for me when I
start it up.
Mark
>
> -chris
>
> On 11/21/18 10:15, schultz@apache.org wrote:
>> Author: schultz Date: Wed Nov 21 15:15:34 2018 New Revision:
>> 1847118
>
>> URL: http://svn.apache.org/viewvc?rev=1847118&view=rev Log: Use
>> random IVs for encryption. Support cipher block modes other than
>> CBC. Expand unit tests.
>
>
>> Modified:
>> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/Encryp
> tInterceptor.java
>
>
> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/LocalStr
> ings.properties
>> tomcat/trunk/test/org/apache/catalina/tribes/group/interceptors/TestEn
> cryptInterceptor.java
>
>> Modified:
>> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/Encryp
> tInterceptor.java
>
>
> URL:
> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/tribe
> s/group/interceptors/EncryptInterceptor.java?rev=1847118&r1=1847117&r2=1
> 847118&view=diff
>> ======================================================================
> ========
>
>
> ---
> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/EncryptI
> nterceptor.java
> (original)
>> +++
>> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/Encryp
> tInterceptor.java
>> Wed Nov 21 15:15:34 2018 @@ -17,6 +17,8 @@ package
>> org.apache.catalina.tribes.group.interceptors;
>
>> import java.security.GeneralSecurityException; +import
>> java.security.InvalidAlgorithmParameterException; +import
>> java.security.InvalidKeyException; import
>> java.security.SecureRandom;
>
>> import javax.crypto.BadPaddingException; @@ -59,6 +61,7 @@ public
>> class EncryptInterceptor extends private String encryptionAlgorithm
>> = DEFAULT_ENCRYPTION_ALGORITHM; private byte[] encryptionKeyBytes;
>> private String encryptionKeyString; + private SecretKeySpec
>> secretKey;
>
>> private Cipher encryptionCipher; private Cipher decryptionCipher;
>> @@ -92,7 +95,7 @@ public class EncryptInterceptor extends
>> XByteBuffer xbb = msg.getMessage();
>
>> // Completely replace the message - xbb.setLength(0); +
>> xbb.clear(); xbb.append(bytes[0], 0, bytes[0].length);
>> xbb.append(bytes[1], 0, bytes[1].length);
>
>> @@ -104,6 +107,12 @@ public class EncryptInterceptor extends }
>> catch (BadPaddingException bpe) {
>> log.error(sm.getString("encryptInterceptor.encrypt.failed")); throw
>> new ChannelException(bpe); + } catch (InvalidKeyException
>> ike) { +
>> log.error(sm.getString("encryptInterceptor.encrypt.failed")); +
>> throw new ChannelException(ike); + } catch
>> (InvalidAlgorithmParameterException iape) { +
>> log.error(sm.getString("encryptInterceptor.encrypt.failed")); +
>> throw new ChannelException(iape); } }
>
>> @@ -114,25 +123,21 @@ public class EncryptInterceptor extends
>
>> data = decrypt(data);
>
>> - // Remove the decrypted IV/nonce block from the front
>> of the message - int blockSize =
>> getDecryptionCipher().getBlockSize(); - int trimmedSize
>> = data.length - blockSize; - if(trimmedSize < 0) { -
>> log.error(sm.getString("encryptInterceptor.decrypt.error.short-message
> "));
>
>
> - throw new
> IllegalStateException(sm.getString("encryptInterceptor.decrypt.error.sho
> rt-message"));
>> - } - XByteBuffer xbb = msg.getMessage();
>
>> // Completely replace the message with the decrypted one -
>> xbb.setLength(0); - xbb.append(data, blockSize,
>> data.length - blockSize); + xbb.clear(); +
>> xbb.append(data, 0, data.length);
>
>> super.messageReceived(msg); } catch (IllegalBlockSizeException
>> ibse) {
>> log.error(sm.getString("encryptInterceptor.decrypt.failed"),
>> ibse); } catch (BadPaddingException bpe) {
>> log.error(sm.getString("encryptInterceptor.decrypt.failed"), bpe);
>> + } catch (InvalidKeyException ike) { +
>> log.error(sm.getString("encryptInterceptor.decrypt.failed"), ike);
>> + } catch (InvalidAlgorithmParameterException iape) { +
>> log.error(sm.getString("encryptInterceptor.decrypt.failed"),
>> iape); } }
>
>> @@ -262,55 +267,51 @@ public class EncryptInterceptor extends
>
>> String algorithm = getEncryptionAlgorithm();
>
>> - String mode = getAlgorithmMode(algorithm); - -
>> if(!"CBC".equalsIgnoreCase(mode)) - throw new
>> IllegalArgumentException(sm.getString("encryptInterceptor.algorithm.re
> quires-cbc-mode",
>> mode)); - - Cipher cipher; - - String providerName =
>> getProviderName(); - if(null == providerName) { -
>> cipher = Cipher.getInstance(algorithm); - } else { -
>> cipher = Cipher.getInstance(algorithm, getProviderName()); -
>> } - - byte[] iv = new byte[cipher.getBlockSize()]; +
>> String algorithmName; + String algorithmMode;
>
>> - // Always use a random IV For cipher setup. - //
>> The recipient doesn't need the (matching) IV because we will
>> always - // pre-pad messages with the IV as a nonce. -
>> new SecureRandom().nextBytes(iv); - - IvParameterSpec IV =
>> new IvParameterSpec(iv); - - // If this is a cipher
>> transform of the form ALGO/MODE/PAD, + // We need to
>> break-apart the algorithm name e.g. AES/CBC/PKCS5Padding // take
>> just the algorithm part. int pos = algorithm.indexOf('/');
>
>> - String bareAlgorithm; if(pos >= 0) { -
>> bareAlgorithm = algorithm.substring(0, pos); +
>> algorithmName = algorithm.substring(0, pos); + int pos2
>> = algorithm.indexOf('/', pos+1); + + if(pos2 >= 0) { +
>> algorithmMode = algorithm.substring(pos + 1, pos2); + }
>> else { + algorithmMode = "CBC"; + } }
>> else { - bareAlgorithm = algorithm; +
>> algorithmName = algorithm; + algorithmMode = "CBC"; }
>
>> - SecretKeySpec encryptionKey = new
>> SecretKeySpec(getEncryptionKey(), bareAlgorithm); + // Note:
>> ECB is not an appropriate mode for secure communications. +
>> if(!("CBC".equalsIgnoreCase(algorithmMode) + ||
>> "OFB".equalsIgnoreCase(algorithmMode) + ||
>> "CFB".equalsIgnoreCase(algorithmMode))) + throw new
>> IllegalArgumentException(sm.getString("encryptInterceptor.algorithm.un
> supported-mode",
>> algorithmMode));
>
>> - cipher.init(Cipher.ENCRYPT_MODE, encryptionKey, IV); - -
>> encryptionCipher = cipher; + setSecretKey(new
>> SecretKeySpec(getEncryptionKeyInternal(), algorithmName));
>
>> + String providerName = getProviderName(); if(null ==
>> providerName) { - cipher =
>> Cipher.getInstance(algorithm); + encryptionCipher =
>> Cipher.getInstance(algorithm); + decryptionCipher =
>> Cipher.getInstance(algorithm); } else { - cipher =
>> Cipher.getInstance(algorithm, getProviderName()); +
>> encryptionCipher = Cipher.getInstance(algorithm,
>> getProviderName()); + decryptionCipher =
>> Cipher.getInstance(algorithm, getProviderName()); } + }
>
>> - cipher.init(Cipher.DECRYPT_MODE, encryptionKey, new
>> IvParameterSpec(iv)); + private void setSecretKey(SecretKeySpec
>> secretKey) { + this.secretKey = secretKey; + }
>
>> - decryptionCipher = cipher; + private SecretKeySpec
>> getSecretKey() { + return secretKey; }
>
>> private Cipher getEncryptionCipher() { @@ -321,20 +322,9 @@ public
>> class EncryptInterceptor extends return decryptionCipher; }
>
>> - private static String getAlgorithmMode(String algorithm) { -
>> int start = algorithm.indexOf('/'); - if(start < 0) -
>> throw new
>> IllegalArgumentException(sm.getString("encryptInterceptor.algorithm.re
> quired"));
>
>
> - int end = algorithm.indexOf('/', start + 1);
>> - if(end < 0) - throw new
>> IllegalArgumentException(sm.getString("encryptInterceptor.algorithm.re
> quired"));
>
>
> -
>> - return algorithm.substring(start + 1, end); - } - /** *
>> Encrypts the input <code>bytes</code> into two separate byte
>> arrays: - * one for the initial block (which will be the
>> encrypted random IV) + * one for the random initialization
>> vector (IV) used for this message, * and the second one containing
>> the actual encrypted payload. * * This method returns a pair of
>> byte arrays instead of a single @@ -343,21 +333,32 @@ public class
>> EncryptInterceptor extends * * @param bytes The data to encrypt. *
>> - * @return The encrypted IV block in [0] and the encrypted
>> data in [1]. + * @return The IV in [0] and the encrypted data
>> in [1]. * * @throws IllegalBlockSizeException If the input data is
>> not a multiple of * the block size and no padding has
>> been requested (for block * ciphers) or if the input
>> data cannot be encrypted * @throws BadPaddingException Declared but
>> should not occur during encryption + * @throws
>> InvalidAlgorithmParameterException If the algorithm is invalid +
>> * @throws InvalidKeyException If the key is invalid */ - private
>> byte[][] encrypt(byte[] bytes) throws IllegalBlockSizeException,
>> BadPaddingException { + private byte[][] encrypt(byte[] bytes)
>> throws IllegalBlockSizeException, BadPaddingException,
>> InvalidKeyException, InvalidAlgorithmParameterException { Cipher
>> cipher = getEncryptionCipher();
>
>> - // Adding the IV to the beginning of the encrypted data -
>> byte[] iv = cipher.getIV(); + byte[] iv = new
>> byte[cipher.getBlockSize()]; + + // Always use a random IV
>> For cipher setup. + // The recipient doesn't need the
>> (matching) IV because we will always + // pre-pad messages
>> with the IV as a nonce. + new SecureRandom().nextBytes(iv);
>> + + IvParameterSpec IV = new IvParameterSpec(iv);
>
>> + cipher.init(Cipher.ENCRYPT_MODE, getSecretKey(), IV); + +
>> // Prepend the IV to the beginning of the encrypted data byte[][]
>> data = new byte[2][]; - data[0] = cipher.update(iv, 0,
>> iv.length); + data[0] = iv; data[1] =
>> cipher.doFinal(bytes);
>
>> return data; @@ -373,10 +374,20 @@ public class EncryptInterceptor
>> extends * @throws IllegalBlockSizeException If the input data
>> cannot be encrypted * @throws BadPaddingException If the decrypted
>> data does not include the * expected number of padding
>> bytes - * + * @throws InvalidAlgorithmParameterException If
>> the algorithm is invalid + * @throws InvalidKeyException If the
>> key is invalid */ - private byte[] decrypt(byte[] bytes) throws
>> IllegalBlockSizeException, BadPaddingException { - return
>> getDecryptionCipher().doFinal(bytes); + private byte[]
>> decrypt(byte[] bytes) throws IllegalBlockSizeException,
>> BadPaddingException, InvalidKeyException,
>> InvalidAlgorithmParameterException { + Cipher cipher =
>> getDecryptionCipher(); + + int blockSize =
>> cipher.getBlockSize(); + + // Use first-block of incoming
>> data as IV + IvParameterSpec IV = new IvParameterSpec(bytes,
>> 0, blockSize); + cipher.init(Cipher.DECRYPT_MODE,
>> getSecretKey(), IV); + + // Decrypt remainder of the
>> message. + return cipher.doFinal(bytes, blockSize,
>> bytes.length - blockSize); }
>
>
>
>> Modified:
>> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/LocalS
> trings.properties
>
>
> URL:
> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/tribe
> s/group/interceptors/LocalStrings.properties?rev=1847118&r1=1847117&r2=1
> 847118&view=diff
>> ======================================================================
> ========
>
>
> ---
> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/LocalStr
> ings.properties
> [UTF-8] (original)
>> +++
>> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/LocalS
> trings.properties
>> [UTF-8] Wed Nov 21 15:15:34 2018 @@ -17,7 +17,7 @@
>> domainFilterInterceptor.member.refused=M
>> domainFilterInterceptor.message.refused=Received message from
>> cluster[{0}] was refused.
>
>> encryptInterceptor.algorithm.required=Encryption algorithm is
>> required, fully-specified e.g. AES/CBC/PKCS5Padding
>> -encryptInterceptor.algorithm.requires-cbc-mode=EncryptInterceptor
>> only supports CBC cipher modes, not [{0}]
>> +encryptInterceptor.algorithm.unsupported-mode=EncryptInterceptor
>> does not support block cipher mode [{0}]
>> encryptInterceptor.decrypt.error.short-message=Failed to decrypt
>> message: premature end-of-message
>> encryptInterceptor.decrypt.failed=Failed to decrypt message
>> encryptInterceptor.encrypt.failed=Failed to encrypt message
>
>> Modified:
>> tomcat/trunk/test/org/apache/catalina/tribes/group/interceptors/TestEn
> cryptInterceptor.java
>
>
> URL:
> http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/catalina/tribe
> s/group/interceptors/TestEncryptInterceptor.java?rev=1847118&r1=1847117&
> r2=1847118&view=diff
>> ======================================================================
> ========
>
>
> ---
> tomcat/trunk/test/org/apache/catalina/tribes/group/interceptors/TestEncr
> yptInterceptor.java
> (original)
>> +++
>> tomcat/trunk/test/org/apache/catalina/tribes/group/interceptors/TestEn
> cryptInterceptor.java
>> Wed Nov 21 15:15:34 2018 @@ -18,8 +18,14 @@ package
>> org.apache.catalina.tribes.group
>
>> import org.junit.Assert; import org.junit.Before; +import
>> org.junit.FixMethodOrder; import org.junit.Ignore; import
>> org.junit.Test; +import org.junit.runners.MethodSorters; + +import
>> java.io.File; +import java.io.FileInputStream; +import
>> java.io.FileOutputStream;
>
>> import org.apache.catalina.tribes.Channel; import
>> org.apache.catalina.tribes.ChannelException; @@ -38,8 +44,9 @@
>> import org.apache.catalina.tribes.io.XBy * though the interceptor
>> actually operates on byte arrays. This is done * for readability
>> for the tests and their outputs. */
>> +@FixMethodOrder(MethodSorters.NAME_ASCENDING) public class
>> TestEncryptInterceptor { - private static final String
>> encryptionKey128 = "cafebabedeadbeefcafebabedeadbeef"; + private
>> static final String encryptionKey128 =
>> "cafebabedeadbeefbeefcafecafebabe"; private static final String
>> encryptionKey192 =
>> "cafebabedeadbeefbeefcafecafebabedeadbeefbeefcafe"; private static
>> final String encryptionKey256 =
>> "cafebabedeadbeefcafebabedeadbeefcafebabedeadbeefcafebabedeadbeef";
>
>> @@ -95,7 +102,7 @@ public class TestEncryptInterceptor { }
>
>> @Test - @Ignore // Too big for default settings. Breaks Gump,
>> Eclipse, ... + @Ignore("Too big for default settings. Breaks
>> Gump, Eclipse, ...") public void testHugePayload() throws Exception
>> { src.start(Channel.SND_TX_SEQ); dest.start(Channel.SND_TX_SEQ); @@
>> -157,7 +164,7 @@ public class TestEncryptInterceptor {
>
>> bytes = roundTrip(bytes, src, dest);
>
>> - return new
>> String(((ValueCaptureInterceptor)dest.getPrevious()).getValue(),
>> "UTF-8"); + return new String(bytes, "UTF-8"); }
>
>> /** @@ -171,6 +178,123 @@ public class TestEncryptInterceptor {
>> return ((ValueCaptureInterceptor)dest.getPrevious()).getValue(); }
>
>> + @Test + @Ignore("ECB mode isn't because it's insecure") +
>> public void testECB() throws Exception { +
>> src.setEncryptionAlgorithm("AES/ECB/PKCS5Padding"); +
>> src.start(Channel.SND_TX_SEQ); +
>> dest.setEncryptionAlgorithm("AES/ECB/PKCS5Padding"); +
>> dest.start(Channel.SND_TX_SEQ); + + String testInput = "The
>> quick brown fox jumps over the lazy dog."; + +
>> Assert.assertEquals("Failed in ECB mode", +
>> testInput, + roundTrip(testInput, src, dest));
>> + } + + @Test + public void testOFB() throws Exception { +
>> src.setEncryptionAlgorithm("AES/OFB/PKCS5Padding"); +
>> src.start(Channel.SND_TX_SEQ); +
>> dest.setEncryptionAlgorithm("AES/OFB/PKCS5Padding"); +
>> dest.start(Channel.SND_TX_SEQ); + + String testInput = "The
>> quick brown fox jumps over the lazy dog."; + +
>> Assert.assertEquals("Failed in OFB mode", +
>> testInput, + roundTrip(testInput, src, dest));
>> + } + + @Test + public void testCFB() throws Exception { +
>> src.setEncryptionAlgorithm("AES/CFB/PKCS5Padding"); +
>> src.start(Channel.SND_TX_SEQ); +
>> dest.setEncryptionAlgorithm("AES/CFB/PKCS5Padding"); +
>> dest.start(Channel.SND_TX_SEQ); + + String testInput = "The
>> quick brown fox jumps over the lazy dog."; + +
>> Assert.assertEquals("Failed in CFB mode", +
>> testInput, + roundTrip(testInput, src, dest));
>> + } + + @Test + @Ignore("GCM mode is unsupported because
>> it requires a custom initialization vector") + public void
>> testGCM() throws Exception { +
>> src.setEncryptionAlgorithm("AES/GCM/PKCS5Padding"); +
>> src.start(Channel.SND_TX_SEQ); +
>> dest.setEncryptionAlgorithm("AES/GCM/PKCS5Padding"); +
>> dest.start(Channel.SND_TX_SEQ); + + String testInput = "The
>> quick brown fox jumps over the lazy dog."; + +
>> Assert.assertEquals("Failed in GCM mode", +
>> testInput, + roundTrip(testInput, src, dest));
>> + } + + @Test + public void testViaFile() throws Exception
>> { + src.start(Channel.SND_TX_SEQ); + src.setNext(new
>> ValueCaptureInterceptor()); + + String testInput = "The
>> quick brown fox jumps over the lazy dog."; + + ChannelData
>> msg = new ChannelData(false); + msg.setMessage(new
>> XByteBuffer(testInput.getBytes("UTF-8"), false)); +
>> src.sendMessage(null, msg, null); + + byte[] bytes =
>> ((ValueCaptureInterceptor)src.getNext()).getValue(); + + try
>> (FileOutputStream out = new FileOutputStream("message.bin")) { +
>> out.write(bytes); + } + +
>> dest.start(Channel.SND_TX_SEQ); + + bytes = new byte[8192];
>> + int read; + + try (FileInputStream in = new
>> FileInputStream("message.bin")) { + read =
>> in.read(bytes); + } + + msg = new
>> ChannelData(false); + XByteBuffer xbb = new
>> XByteBuffer(read, false); + xbb.append(bytes, 0, read); +
>> msg.setMessage(xbb); + + dest.messageReceived(msg); + }
>> + + @Test + public void testPickup() throws Exception { +
>> File file = new File("message.bin"); + if(!file.exists()) {
>> + System.err.println("File message.bin does not exist.
>> Skipping test."); + return; + } + +
>> dest.start(Channel.SND_TX_SEQ); + + byte[] bytes = new
>> byte[8192]; + int read; + + try (FileInputStream in =
>> new FileInputStream("message.bin")) { + read =
>> in.read(bytes); + } + + ChannelData msg = new
>> ChannelData(false); + XByteBuffer xbb = new
>> XByteBuffer(read, false); + xbb.append(bytes, 0, read); +
>> msg.setMessage(xbb); + + dest.messageReceived(msg); + }
>> + /** * Interceptor that delivers directly to a destination. */
>
>
>
>> ---------------------------------------------------------------------
>
>
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1847118 - in /tomcat/trunk:
java/org/apache/catalina/tribes/group/interceptors/
test/org/apache/catalina/tribes/group/interceptors/
Posted by Christopher Schultz <ch...@christopherschultz.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
All,
With this last patch, I'm ready for a back-port to tc8.5.x, but I'm
waiting for a user who is trying to get this working on tc9.0 to be
successful.
If anyone else can confirm that this is all working in a real cluster
(dev/test is okay) then I'll go ahead and back-port, assuming there is
some kind of configuration error in that particular user's case.
- -chris
On 11/21/18 10:15, schultz@apache.org wrote:
> Author: schultz Date: Wed Nov 21 15:15:34 2018 New Revision:
> 1847118
>
> URL: http://svn.apache.org/viewvc?rev=1847118&view=rev Log: Use
> random IVs for encryption. Support cipher block modes other than
> CBC. Expand unit tests.
>
>
> Modified:
> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/Encryp
tInterceptor.java
>
>
tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/LocalStr
ings.properties
> tomcat/trunk/test/org/apache/catalina/tribes/group/interceptors/TestEn
cryptInterceptor.java
>
> Modified:
> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/Encryp
tInterceptor.java
>
>
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/tribe
s/group/interceptors/EncryptInterceptor.java?rev=1847118&r1=1847117&r2=1
847118&view=diff
> ======================================================================
========
>
>
- ---
tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/EncryptI
nterceptor.java
(original)
> +++
> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/Encryp
tInterceptor.java
> Wed Nov 21 15:15:34 2018 @@ -17,6 +17,8 @@ package
> org.apache.catalina.tribes.group.interceptors;
>
> import java.security.GeneralSecurityException; +import
> java.security.InvalidAlgorithmParameterException; +import
> java.security.InvalidKeyException; import
> java.security.SecureRandom;
>
> import javax.crypto.BadPaddingException; @@ -59,6 +61,7 @@ public
> class EncryptInterceptor extends private String encryptionAlgorithm
> = DEFAULT_ENCRYPTION_ALGORITHM; private byte[] encryptionKeyBytes;
> private String encryptionKeyString; + private SecretKeySpec
> secretKey;
>
> private Cipher encryptionCipher; private Cipher decryptionCipher;
> @@ -92,7 +95,7 @@ public class EncryptInterceptor extends
> XByteBuffer xbb = msg.getMessage();
>
> // Completely replace the message - xbb.setLength(0); +
> xbb.clear(); xbb.append(bytes[0], 0, bytes[0].length);
> xbb.append(bytes[1], 0, bytes[1].length);
>
> @@ -104,6 +107,12 @@ public class EncryptInterceptor extends }
> catch (BadPaddingException bpe) {
> log.error(sm.getString("encryptInterceptor.encrypt.failed")); throw
> new ChannelException(bpe); + } catch (InvalidKeyException
> ike) { +
> log.error(sm.getString("encryptInterceptor.encrypt.failed")); +
> throw new ChannelException(ike); + } catch
> (InvalidAlgorithmParameterException iape) { +
> log.error(sm.getString("encryptInterceptor.encrypt.failed")); +
> throw new ChannelException(iape); } }
>
> @@ -114,25 +123,21 @@ public class EncryptInterceptor extends
>
> data = decrypt(data);
>
> - // Remove the decrypted IV/nonce block from the front
> of the message - int blockSize =
> getDecryptionCipher().getBlockSize(); - int trimmedSize
> = data.length - blockSize; - if(trimmedSize < 0) { -
> log.error(sm.getString("encryptInterceptor.decrypt.error.short-message
"));
>
>
- - throw new
IllegalStateException(sm.getString("encryptInterceptor.decrypt.error.sho
rt-message"));
> - } - XByteBuffer xbb = msg.getMessage();
>
> // Completely replace the message with the decrypted one -
> xbb.setLength(0); - xbb.append(data, blockSize,
> data.length - blockSize); + xbb.clear(); +
> xbb.append(data, 0, data.length);
>
> super.messageReceived(msg); } catch (IllegalBlockSizeException
> ibse) {
> log.error(sm.getString("encryptInterceptor.decrypt.failed"),
> ibse); } catch (BadPaddingException bpe) {
> log.error(sm.getString("encryptInterceptor.decrypt.failed"), bpe);
> + } catch (InvalidKeyException ike) { +
> log.error(sm.getString("encryptInterceptor.decrypt.failed"), ike);
> + } catch (InvalidAlgorithmParameterException iape) { +
> log.error(sm.getString("encryptInterceptor.decrypt.failed"),
> iape); } }
>
> @@ -262,55 +267,51 @@ public class EncryptInterceptor extends
>
> String algorithm = getEncryptionAlgorithm();
>
> - String mode = getAlgorithmMode(algorithm); - -
> if(!"CBC".equalsIgnoreCase(mode)) - throw new
> IllegalArgumentException(sm.getString("encryptInterceptor.algorithm.re
quires-cbc-mode",
> mode)); - - Cipher cipher; - - String providerName =
> getProviderName(); - if(null == providerName) { -
> cipher = Cipher.getInstance(algorithm); - } else { -
> cipher = Cipher.getInstance(algorithm, getProviderName()); -
> } - - byte[] iv = new byte[cipher.getBlockSize()]; +
> String algorithmName; + String algorithmMode;
>
> - // Always use a random IV For cipher setup. - //
> The recipient doesn't need the (matching) IV because we will
> always - // pre-pad messages with the IV as a nonce. -
> new SecureRandom().nextBytes(iv); - - IvParameterSpec IV =
> new IvParameterSpec(iv); - - // If this is a cipher
> transform of the form ALGO/MODE/PAD, + // We need to
> break-apart the algorithm name e.g. AES/CBC/PKCS5Padding // take
> just the algorithm part. int pos = algorithm.indexOf('/');
>
> - String bareAlgorithm; if(pos >= 0) { -
> bareAlgorithm = algorithm.substring(0, pos); +
> algorithmName = algorithm.substring(0, pos); + int pos2
> = algorithm.indexOf('/', pos+1); + + if(pos2 >= 0) { +
> algorithmMode = algorithm.substring(pos + 1, pos2); + }
> else { + algorithmMode = "CBC"; + } }
> else { - bareAlgorithm = algorithm; +
> algorithmName = algorithm; + algorithmMode = "CBC"; }
>
> - SecretKeySpec encryptionKey = new
> SecretKeySpec(getEncryptionKey(), bareAlgorithm); + // Note:
> ECB is not an appropriate mode for secure communications. +
> if(!("CBC".equalsIgnoreCase(algorithmMode) + ||
> "OFB".equalsIgnoreCase(algorithmMode) + ||
> "CFB".equalsIgnoreCase(algorithmMode))) + throw new
> IllegalArgumentException(sm.getString("encryptInterceptor.algorithm.un
supported-mode",
> algorithmMode));
>
> - cipher.init(Cipher.ENCRYPT_MODE, encryptionKey, IV); - -
> encryptionCipher = cipher; + setSecretKey(new
> SecretKeySpec(getEncryptionKeyInternal(), algorithmName));
>
> + String providerName = getProviderName(); if(null ==
> providerName) { - cipher =
> Cipher.getInstance(algorithm); + encryptionCipher =
> Cipher.getInstance(algorithm); + decryptionCipher =
> Cipher.getInstance(algorithm); } else { - cipher =
> Cipher.getInstance(algorithm, getProviderName()); +
> encryptionCipher = Cipher.getInstance(algorithm,
> getProviderName()); + decryptionCipher =
> Cipher.getInstance(algorithm, getProviderName()); } + }
>
> - cipher.init(Cipher.DECRYPT_MODE, encryptionKey, new
> IvParameterSpec(iv)); + private void setSecretKey(SecretKeySpec
> secretKey) { + this.secretKey = secretKey; + }
>
> - decryptionCipher = cipher; + private SecretKeySpec
> getSecretKey() { + return secretKey; }
>
> private Cipher getEncryptionCipher() { @@ -321,20 +322,9 @@ public
> class EncryptInterceptor extends return decryptionCipher; }
>
> - private static String getAlgorithmMode(String algorithm) { -
> int start = algorithm.indexOf('/'); - if(start < 0) -
> throw new
> IllegalArgumentException(sm.getString("encryptInterceptor.algorithm.re
quired"));
>
>
- - int end = algorithm.indexOf('/', start + 1);
> - if(end < 0) - throw new
> IllegalArgumentException(sm.getString("encryptInterceptor.algorithm.re
quired"));
>
>
- -
> - return algorithm.substring(start + 1, end); - } - /** *
> Encrypts the input <code>bytes</code> into two separate byte
> arrays: - * one for the initial block (which will be the
> encrypted random IV) + * one for the random initialization
> vector (IV) used for this message, * and the second one containing
> the actual encrypted payload. * * This method returns a pair of
> byte arrays instead of a single @@ -343,21 +333,32 @@ public class
> EncryptInterceptor extends * * @param bytes The data to encrypt. *
> - * @return The encrypted IV block in [0] and the encrypted
> data in [1]. + * @return The IV in [0] and the encrypted data
> in [1]. * * @throws IllegalBlockSizeException If the input data is
> not a multiple of * the block size and no padding has
> been requested (for block * ciphers) or if the input
> data cannot be encrypted * @throws BadPaddingException Declared but
> should not occur during encryption + * @throws
> InvalidAlgorithmParameterException If the algorithm is invalid +
> * @throws InvalidKeyException If the key is invalid */ - private
> byte[][] encrypt(byte[] bytes) throws IllegalBlockSizeException,
> BadPaddingException { + private byte[][] encrypt(byte[] bytes)
> throws IllegalBlockSizeException, BadPaddingException,
> InvalidKeyException, InvalidAlgorithmParameterException { Cipher
> cipher = getEncryptionCipher();
>
> - // Adding the IV to the beginning of the encrypted data -
> byte[] iv = cipher.getIV(); + byte[] iv = new
> byte[cipher.getBlockSize()]; + + // Always use a random IV
> For cipher setup. + // The recipient doesn't need the
> (matching) IV because we will always + // pre-pad messages
> with the IV as a nonce. + new SecureRandom().nextBytes(iv);
> + + IvParameterSpec IV = new IvParameterSpec(iv);
>
> + cipher.init(Cipher.ENCRYPT_MODE, getSecretKey(), IV); + +
> // Prepend the IV to the beginning of the encrypted data byte[][]
> data = new byte[2][]; - data[0] = cipher.update(iv, 0,
> iv.length); + data[0] = iv; data[1] =
> cipher.doFinal(bytes);
>
> return data; @@ -373,10 +374,20 @@ public class EncryptInterceptor
> extends * @throws IllegalBlockSizeException If the input data
> cannot be encrypted * @throws BadPaddingException If the decrypted
> data does not include the * expected number of padding
> bytes - * + * @throws InvalidAlgorithmParameterException If
> the algorithm is invalid + * @throws InvalidKeyException If the
> key is invalid */ - private byte[] decrypt(byte[] bytes) throws
> IllegalBlockSizeException, BadPaddingException { - return
> getDecryptionCipher().doFinal(bytes); + private byte[]
> decrypt(byte[] bytes) throws IllegalBlockSizeException,
> BadPaddingException, InvalidKeyException,
> InvalidAlgorithmParameterException { + Cipher cipher =
> getDecryptionCipher(); + + int blockSize =
> cipher.getBlockSize(); + + // Use first-block of incoming
> data as IV + IvParameterSpec IV = new IvParameterSpec(bytes,
> 0, blockSize); + cipher.init(Cipher.DECRYPT_MODE,
> getSecretKey(), IV); + + // Decrypt remainder of the
> message. + return cipher.doFinal(bytes, blockSize,
> bytes.length - blockSize); }
>
>
>
> Modified:
> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/LocalS
trings.properties
>
>
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/tribe
s/group/interceptors/LocalStrings.properties?rev=1847118&r1=1847117&r2=1
847118&view=diff
> ======================================================================
========
>
>
- ---
tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/LocalStr
ings.properties
[UTF-8] (original)
> +++
> tomcat/trunk/java/org/apache/catalina/tribes/group/interceptors/LocalS
trings.properties
> [UTF-8] Wed Nov 21 15:15:34 2018 @@ -17,7 +17,7 @@
> domainFilterInterceptor.member.refused=M
> domainFilterInterceptor.message.refused=Received message from
> cluster[{0}] was refused.
>
> encryptInterceptor.algorithm.required=Encryption algorithm is
> required, fully-specified e.g. AES/CBC/PKCS5Padding
> -encryptInterceptor.algorithm.requires-cbc-mode=EncryptInterceptor
> only supports CBC cipher modes, not [{0}]
> +encryptInterceptor.algorithm.unsupported-mode=EncryptInterceptor
> does not support block cipher mode [{0}]
> encryptInterceptor.decrypt.error.short-message=Failed to decrypt
> message: premature end-of-message
> encryptInterceptor.decrypt.failed=Failed to decrypt message
> encryptInterceptor.encrypt.failed=Failed to encrypt message
>
> Modified:
> tomcat/trunk/test/org/apache/catalina/tribes/group/interceptors/TestEn
cryptInterceptor.java
>
>
URL:
http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/catalina/tribe
s/group/interceptors/TestEncryptInterceptor.java?rev=1847118&r1=1847117&
r2=1847118&view=diff
> ======================================================================
========
>
>
- ---
tomcat/trunk/test/org/apache/catalina/tribes/group/interceptors/TestEncr
yptInterceptor.java
(original)
> +++
> tomcat/trunk/test/org/apache/catalina/tribes/group/interceptors/TestEn
cryptInterceptor.java
> Wed Nov 21 15:15:34 2018 @@ -18,8 +18,14 @@ package
> org.apache.catalina.tribes.group
>
> import org.junit.Assert; import org.junit.Before; +import
> org.junit.FixMethodOrder; import org.junit.Ignore; import
> org.junit.Test; +import org.junit.runners.MethodSorters; + +import
> java.io.File; +import java.io.FileInputStream; +import
> java.io.FileOutputStream;
>
> import org.apache.catalina.tribes.Channel; import
> org.apache.catalina.tribes.ChannelException; @@ -38,8 +44,9 @@
> import org.apache.catalina.tribes.io.XBy * though the interceptor
> actually operates on byte arrays. This is done * for readability
> for the tests and their outputs. */
> +@FixMethodOrder(MethodSorters.NAME_ASCENDING) public class
> TestEncryptInterceptor { - private static final String
> encryptionKey128 = "cafebabedeadbeefcafebabedeadbeef"; + private
> static final String encryptionKey128 =
> "cafebabedeadbeefbeefcafecafebabe"; private static final String
> encryptionKey192 =
> "cafebabedeadbeefbeefcafecafebabedeadbeefbeefcafe"; private static
> final String encryptionKey256 =
> "cafebabedeadbeefcafebabedeadbeefcafebabedeadbeefcafebabedeadbeef";
>
> @@ -95,7 +102,7 @@ public class TestEncryptInterceptor { }
>
> @Test - @Ignore // Too big for default settings. Breaks Gump,
> Eclipse, ... + @Ignore("Too big for default settings. Breaks
> Gump, Eclipse, ...") public void testHugePayload() throws Exception
> { src.start(Channel.SND_TX_SEQ); dest.start(Channel.SND_TX_SEQ); @@
> -157,7 +164,7 @@ public class TestEncryptInterceptor {
>
> bytes = roundTrip(bytes, src, dest);
>
> - return new
> String(((ValueCaptureInterceptor)dest.getPrevious()).getValue(),
> "UTF-8"); + return new String(bytes, "UTF-8"); }
>
> /** @@ -171,6 +178,123 @@ public class TestEncryptInterceptor {
> return ((ValueCaptureInterceptor)dest.getPrevious()).getValue(); }
>
> + @Test + @Ignore("ECB mode isn't because it's insecure") +
> public void testECB() throws Exception { +
> src.setEncryptionAlgorithm("AES/ECB/PKCS5Padding"); +
> src.start(Channel.SND_TX_SEQ); +
> dest.setEncryptionAlgorithm("AES/ECB/PKCS5Padding"); +
> dest.start(Channel.SND_TX_SEQ); + + String testInput = "The
> quick brown fox jumps over the lazy dog."; + +
> Assert.assertEquals("Failed in ECB mode", +
> testInput, + roundTrip(testInput, src, dest));
> + } + + @Test + public void testOFB() throws Exception { +
> src.setEncryptionAlgorithm("AES/OFB/PKCS5Padding"); +
> src.start(Channel.SND_TX_SEQ); +
> dest.setEncryptionAlgorithm("AES/OFB/PKCS5Padding"); +
> dest.start(Channel.SND_TX_SEQ); + + String testInput = "The
> quick brown fox jumps over the lazy dog."; + +
> Assert.assertEquals("Failed in OFB mode", +
> testInput, + roundTrip(testInput, src, dest));
> + } + + @Test + public void testCFB() throws Exception { +
> src.setEncryptionAlgorithm("AES/CFB/PKCS5Padding"); +
> src.start(Channel.SND_TX_SEQ); +
> dest.setEncryptionAlgorithm("AES/CFB/PKCS5Padding"); +
> dest.start(Channel.SND_TX_SEQ); + + String testInput = "The
> quick brown fox jumps over the lazy dog."; + +
> Assert.assertEquals("Failed in CFB mode", +
> testInput, + roundTrip(testInput, src, dest));
> + } + + @Test + @Ignore("GCM mode is unsupported because
> it requires a custom initialization vector") + public void
> testGCM() throws Exception { +
> src.setEncryptionAlgorithm("AES/GCM/PKCS5Padding"); +
> src.start(Channel.SND_TX_SEQ); +
> dest.setEncryptionAlgorithm("AES/GCM/PKCS5Padding"); +
> dest.start(Channel.SND_TX_SEQ); + + String testInput = "The
> quick brown fox jumps over the lazy dog."; + +
> Assert.assertEquals("Failed in GCM mode", +
> testInput, + roundTrip(testInput, src, dest));
> + } + + @Test + public void testViaFile() throws Exception
> { + src.start(Channel.SND_TX_SEQ); + src.setNext(new
> ValueCaptureInterceptor()); + + String testInput = "The
> quick brown fox jumps over the lazy dog."; + + ChannelData
> msg = new ChannelData(false); + msg.setMessage(new
> XByteBuffer(testInput.getBytes("UTF-8"), false)); +
> src.sendMessage(null, msg, null); + + byte[] bytes =
> ((ValueCaptureInterceptor)src.getNext()).getValue(); + + try
> (FileOutputStream out = new FileOutputStream("message.bin")) { +
> out.write(bytes); + } + +
> dest.start(Channel.SND_TX_SEQ); + + bytes = new byte[8192];
> + int read; + + try (FileInputStream in = new
> FileInputStream("message.bin")) { + read =
> in.read(bytes); + } + + msg = new
> ChannelData(false); + XByteBuffer xbb = new
> XByteBuffer(read, false); + xbb.append(bytes, 0, read); +
> msg.setMessage(xbb); + + dest.messageReceived(msg); + }
> + + @Test + public void testPickup() throws Exception { +
> File file = new File("message.bin"); + if(!file.exists()) {
> + System.err.println("File message.bin does not exist.
> Skipping test."); + return; + } + +
> dest.start(Channel.SND_TX_SEQ); + + byte[] bytes = new
> byte[8192]; + int read; + + try (FileInputStream in =
> new FileInputStream("message.bin")) { + read =
> in.read(bytes); + } + + ChannelData msg = new
> ChannelData(false); + XByteBuffer xbb = new
> XByteBuffer(read, false); + xbb.append(bytes, 0, read); +
> msg.setMessage(xbb); + + dest.messageReceived(msg); + }
> + /** * Interceptor that delivers directly to a destination. */
>
>
>
> ---------------------------------------------------------------------
>
>
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
-----BEGIN PGP SIGNATURE-----
Comment: Using GnuPG with Thunderbird - https://www.enigmail.net/
iQIzBAEBCAAdFiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAlv1ed0ACgkQHPApP6U8
pFigtQ/8DqpXZ5lZrU/Wxxa7cXyFvJrzMUjnsMIkcl29e4pZtPeWT6OImyWud2FC
VNtlGBrUhk2xOHLntS562mK0EJjXPWIjCsw2VRETZhKhV/SDpGEhJzgbkMLWdLHQ
sh7egymxIJPyG3NswOyQa6EDSTFz0EA7n9AN3rZ1+Eko38C5CuQEdt2Ew3tlFKAM
F/k/zv90/ZhhQHFPjFkLxokP5SOPdRIMN/cXSgZn4C8mUzsONPRr+Sscp+ztERBM
532fZo69pgyjXFHqTed+95xNMaRMsYpgjrKVxGtlVBjrrgsThSeEwvEVe+/Hshb/
3hASRPLmMMXK6cuuqgxDUCtKNY0Z126GVuUYlqCah1PeJnvEY+fhH5hYDp2Nje0T
xWM3NkJslokel7t25VdmSEbuqHhbqI3uHihaMQTQrFgfj7GDykTZgGb0AIGGvrB0
W795VDD73RKx/wQQxrbc6fQ0yqdx2qOmi0wsClr0Dge8a3xltNW9PB2PfGGGmf+Z
3iI/Z1pFAHM3HFa6fcnPqz4+avrasxGpRpbaRxmM5GVk0I4fqjmHxHLkisuwjrWj
zrL1F3kyUOtV9kYLX0jKsHR5ZgVcosozwIG6ph2ezhSnUXO84BHTcE05IsSRApeT
rfayfZeHo9ja//xgqGgrD8idf+BFeV3c7vQYBLDnPe+Njp4qOOg=
=ewzI
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org