8000 fix S3 pre-signed logic for IGNORED_SIGV4_HEADERS by bentsku · Pull Request #9609 · localstack/localstack · GitHub
[go: up one dir, main page]

Skip to content

Conversation

@bentsku
Copy link
Contributor
@bentsku bentsku commented Nov 12, 2023

Motivation

Following up on #9603, I've quickly investigated the issue for the pre-signed URL message showing a non valid signature for a very simple request and quickly found the issue related to signed headers being in the ignore list.

Changes

  • fix the logic when ignoring SigV4 headers, we should still include them if they're in the SignedHeaders, and only ignore if they're missing
  • add a test validating this behavior and copying the Go SDK behavior with the help of registering handlers. This was extremely tricky to force boto to behave this way, as the AWS CRT library does not allow you to do so. The Go SDK seems a bit weird, but it allowed us to verify the behavior of previously thought "ignored" headers.

@bentsku bentsku added aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases labels Nov 12, 2023
@bentsku bentsku added this to the 3.0 milestone Nov 12, 2023
@bentsku bentsku self-assigned this Nov 12, 2023
@coveralls
Copy link
coveralls commented Nov 12, 2023

Coverage Status

coverage: 84.003% (+0.01%) from 83.992%
when pulling 5297618 on fix-s3-presign-content-sha
into 6adf6da on master.

@github-actions
Copy link
github-actions bot commented Nov 12, 2023

LocalStack Community integration with Pro

       2 files         2 suites   1h 5m 13s ⏱️
2 314 tests 2 015 ✔️ 299 💤 0
2 315 runs  2 015 ✔️ 300 💤 0

Results for commit 5297618.

♻️ This comment has been updated with latest results.

@bentsku bentsku force-pushed the fix-s3-presign-content-sha branch 3 times, most recently from 0f4221c to e6aba66 Compare November 14, 2023 14:58
@bentsku bentsku requested a review from steffyP November 14, 2023 17:43
@bentsku bentsku marked this pull request as ready for review November 14, 2023 17:43
@bentsku bentsku requested a review from macnev2013 as a code owner November 14, 2023 17:43
Comment on lines -69 to -76
ALLOWED_QUERY_PARAMS = [
"x-id",
"x-amz-user-agent",
"x-amz-content-sha256",
"versionid",
"uploadid",
"partnumber",
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was not used anywhere

Copy link
Member
@steffyP steffyP left a comment

Choose a reason for hiding this comment

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

Great work, @bentsku! 🙇‍♀️ 🚀

Love that the test is even aws validated ❤️ and very smart test setup with the meta-events 😄

This is already a very specific case for S3, the new provider is truely amazing 🚀 👏

Just added a few NITs, but nothing blocking the merge!

@bentsku bentsku force-pushed the fix-s3-presign-content-sha branch from 9d278f4 to 5297618 Compare November 14, 2023 21:20
@bentsku bentsku merged commit 46f056d into master Nov 14, 2023
@bentsku bentsku deleted the fix-s3-presign-content-sha branch November 14, 2023 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

0