Support for payload signing + chunked encoding in DefaultAwsV4HttpSigner #6466
Conversation
This commit adds support for SigV4 signing of async request payloads. In addition this commit moves the support for trailing checksums from HttpChecksumStage to the V4 signer implementation; this puts it in line with how sync chunked bodies are already handled.
- Rename to computeAndMoveContentLength - Rename variable to chunkedPayload - Fix javadocs - Sign payload only if non-null
Combine these two steps to improve performance by reducing memory allocations.
dagnir
force-pushed
the
dongie/sra-chunk-encoding-pt1
branch
from
fcda677 to
488b723
Compare
dagnir
force-pushed
the
dongie/sra-chunk-encoding-pt1
branch
from
3a9cb34 to
e52ea62
Compare
|
...aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/util/SignerUtils.java
Outdated
Show resolved
Hide resolved
| String checksumHeaderName = checksumHeaderName(checksumAlgorithm); | ||
| request.appendHeader(X_AMZ_TRAILER, checksumHeaderName); | ||
| } | ||
| request.putHeader(Header.CONTENT_LENGTH, Long.toString(encodedContentLength)); |
There was a problem hiding this comment.
I thought content-length header is not required, is that not the case?
There was a problem hiding this comment.
Hmm not sure. Per the public docs, it's required unless Transfer-Encoding is present and the value is not identity.
This logic is preserved from the existing signer code
@Fred1155 do you have any other info on this?
...ain/java/software/amazon/awssdk/http/auth/aws/internal/signer/AwsChunkedV4PayloadSigner.java
Outdated
Show resolved
Hide resolved
| // should not happen, this header is added by moveContentLength | ||
| .orElseThrow(() -> new RuntimeException(X_AMZ_DECODED_CONTENT_LENGTH | ||
| + " header not present")); |
There was a problem hiding this comment.
Should we fail the future instead?
There was a problem hiding this comment.
Hmm it should fail the future right?
CompletableFuture<String> c1 = new CompletableFuture<>();
c1.complete("Hello");
CompletableFuture<String> c2 = c1.thenApply(s -> {
throw new RuntimeException();
});
c2.join(); // throws| } | ||
| executionAttributes.putAttribute(RESOLVED_CHECKSUM_SPECS, resolvedChecksumSpecs); | ||
|
|
||
| SdkHttpRequest httpRequest = context.executionContext().interceptorContext().httpRequest(); |
There was a problem hiding this comment.
We are going to have a separate PR/release for this change, right?
There was a problem hiding this comment.
I don't think it's possible to separate these two, otherwise the body would get double chunk encoding.
I know we discussed doing a version bump for this removal. We can do the version bump and release both in the same minor version?
There was a problem hiding this comment.
Even if we revert to this change in the signer?
if (isChunkEncoding && isPayloadSigning) {
// TODO(sra-identity-and-auth): We need to implement aws-chunk content-encoding for async.
// For now, we basically have to treat this as an unsigned case because there are existing s3 use-cases for
// Unsigned-payload + HTTP. These requests SHOULD be signed-payload, but are not pre-SRA, hence the problem. This
// will be taken care of in HttpChecksumStage for now, so we shouldn't throw an unsupported exception here, we
// should just fall through to the default since it will already encoded by the time it gets here.
return V4PayloadSigner.create();
NVM if it'd actually make it worse.
...are/amazon/awssdk/http/auth/aws/internal/signer/chunkedencoding/ChunkedEncodedPublisher.java
Outdated
Show resolved
Hide resolved
| * and/or trailers representing trailing headers with their signature at the end. | ||
| */ | ||
| @SdkInternalApi | ||
| public final class AwsChunkedV4aPayloadSigner implements V4aPayloadSigner { |
There was a problem hiding this comment.
Don't we need to support signAsync in this class as well?
There was a problem hiding this comment.
Yes, but I will follow up with a separate PR for that; same for DefaultAwsCrtV4aHttpSigner.
There was a problem hiding this comment.
Gotcha, but as soon as we remove the switching logic in HttpChecksumStage.java, sigv4a streaming operations will be broken, right? since it doesn't do AWS chunked encoding for async
There was a problem hiding this comment.
🤦♂️ Ah I think you are right. In that case I can merge this back to the feature branch, and follow up with that. This PR is big enough as it is
| if (clientType == ClientType.ASYNC && | ||
| isStreamingUnsignedPayload(httpRequest, executionAttributes, resolvedChecksumSpecs, | ||
| resolvedChecksumSpecs.isRequestStreaming())) { | ||
| addFlexibleChecksumInTrailer(request, context, resolvedChecksumSpecs); |
There was a problem hiding this comment.
If a customer had an execution interceptor put on this checksum stage, that now doesnt have the calculated checksum, can this be a potentially breaking change?
There was a problem hiding this comment.
Discussed this offline, there shouldn't really be any risk here. The checksum calculation still happened in a "streaming" fashion before by wrapping the publisher in the checksum publisher; we weren't calculating the checksum upfront.
- Update variable naming - Throw if decoded content length not present - Throw if input request doesn't have content-length header
Motivation and Context
Add support for payload signing of async streaming requests signed with SigV4 using default
AwsV4HttpSigner(usingAwsV4HttpSigner.create()). Note, requests using thehttpURI scheme will not be signed regardless of the value ofAwsV4FamilyHttpSigner.PAYLOAD_SIGNING_ENABLEDto remain consistent with existing behavior. This may change in a future release.Modifications
Testing
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License