Skip to content

StepFunctions: Ensure Service Integrations Assume State Machine Role Credentials #12089

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 2, 2025
Merged

StepFunctions: Ensure Service Integrations Assume State Machine Role Credentials #12089

merged 1 commit into from
Jan 2, 2025

Conversation

MEPalma
Copy link
Contributor

@MEPalma MEPalma commented Jan 2, 2025 •
edited
Loading

Motivation

Currently, the SFN v2 interpreter creates boto clients for service integrations using user credentials if no explicit credentials are provided. This causes IAM policy checks to succeed incorrectly event when ENFORCE_IAM=1 is set, as the checks were based on the user's permissions rather than those of the state machine's assumed role #11927. This fix ensures boto clients are created with the assumed role credentials of the state machine.

Changes

  • In Task states, Credentials values are always computed and defaulted to the state machine role arn if not manual override is given
  • Boto clients are created through assume role logic, using the role arn in the credentials
  • Related minor refactoring to improve readability (mainly around renaming ComputedCredentials -> StateCredentials)

@MEPalma MEPalma added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Jan 2, 2025
@MEPalma MEPalma added this to the 4.1 milestone Jan 2, 2025
@MEPalma MEPalma self-assigned this Jan 2, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 2, 2025

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   33m 34s ⏱️ - 1h 19m 47s
1 333 tests  - 2 571  1 269 ✅  - 2 328  64 💤  - 243  0 ❌ ±0 
1 335 runs   - 2 571  1 269 ✅  - 2 328  66 💤  - 243  0 ❌ ±0 

Results for commit c3e071a. ± Comparison against base commit 5399278.

This pull request removes 2571 tests. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

dominikschubert
dominikschubert approved these changes Jan 2, 2025
View reviewed changes
Copy link
Member

@dominikschubert dominikschubert left a comment

Choose a reason for hiding this comment

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

LGTM, great job figuring this out so quickly.

...calstack/services/stepfunctions/asl/component/state/state_execution/state_task/state_task.py
if not self.credentials:
task_credentials = dict()
state_credentials = StateCredentials(role_arn=env.aws_execution_details.role_arn)
Copy link
Member

Choose a reason for hiding this comment

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

nice 🙌

...calstack/services/stepfunctions/asl/component/state/state_execution/state_task/state_task.py
if not self.credentials:
task_credentials = dict()
state_credentials = StateCredentials(role_arn=env.aws_execution_details.role_arn)
else:
self.credentials.eval(env=env)
Copy link
Member

Choose a reason for hiding this comment

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

Could this fail? if so, what would/should be the behavior here then? 🤔

Also curious if (not here) a failed assume role would also lead to falling back to the role ARN potentially? I'd assume not, since that might be a security issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The computation of the user-defined credentials block can fail, we have some test cases to validate our handling of such failures aws.services.stepfunctions.v2.credentials.test_credentials_base.TestCredentialsBase.test_invalid_credentials_field
In the tandem PR, which adds tests for SFN IAM enforcement, I did not see this behaviour, instead the failure is simply passed upstream

@MEPalma MEPalma merged commit 41ffa76 into master Jan 2, 2025
38 checks passed
@MEPalma MEPalma deleted the MEP-sfn-assume_role_boto_client branch January 2, 2025 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dominikschubert dominikschubert dominikschubert approved these changes

@joe4dev joe4dev Awaiting requested review from joe4dev

@gregfurman gregfurman Awaiting requested review from gregfurman

Assignees

@MEPalma MEPalma

Labels
semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.
2 participants
@MEPalma @dominikschubert