Skip to content

SNS : fix Message Signature typo and add Lambda URL fixture #12181

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
Jan 29, 2025
Merged

SNS : fix Message Signature typo and add Lambda URL fixture #12181

merged 3 commits into from
Jan 29, 2025

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Jan 24, 2025 •
edited
Loading

Motivation

As reported by #12179, we were not calculating the message signature correctly for UnsubscribeConfirmation message for HTTP/HTTPS subscriptions.

I've created an StrEnum to avoid this kind of typo in the future.

I had also been talking about creating a Lambda URL fixture to allow forwarding received message to an HTTP endpoint to an SQS queue, so this was a good occasion to do it.

This can be used as example for other services as well, the fixture could probably live in the global fixture file, once we see we can reuse it, as it can be dependent on what the HTTP endpoint needs to return.
But I'm thinking of making this fixture to do more SNS related things, like being able to return different status code to enable retries and such, or auto-approve the subscription, so we could probably duplicate it in a more general way.

Changes

  • add a new lambda URL function fixture to forward input events to a queue, that allows exposing an accessible HTTP endpoint
  • fix the typo and introduce an StrEnum to avoid future typos
  • create a test to validate the behavior and the proper signature of messages for HTTP endpoints

fixes #12179

@bentsku bentsku added aws:sns Amazon Simple Notification Service semver: patch Non-breaking changes which can be included in patch releases labels Jan 24, 2025
@bentsku bentsku added this to the 4.1 milestone Jan 24, 2025
@bentsku bentsku self-assigned this Jan 24, 2025
@bentsku bentsku requested a review from baermat as a code owner January 24, 2025 18:58
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 24, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   1h 9m 59s ⏱️ - 40m 31s
2 623 tests  - 1 399  2 517 ✅  - 1 189  106 💤  - 210  0 ❌ ±0 
2 625 runs   - 1 399  2 517 ✅  - 1 189  108 💤  - 210  0 ❌ ±0 

Results for commit 4ca3a5a. ± Comparison against base commit 1790318.

This pull request removes 1446 and adds 47 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.s3.test_s3.TestS3 ‑ test_s3_copy_object_with_checksum[CRC64NVME]
tests.aws.services.s3.test_s3.TestS3MultipartUploadChecksum ‑ test_complete_multipart_parts_checksum_composite[CRC32C]
tests.aws.services.s3.test_s3.TestS3MultipartUploadChecksum ‑ test_complete_multipart_parts_checksum_composite[CRC32]
tests.aws.services.s3.test_s3.TestS3MultipartUploadChecksum ‑ test_complete_multipart_parts_checksum_composite[SHA1]
tests.aws.services.s3.test_s3.TestS3MultipartUploadChecksum ‑ test_complete_multipart_parts_checksum_composite[SHA256]
tests.aws.services.s3.test_s3.TestS3MultipartUploadChecksum ‑ test_complete_multipart_parts_checksum_default
tests.aws.services.s3.test_s3.TestS3MultipartUploadChecksum ‑ test_complete_multipart_parts_checksum_full_object[CRC32C]
tests.aws.services.s3.test_s3.TestS3MultipartUploadChecksum ‑ test_complete_multipart_parts_checksum_full_object[CRC32]
tests.aws.services.s3.test_s3.TestS3MultipartUploadChecksum ‑ test_complete_multipart_parts_checksum_full_object[CRC64NVME]
tests.aws.services.s3.test_s3.TestS3MultipartUploadChecksum ‑ test_multipart_checksum_type_compatibility[COMPOSITE-CRC32C]
…

♻️ This comment has been updated with latest results.

@bentsku bentsku requested a review from joe4dev January 27, 2025 10:46
silv-io
silv-io approved these changes Jan 29, 2025
View reviewed changes
Copy link
Member

@silv-io silv-io left a comment

Choose a reason for hiding this comment

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

LGTM! well structured test, nice fixture and good to keep a more central hold on the constant strings 👍🏼

localstack-core/localstack/services/sns/models.py Outdated
Comment on lines 42 to 44
notification = "Notification"
subscription_confirmation = "SubscriptionConfirmation"
unsubscribe_confirmation = "UnsubscribeConfirmation"
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think ASF would create such enums in the original CamelCase, but I don't think this matters a lot :D

Copy link
Contributor Author

@bentsku bentsku Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I was exactly thinking about that. Will update and merge 👍 edit: even PascalCase it seems 😄

@bentsku bentsku merged commit a3cd603 into master Jan 29, 2025
31 checks passed
@bentsku bentsku deleted the fix-sns-signature-typo branch January 29, 2025 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@silv-io silv-io silv-io approved these changes

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

@joe4dev joe4dev Awaiting requested review from joe4dev

Assignees

@bentsku bentsku

Labels
aws:sns Amazon Simple Notification Service semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.

SNS UnsubscribeConfirmation message is incorrectly signed
2 participants
@bentsku @silv-io