subject:"svn commit\: r1847118 \- in \/tomcat\/trunk\: java\/org\/apache\/catalina\/tribes\/group\/interceptors\/ test\/org\/apache\/catalina\/tribes\/group\/interceptors\/"

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

Re: svn commit: r1847118 - in /tomcat/trunk: java/org/apache/catalina/tribes/group/interceptors/ test/org/apache/catalina/tribes/group/interceptors/

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:

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

Re: svn commit: r1847118 - in /tomcat/trunk: java/org/apache/catalina/tribes/group/interceptors/ test/org/apache/catalina/tribes/group/interceptors/

-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,
>> 
> 
 
>>> 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
> 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 { -

Re: svn commit: r1847118 - in /tomcat/trunk: java/org/apache/catalina/tribes/group/interceptors/ test/org/apache/catalina/tribes/group/interceptors/


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,






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

Re: svn commit: r1847118 - in /tomcat/trunk: java/org/apache/catalina/tribes/group/interceptors/ test/org/apache/catalina/tribes/group/interceptors/

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

Re: svn commit: r1847118 - in /tomcat/trunk: java/org/apache/catalina/tribes/group/interceptors/ test/org/apache/catalina/tribes/group/interceptors/

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

Re: svn commit: r1847118 - in /tomcat/trunk: java/org/apache/catalina/tribes/group/interceptors/ test/org/apache/catalina/tribes/group/interceptors/

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

Re: svn commit: r1847118 - in /tomcat/trunk: java/org/apache/catalina/tribes/group/interceptors/ test/org/apache/catalina/tribes/group/interceptors/

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

Re: svn commit: r1847118 - in /tomcat/trunk: java/org/apache/catalina/tribes/group/interceptors/ test/org/apache/catalina/tribes/group/interceptors/

On 21/11/2018 22:39, Christopher Schultz wrote:
> Mark,
> 


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

Re: svn commit: r1847118 - in /tomcat/trunk: java/org/apache/catalina/tribes/group/interceptors/ test/org/apache/catalina/tribes/group/interceptors/

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

Re: svn commit: r1847118 - in /tomcat/trunk: java/org/apache/catalina/tribes/group/interceptors/ test/org/apache/catalina/tribes/group/interceptors/

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

Re: svn commit: r1847118 - in /tomcat/trunk: java/org/apache/catalina/tribes/group/interceptors/ test/org/apache/catalina/tribes/group/interceptors/

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

Re: svn commit: r1847118 - in /tomcat/trunk: java/org/apache/catalina/tribes/group/interceptors/ test/org/apache/catalina/tribes/group/interceptors/

-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, schu...@apache.org wrote:
 Author: schultz Date: Wed Nov 21 15:15:34 2018 New Revision: 
 1847118
>>> 
 URL: http://svn.apache.org/viewvc?rev=1847118=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=1847117=1
>>> 847118=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(); +

Re: svn commit: r1847118 - in /tomcat/trunk: java/org/apache/catalina/tribes/group/interceptors/ test/org/apache/catalina/tribes/group/interceptors/

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

Re: svn commit: r1847118 - in /tomcat/trunk: java/org/apache/catalina/tribes/group/interceptors/ test/org/apache/catalina/tribes/group/interceptors/

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, schu...@apache.org wrote:
>>> Author: schultz Date: Wed Nov 21 15:15:34 2018 New Revision:
>>> 1847118
>>
>>> URL: http://svn.apache.org/viewvc?rev=1847118=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=1847117=1
>> 847118=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"),

Re: svn commit: r1847118 - in /tomcat/trunk: java/org/apache/catalina/tribes/group/interceptors/ test/org/apache/catalina/tribes/group/interceptors/

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, schu...@apache.org wrote:
>> Author: schultz Date: Wed Nov 21 15:15:34 2018 New Revision:
>> 1847118
> 
>> URL: http://svn.apache.org/viewvc?rev=1847118=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=1847117=1
> 847118=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

Re: svn commit: r1847118 - in /tomcat/trunk: java/org/apache/catalina/tribes/group/interceptors/ test/org/apache/catalina/tribes/group/interceptors/