You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@wicket.apache.org by GitBox <gi...@apache.org> on 2021/02/07 14:53:16 UTC
[GitHub] [wicket] svenmeier opened a new pull request #462: WICKET-6864 updated crypt configuration
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:
users@infra.apache.org
[GitHub] [wicket] papegaaij commented on pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [wicket] martin-g commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r592155258
##########
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:
I see. +1 if it works!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [wicket] papegaaij commented on pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [wicket] papegaaij commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
papegaaij commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r591810682
##########
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);
Review comment:
Pass in a SecureRandom here as well using the one from SecuritySettings.getRandomSupplier().getRandom(). That will make sure we use one reliable source of random and it's customizable by the user.
##########
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.
Review comment:
The ivSize is not needed. It is always equal to the blocksize of the cipher. So you can simply use cipher.getBlockSize() to get it.
##########
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:
Better yet, remove this method entirely. Usage of this method should be replaced by ISecureRandomSupplier.getRandomBytes(length). The optimal length of the salt depends on the algorithm used. Some might require 8 bytes, but for example PBKDF2WithHmacSHA512 can use much longer salts and it's recommended to use a salt that's equal in length to your derived key.
##########
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:
Yes, creating a SecureRandom over and over will exhaust your systems entropy. This method already exists in ISecureRandomSupplier (SecuritySettings.getRandomSupplier), so it 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:
users@infra.apache.org
[GitHub] [wicket] papegaaij commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
papegaaij commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r593220468
##########
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:
Having access to a good source of secure random is rather critical for most forms of cryptography. It's unfortunate that these classes live in a place that does not have direct access to the random managed by the application. I'd say move AbstractJceCrypt, AESCrypt and everything that goes with it to core. AbstractJceCrypt can then have convenience methods to fetch the SecureRandom from the application. That way we hopefully encourage other developers to use this as well if they want to implement their own.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [wicket] bitstorm commented on pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [wicket] bitstorm commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
bitstorm commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r592729143
##########
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:
I don't think it can be used if we want to log the exception as well. Am I wrong?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [wicket] bitstorm commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
bitstorm commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r592726278
##########
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:
Unfortunately we are talking of classes from module wicket-util which can't see or use classes Application or ISecureRandomSupplier. But we could move AESCrypt to wicket-core module so it can access to application setting.
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:
users@infra.apache.org
[GitHub] [wicket] bitstorm commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
bitstorm commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r593756360
##########
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:
We already use it inside AbstractJceCrypt and AESCrypt.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [wicket] solomax commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [wicket] martin-g commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [wicket] bitstorm commented on pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
bitstorm commented on pull request #462:
URL: https://github.com/apache/wicket/pull/462#issuecomment-800602317
@martin-g if everything is ok now let me now and I will proceed with the merge
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [wicket] papegaaij commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
papegaaij commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r593866073
##########
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.
Review comment:
You get the Cipher with just `Cipher.getInstance(alg)`. There you can get the block size from. You only need the iv for the call to `init`. I'm typing this on my phone, so posting code is a bit difficult. What about this?
```
public byte[] decrypt(SecureRandom rnd, SecretKey key, byte[] cipherInput) throws GeneralSecurityException {
Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
int blocksize = cipher.getBlockSize();
byte[] iv = new byte[blocksize];
byte[] ciphertext = new byte[cipherInput.length - blocksize];
System.arraycopy(cipherInput, 0, iv, 0, iv.length);
System.arraycopy(cipherInput, blocksize, ciphertext, 0, ciphertext.length);
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:
users@infra.apache.org
[GitHub] [wicket] bitstorm commented on pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [wicket] papegaaij edited a comment on pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [wicket] solomax commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [wicket] papegaaij commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
papegaaij commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r593866073
##########
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.
Review comment:
You get the Cipher with just `Cipher.getInstance(alg)`. There you can get the block size from. You only need the iv for the call to `init`. I'm typing this on my phone, so posting code is a bit difficult. What about this?
```public byte[] decrypt(SecureRandom rnd, SecretKey key, byte[] cipherInput) throws GeneralSecurityException {
Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
int blocksize = cipher.getBlockSize();
byte[] iv = new byte[blocksize];
byte[] ciphertext = new byte[cipherInput.length - blocksize];
System.arraycopy(cipherInput, 0, iv, 0, iv.length);
System.arraycopy(cipherInput, blocksize, ciphertext, 0, ciphertext.length);
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:
users@infra.apache.org
[GitHub] [wicket] solomax commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [wicket] solomax commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
solomax commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r592833393
##########
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:
`log.debug("Error decoding text: {}", text, ex);` will work as expected: will print message and trace :)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [wicket] svenmeier commented on pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [wicket] bitstorm commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
bitstorm commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r593797000
##########
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.
Review comment:
Sorry but you are loosing me here. How could you build a Cipher in DECRYPT_MODE without already having the expected IV (not just the size of it)? Is it possible to write a version of your method _decrypt_ posted above without knowing the fixed size of IV?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [wicket] papegaaij commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
papegaaij commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r593776954
##########
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.
Review comment:
But that's only because the lookup and unit of the Cipher are in `buildCipher`. IMHO that method does not really make things easier to understand. I think the code would be better with that method unlined. Then you can bring the Cipher lookup to the front and use that to get the block size.
This is just my opinion. If you do decide to keep the method, then rename the field to `blockSize`, as that is what it is.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [wicket] martin-g commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [wicket] papegaaij commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
papegaaij commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r593866073
##########
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.
Review comment:
You get the Cipher with just `Cipher.getInstance(alg)`. There you can get the block size from. You only need the iv for the call to `init`. I'm typing this on my phone, so posting code is a bit difficult. What about this?
```
public byte[] decrypt(SecureRandom rnd, SecretKey key, byte[] cipherInput) throws GeneralSecurityException {
Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
int blocksize = cipher.getBlockSize();
byte[] iv = new byte[blocksize];
byte[] ciphertext = new byte[cipherInput.length - blocksize];
System.arraycopy(cipherInput, 0, iv, 0, iv.length);
System.arraycopy(cipherInput, blocksize, ciphertext, 0, ciphertext.length);
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:
users@infra.apache.org
[GitHub] [wicket] bitstorm commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
bitstorm commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r593757595
##########
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:
Good point. Now the abstract class define just "encrypt" and "decrypt" , both operating on byte arrays.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [wicket] bitstorm commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
bitstorm commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r592729143
##########
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:
I don't think it can be used if we want to log the exception as well. Am I wrorng?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [wicket] svenmeier commented on pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [wicket] bitstorm commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
bitstorm commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r593762860
##########
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:
Since this is a legacy class I would leave it as it is as it can not be moved to wicket-core
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [wicket] martin-g commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [wicket] bitstorm commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
bitstorm commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r593928797
##########
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.
Review comment:
Thank you, now I see... :-)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [wicket] bitstorm commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
bitstorm commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r593763080
##########
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.
Review comment:
Right, but during decipher phase you need ivSize before creating the cipher reading the first ivSize of encrypted data. So I would leave this constructor parameter
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [wicket] bitstorm merged pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
bitstorm merged pull request #462:
URL: https://github.com/apache/wicket/pull/462
--
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:
users@infra.apache.org
[GitHub] [wicket] papegaaij commented on pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [wicket] svenmeier commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [wicket] solomax commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [wicket] svenmeier commented on pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [wicket] bitstorm commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
bitstorm commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r593462716
##########
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:
Nice!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [wicket] bitstorm commented on pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [wicket] martin-g commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r594950194
##########
File path: wicket-core/src/main/java/org/apache/wicket/core/util/crypt/AESCrypt.java
##########
@@ -0,0 +1,132 @@
+/*
+ * 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.core.random.ISecureRandomSupplier;
+import org.apache.wicket.util.crypt.ICrypt;
+import org.apache.wicket.util.lang.Args;
+
+import java.security.InvalidAlgorithmParameterException;
+import java.security.InvalidKeyException;
+import java.security.NoSuchAlgorithmException;
+
+import javax.crypto.BadPaddingException;
+import javax.crypto.Cipher;
+import javax.crypto.IllegalBlockSizeException;
+import javax.crypto.NoSuchPaddingException;
+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
+ */
+public class AESCrypt extends AbstractJceCrypt
+{
+
+ private final SecretKey secretKey;
+ private final String algorithm;
+ private ISecureRandomSupplier randomSupplier;
Review comment:
final
##########
File path: wicket-core/src/main/java/org/apache/wicket/core/util/crypt/AbstractJceCrypt.java
##########
@@ -0,0 +1,105 @@
+/*
+ * 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.util.crypt.ICrypt;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
+import java.security.GeneralSecurityException;
+import java.util.Base64;
+
+
+/**
+ * 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;
Review comment:
Any reason this to be `public` ?
Why not just use `StandardCharsets.UTF_8` ? If the encoding should be configurable then there should be a constructor parameter for it.
##########
File path: wicket-core/src/main/java/org/apache/wicket/core/util/crypt/AbstractKeyInSessionCryptFactory.java
##########
@@ -0,0 +1,89 @@
+/*
+ * 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 org.apache.wicket.util.io.IClusterable;
+
+
+/**
+ * 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 IClusterable>
+ implements
+ ICryptFactory
+{
+ /** metadata-key used to store crypto-key in session metadata */
+ private final MetaDataKey<T> KEY = new MetaDataKey<T>()
+ {
+ private static final long serialVersionUID = 1L;
+ };
+
+ public AbstractKeyInSessionCryptFactory()
Review comment:
No need of this constructor, it is implicit
##########
File path: wicket-core/src/test/java/org/apache/wicket/core/util/crypt/AESCryptTest.java
##########
@@ -0,0 +1,54 @@
+/*
+ * 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 static org.junit.jupiter.api.Assertions.assertEquals;
+
+import org.apache.wicket.core.random.DefaultSecureRandomSupplier;
+import org.apache.wicket.util.crypt.CipherUtils;
+import org.junit.jupiter.api.Test;
+
+import java.security.GeneralSecurityException;
+
+import javax.crypto.SecretKey;
+
+public class AESCryptTest
+{
+ @Test
+ public void encrypDecrypt() throws GeneralSecurityException
+ {
+ DefaultSecureRandomSupplier randomSupplier = new DefaultSecureRandomSupplier();
+
+ SecretKey secretKey = CipherUtils.generatePBEKey(
+ "myWeakPassword", "PBKDF2WithHmacSHA1", "AES",
+ randomSupplier.getRandomBytes(16), 65536, 256);
+
+ AbstractJceCrypt crypt = new AESCrypt(secretKey, randomSupplier);
+
+ String input1 = "input1";
+ String encrypted = crypt.encryptUrlSafe(input1);
+
+ String input2 = "input2";
Review comment:
In my previous review I have suggested to use a more fancy input, e.g. one with umlauts or cyrillic characters, to test UTF-8, not just ASCII
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [wicket] bitstorm commented on a change in pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
bitstorm commented on a change in pull request #462:
URL: https://github.com/apache/wicket/pull/462#discussion_r591879350
##########
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:
To use the generic type T filed KEY must be an instance field, I don't think it's a problem even if we usually have static fields for metadata keys. 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:
users@infra.apache.org
[GitHub] [wicket] bitstorm edited a comment on pull request #462: WICKET-6864 updated crypt configuration
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org