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

[FEAT]: AWS SDK Credential Provider Chain Not Following Standard Order in Bedrock Integration #2588

Open
dannysteenman opened this issue Nov 5, 2024 · 5 comments · May be fixed by #2632
Open
Labels
enhancement New feature or request feature request

Comments

@dannysteenman
Copy link

dannysteenman commented Nov 5, 2024

What would you like to see?

The recent implementation of temporary credentials support in PR #2554 (fixing #2299) doesn't fully align with AWS SDK's standard credential provider chain behavior.

Current Behavior

The implementation appears to prioritize explicit credentials (access key, secret key, session token) over the standard AWS credential provider chain.

Expected Behavior

The AWS SDK should follow the standard credential provider chain where:

  1. The SDK first attempts to use credentials from the container/instance profile (ECS task role, EC2 instance profile)
  2. Only if that fails, it should fall back to other credential sources in the defined order:
    • Environment variables
    • Shared credentials file
    • Explicitly provided credentials

This is particularly important for containerized environments (like ECS) where best practice is to use task role credentials rather than long-term or even temporary explicit credentials.

Benefits of Following Standard Chain

  1. Better security by defaulting to short-lived credentials from container/instance roles
  2. Automatic credential rotation
  3. Follows AWS security best practices
  4. Consistent with how other AWS SDKs behave
  5. No need to manually manage credentials in most AWS deployment scenarios

Proposed Solution

Update the Bedrock client initialization to:

  1. First create the client without explicit credentials
  2. Only fall back to explicit credentials if the automatic credential resolution fails
  3. Maintain backward compatibility for cases where explicit credentials are needed
@dannysteenman dannysteenman added enhancement New feature or request feature request labels Nov 5, 2024
@timothycarambat
Copy link
Member

timothycarambat commented Nov 5, 2024

The SDK first attempts to use credentials from the container/instance profile (ECS task role, EC2 instance profile)

Excerpt from @langchain/aws/dist/chat_models.d.ts#L39-L42

/**
     * AWS Credentials. If no credentials are provided, the default credentials from
     * `@aws-sdk/credential-provider-node` will be used.
     */
    credentials?: CredentialType;

Fallback to this is dependency behavior leads to @aws-sdk/credential-provider-node invocation

This module provides a factory function, defaultProvider, that will attempt to source AWS credentials from a Node.JS environment. It will attempt to find credentials from the following sources (listed in order of precedence):

- Environment variables exposed via process.env
- SSO credentials from token cache
- Web identity token credentials
- Shared credentials and config ini files
- The EC2/ECS Instance Metadata Service

The default credential provider will invoke one provider at a time and only continue to the next if no credentials have been located. For example, if the process finds values defined via the AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY environment variables, the files at ~/.aws/credentials and ~/.aws/config will not be read, nor will any messages be sent to the Instance Metadata Service.

However, our implementation of ChatBedrock always provides credentials

credentials: {
accessKeyId: process.env.AWS_BEDROCK_LLM_ACCESS_KEY_ID,
secretAccessKey: process.env.AWS_BEDROCK_LLM_ACCESS_KEY,
...(this.authMethod === "sessionToken"
? { sessionToken: process.env.AWS_BEDROCK_LLM_SESSION_TOKEN }
: {}),
},

And since the only way to invoke that client loader function is preceded by these statements

if (!process.env.AWS_BEDROCK_LLM_ACCESS_KEY_ID)
throw new Error("No AWS Bedrock LLM profile id was set.");
if (!process.env.AWS_BEDROCK_LLM_ACCESS_KEY)
throw new Error("No AWS Bedrock LLM access key was set.");
if (!process.env.AWS_BEDROCK_LLM_REGION)
throw new Error("No AWS Bedrock LLM region was set.");
if (
process.env.AWS_BEDROCK_LLM_CONNECTION_METHOD === "sessionToken" &&
!process.env.AWS_BEDROCK_LLM_SESSION_TOKEN
)
throw new Error(
"No AWS Bedrock LLM session token was set while using session token as the authentication method."
);

This would enable the client on the invocation to always be passed credentials explicitly and the langchain dependency fallback behavior would not be invoked. This same principle/guard is implemented in @agent as well to prevent inadvertent loading of config or credentials that may be adjacent to the environment.

That being said, I do think it is worth addressing more explicitly in the code, but accidental credential loading should be mitigated. If this however is not the case in reality then it requires addressing as a bug

@MounirHader
Copy link

I agree with @dannysteenman, when the app is running on ECS I would expect the implementation to use the container task role instead of having to pass credentials to AnythingLLM explicitly somehow.

@dannysteenman
Copy link
Author

dannysteenman commented Nov 13, 2024

The SDK first attempts to use credentials from the container/instance profile (ECS task role, EC2 instance profile)

Excerpt from @langchain/aws/dist/chat_models.d.ts#L39-L42

/**
     * AWS Credentials. If no credentials are provided, the default credentials from
     * `@aws-sdk/credential-provider-node` will be used.
     */
    credentials?: CredentialType;

Fallback to this is dependency behavior leads to @aws-sdk/credential-provider-node invocation

This module provides a factory function, defaultProvider, that will attempt to source AWS credentials from a Node.JS environment. It will attempt to find credentials from the following sources (listed in order of precedence):

- Environment variables exposed via process.env
- SSO credentials from token cache
- Web identity token credentials
- Shared credentials and config ini files
- The EC2/ECS Instance Metadata Service

The default credential provider will invoke one provider at a time and only continue to the next if no credentials have been located. For example, if the process finds values defined via the AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY environment variables, the files at ~/.aws/credentials and ~/.aws/config will not be read, nor will any messages be sent to the Instance Metadata Service.

However, our implementation of ChatBedrock always provides credentials

credentials: {
accessKeyId: process.env.AWS_BEDROCK_LLM_ACCESS_KEY_ID,
secretAccessKey: process.env.AWS_BEDROCK_LLM_ACCESS_KEY,
...(this.authMethod === "sessionToken"
? { sessionToken: process.env.AWS_BEDROCK_LLM_SESSION_TOKEN }
: {}),
},

And since the only way to invoke that client loader function is preceded by these statements

if (!process.env.AWS_BEDROCK_LLM_ACCESS_KEY_ID)
throw new Error("No AWS Bedrock LLM profile id was set.");
if (!process.env.AWS_BEDROCK_LLM_ACCESS_KEY)
throw new Error("No AWS Bedrock LLM access key was set.");
if (!process.env.AWS_BEDROCK_LLM_REGION)
throw new Error("No AWS Bedrock LLM region was set.");
if (
process.env.AWS_BEDROCK_LLM_CONNECTION_METHOD === "sessionToken" &&
!process.env.AWS_BEDROCK_LLM_SESSION_TOKEN
)
throw new Error(
"No AWS Bedrock LLM session token was set while using session token as the authentication method."
);

This would enable the client on the invocation to always be passed credentials explicitly and the langchain dependency fallback behavior would not be invoked. This same principle/guard is implemented in @agent as well to prevent inadvertent loading of config or credentials that may be adjacent to the environment.

That being said, I do think it is worth addressing more explicitly in the code, but accidental credential loading should be mitigated. If this however is not the case in reality then it requires addressing as a bug

Thanks for the detailed response @timothycarambat. I understand the current implementation always provides explicit credentials, which prevents the default AWS SDK credential provider chain from working as intended.

Let me clarify my proposal:

  1. Instead of requiring explicit credentials (AWS_BEDROCK_LLM_ACCESS_KEY_ID and AWS_BEDROCK_LLM_ACCESS_KEY), we should make these optional and add a new environment variable like AWS_BEDROCK_AUTH_METHOD with possible values:

    • iam-role (default) - Uses the AWS SDK default credential provider chain
    • iam-user - Uses explicit credentials
  2. When iam-role is selected:

    • Remove the credential checks from lines 25-41
    • Initialize the Bedrock client without explicit credentials, allowing the SDK to use its default provider chain
    • This would enable automatic credential rotation when using ECS task roles or EC2 instance profiles
  3. When iam-user is selected:

    • Keep the current behavior requiring explicit credentials
    • This maintains backward compatibility for users who need to use explicit credentials

This approach would:

  • Follow AWS security best practices by defaulting to managed IAM roles
  • Provide better UX by enabling automatic credential rotation
  • Still allow explicit credentials when needed
  • Make it clearer in the code what authentication method is being used

Would you be open to this approach? I'm happy to submit a PR implementing these changes if you agree.

Edit: I did a small change yesterday on our fork to test this and it worked on our own aws environment.

@timothycarambat
Copy link
Member

Im not opposed to that at all if the behavior is known to work. Since the provider is loaded as needed simply creating a new env like

AwsBedrockLLMAuthMethod: {
  envKey: "AWS_BEDROCK_LLM_AUTH_METHOD",
  checks: [(input) => ['iam_role','iam_user'].includes(input) ? null : "invalid Value"]]
}
// rest of checks

The only thing I would elect we change from the proposed above is that we should always assume the default is iam_user so that explicit credentials are used. This ensures people who already use this connector don't have a broken connection when upgrading their container image. On the UI side we can have this option be a dropdown with a hint explaining the difference

So the provider can look like

// ...

verifyAuth() {
   if(process.env.AWS_BEDROCK_LLM_AUTH_METHOD === "iam_role") { // return or check default creds? }
   // do regular checks
}

constructor(embedder = null, modelPreference = null) {
    this.verifyAuth();
    .....
  }

@dannysteenman dannysteenman linked a pull request Nov 14, 2024 that will close this issue
8 tasks
@dannysteenman
Copy link
Author

dannysteenman commented Nov 14, 2024

Thanks for the feedback, I have implemented your suggestion and kept credentials as the default if they are supplied by the user as an env var. Now I've updated the slider to include the iam role in favor of the session token since that essentially replaces that session token feature. The PR can be found over here: #2632 including screenshots of the UI change.

I've tested it on aws ecs fargate with the following policy enabled on the task role ( I think its the same policy that was mentioned in the docs):

    this.taskDefinition.addToTaskRolePolicy(
      new iam.PolicyStatement({
        effect: iam.Effect.ALLOW,
        actions: [
          'bedrock:InvokeModel',
          'bedrock:ListCustomModels',
          'bedrock:GetPrompt',
          'bedrock:GetFoundationModel',
          'bedrock:InvokeModelWithResponseStream',
          'bedrock:GetCustomModel',
          'bedrock:ListFoundationModels',
        ],
        resources: ['*'],
      }),
    );
    

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants