On Thu, Jan 16, 2020 at 10:51 PM <papega...@apache.org> wrote:
> This is an automated email from the ASF dual-hosted git repository.
>
> papegaaij pushed a commit to branch csp
> in repository https://gitbox.apache.org/repos/asf/wicket.git
>
>
> The following commit(s) were added to refs/heads/csp by this push:
> new f5ff7da WICKET-6730: central SecureRandom setting
> new 44c027f Merge remote-tracking branch 'origin/csp' into csp
> f5ff7da is described below
>
> commit f5ff7dad9b5b62e27d2c382d0a79128822753e59
> Author: Emond Papegaaij <emond.papega...@topicus.nl>
> AuthorDate: Thu Jan 16 21:50:06 2020 +0100
>
> WICKET-6730: central SecureRandom setting
> ---
> .../apache/wicket/DefaultPageManagerProvider.java | 2 +-
> .../core/random/DefaultSecureRandomSupplier.java | 50 +++++++++++++++++
> .../wicket/core/random/ISecureRandomSupplier.java | 62
> ++++++++++++++++++++++
> .../apache/wicket/pageStore/CryptingPageStore.java | 23 ++++----
> .../apache/wicket/settings/SecuritySettings.java | 36 +++++++++++++
> .../csp/CSPSettingRequestCycleListenerTest.java | 23 ++++----
> .../wicket/pageStore/CryptingPageStoreTest.java | 8 +--
> 7 files changed, 172 insertions(+), 32 deletions(-)
>
> diff --git
> a/wicket-core/src/main/java/org/apache/wicket/DefaultPageManagerProvider.java
> b/wicket-core/src/main/java/org/apache/wicket/DefaultPageManagerProvider.java
> index 1e8ec6f..e128266 100644
> ---
> a/wicket-core/src/main/java/org/apache/wicket/DefaultPageManagerProvider.java
> +++
> b/wicket-core/src/main/java/org/apache/wicket/DefaultPageManagerProvider.java
> @@ -183,7 +183,7 @@ public class DefaultPageManagerProvider implements
> IPageManagerProvider
>
> if (storeSettings.isEncrypted())
> {
> - pageStore = new CryptingPageStore(pageStore);
> + pageStore = new CryptingPageStore(pageStore,
> application);
> }
>
> return pageStore;
> diff --git
> a/wicket-core/src/main/java/org/apache/wicket/core/random/DefaultSecureRandomSupplier.java
> b/wicket-core/src/main/java/org/apache/wicket/core/random/DefaultSecureRandomSupplier.java
> new file mode 100644
> index 0000000..cb00235
> --- /dev/null
> +++
> b/wicket-core/src/main/java/org/apache/wicket/core/random/DefaultSecureRandomSupplier.java
> @@ -0,0 +1,50 @@
> +/*
> + * 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.random;
> +
> +import java.security.NoSuchAlgorithmException;
> +import java.security.SecureRandom;
> +
> +import org.apache.wicket.WicketRuntimeException;
> +
> +/**
> + * A very simple {@link ISecureRandomSupplier} that holds a strong {@code
> SecureRandom}.
> + *
> + * @author papegaaij
> + */
> +public class DefaultSecureRandomSupplier implements ISecureRandomSupplier
> +{
> + private SecureRandom random;
>
final
> +
> + public DefaultSecureRandomSupplier()
> + {
> + try
> + {
> + random = SecureRandom.getInstanceStrong();
>
Since we have now CryptingPageStore it is possible that an application
store some pages with algo X.
If the application is clustered and rolling upgrade is done with a new
version of JDK it is possible that the new best strong algo is now Y.
In this case the application won't be able to decrypt the old pages.
> + }
> + catch (NoSuchAlgorithmException e)
> + {
> + throw new WicketRuntimeException(e);
> + }
> + }
> +
> + @Override
> + public SecureRandom getRandom()
> + {
> + return random;
> + }
> +}
> diff --git
> a/wicket-core/src/main/java/org/apache/wicket/core/random/ISecureRandomSupplier.java
> b/wicket-core/src/main/java/org/apache/wicket/core/random/ISecureRandomSupplier.java
> new file mode 100644
> index 0000000..9b9fe14
> --- /dev/null
> +++
> b/wicket-core/src/main/java/org/apache/wicket/core/random/ISecureRandomSupplier.java
> @@ -0,0 +1,62 @@
> +/*
> + * 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.random;
> +
> +import java.security.SecureRandom;
> +import java.util.Base64;
> +
> +/**
> + * Supplies the Wicket application with random bytes.
> + *
> + * @author papegaaij
> + */
> +public interface ISecureRandomSupplier
> +{
> + /**
> + * Returns the actual {@code SecureRandom} being used as source.
> + *
> + * @return The {@code SecureRandom}.
> + */
> + public SecureRandom getRandom();
>
nit: no need of `public` in interfaces
> +
> + /**
> + * Returns a byte array with random bytes of the given length.
> + *
> + * @param length
> + * The number of bytes to return.
> + * @return A byte array with random bytes of the given length.
> + */
> + public default byte[] getRandomBytes(int length)
> + {
> + byte[] ret = new byte[length];
> + getRandom().nextBytes(ret);
> + return ret;
> + }
> +
> + /**
> + * Returns a base64 encoded string with random content, base on
> {@code length} bytes. The length
> + * of the returned string will be {@code length/3*4}.
>
The javadoc should be improved to say that this is about *url* encoding.
> + *
> + * @param length
> + * The number of random bytes to use as input.
> + * @return A string with random base64 data.
> + */
> + public default String getRandomBase64(int length)
> + {
> + return
> Base64.getUrlEncoder().encodeToString(getRandomBytes(length));
> + }
> +}
> diff --git
> a/wicket-core/src/main/java/org/apache/wicket/pageStore/CryptingPageStore.java
> b/wicket-core/src/main/java/org/apache/wicket/pageStore/CryptingPageStore.java
> index 91ac373..dc0d6eb 100644
> ---
> a/wicket-core/src/main/java/org/apache/wicket/pageStore/CryptingPageStore.java
> +++
> b/wicket-core/src/main/java/org/apache/wicket/pageStore/CryptingPageStore.java
> @@ -17,11 +17,11 @@
> package org.apache.wicket.pageStore;
>
> import java.io.Serializable;
> -import java.security.GeneralSecurityException;
> import java.security.SecureRandom;
>
> import javax.crypto.SecretKey;
>
> +import org.apache.wicket.Application;
> import org.apache.wicket.MetaDataKey;
> import org.apache.wicket.WicketRuntimeException;
> import org.apache.wicket.page.IManageablePage;
> @@ -46,27 +46,20 @@ public class CryptingPageStore extends
> DelegatingPageStore
> private static final long serialVersionUID = 1L;
> };
>
> - private final SecureRandom random;
> -
> private final ICrypter crypter;
>
> + private final Application application;
> +
> /**
> * @param delegate
> * store to delegate to
> * @param applicationName
> * name of application
> */
> - public CryptingPageStore(IPageStore delegate)
> + public CryptingPageStore(IPageStore delegate, Application
> application)
> {
> super(delegate);
> - try
> - {
> - random = SecureRandom.getInstance("SHA1PRNG",
> "SUN");
> - }
> - catch (GeneralSecurityException ex)
> - {
> - throw new WicketRuntimeException(ex);
> - }
> + this.application = Args.notNull(application,
> "application");
> crypter = newCrypter();
> }
>
> @@ -94,7 +87,8 @@ public class CryptingPageStore extends
> DelegatingPageStore
>
> private SessionData getSessionData(IPageContext context)
> {
> - return context.getSessionData(KEY, () -> new
> SessionData(crypter.generateKey(random)));
> + return context.getSessionData(KEY, () -> new
> SessionData(crypter
> +
>
> .generateKey(application.getSecuritySettings().getRandomSupplier().getRandom())));
> }
>
> /**
> @@ -138,7 +132,8 @@ public class CryptingPageStore extends
> DelegatingPageStore
> SerializedPage serializedPage = (SerializedPage) page;
>
> byte[] decrypted = serializedPage.getData();
> - byte[] encrypted =
> getSessionData(context).encrypt(decrypted, crypter, random);
> + byte[] encrypted =
> getSessionData(context).encrypt(decrypted, crypter,
> +
> application.getSecuritySettings().getRandomSupplier().getRandom());
>
> page = new SerializedPage(page.getPageId(),
> serializedPage.getPageType(), encrypted);
>
> 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 94eadea..94e2390 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
> @@ -24,6 +24,8 @@ import
> org.apache.wicket.authorization.IAuthorizationStrategy;
> import
> org.apache.wicket.authorization.IUnauthorizedComponentInstantiationListener;
> import
> org.apache.wicket.authorization.IUnauthorizedResourceRequestListener;
> import org.apache.wicket.authorization.UnauthorizedInstantiationException;
> +import org.apache.wicket.core.random.DefaultSecureRandomSupplier;
> +import org.apache.wicket.core.random.ISecureRandomSupplier;
> import org.apache.wicket.core.util.crypt.KeyInSessionSunJceCryptFactory;
> import org.apache.wicket.util.crypt.ICryptFactory;
> import org.apache.wicket.util.lang.Args;
> @@ -55,6 +57,9 @@ public class SecuritySettings
>
> /** factory for creating crypt objects */
> private ICryptFactory cryptFactory;
> +
> + /** supplier of random data and SecureRandom */
> + private ISecureRandomSupplier randomSupplier;
>
> /**
> * Whether mounts should be enforced. If {@code true}, requests
> for a page will be
> @@ -115,6 +120,21 @@ public class SecuritySettings
> }
>
> /**
> + * Returns the {@link ISecureRandomSupplier} to use for secure
> random data. If no supplier is
> + * set, a {@link DefaultSecureRandomSupplier} is instantiated.
> + *
> + * @return The {@link ISecureRandomSupplier} to use for secure
> random data.
> + */
> + public ISecureRandomSupplier getRandomSupplier()
>
synchronized ?
> + {
> + if (randomSupplier == null)
> + {
> + randomSupplier = new DefaultSecureRandomSupplier();
> + }
> + return randomSupplier;
> + }
> +
> + /**
> * Gets whether page mounts should be enforced. If {@code true},
> requests for a page will be
> * allowed only if the page has been explicitly mounted in {@link
> Application#init() MyApplication#init()}.
> *
> @@ -163,6 +183,22 @@ public class SecuritySettings
> this.cryptFactory = cryptFactory;
> return this;
> }
> +
> + /**
> + * Sets the supplier of secure random data for Wicket. The
> implementation must use a strong
> + * source of random data and be able to generate a lot of random
> data without running out of
> + * entropy.
> + *
> + * @param randomSupplier
> + * The new supplier, must not be null.
> + * @return {@code this} object for chaining
> + */
> + public SecuritySettings setRandomSupplier(ISecureRandomSupplier
> randomSupplier)
>
synchronized ?
> + {
> + Args.notNull(randomSupplier, "randomSupplier");
> + this.randomSupplier = randomSupplier;
> + return this;
> + }
>
> /**
> * Sets whether mounts should be enforced. If true, requests for
> mounted targets have to done
> diff --git
> a/wicket-core/src/test/java/org/apache/wicket/csp/CSPSettingRequestCycleListenerTest.java
> b/wicket-core/src/test/java/org/apache/wicket/csp/CSPSettingRequestCycleListenerTest.java
> index 14ba223..cdb1bc6 100644
> ---
> a/wicket-core/src/test/java/org/apache/wicket/csp/CSPSettingRequestCycleListenerTest.java
> +++
> b/wicket-core/src/test/java/org/apache/wicket/csp/CSPSettingRequestCycleListenerTest.java
> @@ -16,16 +16,16 @@
> */
> package org.apache.wicket.csp;
>
> -import static
> org.apache.wicket.csp.CSPSettingRequestCycleListener.CSPDirective.CHILD_SRC;
> -import static
> org.apache.wicket.csp.CSPSettingRequestCycleListener.CSPDirective.DEFAULT_SRC;
> -import static
> org.apache.wicket.csp.CSPSettingRequestCycleListener.CSPDirective.FRAME_SRC;
> -import static
> org.apache.wicket.csp.CSPSettingRequestCycleListener.CSPDirective.REPORT_URI;
> -import static
> org.apache.wicket.csp.CSPSettingRequestCycleListener.CSPDirective.SANDBOX;
> -import static
> org.apache.wicket.csp.CSPSettingRequestCycleListener.CSPDirectiveSandboxValue.ALLOW_FORMS;
> -import static
> org.apache.wicket.csp.CSPSettingRequestCycleListener.CSPDirectiveSandboxValue.EMPTY;
> -import static
> org.apache.wicket.csp.CSPSettingRequestCycleListener.CSPDirectiveSrcValue.NONE;
> -import static
> org.apache.wicket.csp.CSPSettingRequestCycleListener.CSPDirectiveSrcValue.SELF;
> -import static
> org.apache.wicket.csp.CSPSettingRequestCycleListener.CSPDirectiveSrcValue.WILDCARD;
> +import static org.apache.wicket.csp.CSPDirective.CHILD_SRC;
> +import static org.apache.wicket.csp.CSPDirective.DEFAULT_SRC;
> +import static org.apache.wicket.csp.CSPDirective.FRAME_SRC;
> +import static org.apache.wicket.csp.CSPDirective.REPORT_URI;
> +import static org.apache.wicket.csp.CSPDirective.SANDBOX;
> +import static org.apache.wicket.csp.CSPDirectiveSandboxValue.ALLOW_FORMS;
> +import static org.apache.wicket.csp.CSPDirectiveSandboxValue.EMPTY;
> +import static org.apache.wicket.csp.CSPDirectiveSrcValue.NONE;
> +import static org.apache.wicket.csp.CSPDirectiveSrcValue.SELF;
> +import static org.apache.wicket.csp.CSPDirectiveSrcValue.WILDCARD;
>
> import java.net.URI;
> import java.net.URISyntaxException;
> @@ -37,9 +37,6 @@ import java.util.Set;
> import java.util.stream.Collectors;
> import java.util.stream.Stream;
>
> -import org.apache.wicket.csp.CSPSettingRequestCycleListener.CSPDirective;
> -import
> org.apache.wicket.csp.CSPSettingRequestCycleListener.CSPDirectiveSandboxValue;
> -import
> org.apache.wicket.csp.CSPSettingRequestCycleListener.CSPDirectiveSrcValue;
> import org.apache.wicket.mock.MockHomePage;
> import org.apache.wicket.util.tester.WicketTester;
> import org.junit.jupiter.api.Assertions;
> diff --git
> a/wicket-core/src/test/java/org/apache/wicket/pageStore/CryptingPageStoreTest.java
> b/wicket-core/src/test/java/org/apache/wicket/pageStore/CryptingPageStoreTest.java
> index 755a000..48e3def 100644
> ---
> a/wicket-core/src/test/java/org/apache/wicket/pageStore/CryptingPageStoreTest.java
> +++
> b/wicket-core/src/test/java/org/apache/wicket/pageStore/CryptingPageStoreTest.java
> @@ -23,10 +23,10 @@ import java.io.StreamCorruptedException;
> import java.security.GeneralSecurityException;
>
> import org.apache.wicket.MockPage;
> -import org.apache.wicket.WicketRuntimeException;
> import org.apache.wicket.mock.MockPageContext;
> import org.apache.wicket.mock.MockPageStore;
> import org.apache.wicket.serialize.java.JavaSerializer;
> +import org.apache.wicket.util.tester.WicketTester;
> import org.junit.jupiter.api.Test;
>
> /**
> @@ -34,13 +34,13 @@ import org.junit.jupiter.api.Test;
> *
> * @author svenmeier
> */
> -public class CryptingPageStoreTest
> +public class CryptingPageStoreTest extends WicketTester
>
extends WicketTester ?!
maybe you meant WicketTestCase ?
> {
>
> @Test
> void test()
> {
> - CryptingPageStore store = new CryptingPageStore(new
> MockPageStore());
> + CryptingPageStore store = new CryptingPageStore(new
> MockPageStore(), getApplication());
> JavaSerializer serializer = new JavaSerializer("test");
>
> IPageContext context = new MockPageContext();
> @@ -60,7 +60,7 @@ public class CryptingPageStoreTest
> @Test
> void testFail()
> {
> - CryptingPageStore store = new CryptingPageStore(new
> MockPageStore());
> + CryptingPageStore store = new CryptingPageStore(new
> MockPageStore(), getApplication());
> JavaSerializer serializer = new JavaSerializer("test");
>
> MockPageContext context = new MockPageContext();
>
>
