Add support for Serverless OpenSearch signing #90

acm19 · 2023-01-06T18:48:01Z

Adds support for signing Serverless OpenSearch requests as described: https://docs.aws.amazon.com/opensearch-service/latest/developerguide/serverless-clients.html#serverless-signing.

Requirements:

You must specify the service name as aoss.
You can't include Content-Length as a signed header, otherwise you'll get an invalid signature error.
Uses Aws4Signer while serverless uses Aws4UnsignedPayloadSigner.

Removes rule to have final for all parameters as it create verbose code while not enforcing strong enough protection. All parameters are consider immutable as a good practice anyway.

Issue #, if available:

Description of changes:

Pull Request Checklist:

I have added a contribution line at the top of CHANGELOG.md.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

acm19 · 2023-01-06T18:57:22Z

@dblock I've tested it already, but if you have time a second testing before merging would be great.

dblock

I tested this against my instance of OpenSearch Serverless and it works.

The isServerlessOpenSearch rubs me the wrong way. The interceptor code didn't have "es" hard-coded anywhere before this (only in samples), so it theoretically works with any service. Picking "aoss" gets super specific now to a service. I feel like an additional level of indirection of shouldIncludeContentLength() or a subclasses version of the interceptor with methods like copyHeaders would be cleaner. Or maybe we need to switch/case 'es', 'aoss', or supply some kind of configuration options in the constructor?

dblock · 2023-01-06T20:44:27Z

src/main/java/io/github/acm19/aws/interceptor/http/AwsRequestSigningApacheInterceptor.java

+            // copy everything back
+            request.setHeaders(mapToHeaderArray(signedRequest.headers()));
+        } else {
+            // copy everything back, don't override headers as no all of them were used for signing the request


no -> not

Why is this different for serverless? What headers should not be removed? Does the serverless version not work against managed service?

TL;DR: Content-Length.

For Managed OpenSearch we include Content-Length in the signature when it's different that 0, so it ends up in signedRequest so we just override all the headers and that's it. In the case of OpenSearch Sls we don't include it in the signature, if we override all headers we get rid of it and OpenSearch Sls doesn't like that. In order to preserve it then add back headers that are included in the request while keeping the original headers. It's not possible to do this for Managed OpenSearch because you'd then have it for content-length equals 0 which I understand we shouldn't, so test fails, but I didn't test against an actual instance, maybe I should before we merge this.

dblock · 2023-01-06T20:49:27Z

src/main/java/io/github/acm19/aws/interceptor/http/AwsRequestSigningApacheInterceptor.java

        return (HTTP.CONTENT_LEN.equalsIgnoreCase(header.getName())
-                && "0".equals(header.getValue())) // Strip Content-Length: 0
+                && (("0".equals(header.getValue())) || isServerlessOpenSearch))


Something doesn't add up here for the managed service. Maybe we didn't need Content-Length for it if we included x-amz-content-sha256: UNSIGNED-PAYLOAD?

Not sure I follow you, if you don't want to sign the payload, why using the interceptor? 🤔

Signing headers?

With TLS signing more or less data (body vs. headers) shouldn't really matter, as long as something is signed the server can ensure that the payload came from who claimed to send it.

dblock

CHANGELOG typo is a must ;)

Also add to README that both Amazon Managed OpenSearch and OpenSearch Serverless are supported and an example of usage (with "oass")?

dblock · 2023-01-06T22:14:35Z

CHANGELOG.md

@@ -1,5 +1,7 @@
 ### 2.2.1 (Next)

+* [#90](https://github.com/acm19/aws-request-signing-apache-interceptor/pull/90): Add support for Serverless OpeanSearch - [@acm19](https://github.com/acm19).


Typo: OpeanSearch -> OpenSearch

We use the terminology of "OpenSearch Serverless" and "Managed OpenSearch".

Fixed, thanks.

acm19 · 2023-01-08T22:44:32Z

The isServerlessOpenSearch rubs me the wrong way. The interceptor code didn't have "es" hard-coded anywhere before this (only in samples), so it theoretically works with any service. Picking "aoss" gets super specific now to a service. I feel like an additional level of indirection of shouldIncludeContentLength() or a subclasses version of the interceptor with methods like copyHeaders would be cleaner. Or maybe we need to switch/case 'es', 'aoss', or supply some kind of configuration options in the constructor?

I was expecting it ;) A bit of context, originally I thought of creating an interface, something like:

interface HeaderFilter {
  boolean shouldSkip(header);
  Header[] computeFinalHeader(originalHeader, signedHeaders);
}

Pass it over to the signer. That's generic enough. But it felt a bit over engineered, introducing some extra complexity. Then I thought, this is still beta and can change better make a simple as possible, add a special case for aoss and then refactor once it's stable if it doesn't change. I understand that future services could change in any other way, not only in headers so even this wouldn't be generic enough, but I'm happy to do something like this if you think the signing won't change after it becomes stable. For me the important thing is that it remains agnostic to the user.

Adds support for signing Serverless OpenSearch requests as described: https://docs.aws.amazon.com/opensearch-service/latest/developerguide/serverless-clients.html#serverless-signing. Requirements: * You must specify the service name as aoss. * You can't include Content-Length as a signed header, otherwise you'll get an invalid signature error. * Uses Aws4Signer while serverless uses Aws4UnsignedPayloadSigner. Removes rule to have `final` for all parameters as it create verbose code while not enforcing strong enough protection. All parameters are consider immutable as a good practice anyway. Resolves: #88

dblock

I'd like to hold this a bit. The server side is looking into aligning services Sigv4 and enabling content-length signing. We shouldn't need the special case.

dblock · 2023-01-12T19:11:09Z

src/main/java/io/github/acm19/aws/interceptor/http/AwsRequestSigningApacheInterceptor.java

        return (HTTP.CONTENT_LEN.equalsIgnoreCase(header.getName())
-                && "0".equals(header.getValue())) // Strip Content-Length: 0
+                && (("0".equals(header.getValue())) || isServerlessOpenSearch))


Signing headers?

With TLS signing more or less data (body vs. headers) shouldn't really matter, as long as something is signed the server can ensure that the payload came from who claimed to send it.

dblock · 2023-01-24T14:18:43Z

Closing in favor of #94.

acm19 force-pushed the serverless-support branch 2 times, most recently from cc1207d to 69829b4 Compare January 6, 2023 18:53

acm19 requested a review from dblock January 6, 2023 18:56

acm19 force-pushed the serverless-support branch from 69829b4 to eb957fe Compare January 6, 2023 18:58

dblock approved these changes Jan 6, 2023

View reviewed changes

dblock requested changes Jan 6, 2023

View reviewed changes

dblock mentioned this pull request Jan 6, 2023

[FEATURE] Add support for OpenSearch Serverless opensearch-project/opensearch-js#356

Closed

acm19 force-pushed the serverless-support branch 2 times, most recently from 4a799f1 to 165d63c Compare January 8, 2023 20:03

acm19 force-pushed the serverless-support branch from 165d63c to b7e2798 Compare January 8, 2023 22:45

dblock reviewed Jan 12, 2023

View reviewed changes

dblock mentioned this pull request Jan 24, 2023

Added support for OpenSearch Serverless. #94

Merged

1 task

dblock closed this Jan 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Serverless OpenSearch signing #90

Add support for Serverless OpenSearch signing #90

acm19 commented Jan 6, 2023 •

edited

Loading

acm19 commented Jan 6, 2023

dblock left a comment •

edited

Loading

dblock Jan 6, 2023

acm19 Jan 8, 2023

dblock Jan 6, 2023

acm19 Jan 8, 2023

dblock Jan 12, 2023

dblock left a comment •

edited

Loading

dblock Jan 6, 2023

acm19 Jan 8, 2023

acm19 commented Jan 8, 2023

dblock left a comment •

edited

Loading

dblock Jan 12, 2023

dblock commented Jan 24, 2023

		@@ -1,5 +1,7 @@
		### 2.2.1 (Next)

		* [#90](https://github.com/acm19/aws-request-signing-apache-interceptor/pull/90): Add support for Serverless OpeanSearch - [@acm19](https://github.com/acm19).

Add support for Serverless OpenSearch signing #90

Add support for Serverless OpenSearch signing #90

Conversation

acm19 commented Jan 6, 2023 • edited Loading

Pull Request Checklist:

acm19 commented Jan 6, 2023

dblock left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acm19 commented Jan 8, 2023

dblock left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Jan 24, 2023

acm19 commented Jan 6, 2023 •

edited

Loading

dblock left a comment •

edited

Loading

dblock left a comment •

edited

Loading

dblock left a comment •

edited

Loading