This is an automated email from the ASF dual-hosted git repository.
adoroszlai pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/ozone.git
The following commit(s) were added to refs/heads/master by this push:
new ade69e3f82 HDDS-12649. Include name of volume or bucket in length
validation error (#8322)
ade69e3f82 is described below
commit ade69e3f821ea6c6a5ff199c8aac7987e44d48cf
Author: Roland Elek <[email protected]>
AuthorDate: Mon Apr 28 22:15:59 2025 +0200
HDDS-12649. Include name of volume or bucket in length validation error
(#8322)
---
.../hadoop/hdds/scm/client/HddsClientUtils.java | 35 +++++--
.../hdds/scm/client/TestHddsClientUtils.java | 101 ++++++++++++++++++++-
2 files changed, 125 insertions(+), 11 deletions(-)
diff --git
a/hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/client/HddsClientUtils.java
b/hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/client/HddsClientUtils.java
index a09ab64eae..ea5fc908a9 100644
---
a/hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/client/HddsClientUtils.java
+++
b/hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/client/HddsClientUtils.java
@@ -48,6 +48,14 @@
@InterfaceAudience.Public
@InterfaceStability.Unstable
public final class HddsClientUtils {
+ static final int MAX_BUCKET_NAME_LENGTH_IN_LOG =
+ 2 * OzoneConsts.OZONE_MAX_BUCKET_NAME_LENGTH;
+
+ private static final String VALID_LENGTH_MESSAGE = String.format(
+ "valid length is %d-%d characters",
+ OzoneConsts.OZONE_MIN_BUCKET_NAME_LENGTH,
+ OzoneConsts.OZONE_MAX_BUCKET_NAME_LENGTH);
+
private static final List<Class<? extends Exception>> EXCEPTION_LIST =
ImmutableList.<Class<? extends Exception>>builder()
.add(TimeoutException.class)
@@ -68,10 +76,26 @@ private static void doNameChecks(String resName, String
resType) {
throw new IllegalArgumentException(resType + " name is null");
}
- if (resName.length() < OzoneConsts.OZONE_MIN_BUCKET_NAME_LENGTH ||
- resName.length() > OzoneConsts.OZONE_MAX_BUCKET_NAME_LENGTH) {
+ if (resName.length() < OzoneConsts.OZONE_MIN_BUCKET_NAME_LENGTH) {
+ throw new IllegalArgumentException(resType +
+ " name '" + resName + "' is too short, " +
+ VALID_LENGTH_MESSAGE);
+ }
+
+ if (resName.length() > OzoneConsts.OZONE_MAX_BUCKET_NAME_LENGTH) {
+ String nameToReport;
+
+ if (resName.length() > MAX_BUCKET_NAME_LENGTH_IN_LOG) {
+ nameToReport = String.format(
+ "%s...",
+ resName.substring(0, MAX_BUCKET_NAME_LENGTH_IN_LOG));
+ } else {
+ nameToReport = resName;
+ }
+
throw new IllegalArgumentException(resType +
- " length is illegal, " + "valid length is 3-63 characters");
+ " name '" + nameToReport + "' is too long, " +
+ VALID_LENGTH_MESSAGE);
}
if (resName.charAt(0) == '.' || resName.charAt(0) == '-') {
@@ -150,9 +174,6 @@ public static void verifyResourceName(String resName,
String resType) {
* @throws IllegalArgumentException
*/
public static void verifyResourceName(String resName, String resType,
boolean isStrictS3) {
-
- doNameChecks(resName, resType);
-
boolean isIPv4 = true;
char prev = (char) 0;
@@ -169,6 +190,8 @@ public static void verifyResourceName(String resName,
String resType, boolean is
throw new IllegalArgumentException(resType +
" name cannot be an IPv4 address or all numeric");
}
+
+ doNameChecks(resName, resType);
}
/**
diff --git
a/hadoop-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/client/TestHddsClientUtils.java
b/hadoop-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/client/TestHddsClientUtils.java
index 8d0eae226c..281cc12fa3 100644
---
a/hadoop-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/client/TestHddsClientUtils.java
+++
b/hadoop-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/client/TestHddsClientUtils.java
@@ -23,12 +23,15 @@
import static
org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_CLIENT_PORT_DEFAULT;
import static
org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_CLIENT_PORT_KEY;
import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_NAMES;
+import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
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 com.google.common.collect.ImmutableList;
import java.io.IOException;
import java.net.ConnectException;
import java.net.InetSocketAddress;
@@ -191,6 +194,10 @@ public void testBlockClientFallbackToClientWithPort() {
assertEquals(OZONE_SCM_BLOCK_CLIENT_PORT_DEFAULT, address.getPort());
}
+ private String getInvalidNameMessage(String invalidString) {
+ return "Did not reject invalid string [" + invalidString + "] as a name";
+ }
+
@Test
public void testVerifyResourceName() {
final String validName = "my-bucket.01";
@@ -209,8 +216,7 @@ public void testVerifyResourceName() {
final String endDot = "notaname.";
final String startDot = ".notaname";
final String unicodeCharacters = "ｚｚｚ";
- final String tooShort = StringUtils.repeat("a",
- OzoneConsts.OZONE_MIN_BUCKET_NAME_LENGTH - 1);
+ // Other tests cover the "too short" case.
List<String> invalidNames = new ArrayList<>();
invalidNames.add(ipaddr);
@@ -221,14 +227,99 @@ public void testVerifyResourceName() {
invalidNames.add(endDot);
invalidNames.add(startDot);
invalidNames.add(unicodeCharacters);
- invalidNames.add(tooShort);
for (String name : invalidNames) {
assertThrows(IllegalArgumentException.class, () ->
HddsClientUtils.verifyResourceName(name),
- "Did not reject invalid string [" + name + "] as a name");
+ getInvalidNameMessage(name));
+ }
+ }
+
+ private void doTestBadResourceNameLengthReported(String name, String reason)
{
+ // The message should include the name, resource type, range for acceptable
+ // length, and expected reason for rejecting the name.
+ List<String> resourceTypes = ImmutableList.of("bucket", "volume");
+ for (int i = 0; i < 2; i++) {
+ String resType = resourceTypes.get(i);
+ String otherResType = resourceTypes.get(1 - i);
+ Throwable thrown = assertThrows(
+ IllegalArgumentException.class,
+ () -> HddsClientUtils.verifyResourceName(name, resType, true),
+ getInvalidNameMessage(name));
+
+ String message = thrown.getMessage();
+ assertNotNull(message);
+ assertThat(message).contains(resType);
+ assertThat(message).doesNotContain(otherResType);
+ assertThat(message).contains(name);
+ assertThat(message).contains(reason);
+ assertThat(message).contains(
+ Integer.toString(OzoneConsts.OZONE_MIN_BUCKET_NAME_LENGTH));
+ assertThat(message).contains(
+ Integer.toString(OzoneConsts.OZONE_MAX_BUCKET_NAME_LENGTH));
}
}
+ @Test
+ public void testNameTooShort() {
+ final String tooShort = StringUtils.repeat("a",
+ OzoneConsts.OZONE_MIN_BUCKET_NAME_LENGTH - 1);
+
+ doTestBadResourceNameLengthReported(tooShort, "too short");
+ }
+
+ @Test
+ public void testNameTooLong() {
+ // Too long, but within the limit for logging (no truncation).
+ final String tooLong = StringUtils.repeat("a",
+ OzoneConsts.OZONE_MAX_BUCKET_NAME_LENGTH + 1);
+
+ doTestBadResourceNameLengthReported(tooLong, "too long");
+ }
+
+ @Test
+ public void testNameTooLongCapped() {
+ // Logging arbitrarily long names is a readability concern and possibly a
+ // vulnerability. Report a prefix with a truncation marker instead if the
+ // maximum length is exceeded by a large margin.
+
+ final String exceedsLogLimit = "x" + StringUtils.repeat(
+ "a", HddsClientUtils.MAX_BUCKET_NAME_LENGTH_IN_LOG);
+ final String truncationMarker = "...";
+
+ Throwable thrown = assertThrows(
+ IllegalArgumentException.class,
+ () -> HddsClientUtils.verifyResourceName(exceedsLogLimit),
+ getInvalidNameMessage(exceedsLogLimit));
+
+ String message = thrown.getMessage();
+ assertNotNull(message);
+
+ String truncatedName = exceedsLogLimit.substring(
+ 0,
+ HddsClientUtils.MAX_BUCKET_NAME_LENGTH_IN_LOG);
+ String expectedInMessage = truncatedName + truncationMarker;
+
+ assertThat(message).contains(expectedInMessage);
+ assertThat(message).doesNotContain(exceedsLogLimit);
+ }
+
+ @Test
+ public void testInvalidCharactersNotReported() {
+ // Names of illegal length may appear in logs through the exception
+ // message. If they also contain invalid characters, they should not be
+ // included verbatim. This is to avoid vulnerabilities like Log4Shell.
+
+ final String tooShortInvalidChar = "$a";
+ Throwable thrown = assertThrows(
+ IllegalArgumentException.class,
+ () -> HddsClientUtils.verifyResourceName(tooShortInvalidChar),
+ getInvalidNameMessage(tooShortInvalidChar));
+
+ String message = thrown.getMessage();
+ assertNotNull(message);
+ assertThat(message).doesNotContain(tooShortInvalidChar);
+ }
+
@Test
void testVerifyKeyName() throws IllegalArgumentException {
List<String> invalidNames = new ArrayList<>();
@@ -250,7 +341,7 @@ void testVerifyKeyName() throws IllegalArgumentException {
for (String name : invalidNames) {
assertThrows(IllegalArgumentException.class, () ->
HddsClientUtils.verifyKeyName(name),
- "Did not reject invalid string [" + name + "] as a name");
+ getInvalidNameMessage(name));
}
List<String> validNames = new ArrayList<>();
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
