You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@wicket.apache.org by sv...@apache.org on 2021/02/07 14:51:25 UTC

[wicket] 01/01: WICKET-6864 updated crypt configuration

This is an automated email from the ASF dual-hosted git repository.

svenmeier pushed a commit to branch WICKET-6864-crypt-enhancement
in repository https://gitbox.apache.org/repos/asf/wicket.git

commit 7bc47ec03cbbf273570ae72b535fa4663024737e
Author: Sven Meier <sv...@apache.org>
AuthorDate: Sun Feb 7 11:58:34 2021 +0100

    WICKET-6864 updated crypt configuration
    
    to recommended standards
---
 .../strategy/DefaultAuthenticationStrategy.java    | 62 ++++++++++++++------
 .../util/crypt/KeyInSessionSunJceCryptFactory.java | 41 +++++++++----
 .../apache/wicket/settings/SecuritySettings.java   | 10 +++-
 .../core/request/mapper/CryptoMapperTest.java      | 26 ++-------
 .../wicket/examples/WicketExampleApplication.java  |  5 +-
 .../src/main/asciidoc/internals/pagestoring.adoc   |  8 +--
 .../util/crypt/CachingSunJceCryptFactory.java      |  2 +
 .../wicket/util/crypt/ClassCryptFactory.java       |  2 +
 .../org/apache/wicket/util/crypt/SunJceCrypt.java  | 67 +++++++++++++++++++---
 .../apache/wicket/util/crypt/SunJceCryptTest.java  |  2 +
 10 files changed, 159 insertions(+), 66 deletions(-)

diff --git a/wicket-core/src/main/java/org/apache/wicket/authentication/strategy/DefaultAuthenticationStrategy.java b/wicket-core/src/main/java/org/apache/wicket/authentication/strategy/DefaultAuthenticationStrategy.java
index d51efbc..6567bdd 100644
--- a/wicket-core/src/main/java/org/apache/wicket/authentication/strategy/DefaultAuthenticationStrategy.java
+++ b/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;
+import java.util.UUID;
+
 import org.apache.wicket.authentication.IAuthenticationStrategy;
 import org.apache.wicket.util.cookies.CookieDefaults;
 import org.apache.wicket.util.cookies.CookieUtils;
-import org.apache.wicket.util.crypt.CachingSunJceCryptFactory;
 import org.apache.wicket.util.crypt.ICrypt;
+import org.apache.wicket.util.crypt.SunJceCrypt;
 import org.apache.wicket.util.lang.Args;
 import org.apache.wicket.util.string.Strings;
 import org.slf4j.Logger;
@@ -30,6 +32,9 @@ import org.slf4j.LoggerFactory;
 /**
  * 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[])}.
  * 
  * @author Juergen Donnerstag
  */
@@ -40,8 +45,11 @@ public class DefaultAuthenticationStrategy implements IAuthenticationStrategy
 	/** The cookie name to store the username and password */
 	protected final String cookieKey;
 
-	/** The key to use for encrypting/decrypting the cookie value  */
-	protected final String encryptionKey;
+	/**
+	 * @deprecated no longer used TODO remove in Wicket 10
+	 */
+	@Deprecated
+	protected final String encryptionKey = null;
 
 	/** The separator used to concatenate the username and password */
 	protected final String VALUE_SEPARATOR = "-sep-";
@@ -57,25 +65,50 @@ public class DefaultAuthenticationStrategy implements IAuthenticationStrategy
 	 * 
 	 * @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)
+	{
 		this.cookieKey = Args.notEmpty(cookieKey, "cookieKey");
-		this.encryptionKey = Args.notEmpty(encryptionKey, "encryptionKey");
+		this.crypt = Args.notNull(crypt, "crypt");
 	}
 
 	/**
@@ -99,11 +132,6 @@ public class DefaultAuthenticationStrategy implements IAuthenticationStrategy
 	 */
 	protected ICrypt getCrypt()
 	{
-		if (crypt == null)
-		{
-			CachingSunJceCryptFactory cryptFactory = new CachingSunJceCryptFactory(encryptionKey);
-			crypt = cryptFactory.newCrypt();
-		}
 		return crypt;
 	}
 
diff --git a/wicket-core/src/main/java/org/apache/wicket/core/util/crypt/KeyInSessionSunJceCryptFactory.java b/wicket-core/src/main/java/org/apache/wicket/core/util/crypt/KeyInSessionSunJceCryptFactory.java
index 556c1ff..381b83f 100644
--- a/wicket-core/src/main/java/org/apache/wicket/core/util/crypt/KeyInSessionSunJceCryptFactory.java
+++ b/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;
 import java.util.UUID;
 
 import org.apache.wicket.MetaDataKey;
@@ -38,8 +40,8 @@ import org.apache.wicket.util.lang.Args;
  */
 public class KeyInSessionSunJceCryptFactory implements ICryptFactory
 {
-	/** metadata-key used to store crypto-key in session metadata */
-	private static final MetaDataKey<String> KEY = new MetaDataKey<>()
+	/** metadata-key used to store crypt data in session metadata */
+	private static final MetaDataKey<CryptData> KEY = new MetaDataKey<>()
 	{
 		private static final long serialVersionUID = 1L;
 	};
@@ -89,25 +91,44 @@ public class KeyInSessionSunJceCryptFactory implements ICryptFactory
 		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
 	 */
 	protected ICrypt createCrypt()
 	{
-		return new SunJceCrypt(cryptMethod);
+		return null;
+	}
+	
+	private static final class CryptData implements Serializable {
+		final String key;
+		
+		final byte[] salt;
+		
+		CryptData(String key, byte[] salt) {
+			this.key = key;
+			this.salt = salt;
+		}
 	}
 }
diff --git a/wicket-core/src/main/java/org/apache/wicket/settings/SecuritySettings.java b/wicket-core/src/main/java/org/apache/wicket/settings/SecuritySettings.java
index a618ee2..e05a061 100644
--- a/wicket-core/src/main/java/org/apache/wicket/settings/SecuritySettings.java
+++ b/wicket-core/src/main/java/org/apache/wicket/settings/SecuritySettings.java
@@ -49,8 +49,11 @@ import org.apache.wicket.util.lang.Args;
 public class SecuritySettings
 {
 	/**
-	 * encryption key used by default crypt factory
+	 * encryption key is no longer used
+	 * 
+	 * @deprecated
 	 */
+	@Deprecated
 	public static final String DEFAULT_ENCRYPTION_KEY = "WiCkEt-FRAMEwork";
 
 	/** The authorization strategy. */
@@ -121,8 +124,8 @@ public class SecuritySettings
 	}
 
 	/**
-	 * Note: Prints a warning to stderr if no factory was set and {@link #DEFAULT_ENCRYPTION_KEY} is
-	 * used instead.
+	 * Returns the {@link ICryptFactory}. If no factory is set, a {@link KeyInSessionSunJceCryptFactory}
+	 * is used.
 	 * 
 	 * @return crypt factory used to generate crypt objects
 	 */
@@ -270,6 +273,7 @@ public class SecuritySettings
 	 *
 	 * @return Returns the authentication strategy.
 	 */
+	@SuppressWarnings("deprecation")
 	public IAuthenticationStrategy getAuthenticationStrategy()
 	{
 		if (authenticationStrategy == null)
diff --git a/wicket-core/src/test/java/org/apache/wicket/core/request/mapper/CryptoMapperTest.java b/wicket-core/src/test/java/org/apache/wicket/core/request/mapper/CryptoMapperTest.java
index 17d2273..3a89b26 100644
--- a/wicket-core/src/test/java/org/apache/wicket/core/request/mapper/CryptoMapperTest.java
+++ b/wicket-core/src/test/java/org/apache/wicket/core/request/mapper/CryptoMapperTest.java
@@ -24,8 +24,6 @@ import static org.junit.jupiter.api.Assertions.assertSame;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
-import java.util.function.Supplier;
-
 import org.apache.wicket.MockPage;
 import org.apache.wicket.core.request.handler.BookmarkableListenerRequestHandler;
 import org.apache.wicket.core.request.handler.ListenerRequestHandler;
@@ -50,10 +48,8 @@ import org.apache.wicket.request.mapper.info.PageComponentInfo;
 import org.apache.wicket.request.mapper.parameter.PageParameters;
 import org.apache.wicket.request.resource.PackageResourceReference;
 import org.apache.wicket.request.resource.UrlResourceReference;
-import org.apache.wicket.settings.SecuritySettings;
-import org.apache.wicket.util.crypt.CachingSunJceCryptFactory;
 import org.apache.wicket.util.crypt.ICrypt;
-import org.apache.wicket.util.crypt.ICryptFactory;
+import org.apache.wicket.util.crypt.SunJceCrypt;
 import org.apache.wicket.util.string.StringValue;
 import org.apache.wicket.util.string.Strings;
 import org.apache.wicket.util.tester.WicketTester;
@@ -100,23 +96,11 @@ class CryptoMapperTest extends AbstractMapperTest
 		WebApplication application = tester.getApplication();
 		application.mountPage(MOUNTED_URL, Page1.class);
 
-		/**
-		 * Use explicit crypt provider to prevent crypt warning output, see
-		 * SecuritySettings#getCryptFactory()
-		 */
-		Supplier<ICrypt> cryptProvider = new Supplier<ICrypt>()
-		{
-			private ICryptFactory cryptFactory = new CachingSunJceCryptFactory(
-				SecuritySettings.DEFAULT_ENCRYPTION_KEY);
-
-			@Override
-			public ICrypt get()
-			{
-				return cryptFactory.newCrypt();
-			}
-		};
+		ICrypt crypt = new SunJceCrypt(new byte[]{ (byte)0x15, (byte)0x8c, (byte)0xa3, (byte)0x4a,
+			(byte)0x66, (byte)0x51, (byte)0x2a, (byte)0xbc }, 17);
+		crypt.setKey("WiCkEt-FRAMEwork");
 
-		mapper = new CryptoMapper(application.getRootRequestMapper(), cryptProvider);
+		mapper = new CryptoMapper(application.getRootRequestMapper(), () -> crypt);
 	}
 
 	/**
diff --git a/wicket-examples/src/main/java/org/apache/wicket/examples/WicketExampleApplication.java b/wicket-examples/src/main/java/org/apache/wicket/examples/WicketExampleApplication.java
index aa083c3..9430b48 100644
--- a/wicket-examples/src/main/java/org/apache/wicket/examples/WicketExampleApplication.java
+++ b/wicket-examples/src/main/java/org/apache/wicket/examples/WicketExampleApplication.java
@@ -22,8 +22,6 @@ import org.apache.wicket.request.cycle.IRequestCycleListener;
 import org.apache.wicket.request.cycle.RequestCycle;
 import org.apache.wicket.request.http.WebResponse;
 import org.apache.wicket.resource.CssUrlReplacer;
-import org.apache.wicket.settings.SecuritySettings;
-import org.apache.wicket.util.crypt.ClassCryptFactory;
 import org.apache.wicket.util.crypt.NoCrypt;
 
 
@@ -57,8 +55,7 @@ public abstract class WicketExampleApplication extends WebApplication
 		// 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());
 
 		getDebugSettings().setDevelopmentUtilitiesEnabled(true);
 		
diff --git a/wicket-user-guide/src/main/asciidoc/internals/pagestoring.adoc b/wicket-user-guide/src/main/asciidoc/internals/pagestoring.adoc
index 273a874..4bb85b9 100644
--- a/wicket-user-guide/src/main/asciidoc/internals/pagestoring.adoc
+++ b/wicket-user-guide/src/main/asciidoc/internals/pagestoring.adoc
@@ -9,12 +9,12 @@ image::../img/page-storage.png[]
 === IPageManager
 
 _org.apache.wicket.page.IPageManager_'s task is to manage which pages have been used in a request and store their last state in the backing stores, namely _IPageStore_.
-The default implementation _org.apache.wicket.page.PageStoreManager_ collects all stateful pages which have been used in the request cycle (more than one page can be used in a single request if for example _setResponsePage()_ or _RestartResponseException_ is used).
-At the end of the request all collected page instances are being stored in the first level cache - http session. They are stored in http session attribute named "_wicket:persistentPageManagerData-APPLICATION_NAME_" and passed to the underlying _IPageStore_.
-When the next http request comes _IPageProvider_ will ask for page with specific id and _PageStoreManager_ will look first in the http session and if no match is found then it will delegate to the IPageStore. At the end of the second request the http session based cache is being overwritten completely with the newly used page instances.
+The default implementation _org.apache.wicket.page.PageManager_ uses a chaing of page stores to collect all stateful pages which have been used in the request cycle (more than one page can be used in a single request if for example _setResponsePage()_ or _RestartResponseException_ is used).
+At the end of the request all collected page instances are being stored in the first level cache - http session. They are stored as metadata in the http session and passed to an underlying _IPageStore_.
+When the next http request is handled, _IPageProvider_ will ask for page with specific id and _PageManager_ will look first in the http session and if no match is found then it will delegate to any further IPageStore. At the end of the second request the http session based cache is being overwritten completely with the newly used page instances.
 
 To setup another _IPageManager_ implementation use _org.apache.wicket.Application.setPageManagerProvider(IPageManagerProvider)_.
-The custom _IPageManager_ implementation may or may not use _IPageStore/IDataStore_.
+The custom _IPageManager_ implementation may use a custom chain of _IPageStore_s as needed.
 
 === IPageStore
 
diff --git a/wicket-util/src/main/java/org/apache/wicket/util/crypt/CachingSunJceCryptFactory.java b/wicket-util/src/main/java/org/apache/wicket/util/crypt/CachingSunJceCryptFactory.java
index 65db9e4..8edf89e 100644
--- a/wicket-util/src/main/java/org/apache/wicket/util/crypt/CachingSunJceCryptFactory.java
+++ b/wicket-util/src/main/java/org/apache/wicket/util/crypt/CachingSunJceCryptFactory.java
@@ -21,6 +21,8 @@ package org.apache.wicket.util.crypt;
  * all further invocations of {@link #newCrypt()}.
  * 
  * @author Igor Vaynberg (ivaynberg)
+ * 
+ * @deprecated use a lambda expression instead TODO remove in Wicket 10
  */
 public class CachingSunJceCryptFactory extends CryptFactoryCachingDecorator
 {
diff --git a/wicket-util/src/main/java/org/apache/wicket/util/crypt/ClassCryptFactory.java b/wicket-util/src/main/java/org/apache/wicket/util/crypt/ClassCryptFactory.java
index 5216c3b..e74d758 100644
--- a/wicket-util/src/main/java/org/apache/wicket/util/crypt/ClassCryptFactory.java
+++ b/wicket-util/src/main/java/org/apache/wicket/util/crypt/ClassCryptFactory.java
@@ -28,6 +28,8 @@ import org.slf4j.LoggerFactory;
  * must implement {@link ICrypt}.
  * 
  * @author Igor Vaynberg (ivaynberg)
+ * 
+ * @deprecated use a lambda expression instead TODO remove in Wicket 10
  */
 public class ClassCryptFactory implements ICryptFactory
 {
diff --git a/wicket-util/src/main/java/org/apache/wicket/util/crypt/SunJceCrypt.java b/wicket-util/src/main/java/org/apache/wicket/util/crypt/SunJceCrypt.java
index b702851..32bd77d 100644
--- a/wicket-util/src/main/java/org/apache/wicket/util/crypt/SunJceCrypt.java
+++ b/wicket-util/src/main/java/org/apache/wicket/util/crypt/SunJceCrypt.java
@@ -21,6 +21,7 @@ import java.security.NoSuchAlgorithmException;
 import java.security.spec.AlgorithmParameterSpec;
 import java.security.spec.InvalidKeySpecException;
 import java.security.spec.KeySpec;
+import java.util.Random;
 
 import javax.crypto.Cipher;
 import javax.crypto.SecretKey;
@@ -44,22 +45,31 @@ public class SunJceCrypt extends AbstractCrypt
 	/**
 	 * 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()
 	{
@@ -67,16 +77,45 @@ public class SunJceCrypt extends AbstractCrypt
 	}
 
 	/**
+	 * 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;
 	}
 
 	/**
@@ -138,12 +177,13 @@ public class SunJceCrypt extends AbstractCrypt
 		return keyFactory.generateSecret(spec);
 	}
 
+	
 	/**
 	 * @return the parameter spec to be used for the configured crypt method
 	 */
 	protected AlgorithmParameterSpec createParameterSpec()
 	{
-		return PARAMETER_SPEC;
+		return new PBEParameterSpec(salt, iterationCount);
 	}
 
 	/**
@@ -153,4 +193,17 @@ public class SunJceCrypt extends AbstractCrypt
 	{
 		return new PBEKeySpec(getKey().toCharArray());
 	}
+
+	/**
+	 * Create a random salt.
+	 * 
+	 * @return salt
+	 */
+	public static byte[] randomSalt()
+	{
+		// only 8 bytes long supported
+		byte[] salt = new byte[8];
+		new Random().nextBytes(salt);
+		return salt;
+	}
 }
diff --git a/wicket-util/src/test/java/org/apache/wicket/util/crypt/SunJceCryptTest.java b/wicket-util/src/test/java/org/apache/wicket/util/crypt/SunJceCryptTest.java
index fdd3164..bed4e8f 100644
--- a/wicket-util/src/test/java/org/apache/wicket/util/crypt/SunJceCryptTest.java
+++ b/wicket-util/src/test/java/org/apache/wicket/util/crypt/SunJceCryptTest.java
@@ -34,6 +34,7 @@ public class SunJceCryptTest
 	@Test
 	public void defaultEncryption() throws GeneralSecurityException
 	{
+		@SuppressWarnings("deprecation")
 		SunJceCrypt crypt = new SunJceCrypt();
 		String input = "input";
 		byte[] encrypted = crypt.crypt(input.getBytes(), Cipher.ENCRYPT_MODE);
@@ -51,6 +52,7 @@ public class SunJceCryptTest
 		boolean unlimitedStrengthJurisdictionPolicyInstalled = isUnlimitedStrengthJurisdictionPolicyInstalled();
 		Assumptions.assumeTrue(unlimitedStrengthJurisdictionPolicyInstalled);
 
+		@SuppressWarnings("deprecation")
 		SunJceCrypt crypt = new SunJceCrypt("PBEWithMD5AndTripleDES");
 		String input = "input";
 		byte[] encrypted = crypt.crypt(input.getBytes(), Cipher.ENCRYPT_MODE);