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

ISSUE #240: Fix signing issues for endpoints with a path component #242

Merged
merged 1 commit into from Sep 4, 2015

Conversation

Projects
None yet
2 participants
@rafd123
Copy link
Contributor

commented Sep 2, 2015

In order to properly preserve the relationship between IRequest.Endpoint and IRequest.ResourcePath, have, AWS3Signer and AWS4Signer canonicalize the URL returned by AmazonServiceClient.ComposeUrl()

@steveataws

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2015

Thanks for the PR. We're running it through our integration test suite and will follow up later.

@rafd123

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2015

Thanks, @steveataws

I have to admit, going into this, I didn't fully understand the intent of IRequest.ResourcePath as it relates to IRequest.Endpoint.

The closer I look at the code, I'm wondering if the signers should be using AmazonServiceClient.ComposeUrl() as a means to resolve the URL that ultimately gets canonicalized.

Do you agree? If so, the PR should be revised.

@rafd123 rafd123 force-pushed the rafd123:bug/issue-204 branch from 84e14cb to 247fac9 Sep 2, 2015

@steveataws

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2015

Hi,

When I ran the integration tests we had a couple of fails in a test which validate some S3 keys we've had issues with in the past. You can find the test class in sdk\test\IntegrationTests\Tests\S3\KeyNameTests.cs called TestKeyNameWithAwkwardChars_AWS4Signing if you want to take a look.

The two keys that triggered fails are:

ObjectWith!and?+forgood:measureThis=And&InKeynÄme

and

ObjectWith\InKeyname

In the first case, the use of Uri.AbsolutePath returns 'ObjectWith!and', terminating on the ? character. In the second case the Uri class is encoding the \ before we get a chance to work with the url in canonicalization and as a result we end up working with a double-encoding (this has been a common problem for use with use of the Uri class, which is why we tend to prefer string manipulation for urls for as long as possible).

Do you want to take another look at the fix in light of these two keys? The rest of the integration test suite ran without any issues.

@rafd123

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2015

Will do!

ISSUE #240: Fix signing issues for endpoints with a path component
In order to properly preserve the relationship between IRequest.Endpoint and IRequest.ResourcePath, have, AWS3Signer and AWS4Signer canonicalize the URL returned by AmazonServiceClient.ComposeUrl()

@rafd123 rafd123 force-pushed the rafd123:bug/issue-204 branch from 247fac9 to 608fdcf Sep 3, 2015

@rafd123

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2015

I addressed the problematic keys by grabbing the unescaped path from the composed request URL and feeding it to the original logic of "CanonicalizeResourcePath."

I ran the failing integration test using my account and verified that it now passes.

Let me know! :)

@steveataws

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2015

OK, will do. Thanks!

@steveataws

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2015

Hi,

I've completed running all of the integration tests against the revised update and all seems good. We'll get started on incorporating the changes.

Thanks for the contribution!

@steveataws steveataws merged commit 608fdcf into aws:master Sep 4, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@steveataws

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2015

PR is merged into core version 3.1.1.1.

@rafd123 rafd123 deleted the rafd123:bug/issue-204 branch Nov 17, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.