acm19 / aws-request-signing-apache-interceptor

https://acm19.github.io/aws-request-signing-apache-interceptor/
Apache License 2.0
16 stars 6 forks source link

Add support for Serverless OpenSearch signing #90

Closed acm19 closed 1 year ago

acm19 commented 1 year ago

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

Requirements:

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:

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

acm19 commented 1 year ago

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

acm19 commented 1 year ago

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.

dblock commented 1 year ago

Closing in favor of #94.