Skip to content

Conversation

narengogi
Copy link
Collaborator

closes #1294

Currently there are two types of supported authentication for bedrock in portkey

  1. assumed role
  2. api secret and key id

This PR adds support for Bearer token based auth

Testing done:
Tested with combinations of assumed role, api secret and Bearer
precedence is for assumed role and api secret as we dont want to disturb existing deployments

@narengogi narengogi requested review from VisargD and b4s36t4 August 21, 2025 14:43
Copy link
Contributor

matter-code-review bot commented Aug 21, 2025

Code Quality security maintainability new feature

Summary By MatterAI MatterAI logo

🔄 What Changed

Added support for Bearer token-based authentication in Bedrock by introducing awsAuthType from providerOptions in src/providers/bedrock/api.ts. The logic now conditionally invokes providerAssumedRoleCredentials only when awsAuthType === 'assumedRole', avoiding redundant calls. This enhances flexibility by allowing API key-based auth while preserving assumed role precedence.

🔍 Impact of the Change

Enables simpler, token-based authentication for Bedrock without disrupting existing AWS IAM workflows. Critical for users seeking lightweight integration without full IAM setup. Fixes a logic duplication issue where providerAssumedRoleCredentials was called twice under the same condition.

📁 Total Files Changed

  • src/providers/bedrock/api.ts: Introduced awsAuthType check; removed duplicate credential call; streamlined auth flow.

🧪 Test Added

Manual testing confirmed correct behavior across combinations: assumed role, API secret, and Bearer token. Precedence rules validated to ensure backward compatibility and secure fallback.

🔒Security Vulnerabilities

N/A

Tip

Quality Recommendations

  1. Add explicit input validation for awsAuthType to prevent invalid string values

  2. Remove duplicated if-block for awsAuthType === 'assumedRole' to eliminate redundant checks

  3. Add logging to trace authentication method selection for observability

Tanka Poem ♫

Code flows with new key,
Bearer tokens now take the stage,
Cleaner paths emerge.
No more dual calls—elegant,
Auth evolves, light and precise. 🌿🔐

Sequence Diagram

sequenceDiagram
    participant Client
    participant BedrockAPI
    participant AuthUtil

    Client->>BedrockAPI: invoke(fn, headers, providerOptions)
    BedrockAPI->>BedrockAPI: extract awsAuthType from providerOptions
    
    alt awsAuthType is 'assumedRole'
        BedrockAPI->>AuthUtil: await providerAssumedRoleCredentials(c, providerOptions)
    end

    BedrockAPI->>BedrockAPI: setRouteSpecificHeaders(fn, headers, providerOptions)
    BedrockAPI-->>Client: proceed with request
Loading

Copy link
Contributor

@matter-code-review matter-code-review bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added bearer token authentication support for Bedrock API. Code looks clean with proper early return pattern.

Copy link
Contributor

Important

PR Review Skipped

PR review skipped as per the configuration setting. Run a manually review by commenting /matter review

💡Tips to use Matter AI

Command List

  • /matter summary: Generate AI Summary for the PR
  • /matter review: Generate AI Reviews for the latest commit in the PR
  • /matter review-full: Generate AI Reviews for the complete PR
  • /matter release-notes: Generate AI release-notes for the PR
  • /matter : Chat with your PR with Matter AI Agent
  • /matter remember : Generate AI memories for the PR
  • /matter explain: Get an explanation of the PR
  • /matter help: Show the list of available commands and documentation
  • Need help? Join our Discord server: https://discord.gg/fJU5DvanU3

delete headers['content-type'];
}

if (isBearerTokenBasedAuth(providerOptions)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use awsAuthType to identify instead of deriving it from other details to keep the implementation aligned with how other providers support various auth modes.

@narengogi narengogi force-pushed the chore/bedrock-api-key-support branch from fbc0776 to 87a4323 Compare October 15, 2025 09:46
Copy link
Contributor

@matter-code-review matter-code-review bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant authentication logic and missing validation for API key based auth

Comment on lines +160 to +162
if (awsAuthType === 'assumedRole') {
await providerAssumedRoleCredentials(c, providerOptions);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 Bug

Issue: The if (awsAuthType === 'assumedRole') check is duplicated, causing providerAssumedRoleCredentials to be called twice when awsAuthType is 'assumedRole'. This is redundant and could lead to unexpected behavior or performance issues.

Fix: Remove the first duplicated if block (lines L160-L162) to ensure providerAssumedRoleCredentials is called only once.

Impact: Improves code clarity, reduces redundancy, and prevents potential double execution of credential provisioning.

Suggested change
if (awsAuthType === 'assumedRole') {
await providerAssumedRoleCredentials(c, providerOptions);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Support API key based auth for bedrock

2 participants