AwsSdk2Transport throw exception when using ApacheHttpClient to make an unsupported DELETE/GET request with a body #1256

Xtansia · 2024-11-01T03:43:04Z

Description

The AWS SDK's ApacheHttpClient implementation does not send the request body on DELETE or GET requests, https://github.com/aws/aws-sdk-java-v2/blob/master/http-clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/internal/impl/ApacheHttpRequestFactory.java#L118-L137.

Additionally moves to the supported AwsV4HttpSigner as Aws4Signer is now deprecated: https://github.com/aws/aws-sdk-java-v2/blob/88abec27e7d5d35b21545c7e05875a7cc3d0f46e/core/auth/src/main/java/software/amazon/awssdk/auth/signer/Aws4Signer.java

Issues Resolved

Relates to #712
Relates to #521
Fixes #503

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

java-client/build.gradle.kts

java-client/src/main/java/org/opensearch/client/transport/aws/AwsSdk2Transport.java

java-client/build.gradle.kts

dblock

Looks good. A lot going on that's somewhat unrelated to the signing fix, you might want to extract it first, but up to you.

CHANGELOG.md

...lient/src/test/java/org/opensearch/client/transport/util/GeneratedCertificateSSLContext.java

java-client/src/test/java/org/opensearch/client/transport/aws/AwsSdk2TransportTests.java

...lient/src/test/java/org/opensearch/client/transport/util/SelfSignedCertificateAuthority.java

…an unsupported DELETE/GET request with a body The AWS SDK's ApacheHttpClient implementation does not send the request body on DELETE or GET requests, https://github.com/aws/aws-sdk-java-v2/blob/master/http-clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/internal/impl/ApacheHttpRequestFactory.java#L118-L137. Additionally moves to the supported `AwsV4HttpSigner` as `Aws4Signer` is now deprecated: https://github.com/aws/aws-sdk-java-v2/blob/88abec27e7d5d35b21545c7e05875a7cc3d0f46e/core/auth/src/main/java/software/amazon/awssdk/auth/signer/Aws4Signer.java Signed-off-by: Thomas Farr <tsfarr@amazon.com>

Xtansia · 2024-11-08T10:16:44Z

Unfortunately on further digging and expanding the tests a bit, AWS SDK's ApacheHttpClient does not send the request body at all for DELETE or GET requests, https://github.com/aws/aws-sdk-java-v2/blob/master/http-clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/internal/impl/ApacheHttpRequestFactory.java#L118-L137

So this doesn't actually fix anything, as such I've pivoted to pre-emptively throwing an exception as the request would fail signature validation anyway and gives a clear reasoning.

Signed-off-by: Thomas Farr <tsfarr@amazon.com>

dblock · 2024-11-08T13:12:49Z

@Xtansia Probably worth opening an issue in that SDK, AFAIK nothing says GET or DELETE can't have a body.

reta · 2024-11-08T13:29:28Z

@Xtansia Probably worth opening an issue in that SDK, AFAIK nothing says GET or DELETE can't have a body.

@dblock indeed the spec [1] is not strict on that but the general recommendation (like [2]) is that GET should not have a body, DELETE may though (but shouldn't) [3]. Fe, https://www.rfc-editor.org/rfc/rfc9110.html#name-get says (similar for DELETE):

Although request message framing is independent of the method used, content received in a GET request has no generally defined semantics, cannot alter the meaning or target of the request, and might lead some implementations to reject the request and close the connection because of its potential as a request smuggling attack (Section 11.2 of [HTTP/1.1]). A client SHOULD NOT generate content in a GET request unless it is made directly to an origin server that has previously indicated, in or out of band, that such a request has a purpose and will be adequately supported. An origin server SHOULD NOT rely on private agreements to receive content, since participants in HTTP communication are often unaware of intermediaries along the request chain.

[1] https://www.rfc-editor.org/rfc/rfc9110.html#name-changes-from-rfc-7231
[2] https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/GET
[3] https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/DELETE

java-client/src/main/java/org/opensearch/client/transport/aws/AwsSdk2Transport.java

reta · 2024-11-08T14:06:53Z

java-client/src/main/java/org/opensearch/client/transport/aws/AwsSdk2Transport.java

@@ -118,6 +122,10 @@ public AwsSdk2Transport(
     * @param options Options that apply to all requests. Can be null. Create with
     *                {@link AwsSdk2TransportOptions#builder()} and use these to specify non-default credentials,
     *                compression options, etc.
+     *
+     * @implNote Using {@code software.amazon.awssdk.http.apache.ApacheHttpClient} is discouraged as it does not support request bodies on GET or DELETE requests.


Signed-off-by: Thomas Farr <tsfarr@amazon.com>

Xtansia · 2024-11-11T01:24:11Z

I might just try PR'ing a fix into aws-sdk seeing as it's relatively simple change, and hopefully mean more likely to actually get fixed.

Signed-off-by: Thomas Farr <tsfarr@amazon.com>

Xtansia · 2024-11-11T04:51:59Z

Created aws/aws-sdk-java-v2#5704

dblock · 2024-11-11T13:22:12Z

Created aws/aws-sdk-java-v2#5704

Good work 👏

opensearch-trigger-bot · 2024-11-12T02:48:36Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/opensearch-java/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/opensearch-java/backport-2.x
# Create a new branch
git switch --create backport/backport-1256-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 e8e3a9942c1d2e006ea82ce8f4395fc94a4dd53b
# Push it to GitHub
git push --set-upstream origin backport/backport-1256-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/opensearch-java/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-1256-to-2.x.

…an unsupported DELETE/GET request with a body (opensearch-project#1256) * AwsSdk2Transport throw exception when using ApacheHttpClient to make an unsupported DELETE/GET request with a body The AWS SDK's ApacheHttpClient implementation does not send the request body on DELETE or GET requests, https://github.com/aws/aws-sdk-java-v2/blob/master/http-clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/internal/impl/ApacheHttpRequestFactory.java#L118-L137. Additionally moves to the supported `AwsV4HttpSigner` as `Aws4Signer` is now deprecated: https://github.com/aws/aws-sdk-java-v2/blob/88abec27e7d5d35b21545c7e05875a7cc3d0f46e/core/auth/src/main/java/software/amazon/awssdk/auth/signer/Aws4Signer.java Signed-off-by: Thomas Farr <tsfarr@amazon.com> * Add guide note Signed-off-by: Thomas Farr <tsfarr@amazon.com> * Fix javadoc Signed-off-by: Thomas Farr <tsfarr@amazon.com> * Re-use ContentStreamProvider Signed-off-by: Thomas Farr <tsfarr@amazon.com> * Also validate URLConnection client Signed-off-by: Thomas Farr <tsfarr@amazon.com> * spotless Signed-off-by: Thomas Farr <tsfarr@amazon.com> * Test HEAD and OPTIONS Signed-off-by: Thomas Farr <tsfarr@amazon.com> --------- Signed-off-by: Thomas Farr <tsfarr@amazon.com> (cherry picked from commit e8e3a99)

…an unsupported DELETE/GET request with a body (#1256) (#1288) * AwsSdk2Transport throw exception when using ApacheHttpClient to make an unsupported DELETE/GET request with a body The AWS SDK's ApacheHttpClient implementation does not send the request body on DELETE or GET requests, https://github.com/aws/aws-sdk-java-v2/blob/master/http-clients/apache-client/src/main/java/software/amazon/awssdk/http/apache/internal/impl/ApacheHttpRequestFactory.java#L118-L137. Additionally moves to the supported `AwsV4HttpSigner` as `Aws4Signer` is now deprecated: https://github.com/aws/aws-sdk-java-v2/blob/88abec27e7d5d35b21545c7e05875a7cc3d0f46e/core/auth/src/main/java/software/amazon/awssdk/auth/signer/Aws4Signer.java Signed-off-by: Thomas Farr <tsfarr@amazon.com> * Add guide note Signed-off-by: Thomas Farr <tsfarr@amazon.com> * Fix javadoc Signed-off-by: Thomas Farr <tsfarr@amazon.com> * Re-use ContentStreamProvider Signed-off-by: Thomas Farr <tsfarr@amazon.com> * Also validate URLConnection client Signed-off-by: Thomas Farr <tsfarr@amazon.com> * spotless Signed-off-by: Thomas Farr <tsfarr@amazon.com> * Test HEAD and OPTIONS Signed-off-by: Thomas Farr <tsfarr@amazon.com> --------- Signed-off-by: Thomas Farr <tsfarr@amazon.com> (cherry picked from commit e8e3a99)

houssain-barouni · 2024-11-27T16:49:22Z

Hello @Xtansia ,
as aws/aws-sdk-java-v2#5704 is now merged and released in awssdk 2.29.22
should we remove this check? or skip it for DELETE, GET, HEAD & OPTIONS methods?

Xtansia · 2024-11-27T23:45:24Z

@houssain-barouni Yes, I have created #1333

Xtansia requested review from reta, Bukhtawar, dblock, szczepanczykd, madhusudhankonda, saratvemulapalli and VachaShah as code owners November 1, 2024 03:43

reta reviewed Nov 1, 2024

View reviewed changes

java-client/build.gradle.kts Show resolved Hide resolved

reta reviewed Nov 1, 2024

View reviewed changes

java-client/src/main/java/org/opensearch/client/transport/aws/AwsSdk2Transport.java Outdated Show resolved Hide resolved

reta reviewed Nov 1, 2024

View reviewed changes

java-client/build.gradle.kts Outdated Show resolved Hide resolved

ThorbenLindhauer mentioned this pull request Nov 4, 2024

Scrolls are not sufficiently cleared on AWS Opensearch: rework fix for #19869 camunda/camunda#24321

Open

5 tasks

Xtansia marked this pull request as draft November 4, 2024 20:52

Xtansia force-pushed the fix/aws-sigv4-delete-pit-scroll branch from 8e1d91a to d837e5c Compare November 5, 2024 04:00

houssain-barouni mentioned this pull request Nov 5, 2024

fix: Switch back to Apache client for AWS Opensearch requests camunda/camunda#24334

Merged

3 tasks

Xtansia force-pushed the fix/aws-sigv4-delete-pit-scroll branch 4 times, most recently from e75c9ec to a8a42f8 Compare November 6, 2024 02:28

Xtansia marked this pull request as ready for review November 6, 2024 02:54

Xtansia requested a review from reta November 6, 2024 03:12

Xtansia force-pushed the fix/aws-sigv4-delete-pit-scroll branch from 214545a to febd596 Compare November 6, 2024 04:00

dblock previously approved these changes Nov 6, 2024

View reviewed changes

reta reviewed Nov 6, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

reta reviewed Nov 6, 2024

View reviewed changes

...lient/src/test/java/org/opensearch/client/transport/util/GeneratedCertificateSSLContext.java Outdated Show resolved Hide resolved

Xtansia dismissed dblock’s stale review via 3e8c288 November 7, 2024 11:27

Xtansia force-pushed the fix/aws-sigv4-delete-pit-scroll branch from febd596 to 3e8c288 Compare November 7, 2024 11:27

Xtansia requested review from reta and dblock November 7, 2024 11:28

reta reviewed Nov 7, 2024

View reviewed changes

java-client/src/test/java/org/opensearch/client/transport/aws/AwsSdk2TransportTests.java Outdated Show resolved Hide resolved

reta reviewed Nov 7, 2024

View reviewed changes

...lient/src/test/java/org/opensearch/client/transport/util/SelfSignedCertificateAuthority.java Show resolved Hide resolved

Xtansia force-pushed the fix/aws-sigv4-delete-pit-scroll branch from 3e8c288 to 031f30b Compare November 8, 2024 10:13

Xtansia requested a review from reta November 8, 2024 10:13

Xtansia changed the title ~~Fix AWS SigV4 on delete requests when using AWS SDK's Apache client~~ AwsSdk2Transport throw exception when using ApacheHttpClient to make an unsupported DELETE/GET request with a body Nov 8, 2024

Xtansia force-pushed the fix/aws-sigv4-delete-pit-scroll branch from 031f30b to 92d1480 Compare November 8, 2024 10:14

Xtansia added 2 commits November 8, 2024 23:24

Add guide note

c2564c9

Signed-off-by: Thomas Farr <tsfarr@amazon.com>

Fix javadoc

58823eb

Signed-off-by: Thomas Farr <tsfarr@amazon.com>

reta reviewed Nov 8, 2024

View reviewed changes

java-client/src/main/java/org/opensearch/client/transport/aws/AwsSdk2Transport.java Outdated Show resolved Hide resolved

reta reviewed Nov 8, 2024

View reviewed changes

Re-use ContentStreamProvider

6e4e8c2

Signed-off-by: Thomas Farr <tsfarr@amazon.com>

Xtansia requested a review from reta November 11, 2024 01:17

Xtansia added 3 commits November 11, 2024 14:44

Also validate URLConnection client

134b7bd

Signed-off-by: Thomas Farr <tsfarr@amazon.com>

spotless

60d3959

Signed-off-by: Thomas Farr <tsfarr@amazon.com>

Test HEAD and OPTIONS

30f0ac7

Signed-off-by: Thomas Farr <tsfarr@amazon.com>

Xtansia mentioned this pull request Nov 11, 2024

Fix ApacheHttpClient's handling of request bodies on DELETE, GET, HEAD & OPTIONS requests aws/aws-sdk-java-v2#5704

Merged

12 tasks

reta approved these changes Nov 11, 2024

View reviewed changes

Xtansia merged commit e8e3a99 into opensearch-project:main Nov 11, 2024
56 checks passed

Xtansia added the backport 2.x Backport to 2.x branch label Nov 11, 2024

Xtansia deleted the fix/aws-sigv4-delete-pit-scroll branch November 11, 2024 22:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AwsSdk2Transport throw exception when using ApacheHttpClient to make an unsupported DELETE/GET request with a body #1256

AwsSdk2Transport throw exception when using ApacheHttpClient to make an unsupported DELETE/GET request with a body #1256

Xtansia commented Nov 1, 2024 •

edited

Loading

dblock left a comment

Xtansia commented Nov 8, 2024

dblock commented Nov 8, 2024

reta commented Nov 8, 2024 •

edited

Loading

reta Nov 8, 2024

Xtansia commented Nov 11, 2024

Xtansia commented Nov 11, 2024

dblock commented Nov 11, 2024

opensearch-trigger-bot bot commented Nov 12, 2024

houssain-barouni commented Nov 27, 2024 •

edited

Loading

Xtansia commented Nov 27, 2024

AwsSdk2Transport throw exception when using ApacheHttpClient to make an unsupported DELETE/GET request with a body #1256

AwsSdk2Transport throw exception when using ApacheHttpClient to make an unsupported DELETE/GET request with a body #1256

Conversation

Xtansia commented Nov 1, 2024 • edited Loading

Description

Issues Resolved

dblock left a comment

Choose a reason for hiding this comment

Xtansia commented Nov 8, 2024

dblock commented Nov 8, 2024

reta commented Nov 8, 2024 • edited Loading

reta Nov 8, 2024

Choose a reason for hiding this comment

Xtansia commented Nov 11, 2024

Xtansia commented Nov 11, 2024

dblock commented Nov 11, 2024

opensearch-trigger-bot bot commented Nov 12, 2024

houssain-barouni commented Nov 27, 2024 • edited Loading

Xtansia commented Nov 27, 2024

Xtansia commented Nov 1, 2024 •

edited

Loading

reta commented Nov 8, 2024 •

edited

Loading

houssain-barouni commented Nov 27, 2024 •

edited

Loading