Skip to content

Add SQS source URL format validation and require External ID when role ARN is configured#7004

Open
Davidding4718 wants to merge 1 commit into
opensearch-project:mainfrom
Davidding4718:fix/sqs-source-security-validation
Open

Add SQS source URL format validation and require External ID when role ARN is configured#7004
Davidding4718 wants to merge 1 commit into
opensearch-project:mainfrom
Davidding4718:fix/sqs-source-security-validation

Conversation

@Davidding4718

Copy link
Copy Markdown
Contributor

Description

Add input validation to the SQS source plugin to address two security concerns:

  1. URL format validation (QueueConfig): Add @Pattern annotation to the url field to enforce that queue URLs match the canonical SQS endpoint format (https://sqs.<region>.amazonaws.com/<12-digit-account-id>/<queue-name>). This prevents arbitrary URLs from being submitted as queue configurations, mitigating potential Server-Side Request Forgery (SSRF) vectors.

  2. External ID requirement (AwsAuthenticationOptions): Add @AssertTrue validation that requires sts_external_id to be present and non-blank when sts_role_arn is configured. This mitigates the confused deputy problem where a pipeline could be configured to assume another account's IAM role without proper authorization.

Both validations include corresponding unit tests.

Issues Resolved

N/A

Check List

  • New functionality includes testing.
  • New functionality has a documentation issue. Please link to it in this PR.
    • New functionality has javadoc added
  • Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…e ARN is configured

Signed-off-by: Siqi Ding <dingdd@amazon.com>
return awsStsHeaderOverrides;
}

@AssertTrue(message = "sts_external_id is required when sts_role_arn is specified to prevent confused deputy attacks. " +

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think we need this AssertTrue

@Davidding4718

Copy link
Copy Markdown
Contributor Author

Update: Both validations (URL format and External ID requirement) are already enforced at the service layer during pipeline creation, so this PR is supplementary — it adds plugin-level checks as an extra safety net but won't affect current behavior since invalid configs are already rejected upstream. Leaving this open in case the maintainers feel it's worth including as a best practice.

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.

2 participants