Skip to content
New issue

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

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

Already on GitHub? # to your account

Regression in credential provider force refresh starting in 3.667, now affects lambda runners in us-east-1 #6960

Open
4 tasks done
RossWilliams opened this issue Mar 22, 2025 · 10 comments
Assignees
Labels
bug This issue is a bug. closing-soon This issue will automatically close in 4 days unless further comments are made. p0 This issue is the highest priority

Comments

@RossWilliams
Copy link
Contributor

RossWilliams commented Mar 22, 2025

Checkboxes for prior research

Describe the bug

A change in 3.667, PR #6546 , accidentally changed behaviour of the credential provider. This change is now in the lambda runner in US-East-1, causing production issues for users not bundling the SDK, or for users that bundle version 3.667 or later.

This line invokes the customer-supplied credential provider, but does not pass through the credential arguments, causing any arguments supplied by the user to be dropped.

A fix for the issue is to pass through the parameters.

A specific parameter that gets dropped in my case is the forceRefresh parameter, which is used in a more complex scenario to provide tenant-scoped credentials.

Regression Issue

  • Select this option if this issue appears to be a regression.

SDK version number

@aws-sdk/core@3.667

Which JavaScript Runtime is this issue in?

Node.js

Details of the browser/Node.js/ReactNative version

v22.13.0

Reproduction Steps

Minimal reproduction which passes in 3.666 and fails in 3.667 (and later)

import { DynamoDBClient, ListTablesCommand } from "@aws-sdk/client-dynamodb";
import { assert } from "console";

let client = null;
async function getClient() {
  if (!client) {
    client = new DynamoDBClient({
      credentials: credentialsProvider(),
      region: "us-east-1",
    });
  }
  await client.config.credentials({ forceRefresh: true });
  return client;
}

let counter = 0;
function credentialsProvider() {
  return async function () {
    counter += 1;
    console.log("Getting credentials");
    return {
      accessKeyId: process.env.AWS_ACCESS_KEY_ID,
      secretAccessKey: process.env.AWS_SECRET_ACCESS_KEY,
      sessionToken: process.env.AWS_SESSION_TOKEN,
      expiration: new Date(Date.now() + 3600 * 1000), // 1 hour
    };
  };
}

for (let i = 0; i < 2; i++) {
  let client = await getClient();
  console.log(`Got Client: ${i + 1}`);
  client.send(new ListTablesCommand({}));
  console.log(`Got result:  ${i + 1}`);
}

assert(counter === 2, "Counter should be 2");

package.json

{
  "name": "aws-sdk-regression",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "keywords": [],
  "author": "",
  "license": "ISC",
  "type": "module",
  "dependencies": {
    "@aws-sdk/client-dynamodb": "3.667",
    "@aws-sdk/types": "3.664"
  },
  "devDependencies": {
    "@types/node": "^22.13.11"
  },
  "pnpm": {
    "overrides": {
      "@aws-sdk/core": "3.666"
    }
  }
}

Observed Behavior

Credentials fetched once

Expected Behavior

Credentials fetched twice

Possible Solution

credentials: isUserSupplied
      ? async (...parameters/*<-- added parameters param*/) => 
          normalizedCreds!(...parameters/* <-- added parameters param*/).then((creds: AttributedAwsCredentialIdentity) =>
            setCredentialFeature(creds, "CREDENTIALS_CODE", "e")
          )
      : normalizedCreds!,

Additional Information/Context

The PR that introduced this change was a chore PR to introduce logging, I believe the change was inadvertent.

@RossWilliams RossWilliams added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 22, 2025
@github-actions github-actions bot added the potential-regression Marking this issue as a potential regression to be checked by team member label Mar 22, 2025
RossWilliams added a commit to RossWilliams/aws-sdk-js-v3 that referenced this issue Mar 22, 2025
@kuhe
Copy link
Contributor

kuhe commented Mar 22, 2025

The credentials provider function given to AWS SDK clients should not have parameters.
If you need to call it with parameters, you should retain a reference to the original function rather than accessing it via the client.config object.

Also, interacting with the client.config object directly after client instantiation is discouraged, since it has been transformed.


I think our documentation is not clear enough about this, so we need to fix that.

I will look into incorporating your PR's fix. It may be more complex than it appears, and will take significant time, both to merge it and then to get the change onto AWS Lambda.

@kuhe kuhe self-assigned this Mar 22, 2025
@kuhe kuhe added p2 This is a standard priority issue and removed needs-triage This issue or PR still needs to be triaged. labels Mar 22, 2025
@RossWilliams
Copy link
Contributor Author

RossWilliams commented Mar 22, 2025

@kuhe My example shows the credential provider given to the SDK does not have parameters, you may be mis-understanding the issue here. The issue is that the memoized provider (auto-created by the SDK) accepts a parameter object when the provider is called, and the internal code made a mistake in dropping that parameter.

I understand that today you prefer SDK users to not interact with client.config object directly, but it is part of the public API and the public documentation here shows no indication it is internal only, and its use broke due to a mistake while adding logging.

The history of the forceRefresh parameter in this PR suggests it was created for SDK users to specifically overcome a missing feature of the v2 sdk, which I had previously been using. It has no internal use. The PR's single example shows an SDK user creating a client and then accessing the config object for force a refresh. The suggestion that it was always meant to be an internal-only feature does not match my research.

I am not asking for an immediate upgrade of us-east-1 as we have worked around the issue by bundling an outdated SDK. But I am highlighting the severity of the issue and its potential to impact other customers as the SDK version is bumped in other regions (I see us-west-2 has an older version that is not impacted). As it is a breaking change to a public API, I'm requesting a higher priority in providing a fix.

RossWilliams added a commit to RossWilliams/aws-sdk-js-v3 that referenced this issue Mar 22, 2025
@kuhe
Copy link
Contributor

kuhe commented Mar 22, 2025

I looked into the history of the interface and you're right that

await client.config.credentials({
  forceRefresh: true,
});

was a public API with signature (options?: { forceRefresh?: boolean }): Promise<T>; i.e. MemoizedProvider<T>.

Around v3.465-3.485, with the introduction of SRA ID & auth (you don't need to know what that is, it's for maintainer reference) the type was loosened excessively to

/**
 * @public
 */
export interface IdentityProvider<IdentityT extends Identity> {
  (identityProperties?: Record<string, any>): Promise<IdentityT>;
}

We intend to merge your fix. I am not able to give an ETA, but we will treat it as a high priority.

@trivikr
Copy link
Member

trivikr commented Mar 24, 2025

PR #6961 was merged, and this fix will be published in https://github.com/aws/aws-sdk-js-v3/releases/tag/v3.774.0 around 12 noon Pacific today (Mon March 24th).

@atresca-growie

This comment has been minimized.

@trivikr

This comment has been minimized.

@trivikr trivikr removed the potential-regression Marking this issue as a potential regression to be checked by team member label Mar 26, 2025
@kuhe

This comment has been minimized.

@atresca-growie

This comment has been minimized.

@trivikr

This comment has been minimized.

@aBurmeseDev
Copy link
Contributor

Hi @RossWilliams - checking in with you to confirm the fix that was released in v3.774.0. Please let us know if anything before closing the issue.

@aBurmeseDev aBurmeseDev added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Apr 1, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug This issue is a bug. closing-soon This issue will automatically close in 4 days unless further comments are made. p0 This issue is the highest priority
Projects
None yet
Development

No branches or pull requests

5 participants