Skip to content
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.

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

fix: sigv4 now correctly trims spaces #799

Merged
merged 17 commits into from
Oct 26, 2021

Conversation

Velfi
Copy link
Contributor

@Velfi Velfi commented Oct 22, 2021

add: test for space trimming
add: s3 signing integration test
add: lambda signing integration test

Motivation and Context

This will fix awslabs/aws-sdk-rust#248 and close #761

Description

Our implementation of SigV4 signing didn't implement all required behavior. This PR implements that behavior and includes tests to ensure that it keeps working.

Testing

I have written tests for the new functions and an integration test to check that bug reported for S3 metadata is fixed.

Checklist

  • I have updated CHANGELOG.md
  • I have updated aws/SDK_CHANGELOG.md if applicable

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

add: test for space trimming
add: s3 signing integration test
add: lambda signing integration test
@Velfi Velfi marked this pull request as draft October 22, 2021 17:35
@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-signing-should-correctly-trim-spaces

Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

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

Looks good so far!

aws/sdk/integration-tests/blns.txt Outdated Show resolved Hide resolved
@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-signing-should-correctly-trim-spaces

add: missing LICENSE for BLNS
@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-signing-should-correctly-trim-spaces

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-signing-should-correctly-trim-spaces

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-signing-should-correctly-trim-spaces

update: ignore naughty strings tests that require real aws connection
fix: s3 naughty strings metadata signing test
@Velfi Velfi marked this pull request as ready for review October 25, 2021 17:11
@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-signing-should-correctly-trim-spaces

remove: circular dep aws-config from integration testing crates
update: comment out tests not runnable by CI
format: run cargo fmt
@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-signing-should-correctly-trim-spaces

update: hide lambda tests from integration runner
@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-signing-should-correctly-trim-spaces

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-signing-should-correctly-trim-spaces

Copy link
Collaborator

@rcoh rcoh left a comment

Choose a reason for hiding this comment

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

LGTM! couple of questions inline

Velfi and others added 2 commits October 26, 2021 10:46
add: proptest for trim_all
update: convert trimming to work on byte slices instead of strings
update: update trimming test to use byte slices
@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-signing-should-correctly-trim-spaces

1 similar comment
@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-signing-should-correctly-trim-spaces

update: test_trim_all_ignores_other_forms_of_whitespace to be more robust
update: use indexes over iterators in an attempt to appease the optimizer
update: test_s3_signer_with_naughty_string_metadata expected signature
@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-signing-should-correctly-trim-spaces

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-signing-should-correctly-trim-spaces

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-signing-should-correctly-trim-spaces

@Velfi Velfi merged commit 2757fcb into main Oct 26, 2021
@Velfi Velfi deleted the fix/signing-should-correctly-trim-spaces branch October 26, 2021 20:10
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.

S3 signing fuzz / stress test Extra whitespace in S3 object metadata causes SignatureDoesNotMatch error
3 participants