Adding x-amz-content-sha256 header for signed requests

[VachaShah]

Description

X-Amz-Content-Sha256 is a required header for Serverless.

Tested this code against both Managed Service and Serverless.

Issues Resolved

#287

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.

[dblock]

Add a test that checks that the x-amz-... header is added?

[dblock]

What a weird API! Took me a while to find https://github.com/aws/aws-sdk-java-v2/blob/0510a17ae41d601cf5f03e7af01e4519a6b3a744/core/auth/src/main/java/software/amazon/awssdk/auth/signer/internal/AbstractAws4Signer.java#L97 that handles this.

Is there a higher level method we can use like https://github.com/aws/aws-sdk-java-v2/blob/0510a17ae41d601cf5f03e7af01e4519a6b3a744/core/auth/src/main/java/software/amazon/awssdk/auth/signer/Aws4UnsignedPayloadSigner.java#L69 so we don't have to hard-code "required"?

[VachaShah]

Yeah it took me a while to find too. We use Aws4Signer which does not have the sign method overriden like Aws4UnsignedPayloadSigner. According to documentation, the Aws4UnsignedPayloadSigner is similar to Aws4Signer but just adds UNSIGNED-PAYLOAD when protocol is HTTPS. Can we use Aws4UnsignedPayloadSigner?

[dblock]

If it works we sure can I think.

[VachaShah]

I tried the Aws4UnsignedPayloadSigner but it does not work for Amazon OpenSearch Service since it signs the payload with UNSIGNED_PAYLOAD over https protocol. Works for Amazon OpenSearch Serverless. I can PR this change on their repo may be as a new signer class but for now looks like we might have to use the hard-coded "required".

[VachaShah]

Add a test that checks that the x-amz-... header is added?

Added test.

[reta]

Those should probably not be public

[VachaShah]

I had to do that since I had to write a test for the header :( The AwsSdk2Transport returns a parsed response which does not have the headers for me to test. So I had to test it when the request gets signed.

[dblock]

I think we need to find a way. How about making it protected, subclassing AwsSdk2Transport into a test class and saving headers into it?

[VachaShah]

Do you mean something like this?

public class AwsSdk2TransportMock extends AwsSdk2Transport {

    public AwsSdk2TransportMock(
            @Nonnull SdkHttpClient httpClient,
            @Nonnull String host,
            @Nonnull String signingServiceName,
            @Nonnull Region signingRegion,
            @CheckForNull AwsSdk2TransportOptions options) {
        super(httpClient, null, host, signingServiceName, signingRegion, options);
    }

    @Override
    public <RequestT> OpenSearchRequestBodyBuffer prepareRequestBody(RequestT request,
            Endpoint<RequestT, ?, ?> endpoint, TransportOptions options) throws IOException {
        return super.prepareRequestBody(request, endpoint, options);
    }
    @Override
    public <RequestT> SdkHttpFullRequest prepareRequest(RequestT request, Endpoint<RequestT, ?, ?> endpoint,
            TransportOptions options, OpenSearchRequestBodyBuffer body) {
        return super.prepareRequest(request, endpoint, options, body);
    }
}

where the prepareRequest and prepareRequestBody in AwsSdk2Transport are protected?

[dblock]

Right. Looking at your implementation I think that was not a good suggestion, sorry. I say we open a bug for this and call it a day instead of adding the child class.

[dblock]

I think while the inheritance here works around the problem, it's a bad idea. This test is way too complicated for something as simple as a header.

@VachaShah I vote to remove testContentShaHeader (and the new mock class), and manually confirm that ./gradlew integrationTest --tests "*AwsSdk2SearchIT*" -Dtests.awsSdk2support.domainHost=$ENDPOINT -Dtests.awsSdk2support.domainRegion=us-west-2 ... works for both managed OpenSearch and Serverless.

@reta do you have any other ideas?

[dblock]

I don't think that "mock" is correct naming here. That implies something that "fakes" a certain operation, which this does not.

[reta]

@reta do you have any other ideas?

Sorry for delay, I thought about simple approach with test interceptor [1]:

    protected SdkHttpClient getHttpClient() {
        if (httpClient == null) {
            httpClient = ApacheHttpClient
            		.builder()
            		.buildWithDefaults(AttributeMap.builder()
            			.put(SdkClientOption.EXECUTION_INTERCEPTORS, List.of(/* test interceptor */))
            			.build());
        }
        return httpClient;
    }

By adding one, we should be able to verify presence of all headers before request is sent off.

[1] https://github.com/aws/aws-sdk-java-v2/blob/master/core/sdk-core/src/main/java/software/amazon/awssdk/core/interceptor/ExecutionInterceptor.java#L198

[VachaShah]

I think while the inheritance here works around the problem, it's a bad idea. This test is way too complicated for something as simple as a header.

@VachaShah I vote to remove testContentShaHeader (and the new mock class), and manually confirm that ./gradlew integrationTest --tests "*AwsSdk2SearchIT*" -Dtests.awsSdk2support.domainHost=$ENDPOINT -Dtests.awsSdk2support.domainRegion=us-west-2 ... works for both managed OpenSearch and Serverless.

@reta do you have any other ideas?

Sure! I removed the test and the sub class.

For confirmation, I have manually tested this change against both OpenSearch and OpenSearch Serverless with the integ tests and a client app.

[VachaShah]

I think while the inheritance here works around the problem, it's a bad idea. This test is way too complicated for something as simple as a header.
@VachaShah I vote to remove testContentShaHeader (and the new mock class), and manually confirm that ./gradlew integrationTest --tests "*AwsSdk2SearchIT*" -Dtests.awsSdk2support.domainHost=$ENDPOINT -Dtests.awsSdk2support.domainRegion=us-west-2 ... works for both managed OpenSearch and Serverless.
@reta do you have any other ideas?

Sure! I removed the test and the sub class.

For confirmation, I have manually tested this change against both OpenSearch and OpenSearch Serverless with the integ tests and a client app.

Actually, for OpenSearch Serverless, wait for refresh policy does not work.

@reta do you have any other ideas?

Sorry for delay, I thought about simple approach with test interceptor [1]:

    protected SdkHttpClient getHttpClient() {
        if (httpClient == null) {
            httpClient = ApacheHttpClient
            		.builder()
            		.buildWithDefaults(AttributeMap.builder()
            			.put(SdkClientOption.EXECUTION_INTERCEPTORS, List.of(/* test interceptor */))
            			.build());
        }
        return httpClient;
    }

By adding one, we should be able to verify presence of all headers before request is sent off.

[1] https://github.com/aws/aws-sdk-java-v2/blob/master/core/sdk-core/src/main/java/software/amazon/awssdk/core/interceptor/ExecutionInterceptor.java#L198

Thats interesting. I will add this test in an upcoming PR. Thank you @reta!

[dblock]

I’m good with this. We can improve tests and what not later.

[VachaShah]

I think while the inheritance here works around the problem, it's a bad idea. This test is way too complicated for something as simple as a header.
@VachaShah I vote to remove testContentShaHeader (and the new mock class), and manually confirm that ./gradlew integrationTest --tests "*AwsSdk2SearchIT*" -Dtests.awsSdk2support.domainHost=$ENDPOINT -Dtests.awsSdk2support.domainRegion=us-west-2 ... works for both managed OpenSearch and Serverless.
@reta do you have any other ideas?

Sure! I removed the test and the sub class.

For confirmation, I have manually tested this change against both OpenSearch and OpenSearch Serverless with the integ tests and a client app.

Actually, I wrote too fast. The integ tests for serverless might need to be modified since there are certain things that don't work for Serverless like wait for refresh policy. Manual testing it with a client app works successfully.

[dblock]

@VachaShah you can remove the refresh policy altogether, that’s what I did in the demos

make sure dev guide says all the right things

[VachaShah]

@VachaShah you can remove the refresh policy altogether, that’s what I did in the demos

make sure dev guide says all the right things

Removing the refresh policy address a problem but then other issues stem up. Integ tests would need to be modified to work with Serverless.

[dblock]

@VachaShah you can remove the refresh policy altogether, that’s what I did in the demos
make sure dev guide says all the right things

Removing the refresh policy address a problem but then other issues stem up. Integ tests would need to be modified to work with Serverless.

Do they pass with a sleep, or at least once? Since we aren’t running them with automation rn, I would consider that good enough.

[VachaShah]

@VachaShah you can remove the refresh policy altogether, that’s what I did in the demos
make sure dev guide says all the right things

Removing the refresh policy address a problem but then other issues stem up. Integ tests would need to be modified to work with Serverless.

Do they pass with a sleep, or at least once? Since we aren’t running them with automation rn, I would consider that good enough.

They passed with a sleep once without the refresh policy. But keep failing intermittently.

[dblock]

Serverless may have a cold start thing going on causing intermittent delays, I would ignore that.

[VachaShah]

@dblock Review again please? Github dismissed your approval 🤦🏼‍♀️

[dblock]

@reta good with you?

[reta]

This is a recipe for flakyness ....

[reta]

@reta good with you?

@dblock we need to improve tests right away, using something like https://github.com/awaitility/awaitility to replace Thread.sleep() calls.

[VachaShah]

@reta good with you?

@dblock we need to improve tests right away, using something like https://github.com/awaitility/awaitility to replace Thread.sleep() calls.

@reta I will work on doing that as part of #343.

[opensearch-trigger-bot]

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:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-339-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 41beeda73115bf000bcda05a94e107491a84dd1b
# Push it to GitHub
git push --set-upstream origin backport/backport-339-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

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

[dblock]

@reta good with you?

@dblock we need to improve tests right away, using something like https://github.com/awaitility/awaitility to replace Thread.sleep() calls.

For sure. It’s not that simple and those tests don’t run. We’ll queue up getting infrastructure and repeatable tests for managed and serverless for all clients. Cc: @wbeckler