diff --git a/tapestry-core/src/main/java/org/apache/tapestry5/SymbolConstants.java b/tapestry-core/src/main/java/org/apache/tapestry5/SymbolConstants.java index 9a5c845b0f..810f5b0180 100644 --- a/tapestry-core/src/main/java/org/apache/tapestry5/SymbolConstants.java +++ b/tapestry-core/src/main/java/org/apache/tapestry5/SymbolConstants.java @@ -397,9 +397,13 @@ public class SymbolConstants /** * A passphrase used as the basis of hash-based message authentication (HMAC) for any object stream data stored on - * the client. The default phrase is the empty string, which will result in a logged runtime error. - * You should configure this to a reasonable value (longer is better) and ensure that all servers in your cluster - * share the same value (configuring this in code, rather than the command line, is preferred). + * the client. The passphrase must be at least 20 characters long (aligned with the RFC 2104 SHA-1 digest length). + * You should configure this to a strong, random value and ensure that all servers in your cluster share the same + * value (configuring this in code, rather than on the command line, is preferred). + * + *

In production mode, a missing or too-short passphrase causes a startup exception. + * In development mode, a warning is logged and a random per-startup fallback key is used instead, + * meaning client data will not survive server restarts.

* * @see org.apache.tapestry5.services.ClientDataEncoder * @since 5.3.6 diff --git a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ClientDataEncoderImpl.java b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ClientDataEncoderImpl.java index ec6063acd4..e168e38d9e 100644 --- a/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ClientDataEncoderImpl.java +++ b/tapestry-core/src/main/java/org/apache/tapestry5/internal/services/ClientDataEncoderImpl.java @@ -1,4 +1,4 @@ -// Copyright 2009, 2012 The Apache Software Foundation +// Copyright 2009, 2012, 2026 The Apache Software Foundation // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -14,9 +14,9 @@ package org.apache.tapestry5.internal.services; +import org.apache.commons.lang3.StringUtils; import org.apache.tapestry5.SymbolConstants; import org.apache.tapestry5.alerts.AlertManager; -import org.apache.tapestry5.http.internal.TapestryHttpInternalConstants; import org.apache.tapestry5.internal.TapestryInternalUtils; import org.apache.tapestry5.internal.util.Base64InputStream; import org.apache.tapestry5.internal.util.MacOutputStream; @@ -30,43 +30,53 @@ import java.io.BufferedInputStream; import java.io.IOException; import java.io.ObjectInputStream; -import java.io.UnsupportedEncodingException; +import java.nio.charset.StandardCharsets; import java.security.Key; import java.security.MessageDigest; +import java.util.UUID; import java.util.zip.GZIPInputStream; public class ClientDataEncoderImpl implements ClientDataEncoder { + private static final int MIN_PASSPHRASE_LENGTH = 20; + private final URLEncoder urlEncoder; private final Key hmacKey; - public ClientDataEncoderImpl(URLEncoder urlEncoder, @Symbol(SymbolConstants.HMAC_PASSPHRASE) String passphrase, + public ClientDataEncoderImpl(URLEncoder urlEncoder, + @Symbol(SymbolConstants.HMAC_PASSPHRASE) String passphrase, Logger logger, - @Symbol(TapestryHttpInternalConstants.TAPESTRY_APP_PACKAGE_PARAM) - String applicationPackageName, AlertManager alertManager) throws UnsupportedEncodingException + AlertManager alertManager, + @Symbol(SymbolConstants.PRODUCTION_MODE) boolean productionMode) { this.urlEncoder = urlEncoder; - if (passphrase.equals("")) + // TAP5-2834: Also check for minimum length + if (StringUtils.isBlank(passphrase) || passphrase.length() < MIN_PASSPHRASE_LENGTH) { - String message = String.format("The symbol '%s' has not been configured. " + - "This is used to configure hash-based message authentication of Tapestry data stored in forms, or in the URL. " + - "You application is less secure, and more vulnerable to denial-of-service attacks, when this symbol is not configured.", - SymbolConstants.HMAC_PASSPHRASE); + String message = String.format( + "SymbolConstants.HMAC_PASSPHRASE '%s' has not been configured or is too short (minimum %d characters). " + + "This is used to configure hash-based message authentication of Tapestry data stored in forms or in the URL. " + + "Your application is less secure and more vulnerable to denial-of-service attacks when this symbol is not properly configured.", + SymbolConstants.HMAC_PASSPHRASE, MIN_PASSPHRASE_LENGTH); - // Now to really get the attention of the developer! + if (productionMode) + { + throw new RuntimeException(message); + } alertManager.error(message); - logger.error(message); - // Override the blank parameter to set a default value. Use the application package name, - // which is justly slightly more secure than having a fixed default. - passphrase = applicationPackageName; + // TAP5-2834: No longer fall back to application package, as it's easier to guess/deduct. + // As we disallow an empty passphrase in production, this should only become an issue + // if run in non-production in a clustered environment. and even then, the easy fix + // will be configuring a custom passphrase. + passphrase = UUID.randomUUID().toString(); } - hmacKey = new SecretKeySpec(passphrase.getBytes("UTF8"), "HmacSHA1"); + hmacKey = new SecretKeySpec(passphrase.getBytes(StandardCharsets.UTF_8), "HmacSHA1"); } public ClientDataSink createSink() @@ -127,7 +137,7 @@ private void validateHMAC(String storedHmacResult, Base64InputStream b64in) thro String actual = macOs.getResult(); - if (!MessageDigest.isEqual(storedHmacResult.getBytes(), actual.getBytes())) + if (!MessageDigest.isEqual(storedHmacResult.getBytes(StandardCharsets.UTF_8), actual.getBytes(StandardCharsets.UTF_8))) { throw new IOException("Client data associated with the current request appears to have been tampered with " + "(the HMAC signature does not match)."); diff --git a/tapestry-core/src/main/java/org/apache/tapestry5/internal/test/PageTesterModule.java b/tapestry-core/src/main/java/org/apache/tapestry5/internal/test/PageTesterModule.java index 0dab8edb40..3cfb4cb945 100644 --- a/tapestry-core/src/main/java/org/apache/tapestry5/internal/test/PageTesterModule.java +++ b/tapestry-core/src/main/java/org/apache/tapestry5/internal/test/PageTesterModule.java @@ -12,6 +12,7 @@ package org.apache.tapestry5.internal.test; +import org.apache.tapestry5.SymbolConstants; import org.apache.tapestry5.commons.MappedConfiguration; import org.apache.tapestry5.commons.ObjectLocator; import org.apache.tapestry5.commons.OrderedConfiguration; @@ -26,7 +27,10 @@ import org.apache.tapestry5.ioc.ServiceBinder; import org.apache.tapestry5.ioc.annotations.Contribute; import org.apache.tapestry5.ioc.annotations.Local; +import org.apache.tapestry5.ioc.services.ApplicationDefaults; +import org.apache.tapestry5.ioc.services.FactoryDefaults; import org.apache.tapestry5.ioc.services.ServiceOverride; +import org.apache.tapestry5.ioc.services.SymbolProvider; import org.apache.tapestry5.services.MarkupRendererFilter; import org.apache.tapestry5.test.PageTester; @@ -86,4 +90,11 @@ public static void contributeMarkupRenderer(OrderedConfiguration configuration) + { + configuration.add(SymbolConstants.HMAC_PASSPHRASE, "PageTester default passphrase for testing"); + } } diff --git a/tapestry-core/src/test/groovy/org/apache/tapestry5/internal/services/ClientDataEncoderImplTest.groovy b/tapestry-core/src/test/groovy/org/apache/tapestry5/internal/services/ClientDataEncoderImplTest.groovy deleted file mode 100644 index 71cc545876..0000000000 --- a/tapestry-core/src/test/groovy/org/apache/tapestry5/internal/services/ClientDataEncoderImplTest.groovy +++ /dev/null @@ -1,136 +0,0 @@ -package org.apache.tapestry5.internal.services - -import org.apache.tapestry5.alerts.AlertManager -import org.apache.tapestry5.services.ClientDataEncoder -import org.apache.tapestry5.test.ioc.TestBase -import org.easymock.EasyMock -import org.slf4j.Logger -import org.testng.annotations.Test - -class ClientDataEncoderImplTest extends TestBase { - - def tryEncodeAndDecode(ClientDataEncoder cde) { - def now = new Date() - def input = "The current time is $now" - - - String clientData = convertToClientData cde, input - - def stream = cde.decodeClientData clientData - - def output = stream.readObject() - - assert !input.is(output) - assert input == output - } - - def String convertToClientData(ClientDataEncoder cde, input) { - def sink = cde.createSink() - - sink.getObjectOutputStream().with { stream -> - stream << input - stream.close() - } - - sink.clientData - } - - def extractData(String encoded) { - def colonx = encoded.indexOf(':') - - encoded.substring(colonx + 1) - } - - @Test - void blank_passphrase_works_but_logs_error() { - Logger logger = newMock Logger - AlertManager alertManager = newMock AlertManager - - logger.error(EasyMock.isA(String)) - alertManager.error(EasyMock.isA(String)) - - replay() - - ClientDataEncoder cde = new ClientDataEncoderImpl(null, "", logger, "foo.bar", alertManager) - - tryEncodeAndDecode cde - - verify() - } - - @Test - void no_logged_error_with_non_blank_passphrase() { - ClientDataEncoder cde = new ClientDataEncoderImpl(null, "Testing, Testing, 1.., 2.., 3...", null, "foo.bar", null) - - tryEncodeAndDecode cde - } - - @Test - void passphrase_affects_encoded_output() { - ClientDataEncoder first = new ClientDataEncoderImpl(null, "first passphrase", null, "foo.bar", null) - ClientDataEncoder second = new ClientDataEncoderImpl(null, " different passphrase ", null, "foo.bar", null) - - def input = "current time millis is ${System.currentTimeMillis()} ms" - - def output1 = convertToClientData first, input - def output2 = convertToClientData second, input - - assert output1 != output2 - - assert extractData(output1) == extractData(output2) - } - - @Test(expectedExceptions = IllegalArgumentException) - void decode_with_missing_hmac_prefix_is_a_failure() { - ClientDataEncoder cde = new ClientDataEncoderImpl(null, "a passphrase", null, "foo.bar", null) - - cde.decodeClientData("so completely invalid") - } - - @Test - void incorrect_hmac_is_detected() { - - // Simulate tampering by encoding with one passphrase and attempting to decode with a different - // passphrase. - ClientDataEncoder first = new ClientDataEncoderImpl(null, "first passphrase", null, "foo.bar", null) - ClientDataEncoder second = new ClientDataEncoderImpl(null, " different passphrase ", null, "foo.bar", null) - - def input = "current time millis is ${System.currentTimeMillis()} ms" - - def output = convertToClientData first, input - - try { - second.decodeClientData(output) - unreachable() - } - catch (Exception e) { - assert e.message.contains("HMAC signature does not match") - } - } - - @Test(expectedExceptions = EOFException) - void check_for_eof() { - ClientDataEncoder cde = new ClientDataEncoderImpl(null, "hmac passphrase", null, "foo.bar", null) - - def sink = cde.createSink() - - def os = sink.objectOutputStream - - def names = ["fred", "barney", "wilma"] - - names.each { os.writeObject it } - - os.close() - - def ois = cde.decodeClientData(sink.clientData) - - names.each { assert (ois.readObject() == it )} - - // This should fail: - - ois.readObject() - - unreachable() - } - -} diff --git a/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/ClientDataEncoderImplTest.java b/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/ClientDataEncoderImplTest.java new file mode 100644 index 0000000000..91c4ed593a --- /dev/null +++ b/tapestry-core/src/test/java/org/apache/tapestry5/internal/services/ClientDataEncoderImplTest.java @@ -0,0 +1,201 @@ +// Copyright 2012, 2020, 2026 The Apache Software Foundation +// +// Licensed 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.tapestry5.internal.services; + +import org.apache.tapestry5.alerts.AlertManager; +import org.apache.tapestry5.services.ClientDataEncoder; +import org.apache.tapestry5.services.ClientDataSink; +import org.easymock.EasyMock; +import org.junit.jupiter.api.Test; +import org.slf4j.Logger; + +import java.io.EOFException; +import java.io.IOException; +import java.io.ObjectInputStream; +import java.io.ObjectOutputStream; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.*; + +class ClientDataEncoderImplTest +{ + private static final String VALID_PASSPHRASE = "a sufficiently long passphrase"; + + private String convertToClientData(ClientDataEncoder cde, Object input) throws IOException + { + ClientDataSink sink = cde.createSink(); + ObjectOutputStream stream = sink.getObjectOutputStream(); + stream.writeObject(input); + stream.close(); + return sink.getClientData(); + } + + private void tryEncodeAndDecode(ClientDataEncoder cde) throws Exception + { + String input = "The current time is " + new java.util.Date(); + String clientData = convertToClientData(cde, input); + ObjectInputStream stream = cde.decodeClientData(clientData); + Object output = stream.readObject(); + assertNotSame(input, output); + assertEquals(input, output); + } + + private String extractData(String encoded) + { + int colonx = encoded.indexOf(':'); + return encoded.substring(colonx + 1); + } + + @Test + void blank_passphrase_warns_in_development() throws Exception + { + Logger logger = EasyMock.mock(Logger.class); + AlertManager alertManager = EasyMock.mock(AlertManager.class); + + logger.error(EasyMock.isA(String.class)); + alertManager.error(EasyMock.isA(String.class)); + + EasyMock.replay(logger, alertManager); + + ClientDataEncoder cde = new ClientDataEncoderImpl(null, "", logger, alertManager, false); + tryEncodeAndDecode(cde); + + EasyMock.verify(logger, alertManager); + } + + @Test + void whitespace_passphrase_warns_in_development() throws Exception + { + Logger logger = EasyMock.mock(Logger.class); + AlertManager alertManager = EasyMock.mock(AlertManager.class); + + logger.error(EasyMock.isA(String.class)); + alertManager.error(EasyMock.isA(String.class)); + + EasyMock.replay(logger, alertManager); + + ClientDataEncoder cde = new ClientDataEncoderImpl(null, " ", logger, alertManager, false); + tryEncodeAndDecode(cde); + + EasyMock.verify(logger, alertManager); + } + + @Test + void short_passphrase_warns_in_development() throws Exception + { + Logger logger = EasyMock.mock(Logger.class); + AlertManager alertManager = EasyMock.mock(AlertManager.class); + + logger.error(EasyMock.isA(String.class)); + alertManager.error(EasyMock.isA(String.class)); + + EasyMock.replay(logger, alertManager); + + ClientDataEncoder cde = new ClientDataEncoderImpl(null, "short", logger, alertManager, false); + tryEncodeAndDecode(cde); + + EasyMock.verify(logger, alertManager); + } + + @Test + void blank_passphrase_fails_in_production() + { + assertThrows(RuntimeException.class, + () -> new ClientDataEncoderImpl(null, "", null, null, true)); + } + + @Test + void short_passphrase_fails_in_production() + { + assertThrows(RuntimeException.class, + () -> new ClientDataEncoderImpl(null, "too short", null, null, true)); + } + + @Test + void valid_passphrase_works_in_production() throws Exception + { + ClientDataEncoder cde = new ClientDataEncoderImpl(null, VALID_PASSPHRASE, null, null, true); + tryEncodeAndDecode(cde); + } + + @Test + void no_logged_error_with_sufficient_passphrase() throws Exception + { + ClientDataEncoder cde = new ClientDataEncoderImpl(null, "Testing, Testing, 1.., 2.., 3...", null, null, false); + tryEncodeAndDecode(cde); + } + + @Test + void passphrase_affects_encoded_output() throws Exception + { + ClientDataEncoder first = new ClientDataEncoderImpl(null, "first-passphrase-long-enough", null, null, false); + ClientDataEncoder second = new ClientDataEncoderImpl(null, "different-passphrase-long-enough", null, null, false); + + String input = "current time millis is " + System.currentTimeMillis() + " ms"; + + String output1 = convertToClientData(first, input); + String output2 = convertToClientData(second, input); + + assertNotEquals(output1, output2); + assertEquals(extractData(output1), extractData(output2)); + } + + @Test + void decode_with_missing_hmac_prefix_is_a_failure() throws Exception + { + ClientDataEncoder cde = new ClientDataEncoderImpl(null, VALID_PASSPHRASE, null, null, false); + + assertThrows(IllegalArgumentException.class, + () -> cde.decodeClientData("so completely invalid")); + } + + @Test + void incorrect_hmac_is_detected() throws Exception + { + ClientDataEncoder first = new ClientDataEncoderImpl(null, "first-passphrase-long-enough", null, null, false); + ClientDataEncoder second = new ClientDataEncoderImpl(null, "different-passphrase-long-enough", null, null, false); + + String input = "current time millis is " + System.currentTimeMillis() + " ms"; + String encoded = convertToClientData(first, input); + + RuntimeException ex = assertThrows(RuntimeException.class, + () -> second.decodeClientData(encoded)); + assertTrue(ex.getMessage().contains("HMAC signature does not match")); + } + + @Test + void check_for_eof() throws Exception + { + ClientDataEncoder cde = new ClientDataEncoderImpl(null, "hmac passphrase is valid here", null, null, false); + + ClientDataSink sink = cde.createSink(); + ObjectOutputStream os = sink.getObjectOutputStream(); + + List names = List.of("fred", "barney", "wilma"); + for (String name : names) + { + os.writeObject(name); + } + os.close(); + + ObjectInputStream ois = cde.decodeClientData(sink.getClientData()); + for (String name : names) + { + assertEquals(name, ois.readObject()); + } + + assertThrows(EOFException.class, ois::readObject); + } +}