HDDS-17476. Return correct S3 errors for malformed aws-chunked bodies

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/AwsChunkedInputStream.java

@@ -189,9 +189,15 @@ private void endChunkIfComplete() throws IOException {
    * The message is specific; the S3 error code is the generic InvalidRequest (HTTP 400).
    */
   OS3Exception invalidBody(String message) {
-    OS3Exception ex = S3ErrorTable.newError(S3ErrorTable.INVALID_REQUEST, keyPath);
-    ex.setErrorMessage(message);
-    return ex;
+    return bodyError(S3ErrorTable.INVALID_REQUEST, message);
+  }
+
+  OS3Exception malformedTrailer(String message) {
+    return bodyError(S3ErrorTable.MALFORMED_TRAILER_ERROR, message);
+  }
+
+  private OS3Exception bodyError(S3ErrorTable error, String message) {
+    return S3ErrorTable.newError(error, keyPath).withMessage(message);
   }
 
   private void stopAtUnexpectedEof() throws IOException {
@@ -219,53 +225,62 @@ public byte[] getTrailerChecksum() {
   }
 
   /**
-   * A verified stream must end with the terminating 0-byte chunk. Reaching EOF before it means the
-   * body was truncated, so the payload could not be fully verified. Unverified streams without a
+   * A verified stream must end with the terminating 0-byte chunk and, when declared, its complete trailer.
+   * Reaching EOF before completion means the payload could not be fully verified. Unverified streams without a
    * declared trailer keep their lenient behavior.
    */
   private void checkNotTruncated() throws IOException {
     if (requiresCompletion()) {
-      throw invalidBody("Chunked stream ended before the terminating 0-byte chunk");
+      throw bodyError(S3ErrorTable.INCOMPLETE_BODY, "Chunked stream ended before completion");
     }
   }
 
   private void readChunkTerminator() throws IOException {
     int carriageReturn = originalStream.read();
+    if (requiresCompletion() && carriageReturn != '\r') {
+      if (carriageReturn == -1) {
+        checkNotTruncated();
+      }
+      throw invalidBody("Invalid chunk data terminator");
+    }
     int lineFeed = originalStream.read();
-    if (requiresCompletion() && (carriageReturn != '\r' || lineFeed != '\n')) {
+    if (requiresCompletion() && lineFeed != '\n') {
+      if (lineFeed == -1) {
+        checkNotTruncated();
+      }
       throw invalidBody("Invalid chunk data terminator");
     }
   }
 
   /** Check framing, authenticate the trailer if possible, then compare its checksum after the final terminator. */
   private void readTrailer() throws IOException {
     String trailerLine = readTrailerLine();
-    if (trailerLine == null || trailerLine.isEmpty()) {
-      throw invalidBody("Missing trailing checksum header");
+    if (trailerLine.isEmpty()) {
+      throw malformedTrailer("Missing trailing checksum header");
     }
     int separator = trailerLine.indexOf(':');
     if (separator <= 0) {
-      throw invalidBody("Invalid trailing header: " + trailerLine);
+      throw malformedTrailer("Invalid trailing header: " + trailerLine);
     }
     String name = trailerLine.substring(0, separator).trim().toLowerCase(Locale.ROOT);
     String value = trailerLine.substring(separator + 1).trim();
     if (!trailerHeader.getHeaderName().equals(name)) {
-      throw invalidBody("Unexpected trailing header: " + name);
+      throw malformedTrailer("Unexpected trailing header: " + name);
     }
     readTrailerSignature(name, value);
     byte[] expected = trailerHeader.decodeValue(value, keyPath);
     // AWS SDKs terminate the trailer section with a blank line.
     if (!"".equals(readTrailerLine())) {
-      throw invalidBody("Invalid trailer terminator");
+      throw malformedTrailer("Invalid trailer terminator");
     }
     if (originalStream.read() != -1) {
-      throw invalidBody("Unexpected data after trailer terminator");
+      throw malformedTrailer("Unexpected data after trailer terminator");
     }
     payloadChecksum.verify(expected, keyPath);
     trailerChecksum = expected;
   }
 
-  /** Read a line of the trailer section, or null at the end of the stream. */
+  /** Read a line of the trailer section. A declared trailer must be complete, so EOF fails as IncompleteBody. */
   String readTrailerLine() throws IOException {
     return readLine(true);
   }
@@ -276,17 +291,13 @@ private String readLine(boolean trailingHeader) throws IOException {
     while (true) {
       int current = originalStream.read();
       if (current == -1) {
-        if (trailingHeader && requiresCompletion() && line.length() > 0) {
-          throw invalidBody("Truncated trailing header");
-        }
-        if (!trailingHeader) {
-          checkNotTruncated();
-        }
+        checkNotTruncated();
         return line.length() == 0 ? null : line.toString();
       }
       line.append((char) current);
       if (line.length() > MAX_LINE_LENGTH) {
-        throw invalidBody("Chunk or trailing header line is too long");
+        String message = "Chunk or trailing header line is too long";
+        throw trailingHeader ? malformedTrailer(message) : invalidBody(message);
       }
       if (eol(previous, current)) {
         line.setLength(line.length() - 2);

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/SignedChunksInputStream.java

@@ -150,7 +150,7 @@ long parseChunkSize(String signatureLine) {
     // 10000 will be read and decoded from base-16 representation to 65536, which is the size of
     // the subsequent chunk payload.
     signatureLine = signatureLine.trim();
-    if (signatureLine.isEmpty()) {
+    if (signatureLine.isEmpty() && !requiresCompletion()) {
       return -1;
     }
 
@@ -160,18 +160,19 @@ long parseChunkSize(String signatureLine) {
       throw invalidBody("Invalid signature line: " + signatureLine);
     }
     chunkSignature = matcher.group(2);
-    return Integer.parseInt(matcher.group(1), 16);
+    try {
+      return Long.parseLong(matcher.group(1), 16);
+    } catch (NumberFormatException ex) {
+      throw invalidBody("Invalid signed chunk size");
+    }
   }
 
   @Override
   void readTrailerSignature(String name, String value) throws IOException {
     String signatureLine = readTrailerLine();
-    if (signatureLine == null) {
-      throw invalidBody("Missing trailing signature");
-    }
     Matcher matcher = TRAILER_SIGNATURE_PATTERN.matcher(signatureLine.trim());
     if (!matcher.matches()) {
-      throw invalidBody("Invalid trailing signature");
+      throw malformedTrailer("Invalid trailing signature");
     }
     if (validator != null) {
       validator.validateTrailer(matcher.group(1), DigestUtils.sha256Hex(name + ":" + value + "\n"));

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/TrailerHeader.java

@@ -77,7 +77,7 @@ byte[] decodeValue(String value, String resource) {
     } catch (IllegalArgumentException ignored) {
       // Invalid Base64 and incorrect decoded length are both malformed checksum values.
     }
-    OS3Exception ex = S3ErrorTable.newError(S3ErrorTable.INVALID_REQUEST, resource);
+    OS3Exception ex = S3ErrorTable.newError(S3ErrorTable.MALFORMED_TRAILER_ERROR, resource);
     ex.setErrorMessage("Value for " + headerName + " header is invalid");
     throw ex;
   }

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/exception/S3ErrorTable.java

@@ -199,6 +199,13 @@ public enum S3ErrorTable {
   INVALID_DIGEST(
       "InvalidDigest", "The Content-MD5 you specified is not valid.", HTTP_BAD_REQUEST),
 
+  INCOMPLETE_BODY(
+      "IncompleteBody", "You did not provide the number of bytes specified by the Content-Length HTTP header.",
+      HTTP_BAD_REQUEST),
+
+  MALFORMED_TRAILER_ERROR(
+      "MalformedTrailerError", "The request contained malformed trailing data.", HTTP_BAD_REQUEST),
+
   /** STS: Code {@code ValidationError}; message is usually overridden via {@link OS3Exception#withMessage}. */
   STS_VALIDATION_ERROR(
       "ValidationError", "A validation error occurred.", HTTP_BAD_REQUEST),

hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/TestSignedChunksInputStream.java

@@ -113,12 +113,86 @@ void consumesFramingWithoutValidator(boolean buffered, boolean empty, boolean tr
   }
 
   @ParameterizedTest
-  @EmptySource
+  @ValueSource(booleans = {false, true})
+  void chunkLengthSupportsLongRange(boolean buffered) throws IOException {
+    String body = "7fffffffffffffff;chunk-signature=" + FAKE_SIGNATURE + "\r\na";
+    for (boolean verify : new boolean[]{false, true}) {
+      try (SignedChunksInputStream stream = new SignedChunksInputStream(
+          new ByteArrayInputStream(body.getBytes(UTF_8)), KEY_PATH)) {
+        if (verify) {
+          stream.attachValidator(newValidator());
+        }
+        if (buffered) {
+          byte[] buffer = new byte[4];
+          assertThat(stream.read(buffer, 2, 1)).isEqualTo(1);
+          assertThat(buffer[2]).isEqualTo((byte) 'a');
+        } else {
+          assertThat(stream.read()).isEqualTo('a');
+        }
+        if (verify) {
+          OS3Exception error = assertThrows(OS3Exception.class, stream::read);
+          assertThat(error.getCode()).isEqualTo(S3ErrorTable.INCOMPLETE_BODY.getCode());
+        } else {
+          assertThat(stream.read()).isEqualTo(-1);
+        }
+      }
+    }
+  }
+
+  @Test
+  void rejectsOutOfRangeChunkSize() {
+    InputStream stream = wrapContent("8000000000000000;chunk-signature=" + FAKE_SIGNATURE + "\r\n");
+    assertBodyError(stream, S3ErrorTable.INVALID_REQUEST, "Invalid signed chunk size");
+  }
+
+  @Test
+  void rejectsBlankChunkHeader() {
+    assertBodyError(verifiedStream("\r\n"), S3ErrorTable.INVALID_REQUEST, "Invalid signature line");
+  }
+
+  @Test
+  void rejectsWrongChunkTerminatorBeforeEof() {
+    InputStream stream = verifiedStream("0;chunk-signature=" + FAKE_SIGNATURE + "\r\nX");
+    assertBodyError(stream, S3ErrorTable.INVALID_REQUEST, "Invalid chunk data terminator");
+  }
+
+  /** A body without a trailer only has to be complete when it is verified. */
+  @ParameterizedTest
+  @CsvSource({"true,false,false", "true,false,true", "true,true,false", "true,true,true",
+      "false,true,false", "false,true,true"})
+  void rejectEveryTruncatedBody(boolean trailer, boolean verify, boolean singleByte) throws IOException {
+    String body = trailer
+        ? SignatureTestUtils.signedChunkedBodyWithTrailer(new String[]{"1234", "56", "789"}, "x-amz-checksum-crc32c",
+            "4waSgw==")
+        : SignatureTestUtils.signedChunkedBody("123456789");
+    for (int length = 0; length < body.length(); length++) {
+      try (SignedChunksInputStream stream = new SignedChunksInputStream(
+          new ByteArrayInputStream(body.substring(0, length).getBytes(UTF_8)), KEY_PATH,
+          trailer ? TrailerHeader.CRC32C : TrailerHeader.NONE)) {
+        if (verify) {
+          stream.attachValidator(new ChunksValidator(SignatureTestUtils.signingKey(),
+              SignatureTestUtils.signatureInfo().getDateTime(), SignatureTestUtils.signatureInfo().getCredentialScope(),
+              SignatureTestUtils.signatureInfo().getSignature(), KEY_PATH));
+        }
+        OS3Exception error = assertThrows(OS3Exception.class, () -> {
+          if (singleByte) {
+            int value;
+            do {
+              value = stream.read();
+            } while (value != -1);
+          } else {
+            IOUtils.toByteArray(stream);
+          }
+        }, "Accepted truncated body at length " + length);
+        assertThat(error.getCode()).isEqualTo(S3ErrorTable.INCOMPLETE_BODY.getCode());
+        assertThat(error.getHttpCode()).isEqualTo(400);
+      }
+    }
+  }
+
+  @ParameterizedTest
   @ValueSource(strings = {
-      "x-amz-checksum-crc32c:sOO8/Q==",
-      "x-amz-checksum-crc32c:sOO8/Q==\r\n",
-      "x-amz-checksum-crc32c:sOO8/Q==\r\nx-amz-trailer-signature:" + TRAILER_SIGNATURE,
-      "x-amz-checksum-crc32c:sOO8/Q==\r\nx-amz-trailer-signature:" + TRAILER_SIGNATURE + "\r\n",
+      "\r\n",
       "x-amz-checksum-crc32c:sOO8/Q==\r\nx-amz-trailer-signature:not-hex\r\n\r\n",
       "malformed-header\r\nx-amz-trailer-signature:" + TRAILER_SIGNATURE + "\r\n\r\n",
       "x-amz-checksum-sha256:checksum\r\nx-amz-trailer-signature:" + TRAILER_SIGNATURE + "\r\n\r\n"
@@ -129,7 +203,7 @@ void rejectsMalformedTrailersWithoutValidator(String trailers) throws IOExceptio
     ByteArrayInputStream original = new ByteArrayInputStream(body.getBytes(UTF_8));
     try (SignedChunksInputStream stream = new SignedChunksInputStream(original, KEY_PATH, TrailerHeader.CRC32C)) {
       OS3Exception error = assertThrows(OS3Exception.class, () -> IOUtils.toByteArray(stream));
-      assertThat(error.getCode()).isEqualTo(S3ErrorTable.INVALID_REQUEST.getCode());
+      assertThat(error.getCode()).isEqualTo(S3ErrorTable.MALFORMED_TRAILER_ERROR.getCode());
     }
   }
 
@@ -142,7 +216,7 @@ void rejectsOversizedTrailerLine(boolean verify) throws IOException {
       if (verify) {
         stream.attachValidator(newTrailerValidator());
       }
-      assertInvalidBody(stream, "Chunk or trailing header line is too long");
+      assertBodyError(stream, S3ErrorTable.MALFORMED_TRAILER_ERROR, "Chunk or trailing header line is too long");
     }
   }
 
@@ -400,7 +474,7 @@ void rejectsMalformedChecksumWithValidSignature(String value) throws IOException
           SignatureTestUtils.signatureInfo().getDateTime(), SignatureTestUtils.signatureInfo().getCredentialScope(),
           SignatureTestUtils.signatureInfo().getSignature(), KEY_PATH));
       OS3Exception error = assertThrows(OS3Exception.class, stream::verifyComplete);
-      assertThat(error.getCode()).isEqualTo(S3ErrorTable.INVALID_REQUEST.getCode());
+      assertThat(error.getCode()).isEqualTo(S3ErrorTable.MALFORMED_TRAILER_ERROR.getCode());
     }
   }
 
@@ -424,42 +498,13 @@ void rejectsTamperedTrailerSignature() {
     assertSignatureMismatch(is);
   }
 
-  @Test
-  void rejectsMissingTrailerSignature() {
-    String body = trailerBody().replace("x-amz-trailer-signature:" + TRAILER_SIGNATURE + "\r\n\r\n", "");
-    SignedChunksInputStream is = new SignedChunksInputStream(
-        new ByteArrayInputStream(body.getBytes(UTF_8)), KEY_PATH, TrailerHeader.CRC32C);
-    is.attachValidator(newTrailerValidator());
-    assertInvalidBody(is, "Missing trailing signature");
-  }
-
-  @Test
-  void rejectsUnterminatedTrailerSignature() {
-    String body = trailerBody();
-    body = body.substring(0, body.length() - 4);
-    SignedChunksInputStream is = new SignedChunksInputStream(
-        new ByteArrayInputStream(body.getBytes(UTF_8)), KEY_PATH, TrailerHeader.CRC32C);
-    is.attachValidator(newTrailerValidator());
-    assertInvalidBody(is, "Truncated trailing header");
-  }
-
   @Test
   void rejectsDataAfterTrailerSignature() {
     SignedChunksInputStream is = new SignedChunksInputStream(
         new ByteArrayInputStream((trailerBody() + "extra").getBytes(UTF_8)), KEY_PATH,
         TrailerHeader.CRC32C);
     is.attachValidator(newTrailerValidator());
-    assertInvalidBody(is, "Unexpected data after trailer terminator");
-  }
-
-  @Test
-  void rejectsMissingTrailerTerminator() {
-    String body = trailerBody();
-    SignedChunksInputStream is = new SignedChunksInputStream(
-        new ByteArrayInputStream(body.substring(0, body.length() - 2).getBytes(UTF_8)), KEY_PATH,
-        TrailerHeader.CRC32C);
-    is.attachValidator(newTrailerValidator());
-    assertInvalidBody(is, "Invalid trailer terminator");
+    assertBodyError(is, S3ErrorTable.MALFORMED_TRAILER_ERROR, "Unexpected data after trailer terminator");
   }
 
   @ParameterizedTest
@@ -508,31 +553,6 @@ void attachedValidatorRejectsTamperedChunkPayload() {
     assertSignatureMismatch(is);
   }
 
-  @Test
-  void rejectsBodyMissingFinalZeroChunk() throws Exception {
-    String body = "10000;chunk-signature=" + CHUNK1_SIGNATURE + "\r\n"
-        + repeat('a', 65536) + "\r\n"
-        + "400;chunk-signature=" + CHUNK2_SIGNATURE + "\r\n"
-        + repeat('a', 1024) + "\r\n";
-    InputStream is = verifiedStream(body);
-    assertInvalidBody(is, "terminating 0-byte chunk");
-  }
-
-  @Test
-  void rejectsFinalChunkMissingDataTerminator() {
-    String body = signedChunkedBody('a');
-    body = body.substring(0, body.length() - 2);
-    InputStream is = verifiedStream(body);
-    assertInvalidBody(is, "Invalid chunk data terminator");
-  }
-
-  @Test
-  void rejectsBodyTruncatedMidPayload() throws Exception {
-    String body = "10000;chunk-signature=" + CHUNK1_SIGNATURE + "\r\n" + repeat('a', 100);
-    InputStream is = verifiedStream(body);
-    assertInvalidBody(is, "terminating 0-byte chunk");
-  }
-
   @Test
   void rejectsTamperedChunkSignatureHeader() throws IOException {
     String tamperedSig = CHUNK1_SIGNATURE.substring(0, 1)
@@ -551,7 +571,7 @@ void rejectsTamperedChunkSignatureHeader() throws IOException {
   })
   void rejectsChunkSignatureThatIsNotHmacSha256(String signature) {
     InputStream is = wrapContent("0A;chunk-signature=" + signature + "\r\n1234567890\r\n");
-    assertInvalidBody(is, "Invalid signature line");
+    assertBodyError(is, S3ErrorTable.INVALID_REQUEST, "Invalid signature line");
   }
 
   /** A chunk that fails verification must surface as SignatureDoesNotMatch (HTTP 403), not any other error. */
@@ -561,11 +581,10 @@ private static void assertSignatureMismatch(InputStream is) {
     assertEquals(S3ErrorTable.SIGNATURE_DOES_NOT_MATCH.getHttpCode(), ex.getHttpCode());
   }
 
-  /** A malformed body must surface as InvalidRequest (HTTP 400), not as an InternalError. */
-  private static void assertInvalidBody(InputStream is, String expectedMessage) {
+  private static void assertBodyError(InputStream is, S3ErrorTable expected, String expectedMessage) {
     OS3Exception ex = assertThrows(OS3Exception.class, () -> IOUtils.toString(is, UTF_8));
-    assertEquals(S3ErrorTable.INVALID_REQUEST.getCode(), ex.getCode());
-    assertEquals(S3ErrorTable.INVALID_REQUEST.getHttpCode(), ex.getHttpCode());
+    assertEquals(expected.getCode(), ex.getCode());
+    assertEquals(expected.getHttpCode(), ex.getHttpCode());
     assertThat(ex.getErrorMessage()).contains(expectedMessage);
   }