[GitHub] [wicket] svenmeier opened a new pull request #462: WICKET-6864 updated crypt configuration

classic Classic list List threaded Threaded
42 messages Options
123
Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] svenmeier opened a new pull request #462: WICKET-6864 updated crypt configuration

GitBox

svenmeier opened a new pull request #462:
URL: https://github.com/apache/wicket/pull/462


   It's time to update the crypt defaults to recommended standards.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] solomax commented on a change in pull request #462: WICKET-6864 updated crypt configuration

GitBox

solomax commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r571750665



##########
File path: wicket-core/src/main/java/org/apache/wicket/authentication/strategy/DefaultAuthenticationStrategy.java
##########
@@ -57,25 +65,50 @@
  *
  * @param cookieKey
  *            The name of the cookie
+ *            
+ * @deprecated supply a crypt instead TODO remove in Wicket 10
  */
+ @Deprecated
  public DefaultAuthenticationStrategy(final String cookieKey)
  {
- this(cookieKey, defaultEncryptionKey(cookieKey));
+ this(cookieKey, defaultEncryptionKey());
  }
 
- private static String defaultEncryptionKey(String cookieKey)
+ private static String defaultEncryptionKey()
  {
- if (Application.exists())
- {
- return Application.get().getName();
- }
- return cookieKey;
+ return UUID.randomUUID().toString();
  }
 
+ /**
+ * @deprecated supply a crypt instead TODO remove in Wicket 10
+ */
+ @Deprecated

Review comment:
       Mayde it worth to add `forRemoval` instead of `TODO` :) ?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] papegaaij commented on pull request #462: WICKET-6864 updated crypt configuration

GitBox
In reply to this post by GitBox

papegaaij commented on pull request #462:
URL: https://github.com/apache/wicket/pull/462#issuecomment-775786142


   I think this `SunJceCrypt` requires a lot more work to get it up to date. For example, `PBEWithMD5AndDES` is hopelessly outdated. For the cipher we should definitely not use DES, but AES, like `AES/CBC/PKCS5Padding`.
   
   If we want to use PBE, we should switch to PBKDF2 and use PBKDF2WithHmacSHA512 with a key-length of 256 bits and a lot of iterations (way more than 1000, probably 100.000), but actually I fail to see why this class uses password based encryption and not a key directly. IMHO the key should not be a string, but a SecretKey built from 256 bits of secure random.
   
   Note we already have `UnlimitedStrenghtJurisdictionPolicyCrypt` in a test in `wicket-util`. This implementation already is much better and this unlimited strength jce-restriction is not a real issue anymore. I don't know about the latest Oracle JVMs, but OpenJDK does not limit the strength of JCE.
   
   IMHO it's better to deprecate the whole class and replace it with a more secure version.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] svenmeier commented on pull request #462: WICKET-6864 updated crypt configuration

GitBox
In reply to this post by GitBox

svenmeier commented on pull request #462:
URL: https://github.com/apache/wicket/pull/462#issuecomment-776065368


   Even better if we have an improved default implementation +1.
   
   IMHO the worst thing about the current state is having a fixed salt and DefaultAuthenticationStrategy using the application name as default key.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] papegaaij commented on pull request #462: WICKET-6864 updated crypt configuration

GitBox
In reply to this post by GitBox

papegaaij commented on pull request #462:
URL: https://github.com/apache/wicket/pull/462#issuecomment-776147295


   `UnlimitedStrenghtJurisdictionPolicyCrypt` is much better wrt the algorithms used, however, the unpredictability of keys, salt and initialization vectors (iv) is even more important. It makes no sense to encrypt something with a key that can be guessed with just a couple of tries, no matter what algorithm you use. Unfortunately I'm very limited in time at the moment. We've got a very tight schedule at the moment. But IMHO, we should make sure not only the algorithms are up to date, but the input for the keys, salt and iv is secure random as well.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] bitstorm commented on pull request #462: WICKET-6864 updated crypt configuration

GitBox
In reply to this post by GitBox

bitstorm commented on pull request #462:
URL: https://github.com/apache/wicket/pull/462#issuecomment-776194787


   I also agree we should provide a stronger default crypt and UnlimitedStrenghtJurisdictionPolicyCrypt is a good candidate. Unfortunately the current cryptography support is tailored for PB encryption, that's why some time ago I've tried to improve it to allow the adoption of other algorithms. At that time I couldn't finalize the work as we were focused on releasing Wicket 7, but the branch is still around: https://github.com/apache/wicket/compare/WICKET-5768-improve-crypt.
   It's nothing more than a palyground branch but I think it might contain some useful ideas.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] bitstorm edited a comment on pull request #462: WICKET-6864 updated crypt configuration

GitBox
In reply to this post by GitBox

bitstorm edited a comment on pull request #462:
URL: https://github.com/apache/wicket/pull/462#issuecomment-776194787


   I also agree we should provide a stronger default crypt and UnlimitedStrenghtJurisdictionPolicyCrypt is a good candidate. Unfortunately the current cryptography support is tailored for PB encryption, that's why some time ago I've tried to improve it to allow the adoption of other algorithms. At that time I couldn't finalize the work as we were focused on releasing Wicket 7, but the branch is still around: https://github.com/apache/wicket/compare/WICKET-5768-improve-crypt.
   It's nothing more than a palyground branch but I think it might contain some useful ideas to improve the current implementation.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] martin-g commented on a change in pull request #462: WICKET-6864 updated crypt configuration

GitBox
In reply to this post by GitBox

martin-g commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r573495439



##########
File path: wicket-core/src/main/java/org/apache/wicket/authentication/strategy/DefaultAuthenticationStrategy.java
##########
@@ -57,25 +65,50 @@
  *
  * @param cookieKey
  *            The name of the cookie
+ *            
+ * @deprecated supply a crypt instead TODO remove in Wicket 10
  */
+ @Deprecated
  public DefaultAuthenticationStrategy(final String cookieKey)
  {
- this(cookieKey, defaultEncryptionKey(cookieKey));
+ this(cookieKey, defaultEncryptionKey());
  }
 
- private static String defaultEncryptionKey(String cookieKey)
+ private static String defaultEncryptionKey()
  {
- if (Application.exists())
- {
- return Application.get().getName();
- }
- return cookieKey;
+ return UUID.randomUUID().toString();
  }
 
+ /**
+ * @deprecated supply a crypt instead TODO remove in Wicket 10
+ */
+ @Deprecated
  public DefaultAuthenticationStrategy(final String cookieKey, final String encryptionKey)
+ {
+ this(cookieKey, defaultCrypt(encryptionKey));
+ }
+
+ private static ICrypt defaultCrypt(String encryptionKey)
+ {
+ byte[] salt = SunJceCrypt.randomSalt();
+
+ SunJceCrypt crypt = new SunJceCrypt(salt, 1000);
+ crypt.setKey(encryptionKey);
+ return crypt;
+ }
+
+ /**
+ * Constructor
+ *
+ * @param cookieKey
+ *            The name of the cookie
+ * @param crypt
+ *            the crypt
+ */
+ public DefaultAuthenticationStrategy(final String cookieKey, ICrypt crypt)

Review comment:
       I guess this is the stable constructor that should be used by the applications if they want ti be able to decrypt after restarts. Let's add a comment to make it clear/recommended.

##########
File path: wicket-core/src/main/java/org/apache/wicket/authentication/strategy/DefaultAuthenticationStrategy.java
##########
@@ -16,12 +16,14 @@
  */
 package org.apache.wicket.authentication.strategy;
 
-import org.apache.wicket.Application;
+import java.util.Random;

Review comment:
       Is Random used in this class ?

##########
File path: wicket-core/src/main/java/org/apache/wicket/authentication/strategy/DefaultAuthenticationStrategy.java
##########
@@ -30,6 +32,9 @@
 /**
  * Wicket's default implementation of an authentication strategy. It'll concatenate username and
  * password, encrypt it and put it into one Cookie.
+ * <p>
+ * Note: To support automatic authentication across application restarts you have to use
+ * the constructor {@link DefaultAuthenticationStrategy#DefaultAuthenticationStrategy(String, String, byte[])}.

Review comment:
       I don't see such constructor below.

##########
File path: wicket-util/src/main/java/org/apache/wicket/util/crypt/SunJceCrypt.java
##########
@@ -44,39 +45,77 @@
  /**
  * Iteration count used in combination with the salt to create the encryption key.
  */
- private final static int COUNT = 17;
+ private final static int DEFAULT_ITERATION_COUNT = 17;
 
  /** Name of the default encryption method */
  public static final String DEFAULT_CRYPT_METHOD = "PBEWithMD5AndDES";
 
- /** Salt */
+ /**
+ * Default salt.
+ *
+ * @deprecated TODO remove in Wicket 10
+ */
+ @Deprecated
  public final static byte[] SALT = { (byte)0x15, (byte)0x8c, (byte)0xa3, (byte)0x4a,
  (byte)0x66, (byte)0x51, (byte)0x2a, (byte)0xbc };
 
- private static final PBEParameterSpec PARAMETER_SPEC = new PBEParameterSpec(SALT, COUNT);
-
  /** The name of encryption method (cipher) */
  private final String cryptMethod;
-
+
+ private final int iterationCount;
+
+ private final byte[] salt;
+
  /**
  * Constructor
+ *
+ * @deprecated
  */
  public SunJceCrypt()
  {
  this(DEFAULT_CRYPT_METHOD);
  }
 
+ /**
+ * Constructor.
+ *
+ * @param salt
+ *              salt for encryption
+ * @param iterationCount
+ * iteration count
+ */
+ public SunJceCrypt(byte[] salt, int iterationCount)
+ {
+ this(DEFAULT_CRYPT_METHOD, salt, iterationCount);
+ }
+
+ /**
+ * Constructor
+ *
+ * @deprecated
+ */
+ public SunJceCrypt(String cryptMethod)
+ {
+ this(cryptMethod, SALT, DEFAULT_ITERATION_COUNT);
+ }
+
  /**
  * Constructor that uses a custom encryption method (cipher).
  * You may need to override {@link #createKeySpec()} and/or
  * {@link #createParameterSpec()} for the custom cipher.
  *
  * @param cryptMethod
  *              the name of encryption method (the cipher)
+ * @param salt
+ *              salt for encryption
+ * @param iterationCount
+ * iteration count
  */
- public SunJceCrypt(String cryptMethod)
+ public SunJceCrypt(String cryptMethod, byte[] salt, int iterationCount)
  {
  this.cryptMethod = Args.notNull(cryptMethod, "Crypt method");
+ this.salt = Args.notNull(salt, "salt");
+ this.iterationCount = iterationCount;

Review comment:
       do we need a check for a positive `iterationCount` ?

##########
File path: wicket-util/src/main/java/org/apache/wicket/util/crypt/SunJceCrypt.java
##########
@@ -44,39 +45,77 @@
  /**
  * Iteration count used in combination with the salt to create the encryption key.
  */
- private final static int COUNT = 17;
+ private final static int DEFAULT_ITERATION_COUNT = 17;
 
  /** Name of the default encryption method */
  public static final String DEFAULT_CRYPT_METHOD = "PBEWithMD5AndDES";
 
- /** Salt */
+ /**
+ * Default salt.
+ *
+ * @deprecated TODO remove in Wicket 10
+ */
+ @Deprecated
  public final static byte[] SALT = { (byte)0x15, (byte)0x8c, (byte)0xa3, (byte)0x4a,
  (byte)0x66, (byte)0x51, (byte)0x2a, (byte)0xbc };
 
- private static final PBEParameterSpec PARAMETER_SPEC = new PBEParameterSpec(SALT, COUNT);
-
  /** The name of encryption method (cipher) */
  private final String cryptMethod;
-
+
+ private final int iterationCount;
+
+ private final byte[] salt;
+
  /**
  * Constructor
+ *
+ * @deprecated
  */
  public SunJceCrypt()
  {
  this(DEFAULT_CRYPT_METHOD);
  }
 
+ /**
+ * Constructor.
+ *
+ * @param salt
+ *              salt for encryption
+ * @param iterationCount
+ * iteration count
+ */
+ public SunJceCrypt(byte[] salt, int iterationCount)
+ {
+ this(DEFAULT_CRYPT_METHOD, salt, iterationCount);
+ }
+
+ /**
+ * Constructor
+ *
+ * @deprecated
+ */

Review comment:
       `@Deprecated`

##########
File path: wicket-core/src/main/java/org/apache/wicket/core/util/crypt/KeyInSessionSunJceCryptFactory.java
##########
@@ -16,8 +16,10 @@
  */
 package org.apache.wicket.core.util.crypt;
 
+import java.io.Serializable;
 import java.security.Provider;
 import java.security.Security;
+import java.util.Random;

Review comment:
       Again, looks like this new import is not used.

##########
File path: wicket-util/src/main/java/org/apache/wicket/util/crypt/ClassCryptFactory.java
##########
@@ -28,6 +28,8 @@
  * must implement {@link ICrypt}.
  *
  * @author Igor Vaynberg (ivaynberg)
+ *
+ * @deprecated use a lambda expression instead TODO remove in Wicket 10
  */
 public class ClassCryptFactory implements ICryptFactory

Review comment:
       Do we still need `ICryptFactory` ? Or it should be deprecated too ?

##########
File path: wicket-util/src/main/java/org/apache/wicket/util/crypt/SunJceCrypt.java
##########
@@ -153,4 +193,17 @@ protected KeySpec createKeySpec()
  {
  return new PBEKeySpec(getKey().toCharArray());
  }
+
+ /**
+ * Create a random salt.
+ *
+ * @return salt
+ */
+ public static byte[] randomSalt()
+ {
+ // only 8 bytes long supported

Review comment:
       Please add an explanation `why` only 8 bytes long is supported. I honestly don't know why more is not possible. I guess with the time even you may forget the reason.

##########
File path: wicket-core/src/main/java/org/apache/wicket/core/util/crypt/KeyInSessionSunJceCryptFactory.java
##########
@@ -89,25 +91,44 @@ public ICrypt newCrypt()
  session.bind();
 
  // retrieve or generate encryption key from session
- String key = session.getMetaData(KEY);
- if (key == null)
+ CryptData data = session.getMetaData(KEY);
+ if (data == null)
  {
+ // generate new salt
+ byte[] salt = SunJceCrypt.randomSalt();
+
  // generate new key
- key = session.getId() + "." + UUID.randomUUID().toString();
- session.setMetaData(KEY, key);
+ String key = session.getId() + "." + UUID.randomUUID().toString();
+
+ data = new CryptData(key, salt);
+ session.setMetaData(KEY, data);
  }
 
- // build the crypt based on session key
- ICrypt crypt = createCrypt();
- crypt.setKey(key);
+ // build the crypt based on session key and salt
+ SunJceCrypt crypt = new SunJceCrypt(cryptMethod, data.salt, 1000);
+ crypt.setKey(data.key);
+
  return crypt;
  }
 
  /**
  * @return the {@link org.apache.wicket.util.crypt.ICrypt} to use
+ *
+ * @deprecated this method is no longer called

Review comment:
       Please add "TODO Remove in Wicket 10" or `forRemoval=10.0.0`

##########
File path: wicket-util/src/main/java/org/apache/wicket/util/crypt/SunJceCrypt.java
##########
@@ -138,12 +177,13 @@ protected SecretKey generateSecretKey() throws NoSuchAlgorithmException,
  return keyFactory.generateSecret(spec);
  }
 
+

Review comment:
       the rest of the code uses single empty line between the methods




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] svenmeier commented on a change in pull request #462: WICKET-6864 updated crypt configuration

GitBox
In reply to this post by GitBox

svenmeier commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r573913049



##########
File path: wicket-util/src/main/java/org/apache/wicket/util/crypt/ClassCryptFactory.java
##########
@@ -28,6 +28,8 @@
  * must implement {@link ICrypt}.
  *
  * @author Igor Vaynberg (ivaynberg)
+ *
+ * @deprecated use a lambda expression instead TODO remove in Wicket 10
  */
 public class ClassCryptFactory implements ICryptFactory

Review comment:
       CryptoMapper still utilizes SecuritySettings#cryptFactory -> ICryptFactorty

##########
File path: wicket-util/src/main/java/org/apache/wicket/util/crypt/ClassCryptFactory.java
##########
@@ -28,6 +28,8 @@
  * must implement {@link ICrypt}.
  *
  * @author Igor Vaynberg (ivaynberg)
+ *
+ * @deprecated use a lambda expression instead TODO remove in Wicket 10
  */
 public class ClassCryptFactory implements ICryptFactory

Review comment:
       CryptoMapper still utilizes SecuritySettings#cryptFactory -> ICryptFactory




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] svenmeier commented on pull request #462: WICKET-6864 updated crypt configuration

GitBox
In reply to this post by GitBox

svenmeier commented on pull request #462:
URL: https://github.com/apache/wicket/pull/462#issuecomment-776991369


   Thanks for your feedback.
   
   Of course developers can always configure their own crypt implementation. But Wicket should use sensible defaults too - currently this is not the case with a hard-coded salt and DefaultAuthenticationStrategy's default encryption-key.
   
   I'm fine with anybody stepping up and building a better solution. As Andrea pointed out, this might get difficult with the current API.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] bitstorm commented on pull request #462: WICKET-6864 updated crypt configuration

GitBox
In reply to this post by GitBox

bitstorm commented on pull request #462:
URL: https://github.com/apache/wicket/pull/462#issuecomment-778842092


   With my commit I've tried to bring in some ideas to make Wicket more independent from the specific solution currently adopted (a password-based DES cypher):
   
   - I've created a new class GenericJceCrypt, which should serve as base class to implement a specific JCE chyper. This class is meant to replace AbstractCrypt in Wicket 10.
   - I've also created class AbstractKeyInSessionCryptFactory to provide a base class to store a session relative key used for URL encryption. Again, the current class KeyInSessionSunJceCryptFactory only supports password based algorithms. The new class should simplify the implementation of session crypt factories that use more complex values as key (for exemple a  javax.crypto.SecretKey).
   - Finally I've deprecated ICyper#setKey because it supports just string key and (IMHO) in any case it shouldn't be partof the API contract.  
   
   Let me know what you think about this.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] solomax commented on a change in pull request #462: WICKET-6864 updated crypt configuration

GitBox
In reply to this post by GitBox

solomax commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r575887414



##########
File path: wicket-core/src/main/java/org/apache/wicket/core/util/crypt/KeyInSessionSunJceCryptFactory.java
##########
@@ -37,7 +36,7 @@
  *
  * @author igor.vaynberg
  */
-public class KeyInSessionSunJceCryptFactory implements ICryptFactory
+public class KeyInSessionSunJceCryptFactory extends AbstractKeyInSessionCryptFactory<CryptData>
 {
  /** metadata-key used to store crypt data in session metadata */
  private static final MetaDataKey<CryptData> KEY = new MetaDataKey<>()

Review comment:
       this `KEY` can be removed




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] solomax commented on a change in pull request #462: WICKET-6864 updated crypt configuration

GitBox
In reply to this post by GitBox

solomax commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r575887721



##########
File path: wicket-util/src/main/java/org/apache/wicket/util/crypt/GenericJceCrypt.java
##########
@@ -0,0 +1,162 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.util.crypt;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import java.io.UnsupportedEncodingException;
+import java.security.GeneralSecurityException;
+import java.security.InvalidAlgorithmParameterException;
+import java.security.InvalidKeyException;
+import java.security.NoSuchAlgorithmException;
+import java.security.spec.AlgorithmParameterSpec;
+import java.util.Base64;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import javax.crypto.SecretKey;
+
+
+/**
+ * Base class for JCE based ICrypt implementations.
+ *
+ */
+public class GenericJceCrypt implements ICrypt
+{
+ /** Encoding used to convert java String from and to byte[] */
+ private static final String CHARACTER_ENCODING = "UTF-8";

Review comment:
       Maybe it would be better to use `StandardCharsets.UTF8` ?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] svenmeier commented on pull request #462: WICKET-6864 updated crypt configuration

GitBox
In reply to this post by GitBox

svenmeier commented on pull request #462:
URL: https://github.com/apache/wicket/pull/462#issuecomment-779110291


   Fine by me.  @papegaaij wdyt?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] papegaaij commented on pull request #462: WICKET-6864 updated crypt configuration

GitBox
In reply to this post by GitBox

papegaaij commented on pull request #462:
URL: https://github.com/apache/wicket/pull/462#issuecomment-779200758


   I like the idea. The new implementation has one problem though: the IV must be random on every encryption step. Most importantly, you should never encrypt the same data twice with the same IV. You can think of the IV as a salt for your encryption. It ensures that the same data encrypts to different values every time. It is common practice to store the IV together with the ciphertext. The code we use in our application for AES256 is like this:
   
   ```
    public byte[] encrypt(SecureRandom rnd, SecretKey key, byte[] plaintext) throws GeneralSecurityException {
    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(plaintext);
    return Bytes.concat(iv, ciphertext);
    }
   
    public byte[] decrypt(SecureRandom rnd, SecretKey key, byte[] cipherInput) throws GeneralSecurityException {
    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), rnd);
    return cipher.doFinal(ciphertext);
    }
   ```


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] papegaaij edited a comment on pull request #462: WICKET-6864 updated crypt configuration

GitBox
In reply to this post by GitBox

papegaaij edited a comment on pull request #462:
URL: https://github.com/apache/wicket/pull/462#issuecomment-779200758


   I like the idea. The new implementation has one problem though: the IV must be random on every encryption step. Most importantly, you should never encrypt the same data twice with the same IV. You can think of the IV as a salt for your encryption. It ensures that the same data encrypts to different values every time. It is common practice to store the IV together with the ciphertext. The code we use in our application for AES256 is like this:
   
   ```java
   public byte[] encrypt(SecureRandom rnd, SecretKey key, byte[] plaintext) throws GeneralSecurityException {
       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(plaintext);
       return Bytes.concat(iv, ciphertext);
   }
   
   public byte[] decrypt(SecureRandom rnd, SecretKey key, byte[] cipherInput) throws GeneralSecurityException {
       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), rnd);
       return cipher.doFinal(ciphertext);
   }
   ```


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] bitstorm commented on pull request #462: WICKET-6864 updated crypt configuration

GitBox
In reply to this post by GitBox

bitstorm commented on pull request #462:
URL: https://github.com/apache/wicket/pull/462#issuecomment-783677654


   I followed Emond's suggestion and I turned GenericJceCrypt into a more generic AbstractJceCrypt. The idea behind this class is to let final users implement the desired configuration logic for ciphers, so they can decide which level of "strength" to adopt. For example in some use cases we could decide to sacrifice security strength  for a faster encryption/decryption.  This might be the case of class CryptoMapper which uses ICypt to encrypt/decrypt page URLs.
   
   PS:  class UnlimitedStrenghtJurisdictionPolicyCrypt has been reworked  according to the example code provided by Edmond.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] martin-g commented on a change in pull request #462: WICKET-6864 updated crypt configuration

GitBox
In reply to this post by GitBox

martin-g commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r591241937



##########
File path: wicket-core/src/main/java/org/apache/wicket/core/util/crypt/AbstractKeyInSessionCryptFactory.java
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.core.util.crypt;
+
+import org.apache.wicket.MetaDataKey;
+import org.apache.wicket.Session;
+import org.apache.wicket.util.crypt.ICrypt;
+import org.apache.wicket.util.crypt.ICryptFactory;
+
+import java.io.Serializable;
+
+
+/**
+ * Base class to implement crypt factories that store crypt into user session. Note that the use of
+ * this crypt factory will result in an immediate creation of a http session.
+ *
+ * @author andrea del bene
+ *
+ * @param <T>
+ *            the type for the secret key.
+ */
+public abstract class AbstractKeyInSessionCryptFactory<T extends Serializable>
+ implements
+ ICryptFactory
+{
+ /** metadata-key used to store crypto-key in session metadata */
+ private static final MetaDataKey<Serializable> KEY = new MetaDataKey<Serializable>()
+ {
+ private static final long serialVersionUID = 1L;
+ };
+
+ public AbstractKeyInSessionCryptFactory()
+ {
+ super();
+ }
+
+ /**
+ * Creates a new crypt for the current user session. If no user session is available, a new one
+ * is created.
+ *
+ * @return
+ */
+ @Override
+ @SuppressWarnings("unchecked")
+ public ICrypt newCrypt()
+ {
+ Session session = Session.get();
+ session.bind();
+
+ // retrieve or generate encryption key from session
+ Serializable key = session.getMetaData(KEY);

Review comment:
       s/Serializable/T/

##########
File path: wicket-core/src/main/java/org/apache/wicket/core/util/crypt/AbstractKeyInSessionCryptFactory.java
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.core.util.crypt;
+
+import org.apache.wicket.MetaDataKey;
+import org.apache.wicket.Session;
+import org.apache.wicket.util.crypt.ICrypt;
+import org.apache.wicket.util.crypt.ICryptFactory;
+
+import java.io.Serializable;
+
+
+/**
+ * Base class to implement crypt factories that store crypt into user session. Note that the use of
+ * this crypt factory will result in an immediate creation of a http session.
+ *
+ * @author andrea del bene
+ *
+ * @param <T>
+ *            the type for the secret key.
+ */
+public abstract class AbstractKeyInSessionCryptFactory<T extends Serializable>
+ implements
+ ICryptFactory
+{
+ /** metadata-key used to store crypto-key in session metadata */
+ private static final MetaDataKey<Serializable> KEY = new MetaDataKey<Serializable>()

Review comment:
       MetaDataKey<T>

##########
File path: wicket-core/src/main/java/org/apache/wicket/settings/SecuritySettings.java
##########
@@ -49,8 +49,11 @@
 public class SecuritySettings
 {
  /**
- * encryption key used by default crypt factory
+ * encryption key is no longer used
+ *
+ * @deprecated
  */
+ @Deprecated

Review comment:
       forRemoval

##########
File path: wicket-util/src/main/java/org/apache/wicket/util/crypt/AESCrypt.java
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.util.crypt;
+
+import javax.crypto.BadPaddingException;
+import javax.crypto.Cipher;
+import javax.crypto.IllegalBlockSizeException;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.IvParameterSpec;
+
+/**
+ * AES based {@link ICrypt} encrypt and decrypt strings such as passwords or URL segments.
+ * Based on http://stackoverflow.com/a/992413
+ *
+ * @see ICrypt
+ */
+class AESCrypt extends AbstractJceCrypt
+{
+
+ private final SecretKey secretKey;
+ private final String algorithm;
+ private final int ivSize;
+
+
+ /**
+ * Constructor
+ *
+ * @param secretKey
+ *              The {@link SecretKey} to use to initialize the {@link Cipher}.
+ */
+ public AESCrypt(SecretKey secretKey)

Review comment:
       The class is package-private. Should it be `public` or the constructor should be `package-private` too ?

##########
File path: wicket-util/src/test/java/org/apache/wicket/util/crypt/AESCryptTest.java
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.util.crypt;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import org.junit.jupiter.api.Assumptions;
+import org.junit.jupiter.api.Test;
+
+import java.security.GeneralSecurityException;
+
+import javax.crypto.SecretKey;
+
+public class AESCryptTest
+{
+ @Test
+ public void encrypDecrypt() throws GeneralSecurityException
+ {
+ boolean unlimitedStrengthJurisdictionPolicyInstalled = SunJceCryptTest
+ .isUnlimitedStrengthJurisdictionPolicyInstalled();
+ Assumptions.assumeTrue(unlimitedStrengthJurisdictionPolicyInstalled);
+
+ SecretKey secretKey = CipherUtils.generatePBEKey(
+ "myWeakPassword", "PBKDF2WithHmacSHA1", "AES",
+ CipherUtils.randomByteArray(16), 65536, 256);
+
+ AbstractJceCrypt crypt = new AESCrypt(secretKey);
+
+ String input1 = "input1";
+ String encrypted = crypt.encryptUrlSafe(input1);
+
+ String input2 = "input2";

Review comment:
       let's use some UTF-8 characters here

##########
File path: wicket-util/src/main/java/org/apache/wicket/util/crypt/AESCrypt.java
##########
@@ -0,0 +1,112 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.util.crypt;
+
+import javax.crypto.BadPaddingException;
+import javax.crypto.Cipher;
+import javax.crypto.IllegalBlockSizeException;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.IvParameterSpec;
+
+/**
+ * AES based {@link ICrypt} encrypt and decrypt strings such as passwords or URL segments.
+ * Based on http://stackoverflow.com/a/992413
+ *
+ * @see ICrypt
+ */
+class AESCrypt extends AbstractJceCrypt
+{
+
+ private final SecretKey secretKey;
+ private final String algorithm;
+ private final int ivSize;
+
+
+ /**
+ * Constructor
+ *
+ * @param secretKey
+ *              The {@link SecretKey} to use to initialize the {@link Cipher}.
+ */
+ public AESCrypt(SecretKey secretKey)
+ {
+ this(secretKey, "AES/CBC/PKCS5Padding", 16);
+ }
+
+ /**
+ * Constructor
+ *
+ * @param secretKey
+ *              The {@link SecretKey} to use to initialize the {@link Cipher}.
+ * @param algorithm
+ *              The cipher algorithm to use, for example "AES/CBC/PKCS5Padding".
+ * @param ivSize
+ *              The size of the Initialization Vector to use with the cipher.
+ */
+ private AESCrypt(SecretKey secretKey, String algorithm, int ivSize)
+ {
+ this.secretKey = secretKey;

Review comment:
       I think it would be good if we use `Args.non***` checks here.

##########
File path: wicket-core/src/main/java/org/apache/wicket/core/util/crypt/KeyInSessionSunJceCryptFactory.java
##########
@@ -82,32 +76,53 @@ public KeyInSessionSunJceCryptFactory(String cryptMethod)
  }
  }
 
- @Override
- public ICrypt newCrypt()
- {
- Session session = Session.get();
- session.bind();
-
- // retrieve or generate encryption key from session
- String key = session.getMetaData(KEY);
- if (key == null)
- {
- // generate new key
- key = session.getId() + "." + UUID.randomUUID().toString();
- session.setMetaData(KEY, key);
- }
-
- // build the crypt based on session key
- ICrypt crypt = createCrypt();
- crypt.setKey(key);
- return crypt;
- }
-
  /**
  * @return the {@link org.apache.wicket.util.crypt.ICrypt} to use
+ *
+ * @deprecated this method is no longer called TODO remove in Wicket 10
  */
+ @Deprecated(forRemoval = true)
  protected ICrypt createCrypt()
  {
- return new SunJceCrypt(cryptMethod);
+ return null;
+ }
+
+ @Override
+ protected CryptData generateKey(Session session)
+ {
+    // generate new salt
+        byte[] salt = SunJceCrypt.randomSalt();
+        
+    // generate new key
+        String key = session.getId() + "." + UUID.randomUUID().toString();
+        
+        return new CryptData(key, salt);
+ }
+
+ @Override
+ protected ICrypt createCrypt(CryptData keyParams)
+ {
+    SunJceCrypt crypt = new SunJceCrypt(cryptMethod, keyParams.salt, 1000);
+        crypt.setKey(keyParams.key);
+        
+        return crypt;
+ }
+
+    static final class CryptData implements Serializable
+ {
+ /**

Review comment:
       Empty javadoc. Remove it

##########
File path: wicket-core/src/main/java/org/apache/wicket/core/util/crypt/AbstractKeyInSessionCryptFactory.java
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.core.util.crypt;
+
+import org.apache.wicket.MetaDataKey;
+import org.apache.wicket.Session;
+import org.apache.wicket.util.crypt.ICrypt;
+import org.apache.wicket.util.crypt.ICryptFactory;
+
+import java.io.Serializable;
+
+
+/**
+ * Base class to implement crypt factories that store crypt into user session. Note that the use of
+ * this crypt factory will result in an immediate creation of a http session.
+ *
+ * @author andrea del bene
+ *
+ * @param <T>
+ *            the type for the secret key.
+ */
+public abstract class AbstractKeyInSessionCryptFactory<T extends Serializable>

Review comment:
       s/Serializable/IClusterable/ ?!

##########
File path: wicket-examples/src/main/java/org/apache/wicket/examples/WicketExampleApplication.java
##########
@@ -57,8 +55,7 @@ protected void init()
  // has the java security classes required by Crypt installed
  // and we want them to be able to run the examples out of the
  // box.
- getSecuritySettings().setCryptFactory(
- new ClassCryptFactory(NoCrypt.class, SecuritySettings.DEFAULT_ENCRYPTION_KEY));
+ getSecuritySettings().setCryptFactory(() -> new NoCrypt());

Review comment:
       NoCrypt::new

##########
File path: wicket-core/src/main/java/org/apache/wicket/core/util/crypt/KeyInSessionSunJceCryptFactory.java
##########
@@ -82,32 +76,53 @@ public KeyInSessionSunJceCryptFactory(String cryptMethod)
  }
  }
 
- @Override
- public ICrypt newCrypt()
- {
- Session session = Session.get();
- session.bind();
-
- // retrieve or generate encryption key from session
- String key = session.getMetaData(KEY);
- if (key == null)
- {
- // generate new key
- key = session.getId() + "." + UUID.randomUUID().toString();
- session.setMetaData(KEY, key);
- }
-
- // build the crypt based on session key
- ICrypt crypt = createCrypt();
- crypt.setKey(key);
- return crypt;
- }
-
  /**
  * @return the {@link org.apache.wicket.util.crypt.ICrypt} to use
+ *
+ * @deprecated this method is no longer called TODO remove in Wicket 10
  */
+ @Deprecated(forRemoval = true)
  protected ICrypt createCrypt()
  {
- return new SunJceCrypt(cryptMethod);
+ return null;
+ }
+
+ @Override
+ protected CryptData generateKey(Session session)
+ {
+    // generate new salt
+        byte[] salt = SunJceCrypt.randomSalt();
+        
+    // generate new key
+        String key = session.getId() + "." + UUID.randomUUID().toString();
+        
+        return new CryptData(key, salt);
+ }
+
+ @Override
+ protected ICrypt createCrypt(CryptData keyParams)
+ {
+    SunJceCrypt crypt = new SunJceCrypt(cryptMethod, keyParams.salt, 1000);
+        crypt.setKey(keyParams.key);
+        
+        return crypt;
+ }
+
+    static final class CryptData implements Serializable

Review comment:
       IClusterable

##########
File path: wicket-core/src/main/java/org/apache/wicket/core/util/crypt/AbstractKeyInSessionCryptFactory.java
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.core.util.crypt;
+
+import org.apache.wicket.MetaDataKey;
+import org.apache.wicket.Session;
+import org.apache.wicket.util.crypt.ICrypt;
+import org.apache.wicket.util.crypt.ICryptFactory;
+
+import java.io.Serializable;
+
+
+/**
+ * Base class to implement crypt factories that store crypt into user session. Note that the use of
+ * this crypt factory will result in an immediate creation of a http session.
+ *
+ * @author andrea del bene
+ *
+ * @param <T>
+ *            the type for the secret key.
+ */
+public abstract class AbstractKeyInSessionCryptFactory<T extends Serializable>
+ implements
+ ICryptFactory
+{
+ /** metadata-key used to store crypto-key in session metadata */
+ private static final MetaDataKey<Serializable> KEY = new MetaDataKey<Serializable>()
+ {
+ private static final long serialVersionUID = 1L;
+ };
+
+ public AbstractKeyInSessionCryptFactory()
+ {
+ super();
+ }
+
+ /**
+ * Creates a new crypt for the current user session. If no user session is available, a new one
+ * is created.
+ *
+ * @return
+ */
+ @Override
+ @SuppressWarnings("unchecked")
+ public ICrypt newCrypt()
+ {
+ Session session = Session.get();
+ session.bind();
+
+ // retrieve or generate encryption key from session
+ Serializable key = session.getMetaData(KEY);
+ if (key == null)
+ {
+ // generate new key
+ key = generateKey(session);
+ session.setMetaData(KEY, key);
+ }
+
+ // build the crypt based on session key
+ ICrypt crypt = createCrypt((T)key);

Review comment:
       if the above changes are applied then there is no need to cast here and to annotate with `@SuppressWarnings("unchecked")`

##########
File path: wicket-util/src/main/java/org/apache/wicket/util/crypt/AbstractJceCrypt.java
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.util.crypt;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
+import java.security.GeneralSecurityException;
+import java.security.InvalidAlgorithmParameterException;
+import java.security.InvalidKeyException;
+import java.security.NoSuchAlgorithmException;
+import java.security.spec.AlgorithmParameterSpec;
+import java.util.Base64;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import javax.crypto.SecretKey;
+
+
+/**
+ * Base class for JCE based ICrypt implementations.
+ *
+ */
+public abstract class AbstractJceCrypt implements ICrypt
+{
+ /** Encoding used to convert java String from and to byte[] */
+ public static final Charset CHARACTER_ENCODING = StandardCharsets.UTF_8;
+
+ /** Log. */
+ private static final Logger log = LoggerFactory.getLogger(AbstractJceCrypt.class);
+
+    protected Cipher buildCipher(int opMode, SecretKey secretKey, String transformation, AlgorithmParameterSpec params)

Review comment:
       What is `transformation`? This name is not self-explanatory (at least to me)

##########
File path: wicket-util/src/main/java/org/apache/wicket/util/crypt/AbstractJceCrypt.java
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.util.crypt;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
+import java.security.GeneralSecurityException;
+import java.security.InvalidAlgorithmParameterException;
+import java.security.InvalidKeyException;
+import java.security.NoSuchAlgorithmException;
+import java.security.spec.AlgorithmParameterSpec;
+import java.util.Base64;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import javax.crypto.SecretKey;
+
+
+/**
+ * Base class for JCE based ICrypt implementations.
+ *
+ */
+public abstract class AbstractJceCrypt implements ICrypt
+{
+ /** Encoding used to convert java String from and to byte[] */
+ public static final Charset CHARACTER_ENCODING = StandardCharsets.UTF_8;
+
+ /** Log. */
+ private static final Logger log = LoggerFactory.getLogger(AbstractJceCrypt.class);
+
+    protected Cipher buildCipher(int opMode, SecretKey secretKey, String transformation, AlgorithmParameterSpec params)
+    {
+        try
+        {
+            Cipher crypter = Cipher.getInstance(transformation);
+            crypter.init(opMode, secretKey, params);
+            return crypter;
+        } catch (InvalidKeyException | InvalidAlgorithmParameterException
+                | NoSuchAlgorithmException | NoSuchPaddingException e) {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+ * Decrypts a string into a string.
+ *
+ * @param text
+ *            text to decrypt
+ * @return the decrypted text
+ */
+ @Override
+ public final String decryptUrlSafe(final String text)
+ {
+ try
+ {
+ byte[] decoded = java.util.Base64.getUrlDecoder().decode(text);
+ return new String(decryptByteArray(decoded), CHARACTER_ENCODING);
+ }
+ catch (Exception ex)
+ {
+ log.debug("Error decoding text: " + text, ex);
+ return null;
+ }
+ }
+
+ /**
+ * Encrypt a string into a string using URL safe Base64 encoding.
+ *
+ * @param plainText
+ *            text to encrypt
+ * @return encrypted string
+ */
+ @Override
+ public final String encryptUrlSafe(final String plainText)
+ {
+ byte[] encrypted = encryptStringToByteArray(plainText);

Review comment:
       indentation

##########
File path: wicket-util/src/main/java/org/apache/wicket/util/crypt/AbstractJceCrypt.java
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.util.crypt;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
+import java.security.GeneralSecurityException;
+import java.security.InvalidAlgorithmParameterException;
+import java.security.InvalidKeyException;
+import java.security.NoSuchAlgorithmException;
+import java.security.spec.AlgorithmParameterSpec;
+import java.util.Base64;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import javax.crypto.SecretKey;
+
+
+/**
+ * Base class for JCE based ICrypt implementations.
+ *
+ */
+public abstract class AbstractJceCrypt implements ICrypt
+{
+ /** Encoding used to convert java String from and to byte[] */
+ public static final Charset CHARACTER_ENCODING = StandardCharsets.UTF_8;
+
+ /** Log. */
+ private static final Logger log = LoggerFactory.getLogger(AbstractJceCrypt.class);
+
+    protected Cipher buildCipher(int opMode, SecretKey secretKey, String transformation, AlgorithmParameterSpec params)
+    {
+        try
+        {
+            Cipher crypter = Cipher.getInstance(transformation);
+            crypter.init(opMode, secretKey, params);
+            return crypter;
+        } catch (InvalidKeyException | InvalidAlgorithmParameterException
+                | NoSuchAlgorithmException | NoSuchPaddingException e) {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+ * Decrypts a string into a string.
+ *
+ * @param text
+ *            text to decrypt
+ * @return the decrypted text
+ */
+ @Override
+ public final String decryptUrlSafe(final String text)
+ {
+ try
+ {
+ byte[] decoded = java.util.Base64.getUrlDecoder().decode(text);
+ return new String(decryptByteArray(decoded), CHARACTER_ENCODING);
+ }
+ catch (Exception ex)
+ {
+ log.debug("Error decoding text: " + text, ex);

Review comment:
       Use slf4j's {} instead of String concatenation

##########
File path: wicket-core/src/main/java/org/apache/wicket/core/util/crypt/KeyInSessionSunJceCryptFactory.java
##########
@@ -82,32 +76,53 @@ public KeyInSessionSunJceCryptFactory(String cryptMethod)
  }
  }
 
- @Override
- public ICrypt newCrypt()
- {
- Session session = Session.get();
- session.bind();
-
- // retrieve or generate encryption key from session
- String key = session.getMetaData(KEY);
- if (key == null)
- {
- // generate new key
- key = session.getId() + "." + UUID.randomUUID().toString();
- session.setMetaData(KEY, key);
- }
-
- // build the crypt based on session key
- ICrypt crypt = createCrypt();
- crypt.setKey(key);
- return crypt;
- }
-
  /**
  * @return the {@link org.apache.wicket.util.crypt.ICrypt} to use
+ *
+ * @deprecated this method is no longer called TODO remove in Wicket 10
  */
+ @Deprecated(forRemoval = true)
  protected ICrypt createCrypt()
  {
- return new SunJceCrypt(cryptMethod);
+ return null;
+ }
+
+ @Override
+ protected CryptData generateKey(Session session)
+ {
+    // generate new salt
+        byte[] salt = SunJceCrypt.randomSalt();
+        
+    // generate new key
+        String key = session.getId() + "." + UUID.randomUUID().toString();
+        
+        return new CryptData(key, salt);
+ }
+
+ @Override
+ protected ICrypt createCrypt(CryptData keyParams)
+ {
+    SunJceCrypt crypt = new SunJceCrypt(cryptMethod, keyParams.salt, 1000);
+        crypt.setKey(keyParams.key);
+        
+        return crypt;
+ }
+
+    static final class CryptData implements Serializable
+ {
+ /**
+         *
+         */
+        private static final long serialVersionUID = 1L;
+
+        final String key;

Review comment:
       the indentation is wrong

##########
File path: wicket-util/src/main/java/org/apache/wicket/util/crypt/AbstractJceCrypt.java
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.util.crypt;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
+import java.security.GeneralSecurityException;
+import java.security.InvalidAlgorithmParameterException;
+import java.security.InvalidKeyException;
+import java.security.NoSuchAlgorithmException;
+import java.security.spec.AlgorithmParameterSpec;
+import java.util.Base64;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import javax.crypto.SecretKey;
+
+
+/**
+ * Base class for JCE based ICrypt implementations.
+ *
+ */
+public abstract class AbstractJceCrypt implements ICrypt
+{
+ /** Encoding used to convert java String from and to byte[] */
+ public static final Charset CHARACTER_ENCODING = StandardCharsets.UTF_8;
+
+ /** Log. */
+ private static final Logger log = LoggerFactory.getLogger(AbstractJceCrypt.class);
+
+    protected Cipher buildCipher(int opMode, SecretKey secretKey, String transformation, AlgorithmParameterSpec params)
+    {
+        try
+        {
+            Cipher crypter = Cipher.getInstance(transformation);
+            crypter.init(opMode, secretKey, params);
+            return crypter;
+        } catch (InvalidKeyException | InvalidAlgorithmParameterException
+                | NoSuchAlgorithmException | NoSuchPaddingException e) {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+ * Decrypts a string into a string.
+ *
+ * @param text
+ *            text to decrypt
+ * @return the decrypted text
+ */
+ @Override
+ public final String decryptUrlSafe(final String text)
+ {
+ try
+ {
+ byte[] decoded = java.util.Base64.getUrlDecoder().decode(text);
+ return new String(decryptByteArray(decoded), CHARACTER_ENCODING);
+ }
+ catch (Exception ex)
+ {
+ log.debug("Error decoding text: " + text, ex);
+ return null;
+ }
+ }
+
+ /**
+ * Encrypt a string into a string using URL safe Base64 encoding.
+ *
+ * @param plainText
+ *            text to encrypt
+ * @return encrypted string
+ */
+ @Override
+ public final String encryptUrlSafe(final String plainText)
+ {
+ byte[] encrypted = encryptStringToByteArray(plainText);
+        Base64.Encoder encoder = Base64.getUrlEncoder().withoutPadding();
+        byte[] encoded = encoder.encode(encrypted);
+        return new String(encoded, CHARACTER_ENCODING);
+ }
+
+ /**
+ * Decrypts an encrypted byte array.
+ *
+ * @param encrypted
+ *            byte array to decrypt
+ * @return the decrypted text
+ */
+ abstract protected byte[] decryptByteArray(final byte[] encrypted);
+
+
+ /**
+ * Encrypts the given text into a byte array.
+ *
+ * @param plainText
+ *            text to encrypt
+ * @return the string encrypted
+ * @throws GeneralSecurityException
+ */
+ abstract protected byte[] encryptStringToByteArray(final String plainText);
+
+
+    @Override
+    public void setKey(String key) {

Review comment:
       `final` ?!

##########
File path: wicket-util/src/main/java/org/apache/wicket/util/crypt/AbstractJceCrypt.java
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.util.crypt;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
+import java.security.GeneralSecurityException;
+import java.security.InvalidAlgorithmParameterException;
+import java.security.InvalidKeyException;
+import java.security.NoSuchAlgorithmException;
+import java.security.spec.AlgorithmParameterSpec;
+import java.util.Base64;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import javax.crypto.SecretKey;
+
+
+/**
+ * Base class for JCE based ICrypt implementations.
+ *
+ */
+public abstract class AbstractJceCrypt implements ICrypt
+{
+ /** Encoding used to convert java String from and to byte[] */
+ public static final Charset CHARACTER_ENCODING = StandardCharsets.UTF_8;
+
+ /** Log. */
+ private static final Logger log = LoggerFactory.getLogger(AbstractJceCrypt.class);
+
+    protected Cipher buildCipher(int opMode, SecretKey secretKey, String transformation, AlgorithmParameterSpec params)
+    {
+        try
+        {
+            Cipher crypter = Cipher.getInstance(transformation);
+            crypter.init(opMode, secretKey, params);
+            return crypter;
+        } catch (InvalidKeyException | InvalidAlgorithmParameterException
+                | NoSuchAlgorithmException | NoSuchPaddingException e) {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+ * Decrypts a string into a string.
+ *
+ * @param text
+ *            text to decrypt
+ * @return the decrypted text
+ */
+ @Override
+ public final String decryptUrlSafe(final String text)
+ {
+ try
+ {
+ byte[] decoded = java.util.Base64.getUrlDecoder().decode(text);
+ return new String(decryptByteArray(decoded), CHARACTER_ENCODING);
+ }
+ catch (Exception ex)
+ {
+ log.debug("Error decoding text: " + text, ex);
+ return null;
+ }
+ }
+
+ /**
+ * Encrypt a string into a string using URL safe Base64 encoding.
+ *
+ * @param plainText
+ *            text to encrypt
+ * @return encrypted string
+ */
+ @Override
+ public final String encryptUrlSafe(final String plainText)
+ {
+ byte[] encrypted = encryptStringToByteArray(plainText);
+        Base64.Encoder encoder = Base64.getUrlEncoder().withoutPadding();
+        byte[] encoded = encoder.encode(encrypted);
+        return new String(encoded, CHARACTER_ENCODING);
+ }
+
+ /**
+ * Decrypts an encrypted byte array.
+ *
+ * @param encrypted
+ *            byte array to decrypt
+ * @return the decrypted text
+ */
+ abstract protected byte[] decryptByteArray(final byte[] encrypted);
+
+
+ /**
+ * Encrypts the given text into a byte array.
+ *
+ * @param plainText
+ *            text to encrypt
+ * @return the string encrypted
+ * @throws GeneralSecurityException
+ */
+ abstract protected byte[] encryptStringToByteArray(final String plainText);
+
+
+    @Override
+    public void setKey(String key) {
+        throw new UnsupportedOperationException();

Review comment:
       A message why it is not supported would be good

##########
File path: wicket-util/src/main/java/org/apache/wicket/util/crypt/AbstractJceCrypt.java
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.util.crypt;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
+import java.security.GeneralSecurityException;
+import java.security.InvalidAlgorithmParameterException;
+import java.security.InvalidKeyException;
+import java.security.NoSuchAlgorithmException;
+import java.security.spec.AlgorithmParameterSpec;
+import java.util.Base64;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import javax.crypto.SecretKey;
+
+
+/**
+ * Base class for JCE based ICrypt implementations.
+ *
+ */
+public abstract class AbstractJceCrypt implements ICrypt
+{
+ /** Encoding used to convert java String from and to byte[] */
+ public static final Charset CHARACTER_ENCODING = StandardCharsets.UTF_8;
+
+ /** Log. */
+ private static final Logger log = LoggerFactory.getLogger(AbstractJceCrypt.class);
+
+    protected Cipher buildCipher(int opMode, SecretKey secretKey, String transformation, AlgorithmParameterSpec params)
+    {
+        try
+        {
+            Cipher crypter = Cipher.getInstance(transformation);

Review comment:
       s/crypter/cipher/

##########
File path: wicket-util/src/main/java/org/apache/wicket/util/crypt/AbstractJceCrypt.java
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.util.crypt;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
+import java.security.GeneralSecurityException;
+import java.security.InvalidAlgorithmParameterException;
+import java.security.InvalidKeyException;
+import java.security.NoSuchAlgorithmException;
+import java.security.spec.AlgorithmParameterSpec;
+import java.util.Base64;
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
+import javax.crypto.SecretKey;
+
+
+/**
+ * Base class for JCE based ICrypt implementations.
+ *
+ */
+public abstract class AbstractJceCrypt implements ICrypt
+{
+ /** Encoding used to convert java String from and to byte[] */
+ public static final Charset CHARACTER_ENCODING = StandardCharsets.UTF_8;
+
+ /** Log. */
+ private static final Logger log = LoggerFactory.getLogger(AbstractJceCrypt.class);
+
+    protected Cipher buildCipher(int opMode, SecretKey secretKey, String transformation, AlgorithmParameterSpec params)
+    {
+        try
+        {
+            Cipher crypter = Cipher.getInstance(transformation);
+            crypter.init(opMode, secretKey, params);
+            return crypter;
+        } catch (InvalidKeyException | InvalidAlgorithmParameterException
+                | NoSuchAlgorithmException | NoSuchPaddingException e) {
+            throw new RuntimeException(e);
+        }
+    }
+
+    /**
+ * Decrypts a string into a string.
+ *
+ * @param text
+ *            text to decrypt
+ * @return the decrypted text
+ */
+ @Override
+ public final String decryptUrlSafe(final String text)
+ {
+ try
+ {
+ byte[] decoded = java.util.Base64.getUrlDecoder().decode(text);
+ return new String(decryptByteArray(decoded), CHARACTER_ENCODING);
+ }
+ catch (Exception ex)
+ {
+ log.debug("Error decoding text: " + text, ex);
+ return null;
+ }
+ }
+
+ /**
+ * Encrypt a string into a string using URL safe Base64 encoding.
+ *
+ * @param plainText
+ *            text to encrypt
+ * @return encrypted string
+ */
+ @Override
+ public final String encryptUrlSafe(final String plainText)
+ {
+ byte[] encrypted = encryptStringToByteArray(plainText);
+        Base64.Encoder encoder = Base64.getUrlEncoder().withoutPadding();
+        byte[] encoded = encoder.encode(encrypted);
+        return new String(encoded, CHARACTER_ENCODING);
+ }
+
+ /**
+ * Decrypts an encrypted byte array.
+ *
+ * @param encrypted
+ *            byte array to decrypt
+ * @return the decrypted text
+ */
+ abstract protected byte[] decryptByteArray(final byte[] encrypted);
+
+
+ /**
+ * Encrypts the given text into a byte array.
+ *
+ * @param plainText
+ *            text to encrypt
+ * @return the string encrypted
+ * @throws GeneralSecurityException
+ */
+ abstract protected byte[] encryptStringToByteArray(final String plainText);

Review comment:
       Do we need these long method names ? The return type and the argument type already give this info. Just `encrypt` is enough to me

##########
File path: wicket-util/src/main/java/org/apache/wicket/util/crypt/SunJceCrypt.java
##########
@@ -153,4 +194,18 @@ protected KeySpec createKeySpec()
  {
  return new PBEKeySpec(getKey().toCharArray());
  }
+
+ /**
+ * Create a random salt to be used for this crypt.
+ *
+ * @return salt, always 8 bytes long
+ */
+ public static byte[] randomSalt()
+ {
+ // must be 8 bytes - for anything else PBES1Core throws
+ // InvalidAlgorithmParameterException: Salt must be 8 bytes long  
+ byte[] salt = new byte[8];
+ new Random().nextBytes(salt);

Review comment:
       Should we use CipherUtils.randomByteArray(8) here ? It would use SecureRandom




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] martin-g commented on a change in pull request #462: WICKET-6864 updated crypt configuration

GitBox
In reply to this post by GitBox

martin-g commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r591241726



##########
File path: wicket-core/src/main/java/org/apache/wicket/core/util/crypt/AbstractKeyInSessionCryptFactory.java
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.core.util.crypt;
+
+import org.apache.wicket.MetaDataKey;
+import org.apache.wicket.Session;
+import org.apache.wicket.util.crypt.ICrypt;
+import org.apache.wicket.util.crypt.ICryptFactory;
+
+import java.io.Serializable;
+
+
+/**
+ * Base class to implement crypt factories that store crypt into user session. Note that the use of
+ * this crypt factory will result in an immediate creation of a http session.
+ *
+ * @author andrea del bene
+ *
+ * @param <T>
+ *            the type for the secret key.
+ */
+public abstract class AbstractKeyInSessionCryptFactory<T extends Serializable>
+ implements
+ ICryptFactory
+{
+ /** metadata-key used to store crypto-key in session metadata */
+ private static final MetaDataKey<Serializable> KEY = new MetaDataKey<Serializable>()

Review comment:
       `MetaDataKey<T>`




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] solomax commented on a change in pull request #462: WICKET-6864 updated crypt configuration

GitBox
In reply to this post by GitBox

solomax commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r591607503



##########
File path: wicket-util/src/main/java/org/apache/wicket/util/crypt/CipherUtils.java
##########
@@ -0,0 +1,107 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.util.crypt;
+
+import java.security.NoSuchAlgorithmException;
+import java.security.SecureRandom;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.KeySpec;
+
+import javax.crypto.Cipher;
+import javax.crypto.KeyGenerator;
+import javax.crypto.SecretKey;
+import javax.crypto.SecretKeyFactory;
+import javax.crypto.spec.PBEKeySpec;
+import javax.crypto.spec.SecretKeySpec;
+
+/**
+ * Utility class meant to help building {@link Cipher}.
+ */
+public class CipherUtils
+{
+ /**
+ * Generate a new {@link SecretKey} based on the given algorithm and with the given length.
+ *
+ * @param algorithm
+ *              the algorithm that will be used to build the key.
+ * @param keyLength
+ *              the key length
+ * @return a new {@link SecretKey}
+ */
+ public static SecretKey generateKey(String algorithm, int keyLength)
+ {
+ try
+ {
+ KeyGenerator keyGenerator = KeyGenerator.getInstance(algorithm);
+ keyGenerator.init(keyLength);
+ SecretKey key = keyGenerator.generateKey();
+ return key;
+ }
+ catch (NoSuchAlgorithmException e)
+ {
+ throw new RuntimeException(e);
+ }
+ }
+
+ /**
+ *
+ *
+ * @param password
+ *              the password that will be used to build the key.
+ * @param pbeAlgorithm
+ *              the password-based algorithm that will be used to build the key.
+ * @param keyAlgorithm
+ *              the algorithm that will be used to build the key.
+ * @param salt
+ *              salt for encryption.
+ * @param iterationCount
+ * iteration count.
+ * @param keyLength
+ *              the key length.
+ * @return a new {@link SecretKey}
+ */
+ public static SecretKey generatePBEKey(String password, String pbeAlgorithm,
+ String keyAlgorithm, byte[] salt, int iterationCount, int keyLength)
+ {
+ try
+ {
+ SecretKeyFactory factory = SecretKeyFactory.getInstance(pbeAlgorithm);
+ KeySpec spec = new PBEKeySpec(password.toCharArray(), salt, iterationCount, keyLength);
+ SecretKey secret = new SecretKeySpec(factory.generateSecret(spec).getEncoded(),
+ keyAlgorithm);
+ return secret;
+ }
+ catch (NoSuchAlgorithmException | InvalidKeySpecException e)
+ {
+ throw new RuntimeException(e);
+ }
+ }
+
+ /**
+ * Generates a random generated byte array of the given size.
+ *
+ * @param size
+ *              the size of the array.
+ * @return the new byte array of the given size.
+ */
+ public static byte[] randomByteArray(int size)
+ {
+ byte[] iv = new byte[size];
+ new SecureRandom().nextBytes(iv);

Review comment:
       Maybe it would be better to have only one instance of `SecureRandom`?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


123