Add AwsSigV4 signing functionality

[harshavamsi]

Signed-off-by: Harsha Vamsi Kalluri harshavamsi096@gmail.com

Description

Adds the ability to sign requests natively using awsSigV4. An example is given in the README on how to sign every request.

Issues Resolved

Fixes #252

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.

[kavilla]

did we want to bump the minor version to reflect the new feature? https://github.com/opensearch-project/opensearch-js/blob/main/package.json#L14

[kavilla]

This is so awesome! Thank you so much.

Might need to ping docs team to prepare themselves and then release this.

[kavilla]

nit: is Response standard? If it is I'm cool with it but for me feels like Response for something that is happening pre-flight feels off

[kavilla]

Comment for nits and questions.

[dblock]

This is a great start! Haven't looked at the detailed implementation yet. Consider #187 (comment) and other comments in that issue.

My biggest ask is to refactor this in some kind of optional, namespaced module. This is vendor-specific code and thus I don't think it should be included by default in everybody's client usage. WDYT?

[dblock]

This is identical to the samples above? Maybe we should break up the examples to specific sections (indexing, searching, AWS Sigv4 Signing...?

[harshavamsi]

The reason why it lives in a separate code block is just because of the difference in the imports and the way the client is initialized. Because the signer needs credentials, it's an async call. Felt like maybe the two should be written separately.

[harshavamsi]

did we want to bump the minor version to reflect the new feature? https://github.com/opensearch-project/opensearch-js/blob/main/package.json#L14

My assumption was that this would go as a 2.x release instead of a 2.0.x? I could be wrong.

Does it make sense to bump version here or a separate commit before the release?

[harshavamsi]

This is a great start! Haven't looked at the detailed implementation yet. Consider #187 (comment) and other comments in that issue.

My biggest ask is to refactor this in some kind of optional, namespaced module. This is vendor-specific code and thus I don't think it should be included by default in everybody's client usage. WDYT?

I agree with you. I've separated out the signer from the main injection point and users will have to require it explicitly to use it.

[dblock]

did we want to bump the minor version to reflect the new feature? https://github.com/opensearch-project/opensearch-js/blob/main/package.json#L14

My assumption was that this would go as a 2.x release instead of a 2.0.x? I could be wrong.

Yes, if there are no breaking changes and only new features, this is 2.1 per semver. After release we should have incremented the patch version to 2.0.1. So please bump the version in package.json to 2.1.0 as part of this PR?

Does it make sense to bump version here or a separate commit before the release?

Here.

[dblock]

Excellent. I have some asks re:naming of classes/namespaces. Let's get that right since these are hard to change later?

Also license checker is complaining, I didn't look at the errors, needs to be 🍏.

[dblock]

I think I'd want it to match above (must have) and be namespaced under aws if possible (not sure that's the right thing to do here).

const { AwsSigv4Signer } = require ('@opensearch-project/opensearch/aws')

WDYT?

[harshavamsi]

This sounds right. I fixed the namespace to allow this kind of import.

[dblock]

So technically this is "AwsSigv4". I think we should align with other implementations such as Aws::Sigv4::Signer in Java and include "sig" and lowercase the "v", so AwsSigv4Signer...?

[harshavamsi]

Yeah, let's have them be consistent. I renamed everything.

[harshavamsi]

Excellent. I have some asks re:naming of classes/namespaces. Let's get that right since these are hard to change later?

Also license checker is complaining, I didn't look at the errors, needs to be 🍏.

Excellent. I have some asks re:naming of classes/namespaces. Let's get that right since these are hard to change later?

Also license checker is complaining, I didn't look at the errors, needs to be 🍏.

@dblock looks like @aws-sdk/credential-provider-node has tslib dependency. This is the error that the license checker throws: "tslib@1.14.1" is licensed under "0BSD" which is not permitted by the --onlyAllow flag

Not sure what the course of action here is.

[VachaShah]

All of the examples here look very similar to the already existing ones. I see that initializing the client is different, can we provide may be just one example here so we don't duplicate all the test data?

Also, consider moving this to a USER_GUIDE since the number of examples is increasing, might make the README very long.

[dblock]

+1, but I also think we can break this up in this PR or do it in another PR, so no hard ask from me. FYI I do like what they have done in the .NET client: https://github.com/opensearch-project/opensearch-net/blob/main/USER_GUIDE.md

[harshavamsi]

@VachaShah @dblock I've split the code snippets into a new USER_GUIDE. wdyt?

[dblock]

@dblock looks like @aws-sdk/credential-provider-node has tslib dependency. This is the error that the license checker throws: "tslib@1.14.1" is licensed under "0BSD" which is not permitted by the --onlyAllow flag

Not sure what the course of action here is.

Add it to the list of approved licenses in package.json in this PR, while I go check with someone paid to say whether this is A-OK or not.

[dblock]

So does this become a hard dependency for this package? Meaning if I am not doing AWS sigv4, am I still dragging this module in? Can it be avoided? Asking again because I'd like not to drag any vendor specific components in by default.

[harshavamsi]

Removed this dependency like I mentioned below. We are still depending on the aws4 library which we cannot do away(unless we want to re-write all of that implementation) if we want to support signing natively in the client.

[harshavamsi]

@dblock looks like @aws-sdk/credential-provider-node has tslib dependency. This is the error that the license checker throws: "tslib@1.14.1" is licensed under "0BSD" which is not permitted by the --onlyAllow flag
Not sure what the course of action here is.

Add it to the list of approved licenses in package.json in this PR, while I go check with someone paid to say whether this is A-OK or not.

I just realized that we do not need that package. A user should just get the credentials from his machine and pass them in as arguments. We don't need to deal with that license now.

[dblock]

I still have the question on whether we are now requiring vendor-specific aws4 for all scenarios and whether there's a way to avoid that? I've opened #288.

+Some nits.

[dblock]

Nit: To authenticate with the Amazon OpenSearch Service use AwsSigv4Signer.

[dblock]

Pick one or the other in the headings?

a) "Create an Index", "Add a Document", "Delete ..."
b) "Creating an Index", "Adding a Document", "Deleting ..."

[dblock]

@harshavamsi Looks like you'll also need to rebase/resolve conflicts in README. I think this is almost done!

[rawpixel-vincent]

@dblock @harshavamsi good for me 🎉 thank you for allowing me to contribute on this!
I'll open an issue and PR on https://github.com/yosefbs/aws-opensearch-connector to add a deprecation notice on the Readme and forward to the AwsSigV4 docs here once it's released.

[dblock]

I have some more asks around tests (I think matching file names is a must have), easy stuff.

[dblock]

What's the default? Do I need to always implement getCredentials()?

[rawpixel-vincent]

yes there is no default, getCredentials is required.
if not given an error is thrown, it's one of the test case.
I think the wording above make that clear, but it seems not, how can we update it to make it clear that getCredentials() is required?

[dblock]

Maybe we can provide a default version?

[rawpixel-vincent]

I'm not sure what you mean, those two examples are not meant to be copy/pasted, although they would work in the majority of case if people do copy/paste them.
I've tried to make it clear in the comment above how getCredentials() should be implemented.
I think it is good enough, although as discussed we can make it more clear by splitting the code example for v2 and v3.

[rawpixel-vincent]

okay I got you, in my opinion giving a default is not the best idea as this is the kind of thing that should be thought out, and it could lead to confusion for consumer using different version of the sdk or just expecting this library to handle the credentials the way they think it should.
a default getCredentials() method could look very different between implementations and there would be good arguments for every implementation as why it should be the default.

[rawpixel-vincent]

that would also mean adding aws-sdk as a peer-dependency / optional-dependency or something like that.. I think it's best to leave it to the users that implement it.

[harshavamsi]

I agree with @rawpixel-vincent . This allows the user to use the library and implementation of his choice. Looking at community driven implementations of sigv4 there are users using both types of implementations and aws sdk versions.

[dblock]

< or <=?

[rawpixel-vincent]

I think it's better to be strict in that case for the following reasons:

• previous library would renew the credentials on every requests as now we are refreshing the credentials only when needed
• this condition will probably never been triggered, issue is with the AWS.Credentials object definition that gives 3 different way to test if a Credentials object is expired
• mainly because under heavy load it's possible that between the time we check the credentials expiration and the time the request is made the Credentials would be expired, so although that condition seems strict, it isn't.

I think it's good like this, but if it's blocker, maybe we can think of exposing an option that gives that choice to the user

something like:

new Client(
  ...AwsSigV4({
    ...,
    timeMarginBeforeRenewalMs: 1000 * 60
  })
)
// then that condition would be something like
else if (currentCredentials.expireTime && currentCredentials.expireTime < new Date(Date.now() - (opts.timeMarginBeforeRenewalMs || 0)) {

fwiw, I'm good with the current implementation until proven it's an issue, some feedback from users would help to find out.

[rawpixel-vincent]

I don't think we should bother with this unless we have some feedback from aws sdk V2 users.
From the aws docs this condition will not be triggered. But because the Credentials object gives those 3 props, for the sake of following the specs it makes sense to test all possible properties.

      // AWS SDK V2, needsRefresh should be available.
      else if (typeof currentCredentials.needsRefresh === 'function') {
        expired = currentCredentials.needsRefresh();
      }
      // AWS SDK V2, alternative to needsRefresh.
      else if (currentCredentials.expired === true) {
        expired = true;
      }
      // AWS SDK V2, alternative to needsRefresh and expired.
      else if (currentCredentials.expireTime && currentCredentials.expireTime < new Date()) {
        expired = true;
      }

If the Credentials object is consistent in aws sdk v2, Credentials.needsRefresh() will always be used.

that point you made and my answer is still relevant for aws sdk v3, in v3 the Credentials object expose only Credentials.expiration to test if the credentials are expired.
With a Credentials object from aws sdk V3, this is the only condition that will execute

       // AWS SDK V3, Credentials.expiration is a Date object
      else if (currentCredentials.expiration && currentCredentials.expiration < new Date()) {
        expired = true;
      }

fwiw, this is the full block that check if the credentials have expired (or haven't been acquired yet)

      const currentCredentials = credentialsState.credentials;
      let expired = false;
      if (!currentCredentials) {
        // Credentials haven't been acquired yet.
        expired = true;
      }
      // AWS SDK V2, needsRefresh should be available.
      else if (typeof currentCredentials.needsRefresh === 'function') {
        expired = currentCredentials.needsRefresh();
      }
      // AWS SDK V2, alternative to needsRefresh.
      else if (currentCredentials.expired === true) {
        expired = true;
      }
      // AWS SDK V2, alternative to needsRefresh and expired.
      else if (currentCredentials.expireTime && currentCredentials.expireTime < new Date()) {
        expired = true;
      }
      // AWS SDK V3, Credentials.expiration is a Date object
      else if (currentCredentials.expiration && currentCredentials.expiration < new Date()) {
        expired = true;
      }

[harshavamsi]

This explanation looks good to me. Looks like the built-in functions from the library can handle this.

[dblock]

👍

[rawpixel-vincent]

@harshavamsi I wait for your feedback on the review from @dblock and then let me know if you want to address it or leave it to me 🙏🏻

[harshavamsi]

@rawpixel-vincent @dblock I've made changes where you've requested them. I've addressed the comments as well.

[dblock]

I merged this, great work!

[dblock]

I opened #290 for a possible improvement.

[dblock]

@dblock looks like @aws-sdk/credential-provider-node has tslib dependency. This is the error that the license checker throws: "tslib@1.14.1" is licensed under "0BSD" which is not permitted by the --onlyAllow flag
Not sure what the course of action here is.

Add it to the list of approved licenses in package.json in this PR, while I go check with someone paid to say whether this is A-OK or not.

I just realized that we do not need that package. A user should just get the credentials from his machine and pass them in as arguments. We don't need to deal with that license now.

For the record, I got a confirmation that 0BSD was OK if we ever need it.

[rawpixel-vincent]

@harshavamsi @dblock the tests are not running after they been moved to lib/aws see #294

[opensearch-trigger-bot]

The backport to 1.x failed:

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

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-1.x 1.x
# Navigate to the new working tree
cd .worktrees/backport-1.x
# Create a new branch
git switch --create backport/backport-279-to-1.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 70a26d36c9e0111753ba186f3aae34b1d2c3e712
# Push it to GitHub
git push --set-upstream origin backport/backport-279-to-1.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.x

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