From 9f39dc995989e36d86e74fa1b1f0258355ea8ea2 Mon Sep 17 00:00:00 2001 From: Roland Elek Date: Tue, 22 Apr 2025 10:55:53 +0200 Subject: [PATCH 1/4] HDDS-12649. Include name in length validation error --- .../hdds/scm/client/HddsClientUtils.java | 7 ++-- .../hdds/scm/client/TestHddsClientUtils.java | 41 ++++++++++++++++++- 2 files changed, 42 insertions(+), 6 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 ac2503339794..889adc900730 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 @@ -71,7 +71,7 @@ private static void doNameChecks(String resName, String resType) { if (resName.length() < OzoneConsts.OZONE_MIN_BUCKET_NAME_LENGTH || resName.length() > OzoneConsts.OZONE_MAX_BUCKET_NAME_LENGTH) { throw new IllegalArgumentException(resType + - " length is illegal, " + "valid length is 3-63 characters"); + " name '" + resName + "' is of illegal length, " + "valid length is 3-63 characters"); } if (resName.charAt(0) == '.' || resName.charAt(0) == '-') { @@ -150,9 +150,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 +166,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 8d0eae226c8a..d943be1b1ba6 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 @@ -24,11 +24,14 @@ 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.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +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"; @@ -225,8 +232,38 @@ public void testVerifyResourceName() { for (String name : invalidNames) { assertThrows(IllegalArgumentException.class, () -> HddsClientUtils.verifyResourceName(name), - "Did not reject invalid string [" + name + "] as a name"); + getInvalidNameMessage(name)); } + + // Message should include the name and resource type, if the name is of + // illegal length. + List 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(tooShort, resType, true), + getInvalidNameMessage(tooShort)); + + String message = thrown.getMessage(); + assertNotNull(message); + assertTrue(message.contains(resType)); + assertFalse(message.contains(otherResType)); + assertTrue(thrown.getMessage().contains(tooShort)); + } + + // However, such names also containing unsupported characters should not be + // logged 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); + assertFalse(message.contains(tooShortInvalidChar)); } @Test @@ -250,7 +287,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 validNames = new ArrayList<>(); From b0239d684f41fd545f78e2b16133541599765f93 Mon Sep 17 00:00:00 2001 From: Roland Elek Date: Mon, 28 Apr 2025 17:26:32 +0200 Subject: [PATCH 2/4] Address review concerns - Check "too short" and "too long" separately, add tests for this. - Truncate names that are too long to twice the allowed length. - Use assertThat() instead of assertTrue() and assertFalse(). - Split the new cases out from testVerifyResourceName(). --- .../hdds/scm/client/HddsClientUtils.java | 27 ++++++- .../hdds/scm/client/TestHddsClientUtils.java | 78 +++++++++++++++---- 2 files changed, 88 insertions(+), 17 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 889adc900730..19bd02fa967a 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 @@ -17,6 +17,7 @@ package org.apache.hadoop.hdds.scm.client; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import java.util.HashMap; @@ -48,6 +49,10 @@ @InterfaceAudience.Public @InterfaceStability.Unstable public final class HddsClientUtils { + @VisibleForTesting + static final int MAX_BUCKET_NAME_LENGTH_IN_LOG = + 2 * OzoneConsts.OZONE_MAX_BUCKET_NAME_LENGTH; + private static final List> EXCEPTION_LIST = ImmutableList.>builder() .add(TimeoutException.class) @@ -68,10 +73,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 is 3-63 characters"); + } + + 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 + - " name '" + resName + "' is of illegal length, " + "valid length is 3-63 characters"); + " name '" + nameToReport + "' is too long, " + + "valid length is 3-63 characters"); } if (resName.charAt(0) == '.' || resName.charAt(0) == '-') { 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 d943be1b1ba6..e7c3a8856cc0 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,8 +23,8 @@ 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.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertSame; @@ -216,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 invalidNames = new ArrayList<>(); invalidNames.add(ipaddr); @@ -228,33 +227,84 @@ 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), getInvalidNameMessage(name)); } + } - // Message should include the name and resource type, if the name is of - // illegal length. + private void doTestBadResourceNameReported(String name, String reason) { + // The message should include the name, resource type, and expected reason + // for rejecting the name. List 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(tooShort, resType, true), - getInvalidNameMessage(tooShort)); + () -> HddsClientUtils.verifyResourceName(name, resType, true), + getInvalidNameMessage(name)); String message = thrown.getMessage(); assertNotNull(message); - assertTrue(message.contains(resType)); - assertFalse(message.contains(otherResType)); - assertTrue(thrown.getMessage().contains(tooShort)); + assertThat(message).contains(resType); + assertThat(message).doesNotContain(otherResType); + assertThat(message).contains(name); + assertThat(message).contains(reason); } + } + + @Test + public void testNameTooShort() { + final String tooShort = StringUtils.repeat("a", + OzoneConsts.OZONE_MIN_BUCKET_NAME_LENGTH - 1); + + doTestBadResourceNameReported(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); + + doTestBadResourceNameReported(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. - // However, such names also containing unsupported characters should not be - // logged verbatim. This is to avoid vulnerabilities like Log4Shell. final String tooShortInvalidChar = "$a"; Throwable thrown = assertThrows( IllegalArgumentException.class, @@ -263,7 +313,7 @@ public void testVerifyResourceName() { String message = thrown.getMessage(); assertNotNull(message); - assertFalse(message.contains(tooShortInvalidChar)); + assertThat(message).doesNotContain(tooShortInvalidChar); } @Test From f3c961fe12bb10034e47bef1158816032f7313b9 Mon Sep 17 00:00:00 2001 From: Roland Elek Date: Mon, 28 Apr 2025 19:42:19 +0200 Subject: [PATCH 3/4] Apply suggestions from code review Co-authored-by: Doroszlai, Attila <6454655+adoroszlai@users.noreply.github.com> --- .../java/org/apache/hadoop/hdds/scm/client/HddsClientUtils.java | 2 -- 1 file changed, 2 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 19bd02fa967a..d3614df592f7 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 @@ -17,7 +17,6 @@ package org.apache.hadoop.hdds.scm.client; -import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import java.util.HashMap; @@ -49,7 +48,6 @@ @InterfaceAudience.Public @InterfaceStability.Unstable public final class HddsClientUtils { - @VisibleForTesting static final int MAX_BUCKET_NAME_LENGTH_IN_LOG = 2 * OzoneConsts.OZONE_MAX_BUCKET_NAME_LENGTH; From 5463473c2a7335aa39051db64564ca3be0391c03 Mon Sep 17 00:00:00 2001 From: Roland Elek Date: Mon, 28 Apr 2025 20:43:07 +0200 Subject: [PATCH 4/4] Address second round of review concerns --- .../hadoop/hdds/scm/client/HddsClientUtils.java | 9 +++++++-- .../hdds/scm/client/TestHddsClientUtils.java | 14 +++++++++----- 2 files changed, 16 insertions(+), 7 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 d3614df592f7..3997b3182118 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 @@ -51,6 +51,11 @@ 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> EXCEPTION_LIST = ImmutableList.>builder() .add(TimeoutException.class) @@ -74,7 +79,7 @@ private static void doNameChecks(String resName, String resType) { if (resName.length() < OzoneConsts.OZONE_MIN_BUCKET_NAME_LENGTH) { throw new IllegalArgumentException(resType + " name '" + resName + "' is too short, " + - "valid length is 3-63 characters"); + VALID_LENGTH_MESSAGE); } if (resName.length() > OzoneConsts.OZONE_MAX_BUCKET_NAME_LENGTH) { @@ -90,7 +95,7 @@ private static void doNameChecks(String resName, String resType) { throw new IllegalArgumentException(resType + " name '" + nameToReport + "' is too long, " + - "valid length is 3-63 characters"); + VALID_LENGTH_MESSAGE); } if (resName.charAt(0) == '.' || resName.charAt(0) == '-') { 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 e7c3a8856cc0..281cc12fa35c 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 @@ -234,9 +234,9 @@ public void testVerifyResourceName() { } } - private void doTestBadResourceNameReported(String name, String reason) { - // The message should include the name, resource type, and expected reason - // for rejecting the 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 resourceTypes = ImmutableList.of("bucket", "volume"); for (int i = 0; i < 2; i++) { String resType = resourceTypes.get(i); @@ -252,6 +252,10 @@ private void doTestBadResourceNameReported(String name, String reason) { 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)); } } @@ -260,7 +264,7 @@ public void testNameTooShort() { final String tooShort = StringUtils.repeat("a", OzoneConsts.OZONE_MIN_BUCKET_NAME_LENGTH - 1); - doTestBadResourceNameReported(tooShort, "too short"); + doTestBadResourceNameLengthReported(tooShort, "too short"); } @Test @@ -269,7 +273,7 @@ public void testNameTooLong() { final String tooLong = StringUtils.repeat("a", OzoneConsts.OZONE_MAX_BUCKET_NAME_LENGTH + 1); - doTestBadResourceNameReported(tooLong, "too long"); + doTestBadResourceNameLengthReported(tooLong, "too long"); } @Test