Skip to content

Apply IAM patches when loading STS to avoid wrong access key formats #11931

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 3 commits into from
Nov 26, 2024
Merged

Apply IAM patches when loading STS to avoid wrong access key formats #11931

merged 3 commits into from
Nov 26, 2024

Conversation

dfangl
Copy link
Member

@dfangl dfangl commented Nov 26, 2024

Motivation

Currently, if we are using STS (especially the assume-role and assume-role-with-web-identity operations) without ever having loaded IAM (e.g. by referencing a non-existent role in a service like AWS Lambda), we do not have the patches applied patching the access key id format.

This leads to access key ids starting with an A and not a L, which leads to the credentials being ignored, unless PARITY_AWS_ACCESS_KEY_ID=1 is set.

IAM and STS are coupled tightly in moto, which is why the patching of IAM affects STS access key ids.

Changes

  • Prevent IAM patches from loading multiple times
  • Apply IAM patches when STS is loaded

@dfangl dfangl requested a review from pinzon as a code owner November 26, 2024 13:46
@dfangl dfangl added this to the 4.0.3 milestone Nov 26, 2024
@dfangl dfangl added the semver: patch Non-breaking changes which can be included in patch releases label Nov 26, 2024
@dfangl dfangl self-assigned this Nov 26, 2024
@dfangl dfangl requested a review from bentsku November 26, 2024 13:46
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 26, 2024 •
edited
Loading

LocalStack Community integration with Pro

  2 files  ±    0    2 suites  ±0   49m 44s ⏱️ - 1h 1m 17s
977 tests  - 2 750  828 ✅  - 2 553  149 💤  - 197  0 ❌ ±0 
979 runs   - 2 750  828 ✅  - 2 553  151 💤  - 197  0 ❌ ±0 

Results for commit fbcb484. ± Comparison against base commit 5f19fbc.

This pull request removes 2756 and adds 6 tests. Note that renamed tests count towards both. 
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]
…

tests.aws.services.lambda_.test_lambda_common.TestLambdaCallingLocalstack ‑ test_manual_endpoint_injection[nodejs22.x]
tests.aws.services.lambda_.test_lambda_common.TestLambdaRuntimesCommon ‑ test_echo_invoke[nodejs22.x]
tests.aws.services.lambda_.test_lambda_common.TestLambdaRuntimesCommon ‑ test_introspection_invoke[nodejs22.x]
tests.aws.services.lambda_.test_lambda_common.TestLambdaRuntimesCommon ‑ test_runtime_wrapper_invoke[nodejs22.x]
tests.aws.services.lambda_.test_lambda_common.TestLambdaRuntimesCommon ‑ test_uncaught_exception_invoke[nodejs22.x]
tests.aws.services.lambda_.test_lambda_runtimes.TestNodeJSRuntimes ‑ test_invoke_nodejs_es6_lambda[nodejs22.x]

♻️ This comment has been updated with latest results.

bentsku
bentsku approved these changes Nov 26, 2024
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! Thanks a lot for addressing this! 🙏 neat and safe fix! :shipit:
For additional context, this issue was raised by using ESM and DynamoDB with non default credentials, ESM calling sts.AssumeRole and getting ASIA... credentials, leading to ESM trying to get DynamoDB records in the default account. It was hard to reproduce in our tests, as we always load IAM first 😄

@dfangl dfangl merged commit e5aa33d into master Nov 26, 2024
32 checks passed
@dfangl dfangl deleted the sts/iam-patches branch November 26, 2024 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bentsku bentsku bentsku approved these changes

@pinzon pinzon Awaiting requested review from pinzon pinzon is a code owner

Assignees

@dfangl dfangl

Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.0.3
Development

Successfully merging this pull request may close these issues.
2 participants
@dfangl @bentsku