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

amazonka: fixing sigv4 path encoding so that it happens once for s3 and twice for other services #812

Merged
merged 15 commits into from Sep 5, 2022

Conversation

mwu
Copy link
Contributor

@mwu mwu commented Sep 2, 2022

Should fix #529.

I did some manual smoke testing:

  • EC2 describe instances calls
  • lambda invokeFunction using all name forms (arn, partial arn, partial arn + alias, function name, name + alias)
  • S3 getObject on keys including and excluding uri reserved chars

@mwu mwu marked this pull request as ready for review September 2, 2022 19:44
Copy link
Collaborator

@endgame endgame left a comment

Choose a reason for hiding this comment

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

This is excellent work, thank you - I particularly appreciate the extensive test suite. I have a few small nits to pick but I look forward to merging this soon.

lib/amazonka-core/src/Amazonka/Data/Path.hs Outdated Show resolved Hide resolved
lib/amazonka-core/test/Test/Amazonka/Sign/V4/Base.hs Outdated Show resolved Hide resolved
lib/amazonka-core/test/Test/Amazonka/Sign/V4/Base.hs Outdated Show resolved Hide resolved
lib/amazonka/CHANGELOG.md Outdated Show resolved Hide resolved
mwu and others added 3 commits September 2, 2022 20:13
Co-authored-by: endgame <endgame@users.noreply.github.com>
Co-authored-by: endgame <endgame@users.noreply.github.com>
Co-authored-by: endgame <endgame@users.noreply.github.com>
@mwu mwu requested a review from endgame September 3, 2022 03:35
@mwu mwu requested a review from endgame September 4, 2022 03:54
Copy link
Collaborator

@endgame endgame left a comment

Choose a reason for hiding this comment

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

All right, so once you get your secondary PR merged I think this is good to go. Would you mind firing off your manual smoke-test requests one last time, too, just to make sure the tweaks are all good?

Remove metaCanonicalPath from V4 and test directly against escapePath…
@mwu
Copy link
Contributor Author

mwu commented Sep 4, 2022

All right, so once you get your secondary PR merged I think this is good to go. Would you mind firing off your manual smoke-test requests one last time, too, just to make sure the tweaks are all good?

Ok. These all returned success:

  • Lambda InvokeFunction with full arn, partial arn, full and partial arn with alias suffix, full and partial arn with alias in qualifier, function name, name with alias suffix, name with alias qualifier.
  • S3 GetObject with keys test/myron/test, test//myron//test', abc:def$1.foo`
  • Some other requests with non-empty paths: APIGateway GetStages, Route53 GetHostedZoneLimit
  • A few of the examples in the examples project

Copy link
Collaborator

@endgame endgame left a comment

Choose a reason for hiding this comment

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

Excellent, thank you for being so thorough. One more nit, and one last question: I think I have misunderstood the purpose of the various metaCanonicalFoo in the metadata record - it seems like they don't get used anywhere because they get built and immediately passed to signRequest. I think you pointed this out earlier? When I asked you to remove it, I didn't realise there was an established practice of keeping this data around. My question is - do you think we should reinstate metaCanonicalPath? I think your tests are fine as they are.

lib/amazonka-core/test/Test/Amazonka/Sign/V4/Base.hs Outdated Show resolved Hide resolved
Co-authored-by: endgame <endgame@users.noreply.github.com>
@mwu
Copy link
Contributor Author

mwu commented Sep 5, 2022

Excellent, thank you for being so thorough. One more nit, and one last question: I think I have misunderstood the purpose of the various metaCanonicalFoo in the metadata record - it seems like they don't get used anywhere because they get built and immediately passed to signRequest. I think you pointed this out earlier? When I asked you to remove it, I didn't realise there was an established practice of keeping this data around. My question is - do you think we should reinstate metaCanonicalPath? I think your tests are fine as they are.

I think it's fine as it is, too. The canonical path gets logged already as part of the canonical request and I can't see any other use for returning it in the metadata. Carrying it around is only useful for testing but as you said, we should really approach that by expanding the test suite to test the whole signature.

Copy link
Collaborator

@endgame endgame left a comment

Choose a reason for hiding this comment

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

All right then, sounds good. Thanks very much for this.

@endgame endgame merged commit 9bdcef5 into brendanhay:main Sep 5, 2022
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.

SignV4 Canonical URI paths are only encoded once. Should be twice
2 participants