-----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-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
