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:
us...@infra.apache.org
