Handle trailer headers and unsigned aws chunked payloads#202
Open
thinaih wants to merge 1 commit intotrinodb:mainfrom
Open
Handle trailer headers and unsigned aws chunked payloads#202thinaih wants to merge 1 commit intotrinodb:mainfrom
thinaih wants to merge 1 commit intotrinodb:mainfrom
Conversation
thinaih
force-pushed
the
fix-streaming-for-aws-chunked-requests
branch
from
58504f2 to
3704d9a
Compare
thinaih
force-pushed
the
fix-streaming-for-aws-chunked-requests
branch
from
042be34 to
8b12b60
Compare
Reviewer's GuideThis PR adds support for AWS chunked unsigned payloads and trailer headers by extending the content-type model, enhancing AwsChunkedInputStream to accept optional signing and parse trailer header chunks, updating header builders and proxy client to propagate trailer metadata, and expanding the test suite to cover trailer and unsigned scenarios. Class diagram for updated AwsChunkedInputStream and related typesclassDiagram
class AwsChunkedInputStream {
-InputStream delegate
-Optional<ChunkSigningSession> chunkSigningSession
-int bytesRemainingInChunk
-int bytesAccountedFor
-int decodedContentLength
-List<String> trailerHeaders
+AwsChunkedInputStream(InputStream, Optional<ChunkSigningSession>, int, List<String>)
+read()
+read(byte[], int, int)
-nextChunk()
-readTrailingHeadersChunk()
-readTrailingHeaders()
}
class ChunkSigningSession {
+write(byte)
+write(byte[], int, int)
+startChunk(String)
+complete()
}
class TrailerHeaderChunk {
+String trailerHeaders
+Optional<String> signature
}
AwsChunkedInputStream --> "0..1" ChunkSigningSession : optional
AwsChunkedInputStream --> "*" TrailerHeaderChunk : uses
Class diagram for updated ContentType enum in RequestContentclassDiagram
class ContentType {
<<enum>>
STANDARD
W3C_CHUNKED
AWS_CHUNKED
AWS_CHUNKED_UNSIGNED
AWS_CHUNKED_IN_W3C_CHUNKED
AWS_CHUNKED_IN_W3C_CHUNKED_UNSIGNED
}
class RequestContent {
+ContentType contentType()
+Optional<Integer> contentLength()
+List<String> trailerHeaders()
+Optional<InputStream> inputStream()
}
RequestContent --> ContentType
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `trino-aws-proxy/src/main/java/io/trino/aws/proxy/server/rest/AwsChunkedInputStream.java:259` </location>
<code_context>
return line.toString();
}
+
+ private TrailerHeaderChunk readTrailingHeadersChunk()
+ throws IOException
+ {
</code_context>
<issue_to_address>
**issue (bug_risk):** The logic for parsing trailer headers may break early if x-amz-trailer-signature is found before all expected headers.
The loop may exit before processing all expected trailer headers if the signature is encountered early. Please verify whether all headers should be read before breaking, or if the signature is always last.
</issue_to_address>
### Comment 2
<location> `trino-aws-proxy/src/main/java/io/trino/aws/proxy/server/rest/AwsChunkedInputStream.java:268-269` </location>
<code_context>
+ String trailerHeaders = readLine();
+ List<String> trailerHeadersValues = Splitter.on(":").trimResults().limit(2).splitToList(trailerHeaders);
+ String trailerHeaderName = trailerHeadersValues.getFirst();
+ if ((trailerHeadersValues.size() != 2) || !this.trailerHeaders.contains(trailerHeaderName)) {
+ throw new WebApplicationException("Trailer header is invalid: " + trailerHeaders, BAD_REQUEST);
+ }
+ if (trailerHeaderName.equals("x-amz-trailer-signature")) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The check for trailer header validity may be too strict if header names are case-insensitive.
Normalize both the input and expected header names to lower-case before comparison to ensure case-insensitive matching.
Suggested implementation:
```java
String trailerHeaders = readLine();
List<String> trailerHeadersValues = Splitter.on(":").trimResults().limit(2).splitToList(trailerHeaders);
String trailerHeaderName = trailerHeadersValues.getFirst().toLowerCase();
boolean validTrailerHeader = this.trailerHeaders.stream()
.map(String::toLowerCase)
.anyMatch(h -> h.equals(trailerHeaderName));
if ((trailerHeadersValues.size() != 2) || !validTrailerHeader) {
throw new WebApplicationException("Trailer header is invalid: " + trailerHeaders, BAD_REQUEST);
}
if (trailerHeaderName.equals("x-amz-trailer-signature")) {
```
```java
if (trailerHeaderName.equals("x-amz-trailer-signature")) {
signature = Optional.of(trailerHeadersValues.getLast());
break;
}
else {
trailerHeadersChunkBuilder.append(trailerHeaders);
}
```
</issue_to_address>
### Comment 3
<location> `trino-aws-proxy/src/main/java/io/trino/aws/proxy/server/rest/RequestHeadersBuilder.java:206-209` </location>
<code_context>
passthroughHeadersBuilder.addAll(headerName, headerValues);
}
+ private String requiredContentSha256()
+ {
+ return contentSha256.orElseThrow(() -> new WebApplicationException(BAD_REQUEST));
</code_context>
<issue_to_address>
**suggestion:** The requiredContentSha256() method throws a generic BAD_REQUEST error without a message.
Include a descriptive error message in the exception to clarify when the x-amz-content-sha256 header is missing or invalid.
```suggestion
private String requiredContentSha256()
{
return contentSha256.orElseThrow(() -> new WebApplicationException(
"Missing or invalid x-amz-content-sha256 header", BAD_REQUEST));
}
```
</issue_to_address>
### Comment 4
<location> `trino-aws-proxy/src/main/java/io/trino/aws/proxy/server/rest/RequestBuilder.java:193-194` </location>
<code_context>
+ this.trailerHeaders = requireNonNull(ImmutableList.copyOf(trailerHeaders), "trailerHeaders is null");
}
@Override
</code_context>
<issue_to_address>
**issue (bug_risk):** The trailerHeaders() implementation assumes the header is present and non-null.
If the header is missing, ImmutableList.copyOf will throw a NullPointerException. Use Optional.ofNullable or default to an empty list to prevent this.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return line.toString(); | ||
| } | ||
|
|
||
| private TrailerHeaderChunk readTrailingHeadersChunk() |
There was a problem hiding this comment.
issue (bug_risk): The logic for parsing trailer headers may break early if x-amz-trailer-signature is found before all expected headers.
The loop may exit before processing all expected trailer headers if the signature is encountered early. Please verify whether all headers should be read before breaking, or if the signature is always last.
Comment on lines
+268
to
+269
| if ((trailerHeadersValues.size() != 2) || !this.trailerHeaders.contains(trailerHeaderName)) { | ||
| throw new WebApplicationException("Trailer header is invalid: " + trailerHeaders, BAD_REQUEST); |
There was a problem hiding this comment.
suggestion (bug_risk): The check for trailer header validity may be too strict if header names are case-insensitive.
Normalize both the input and expected header names to lower-case before comparison to ensure case-insensitive matching.
Suggested implementation:
String trailerHeaders = readLine();
List<String> trailerHeadersValues = Splitter.on(":").trimResults().limit(2).splitToList(trailerHeaders);
String trailerHeaderName = trailerHeadersValues.getFirst().toLowerCase();
boolean validTrailerHeader = this.trailerHeaders.stream()
.map(String::toLowerCase)
.anyMatch(h -> h.equals(trailerHeaderName));
if ((trailerHeadersValues.size() != 2) || !validTrailerHeader) {
throw new WebApplicationException("Trailer header is invalid: " + trailerHeaders, BAD_REQUEST);
}
if (trailerHeaderName.equals("x-amz-trailer-signature")) { if (trailerHeaderName.equals("x-amz-trailer-signature")) {
signature = Optional.of(trailerHeadersValues.getLast());
break;
}
else {
trailerHeadersChunkBuilder.append(trailerHeaders);
}
Comment on lines
+206
to
+209
| private String requiredContentSha256() | ||
| { | ||
| return contentSha256.orElseThrow(() -> new WebApplicationException(BAD_REQUEST)); | ||
| } |
There was a problem hiding this comment.
suggestion: The requiredContentSha256() method throws a generic BAD_REQUEST error without a message.
Include a descriptive error message in the exception to clarify when the x-amz-content-sha256 header is missing or invalid.
Suggested change
| private String requiredContentSha256() | |
| { | |
| return contentSha256.orElseThrow(() -> new WebApplicationException(BAD_REQUEST)); | |
| } | |
| private String requiredContentSha256() | |
| { | |
| return contentSha256.orElseThrow(() -> new WebApplicationException( | |
| "Missing or invalid x-amz-content-sha256 header", BAD_REQUEST)); | |
| } |
Comment on lines
+193
to
+194
| @Override | ||
| public List<String> trailerHeaders() |
There was a problem hiding this comment.
issue (bug_risk): The trailerHeaders() implementation assumes the header is present and non-null.
If the header is missing, ImmutableList.copyOf will throw a NullPointerException. Use Optional.ofNullable or default to an empty list to prevent this.
Member
|
We currently only support signing chunks with header However, for trailer headers this should be |
Member
|
Had a chat with @thinaih about the way trailer headers are canonicalized. The docs don't actually mention how to canonicalize trailers, unlike for normal headers where they state they need to be sorted. However, looking at the implementation of the SDK it seems like they follow the same logic (i.e., they are sorted): and Since our current chunk signing class is largely copied from the SDK's chunk signer class, we may want to take inspiration from it as a way to implement signing for trailers properly (and also fix the comment I mentioned above re. |
Member
Author
Good catch! Will fix it! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR does Addresses #199
It does provide support for
Overall changes
Summary by Sourcery
Add support for AWS chunked payloads that include trailer headers and handle unsigned AWS-chunked streams throughout the proxy stack.
New Features:
Enhancements:
Tests: