Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] AwsSigv4Signer does not correctly read roles from profile #614

Closed
nick-bir opened this issue Sep 25, 2023 · 5 comments · Fixed by #622
Closed

[BUG] AwsSigv4Signer does not correctly read roles from profile #614

nick-bir opened this issue Sep 25, 2023 · 5 comments · Fixed by #622
Labels
🐛 bug Something isn't working good first issue Good for newcomers

Comments

@nick-bir
Copy link
Contributor

What is the bug?

I am using AwsSigv4Signer lib to sign requests to AWS OSS cluster. I have following setup in my .credentials file:

[custom]
aws_access_key_id = KEY_ID
aws_secret_access_key = ACCESS_KEY

[default]
role_arn = arn:aws:iam::CUSTOM_ACCOUNT_ID:role/CustomAccess
source_profile = custom

I am getting error Profile default requires a role to be assumed, but no role assumption when I'm trying to connect to OSS with following options:

const options = AwsSigv4Signer({region: props.region})

As a workaround I managed to explicitly pass role assumer to the options, but no other aws-sdk v3 or even v2 client requires that, so I assume this is a bug:

const options = AwsSigv4Signer({
  region: props.region,
  getCredentials: defaultProvider({roleAssumer: getDefaultRoleAssumer()}),
});

How can one reproduce the bug?

  1. Create aws profile that requires role assumption
  2. try to use AwsSigv4Signer for any request

What is the expected behavior?

I expect that default role assumer should be used by opensearch client by default, if other assumer was not explicitly passed

What is your host/environment?

mac OS Ventura 13.5
npm 8.19.2
node v18.12.1
opensearch-client 2.3.1

@nhtruong
Copy link
Collaborator

nhtruong commented Sep 29, 2023

There's an IMPORTANT note from this article:

if you intend to acquire credentials using EKS IAM Roles for Service Accounts then you must explicitly specify a value for roleAssumerWithWebIdentity. There is a default function available in @aws-sdk/client-sts package

And the provided example also has explicit roleAssumer:

defaultProvider({
  roleAssumerWithWebIdentity: getDefaultRoleAssumerWithWebIdentity({
    // You must explicitly pass a region if you are not using us-east-1
    region: "eu-west-1"
  }),
});

So, I don't think this is a bug on the client's Sigv4 Signer's end. It's just how @aws-sdk/credential-provider-nodes behaves. I agree that it's not very convenient. You should bring it up to credential-provider-nodes./

@nick-bir
Copy link
Contributor Author

nick-bir commented Oct 2, 2023

Reading the article that you sent, I found another simple convenient and working solution:

const credentials = fromNodeProviderChain();

This does seem to pick up everything I need. It's just a bit weird that all these providers work slightly different, but yeah, it does not look like an opensearch client signer bug.

@nick-bir nick-bir closed this as completed Oct 2, 2023
@dblock
Copy link
Member

dblock commented Oct 2, 2023

@nick-bir Care to add something to the documentation for the next person? Maybe into https://github.com/opensearch-project/opensearch-js/blob/main/USER_GUIDE.md#authenticate-with-amazon-opensearch-service?

@nhtruong nhtruong reopened this Oct 2, 2023
@nhtruong nhtruong added good first issue Good for newcomers and removed untriaged labels Oct 2, 2023
@nhtruong
Copy link
Collaborator

nhtruong commented Oct 2, 2023

Reopen this issue to add this edge case to the doc.

@wbeckler wbeckler added the 🐛 bug Something isn't working label Oct 3, 2023
@nick-bir
Copy link
Contributor Author

nick-bir commented Oct 3, 2023

Added #622, please let me know if it looks good to you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants