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..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 @@ -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> EXCEPTION_LIST = ImmutableList.>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 8d0eae226c8a..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 @@ -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 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 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 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 validNames = new ArrayList<>();