Hi Edmund,

many thanks for your first feedback.

> this is an enormous amount of code to review so this will take some time

Actually its mostly the old code squeezed into new classes. But we can take all the time we want to work on it.

> I would recommend changing the code to using AES/CBC/PKCS5Padding with

Sure, my first try was just using what popped up first in a Google search :P. It was just a nice opportunity to show how the new store chain is capable of adding encryption without much hassle.

I've changed the crypt implementation now, I hope it performs better that way.

Have fun
Sven


generated key:

Am 09.07.2018 um 17:36 schrieb Emond Papegaaij:
Hi Sven,

Thanks for the work you have put into this. However, this is an enormous
amount of code to review so this will take some time. I'll try to get this
done this week, but I can't promise anything.

For now, I can at least comment on CryptingPageStore.java. The ciphers used by
this store are both obsolete and not applicable to this use case. It currently
uses a Password Based Encryption (PBE) and a very insecure one (single DES). I
would recommend changing the code to using AES/CBC/PKCS5Padding with a
generated key:

        SecureRandom rnd = SecureRandom.getInstance("SHA1PRNG", "SUN");
        KeyGenerator keyGen = KeyGenerator.getInstance("AES");
        keyGen.init(256, rnd);
        SecretKey key = keyGen.generateKey();

This key is serializable and can be stored in the session directly. Make sure
that rnd is a SecureRandom with a strong algorithm, and very importantly: it
must be reused. Due to application specific demands on this SecureRandom (such
as automatic reseeding), I think users must be able to provide their own. To
encrypt a page, use:

        Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
        cipher.init(Cipher.ENCRYPT_MODE, key, rnd);
        AlgorithmParameters params = cipher.getParameters();
        byte[] iv = params.getParameterSpec(IvParameterSpec.class).getIV();
        byte[] ciphertext = cipher.doFinal(plainpagebytes);
        byte[] result = Bytes.concat(iv, ciphertext);

Use the same SecureRandom for the cipher.init. This will be used to generate
the IV.  To decrypt the page, use:

        byte[] iv = new byte[16];
        byte[] ciphertext = new byte[cipherInput.length - 16];
        System.arraycopy(cipherInput, 0, iv, 0, iv.length);
        System.arraycopy(cipherInput, 16, ciphertext, 0, ciphertext.length);

        Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
        cipher.init(Cipher.DECRYPT_MODE, key, new IvParameterSpec(iv));
        byte[] plainpagebytes = cipher.doFinal(ciphertext);

This should give a much stronger and faster algorithm than using
PBEWithMD5AndDES.

Best regards,
Emond

On maandag 9 juli 2018 16:45:43 CEST svenmeier wrote:
GitHub user svenmeier opened a pull request:

     https://github.com/apache/wicket/pull/283

     Wicket-6563 page store implementation

     Basically I propose to
     *unify IPageStore with IDateStore
     *allow all IPageStore implementations to use Request/Session data (see
DiskPageStore, GroupingPageStore and CryptingPageStore) as needed *cut down
PageStoreManager to a very simple manager with a chain of stores, each
offering different solutions.

     I appreciate your feedback.

You can merge this pull request into a Git repository by running:

     $ git pull https://github.com/apache/wicket WICKET-6563

Alternatively you can review and apply these changes as the patch at:

     https://github.com/apache/wicket/pull/283.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

     This closes #283

----
commit bcf76f517310ac5d27a2092595c3a925c3973067
Author: Sven Meier <svenmeier@...>
Date:   2018-06-25T15:19:41Z

     WICKET-6563 new IPageStore implementation

commit a3604f7c359f9bbd2df03d57831c3f0d838ef521
Author: Sven Meier <svenmeier@...>
Date:   2018-07-03T18:18:10Z

     WICKET-6563 allow passing of SerializedPage

     between page stores

commit ad1f9b88ce8412e2b8eec5de72073b9688deb3bb
Author: Sven Meier <svenmeier@...>
Date:   2018-07-03T21:52:10Z

     WICKET-6563 javadoc and test

     for SerializingPageStore; keep page type in serializedpage

commit 2f70db06d3aa7f1ee07c96e1287507fe0021f18e
Author: Sven Meier <svenmeier@...>
Date:   2018-07-05T18:09:49Z

     WICKET-6563 crypt page store

commit 7e9c7e5166fda6692163c9551dc56d3177acfe6d
Author: Sven Meier <svenmeier@...>
Date:   2018-07-07T18:37:54Z

     WICKET-6563 IPageContext set synchronization

     prevent multiple threads from settings data into IPageContext
     concurrently

commit fd7b26bac6f72d5ebc647f490263c41f169faec1
Author: Sven Meier <svenmeier@...>
Date:   2018-07-09T08:54:57Z

     WICKET-6563 improved test

----


---




Reply via email to