S3::Client::GetPresignedUrl does not support object keys with a version

[amoustafa7]

Describe the bug

We are not able to generate a presigned url for an S3 object with a version. The "key" parameter is url encoded by S3::Client and we are not able to specify the version.

This is what the calling code looks like

        auto bucket = "myfiles";
        auto filePath = "file.txt";
        auto version = "a2dff23f-ccd5-4177-9798-56756def7f92";
        auto key = filePath + "?versionId=" + version;
        return m_awsSdkClient->GeneratePresignedUrl(
        bucket, key, Aws::Http::HttpMethod::HTTP_GET);

Expected Behavior

I would expect the generated url to be something like:

https://<endpoint>/myfiles/file.txt?versionId=4ef8f967-bb09-4307-a025-6ca5fa36d6d0&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=DJPY5NIRF1B3CKNJHP62%2F20230605%2Fdev-14%2Fs3%2Faws4_request&X-Amz-Date=20230605T154734Z&X-Amz-Expires=604800&X-Amz-Security-Token=eyJhbGciOiJIUzUxMiIsInR5cCI6IkpXVCJ9.eyJhY2Nlc3NLZXkiOiJESlBZNU5JUkYxQjNDS05KSFA2MiIsImF1dGhCYWNrZW5kIjoiSUFNIiwiYnBpIjoiYnBpOnN0YXRpYzpkc21kbWRmaWxlc3J3IiwiZXhwIjoxNjg3MTg5NjU0LCJwYXJlbnQiOiJjdXN0b206YnBpOnN0YXRpYzpkc21kbWRmaWxlc3J3Iiwicm9sZUFybiI6ImFybjptaW5pbzppYW06Ojpyb2xlL2lkbXAtamFudXMiLCJzdWIiOiJjdXN0b206YnBpOnN0YXRpYzpkc21kbWRmaWxlc3J3In0.QrJsCiQc5SnUduqmtVnuteZWeGYvPeIigoH1zEn45pPTLoMQhGeBnYohe2u5q3Rnbb2HHIirbj9PhLwPXpb4Rg&X-Amz-SignedHeaders=host&X-Amz-Signature=1fa1d72e8e5da6e507c0a4008d9b66e061d0495c61d79c0405753d74a77eb393

Current Behavior

The generated url is actually

https://<endpoint>/myfiles/file.txt%253FversionId=4ef8f967-bb09-4307-a025-6ca5fa36d6d0?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=DJPY5NIRF1B3CKNJHP62%2F20230605%2Fdev-14%2Fs3%2Faws4_request&X-Amz-Date=20230605T154734Z&X-Amz-Expires=604800&X-Amz-Security-Token=eyJhbGciOiJIUzUxMiIsInR5cCI6IkpXVCJ9.eyJhY2Nlc3NLZXkiOiJESlBZNU5JUkYxQjNDS05KSFA2MiIsImF1dGhCYWNrZW5kIjoiSUFNIiwiYnBpIjoiYnBpOnN0YXRpYzpkc21kbWRmaWxlc3J3IiwiZXhwIjoxNjg3MTg5NjU0LCJwYXJlbnQiOiJjdXN0b206YnBpOnN0YXRpYzpkc21kbWRmaWxlc3J3Iiwicm9sZUFybiI6ImFybjptaW5pbzppYW06Ojpyb2xlL2lkbXAtamFudXMiLCJzdWIiOiJjdXN0b206YnBpOnN0YXRpYzpkc21kbWRmaWxlc3J3In0.QrJsCiQc5SnUduqmtVnuteZWeGYvPeIigoH1zEn45pPTLoMQhGeBnYohe2u5q3Rnbb2HHIirbj9PhLwPXpb4Rg&X-Amz-SignedHeaders=host&X-Amz-Signature=1fa1d72e8e5da6e507c0a4008d9b66e061d0495c61d79c0405753d74a77eb393

the key passed to the function is url encoded and therefore, we are not able to specify a "versionId" query parameter.

Reproduction Steps

See the code sample above.

Possible Solution

Allow key to include query parameters. I think this bug was a regression in this commit

it was already working before according to this comment

Additional Information/Context

No response

AWS CPP SDK version used

1.10.38

Compiler and Version used

gcc (GCC) 10.2.1 20210130 (Red Hat 10.2.1-11)

Operating System and version

Linux 81148edc27b5 3.10.0-1160.88.1.el7.x86_64

[sbiscigl]

Allow key to include query parameters. I think this bug was a regression in this commit

This is change occurs after the version you are using, and infact i think this change fixes your issue. %253FversionId= appears to be double url encoded which that change was mean to fix.

1.10.38

I think this issue was fixed in version 1.11.68 please refer to the issue issues/2396. please update to get that change and let me know if that resolves the issue.

I am in the meantime validating that the latest version works as intended

[amoustafa7]

version 1.10.38 was broken

It was fixed in this commit but was later reverted in this commit that i linked previously.

[sbiscigl]

version 1.10.38 was broken

what version do you have this working in?

[amoustafa7]

It was working at the time of this comment, I suppose

[sbiscigl]

but was later reverted in this commit that i linked previously.

If you revert this change it is still broken because of double url encoding

It was fixed in this commit

this commit shows that it is not part of our repo and was likely part of the contributors.

It was working at the time of #1068 (comment), I suppose

I'm trying to narrow down between what versions this regression happened for you, did you have it working in any version? could you provide the version number? as-is its working as intended "yourobjectname?versionId=someversionid" is a valid key name in S3, so trying to parse keyname as a url path is incorrect here.

The correct thing to do here imo would be to add another method that allows you to optionally provide version as a path param query param. will converse with the team on this.

[amoustafa7]

I don't have a version where it is working. I have just started to use this library a week ago and the version i am using is 1.10.38 and it doesn't work as expected. The comment I linked is the only info I have about when it could have been working before. I have not tried the latest version or any other version, but from the code on main branch, I believe it will also not work.

The correct thing to do here imo would be to add another method that allows you to optionally provide version as a path param. will converse with the team on this.

But then wouldn't this be inconsistent with other public methods? GetObjectRequest only takes a key for example and you can specify the versionId as part of the key, iirc.

[sbiscigl]

GetObjectRequest only takes a key for example and you can specify the versionId as part of the key, iirc.

no it has a param specifically for version Id. i.e.

auto s3 = Aws::MakeUnique<S3Client>("alloc", clientConfig);
s3->GetObject(GetObjectRequest().WithVersionId("versionId"));

doc reference

VersionId used to reference a specific version of the object.

"yourobjectname?versionId=someversionid" is a valid s3 object name, and as such we have to treat it as a keyname and not as a url path.

Commenting on the overall issue though i do recognize we don't offer a way to pre-sign a specific object version right now. The way the comment outlined how to do it itself was a hiding a bug in itself. Will figure out the best way to add that functionality.

[sbiscigl]

I did find a workaround for you that will work, but i will admit is not customer friendly

#include <aws/core/Aws.h>
#include <aws/core/client/AWSClient.h>
#include <aws/s3/S3Client.h>
#include <aws/s3/model/PutObjectRequest.h>
#include <aws/s3/model/GetObjectRequest.h>
#include <memory>

using namespace Aws;
using namespace Aws::S3;
using namespace Aws::S3::Model;

int main(int argc, char **argv) {
    Aws::SDKOptions options;
    Aws::InitAPI(options);
    {
        Aws::Client::ClientConfiguration clientConfig;
        auto s3 = Aws::MakeShared<S3Client>("alloc", clientConfig);
        auto client = std::dynamic_pointer_cast<Aws::Client::AWSClient>(s3);
        auto bucketName = "your-bucket-name";
        auto endpoint = s3->accessEndpointProvider()->ResolveEndpoint({{Aws::String("Bucket"), bucketName}}).GetResult();
        endpoint.AddPathSegments("your-key");
        auto uri = endpoint.GetURI();
        uri.AddQueryStringParameter("versionId", "your-version");
        auto url = client->GeneratePresignedUrl(uri, Aws::Http::HttpMethod::HTTP_GET, 60 * 5);
        std::cout << url;
    }
    Aws::ShutdownAPI(options);
    return 0;
}

This workaround casts the s3 client to the parent AwsClient class that has the generic GeneratePresignedUrl function that takes a uri. we can use the same endpoint provider used to create the s3 client to generate a endpoint, and subsequently add the query string for version id.

[amoustafa7]

Oh thanks, we will use this workaround for now until you publish a good way to that.