HDDS-12649. Include name in length validation error

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);
   }
 
   /**

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<>();