-
Notifications
You must be signed in to change notification settings - Fork 1k
Enable checksum validation for presigned URL downloads #7035
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feature/master/pre-signed-url-getobject
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -63,14 +63,43 @@ public SdkHttpFullRequest marshall(PresignedUrlDownloadRequestWrapper presignedU | |
| .createProtocolMarshaller(SDK_OPERATION_BINDING); | ||
| URI presignedUri = presignedUrlDownloadRequestWrapper.url().toURI(); | ||
|
|
||
| return protocolMarshaller.marshall(presignedUrlDownloadRequestWrapper) | ||
| .toBuilder() | ||
| .uri(presignedUri) | ||
| .build(); | ||
| SdkHttpFullRequest.Builder requestBuilder = protocolMarshaller | ||
| .marshall(presignedUrlDownloadRequestWrapper) | ||
| .toBuilder() | ||
| .uri(presignedUri); | ||
|
|
||
| addChecksumModeHeaderIfSignedInUrl(requestBuilder, presignedUri); | ||
|
|
||
| return requestBuilder.build(); | ||
| } catch (Exception e) { | ||
| throw SdkClientException.builder() | ||
| .message("Unable to marshall pre-signed URL Request: " + e.getMessage()) | ||
| .cause(e).build(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * If the presigned URL's X-Amz-SignedHeaders contains "x-amz-checksum-mode", automatically add | ||
| * the header with value "ENABLED" so S3 returns checksum headers in the response. | ||
| */ | ||
| private void addChecksumModeHeaderIfSignedInUrl(SdkHttpFullRequest.Builder requestBuilder, URI uri) { | ||
| if (hasChecksumModeInSignedHeaders(uri.getQuery())) { | ||
| requestBuilder.putHeader("x-amz-checksum-mode", "ENABLED"); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Returns true if the decoded query string's X-Amz-SignedHeaders parameter contains "x-amz-checksum-mode". | ||
| */ | ||
| static boolean hasChecksumModeInSignedHeaders(String query) { | ||
| if (query == null) { | ||
| return false; | ||
| } | ||
| for (String param : query.split("&")) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the query parameters here are URL encoded - I'm trying to think through if there are any cases where we would end up with something invalid because we're not actually decoding these first.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We use uri.getQuery() which returns the decoded query string, so parsing operates on decoded values. |
||
| if (param.startsWith("X-Amz-SignedHeaders=")) { | ||
| return param.substring("X-Amz-SignedHeaders=".length()).contains("x-amz-checksum-mode"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't remember exactly how all of the signing works, but are these guaranteed to be lower case?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, V4CanonicalRequest.getCanonicalHeaders() lowercases all header names per the SigV4 specification, so X-Amz-SignedHeaders always contains lowercase values. |
||
| } | ||
| } | ||
| return false; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little worried about the hard coding of these algorithms here. It would be easy for us to forget to update this with new algorithms.... is there any way we could create it dynamically?