-
-
Notifications
You must be signed in to change notification settings - Fork 84
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
@ in key for certain operations causes SignatureDoesNotMatch error #601
Comments
Seems like the exact same behaviour occurs with Which if I understand correctly https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html |
I now have found where this is happening and it's in soto-core. As such I will close this issue and open in the proper repo. |
I have a test for uploading to a S3 key with a load of special characters. For some reason I don't include |
@adam-fowler this is what I was "preparing" to add here since I did some digging around and really wanted to help you figure out if/what/how... The signature seems to be happening in
Looking in to the code it references Point 2. is the one that applies to this specific part "Add the canonical URI parameter" which references "Normalize URI paths according to RFC 3986" RFC 3986 defines,
** Note that
Section 3.3 Path component defines
Thus if I understood this correctly, which I hardly want to claim to have, means that the CharacterSet to use for this part would be : |
I am currently trying to use a local forked version that adds all the sub-delims, ":" and "@" to see if it would at least work (aside from whether it's right or not) to have a starting point... |
Also would you rather I reopen here or create a net new issue in |
I tried manually encoding "@" with %40. It did allow the request to succeed put also resulted in the actual creation having %40 in it... |
It might need percent encoded in the canonical request, not the URL
…On Tue, 14 Jun 2022 at 17:29, jmoisan21 ***@***.***> wrote:
It might be because they need to be percent encoded.
I tried manually encoding "@" with %40. It did allow the request to
succeed put also resulted in the actual creation having %40 in it...
—
Reply to this email directly, view it on GitHub
<#601 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACHSVJ2PGJY5LVVK2CBBQWLVPCXP3ANCNFSM5YVPDKZQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Ok so what I have so far is this is what your code does right now when the key is
Resulting in the |
If I force canonicalPath to be like request.url but I get an error : InvalidArgument: Invalid Argument
|
I then instead tried to match request.url to canonicalPath
That had no effect at all. |
Discovered that So I change that code
And success : definition of success is the key path was accepted, the signature was valid, the request succeeded and the resulting item server side has proper naming. |
With the 3 above comments that should give you the gist of what is happening and why.
I don't want to PR this for you. I do hope this gives you enough to hit the ground running. Ty |
There was a change to that recently. Maybe it broke something. I'll look into it tomorrow |
FWIW I reverted every changed I made, returned to to using your current This results in |
@jmoisan21 Are you working with AWS S3 or an S3 like service? I have just looked through my test code and there is a test that tests against special characters soto/Tests/SotoTests/Services/S3/S3Tests.swift Lines 143 to 146 in ea6c998
This test passes fine when run against S3 |
@adam-fowler S3 like service |
Given the special characters work ok with AWS S3, I'm guessing that service is verifying the signing incorrectly. |
Will report back on it |
File is created and has proper name Additionally... Changing the --endpoint to something that does not exist prints out what
(re that message : #601 (comment)) |
Can you simplify the key to something that still fails eg
And dump the output here. |
After that can you run the Soto code with the following change
And add the contents below. |
Please note that I sanitized the output and therefore you cannot rely on the data here to repro manually the md5/sha/etags/...
|
Please note that I sanitized the output and therefore you cannot rely on the data here to repro manually the md5/sha/etags/... Your soto code unchanged:
|
And in case you wondered, my soto change from #601 (comment)
|
Can you verify #604 fixes your issue |
Will fetch the package using branch "s3-path-characters" and test. Should have results in the next hour. |
@adam-fowler I can confirm that it solves for exactly "@()" characters. But since you are there and doing the work... I tried you test string What was cli sends when using that string is : What you send (with the current branch fix) is : Short version : |
That was my concern. That there would be other characters. But I'm overly keen on just adding a whole load of additional characters, the signing stuff is so frustrating and what we have works for AWS S3, it is bloody frustrating it doesn't work on other S3 like services. I'm not sure this is even a bug with Soto, is it not more a bug with the S3 service you are using. It should be able to deal with the URLs I am passing it. You included |
Argh I am sorry I took as much as my time as I could to make sure I would not make that mistake when comparing the two strings.
Change the chars in current result one by one
Compare again
Add spaces to help the eyes
Resulting list would be : |
I would tend to agree with you. The only thing that makes me side on caution here is that the official aws cli outputs something different then yours and this may be a ticking time bomb for your code... |
I've added those characters in as well if you want to test it |
Updated the package and can see the new change Testing... |
Successfully created I call this one solved! Aligned with aws cli for your test string. Thank you very much |
Sorry cannot say that here. |
I'm about to do a v6 release of Soto, in the next couple of weeks. Would you be happy to upgrade to v6 (You have been using what will be v6 when testing the changes)? I could back port the changes to v5 but would prefer if I could avoid that. You can find a list of the main changes in v6 here. The other major addition, not listed, is v6 also includes |
The tests I have been performing were in really controlled environment. I am happy to run the current v6 you through a real set of test to verify. Based on the fact that you merged #601 in main I assume that means that setting my package to pull from main would effectively include your current work for v6? |
That'd be great if you could do that. Yes |
Been running I almost feel confident answering the question :
But I will give it one more day of load testing before calling it. |
v6 has been released today, so you'll be able to upgrade to a full version if you are happy with your tests |
Yes ty I saw right away since it includes my other PR ;-) Also the fact that this is a release will make things much easier for me since I will be able to pin to a version and not just (I do hope to close this issue by EOD) |
So far I have not encountered any issues with v6. Therefore I am calling it. This fix is not to be back ported for me. If anything comes up I will look forward to getting it fixed in the v6 series! Thanks |
Describe the bug
When the @ sign is present in the key for certain operations the resulting request comes back with
SignatureDoesNotMatch: The request signature we calculated does not match the signature provided. Check the Secret Access Key and signing method.
To Reproduce
Expected behavior
The request signature would be valid.
Setup (please complete the following information):
Additional context
Tried with another client and it worked as expected.
Tried to identify where in the code this signature creation problem would reside so I could just PR you something but I could not figure it out.
The text was updated successfully, but these errors were encountered: