Skip to content

Conversation

@mdkhan-tw
Copy link
Contributor

@mdkhan-tw mdkhan-tw commented Oct 29, 2025

Adding authentication setting and validation for kinesis destination. The authentication mechanism is by IAMRole and externalId where the clients will create IAM role which can be "assumed" by segment's provided IAM role and external Id.

We are only validating if the client provided role can be assumed by segment. We aren't verifying if we can send event to streams because stream configuration will be available at mapping stage.

Testing

Background

Created the below IAM role which allows that segment's IAM role can assume a role provided by clients.

arn:aws:iam::0000000000000:role/mdkhan-assume-role-for-kinesis-destination

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Principal": {
                "AWS": "arn:aws:iam::000000000000000:role/customer-s3-stage-action-destination-access"
            },
            "Action": "sts:AssumeRole",
            "Condition": {
                "StringEquals": {
                    "sts:ExternalId": "mdkhan-external-id"
                }
            }
        }
    ]
}

Test Cases

TestCase Result Response
When valid credentials are provided Successful { "ok": true }
When invalid credentials are provided Successful { "ok": false, "error": "Credentials are invalid: User: arn:aws:sts::0000000000000:assumed-role/customer-s3-stage-action-destination-access/a701dab1-f9c3-4a01-aa2c-0c15db7020a6 is not authorized to perform: sts:AssumeRole on resource: arn:aws:iam::0000000000000:role/mdkhan-assume-role-for-kinesis-destination", "fields": {}}
When empty credentials are provided Successful

Include any additional information about the testing you have completed to
ensure your changes behave as expected. For a speedy review, please check
any of the tasks you completed below during your testing.

  • Added unit tests for new functionality
  • Tested end-to-end using the local server
  • [If destination is already live] Tested for backward compatibility of destination. Note: New required fields are a breaking change.
  • [Segmenters] Tested in the staging environment
  • [Segmenters] [If applicable for this change] Tested for regression with Hadron.

@github-actions
Copy link
Contributor

New required fields detected

Warning

Your PR adds new required fields to an existing destination. Adding new required settings/mappings for a destination already in production requires updating existing customer destination configuration. Ignore this warning if this PR is for a new destination with no active customers in production.

The following required fields were added in this PR:

  • Destination: Aws Kinesis, Settings:iamRoleArn,iamExternalId

Add these new fields as optional instead and assume default values in perform or performBatch block.

@codecov
Copy link

codecov bot commented Oct 30, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.03%. Comparing base (9ed58fa) to head (551510b).
⚠️ Report is 16 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3378      +/-   ##
==========================================
+ Coverage   80.00%   80.03%   +0.03%     
==========================================
  Files        1211     1214       +3     
  Lines       22353    22387      +34     
  Branches     4411     4414       +3     
==========================================
+ Hits        17884    17918      +34     
  Misses       3689     3689              
  Partials      780      780              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@abhandage abhandage requested a review from Copilot November 4, 2025 07:59
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements IAM role assumption authentication for the AWS Kinesis destination. The changes enable secure access to AWS Kinesis streams using role-based authentication with external IDs.

  • Adds IAM role-based authentication with ARN validation
  • Implements STS role assumption with intermediary role support
  • Adds comprehensive test coverage for authentication and validation logic

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
packages/destination-actions/src/lib/AWS/utils.ts Introduces APP_AWS_REGION constant with default fallback
packages/destination-actions/src/lib/AWS/sts.ts Adds assumeRole function implementing two-step role assumption
packages/destination-actions/src/lib/AWS/__test__/index.test.ts Adds unit tests for assumeRole functionality
packages/destination-actions/src/destinations/aws-kinesis/utils.ts Implements IAM Role ARN format validation
packages/destination-actions/src/destinations/aws-kinesis/index.ts Updates authentication to use IAM roles with ARN validation
packages/destination-actions/src/destinations/aws-kinesis/generated-types.ts Updates Settings interface with new authentication fields
packages/destination-actions/src/destinations/aws-kinesis/__test__/utils.test.ts Adds comprehensive tests for ARN validation
packages/destination-actions/src/destinations/aws-kinesis/__test__/index.test.ts Adds tests for destination authentication flow


// Mock dependencies
jest.mock('@aws-sdk/client-sts')
jest.mock('uuid', () => ({ v4: jest.fn(() => 'mocked-session-id') }))
Copy link

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

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

The mock path is incorrect. The code imports from '@lukeed/uuid' but the mock is for 'uuid'. This will cause the mock to fail and the real uuid function to be called during tests. Change to: jest.mock('@lukeed/uuid', () => ({ v4: jest.fn(() => 'mocked-session-id') }))

Suggested change
jest.mock('uuid', () => ({ v4: jest.fn(() => 'mocked-session-id') }))
jest.mock('@lukeed/uuid', () => ({ v4: jest.fn(() => 'mocked-session-id') }))

Copilot uses AI. Check for mistakes.
}

const getSTSCredentials = async (roleId: string, externalId: string, region: string, credentials?: AWSCredentials) => {
const options = { credentials, region: region }
Copy link

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

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

Redundant property assignment. Since the parameter is already named region, you can use object shorthand: const options = { credentials, region }

Suggested change
const options = { credentials, region: region }
const options = { credentials, region }

Copilot uses AI. Check for mistakes.
iamExternalId: {
label: 'IAM External ID',
description:
'The external ID to use when assuming the IAM Role. Generate a secure string and treat it like a password. This is often used as an additional security measure.',
Copy link

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

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

Extra space between sentences. Should have only one space after period.

Suggested change
'The external ID to use when assuming the IAM Role. Generate a secure string and treat it like a password. This is often used as an additional security measure.',
'The external ID to use when assuming the IAM Role. Generate a secure string and treat it like a password. This is often used as an additional security measure.',

Copilot uses AI. Check for mistakes.
@mdkhan-tw mdkhan-tw merged commit ae3cd09 into main Nov 4, 2025
25 of 26 checks passed
@mdkhan-tw mdkhan-tw deleted the mz-kinesis-auth branch November 4, 2025 08:16
@joe-ayoub-segment
Copy link
Contributor

PR deployed

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants