Support generate presigned URL with tagging #29

phulien · 2024-12-31T06:15:47Z

No description provided.

onno-vos-dev · 2025-01-03T07:31:55Z

Thank you @phulien for your contribution! I tried adding a test and it immediately fails with below error:

1) aws_signature:sign_v4_query_params_with_tags_test/0
     Failure/Error: {error,undef,
                        [{hackney_url,urlencode,[<<"host;x-amz-tagging">>],[]},
                         {aws_signature,sign_v4_query_params,8,
                             [{file,
                                  "/Users/onno.vos/src/github/aws_signature/src/aws_signature.erl"},
                              {line,272}]},
                         {aws_signature,sign_v4_query_params_with_tags_test,
                             0,
                             [{file,
                                  "/Users/onno.vos/src/github/aws_signature/src/aws_signature.erl"},
                              {line,1098}]},
                         {aws_signature,sign_v4_query_params_with_tags_test,
                             0,[]}]}

You're relying on hackney_url to encode the query params here while hackney is not available as part of this package. Either we need to include hackney as part of this repo or bring the urlencode in here as a hardcopy.

Considering that aws-elixir has hackney as an optional dependency, I'd like to avoid bringing it in here as a dependency. Since that would force it to become part of aws-elixir (IIRC, I may be wrong and the optional:true would still allow an override on that but I'd need to verify that before chosing a way forward. I know the Elixir folks prefer finch over hackney typically speaking.

Perhaps we can use the uri_string module instead? 👍

onno-vos-dev · 2025-01-03T07:38:07Z

src/aws_signature.erl

+          if Tag == undefined ->
+                  <<"host">>;
+             Tag =/= undefined ->
+                  hackney_url:urlencode(<<"host;x-amz-tagging">>)


Are you sure this works? I'd expect something like below instead:

BaseParams0 = [{<<"X-Amz-Algorithm">>, <<"AWS4-HMAC-SHA256">>}, {<<"X-Amz-SignedHeaders">>, <<"host">>}], BaseParams = maybe_add_tag_header(BaseParams0, Tags),

With the maybe_add_tag_header/2 being defined as something like below (leaving aside the hackney_url problem highlighted in a previous comment) but I'd expect the header to be a part of this?

maybe_add_tag_header(Headers, undefined) -> Headers; maybe_add_tag_header(Headers, [{_K, _V} | _] = Tags) -> [<<"X-Amz-Tagging">>, hackney_url:qs(Tags)} | Headers].

(Notice also the usage of Tags since it can be a list of tags :-) )

That wouldn't be enough since we need to add X-Amz-Tagging to X-Amz-SignedHeaders as defined here: https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-query-string-auth.html

Yes, you're right; it should be a list of tags.

jonatanklosko · 2025-01-03T07:48:24Z

src/aws_signature.erl

+maybe_add_tag_query_param(QueryParams, undefined) ->
+    QueryParams;
+maybe_add_tag_query_param(QueryParams, Tag) ->
+    [{<<"X-Amz-Tagging">>, Tag} | QueryParams].


From quick search online, it seems to me that x-amz-tagging always needs to be send in the headers, so there is no reason to put it in the params.

See https://stackoverflow.com/a/77418874.

Yes, if people want to generate a pre-signed URL with tagging, then x-amz-tagging needs to be included. However, we need to support both scenarios, with and without tagging.

My understanding is that there is no reason to add x-amz-tagging as query param. It's either sent as header or not. I may totally be missing something though :)

I understand what you mean now, and you are correct. It should not be needed.

jonatanklosko · 2025-01-03T07:50:55Z

src/aws_signature.erl

+        if Tag == undefined ->
+                [HostHeader];
+           Tag =/= undefined ->
+                [HostHeader, {<<"X-Amz-Tagging">>, hackney_url:urldecode(Tag)}]


We should accept an unescaped value instead, the same that the caller would use for the header.

But the session_token in Options is escaped here: https://github.com/aws-beam/aws-erlang/blob/master/src/aws_s3_presigned_url.erl#L44C21-L45C66

Correct, that's because we put it in the query params. My point is that we should not add tag to the query params, and only add it to headers for signing. So given that, the value should match the actual header value.

That actually makes a lot of sense. Thanks.

jonatanklosko · 2025-01-03T07:51:51Z

src/aws_signature.erl

+          if Tag == undefined ->
+                  <<"host">>;
+             Tag =/= undefined ->
+                  hackney_url:urlencode(<<"host;x-amz-tagging">>)


Here we can encode by hand to avoid depending on hackney:

Suggested change

hackney_url:urlencode(<<"host;x-amz-tagging">>)

<<"host%3x-amz-tagging">>

onno-vos-dev · 2025-01-03T08:08:22Z

@phulien Can you try branch: add-tagging-to-presign-urls that is in this repo? :-) Perhaps that would do the trick?

If so, I can tidy that up and bring your commit along in there so you'd still get your name as a contributor 😉 👍

phulien · 2025-01-03T08:15:14Z

@phulien Can you try branch: add-tagging-to-presign-urls that is in this repo? :-) Perhaps that would do the trick?

If so, I can tidy that up and bring your commit along in there so you'd still get your name as a contributor 😉 👍

I got this error when trying this branch

<?xml version="1.0" encoding="UTF-8"?>
<Error>
  <Code>AccessDenied</Code>
  <Message>There were headers present in the request which were not signed</Message>
  <HeadersNotSigned>x-amz-tagging</HeadersNotSigned>
  <RequestId>8CA589EDB3KB3EKE</RequestId>
  <HostId>/4HjL9q/xBLps/U8Pd9zbkd3ASCEyScl1lcoxi0SM6y80/2BCX9fvJKwi9IFWMrPLhXwRzzEaOY=</HostId>
</Error>

onno-vos-dev · 2025-01-03T08:20:11Z

@phulien Can you try branch: add-tagging-to-presign-urls that is in this repo? :-) Perhaps that would do the trick?
If so, I can tidy that up and bring your commit along in there so you'd still get your name as a contributor 😉 👍

I got this error when trying this branch
<?xml version="1.0" encoding="UTF-8"?>
<Error>
  <Code>AccessDenied</Code>
  <Message>There were headers present in the request which were not signed</Message>
  <HeadersNotSigned>x-amz-tagging</HeadersNotSigned>
  <RequestId>8CA589EDB3KB3EKE</RequestId>
  <HostId>/4HjL9q/xBLps/U8Pd9zbkd3ASCEyScl1lcoxi0SM6y80/2BCX9fvJKwi9IFWMrPLhXwRzzEaOY=</HostId>
</Error>

@phulien Can you try again after pulling latest change? Sorry, I'm not at the office right now so don't really have an environment where I can test my changes.

phulien · 2025-01-03T08:32:34Z

@phulien Can you try branch: add-tagging-to-presign-urls that is in this repo? :-) Perhaps that would do the trick?
If so, I can tidy that up and bring your commit along in there so you'd still get your name as a contributor 😉 👍

I got this error when trying this branch
<?xml version="1.0" encoding="UTF-8"?>
<Error>
  <Code>AccessDenied</Code>
  <Message>There were headers present in the request which were not signed</Message>
  <HeadersNotSigned>x-amz-tagging</HeadersNotSigned>
  <RequestId>8CA589EDB3KB3EKE</RequestId>
  <HostId>/4HjL9q/xBLps/U8Pd9zbkd3ASCEyScl1lcoxi0SM6y80/2BCX9fvJKwi9IFWMrPLhXwRzzEaOY=</HostId>
</Error>
@phulien Can you try again after pulling latest change? Sorry, I'm not at the office right now so don't really have an environment where I can test my changes.

I got the same error with latest change (54fadb5).

onno-vos-dev · 2025-01-03T08:33:57Z

@phulien Can you try branch: add-tagging-to-presign-urls that is in this repo? :-) Perhaps that would do the trick?
If so, I can tidy that up and bring your commit along in there so you'd still get your name as a contributor 😉 👍

I got this error when trying this branch
<?xml version="1.0" encoding="UTF-8"?>
<Error>
  <Code>AccessDenied</Code>
  <Message>There were headers present in the request which were not signed</Message>
  <HeadersNotSigned>x-amz-tagging</HeadersNotSigned>
  <RequestId>8CA589EDB3KB3EKE</RequestId>
  <HostId>/4HjL9q/xBLps/U8Pd9zbkd3ASCEyScl1lcoxi0SM6y80/2BCX9fvJKwi9IFWMrPLhXwRzzEaOY=</HostId>
</Error>
@phulien Can you try again after pulling latest change? Sorry, I'm not at the office right now so don't really have an environment where I can test my changes.
I got the same error with latest change (54fadb5).

😢 Let me see what I can do without making you my personal guinea pig 😅 Give me an hour or so :-)

jonatanklosko · 2025-01-03T08:42:03Z

@onno-vos-dev the header needs to be listed in X-Amz-SignedHeaders :)

phulien · 2025-01-03T08:42:51Z

@phulien Can you try branch: add-tagging-to-presign-urls that is in this repo? :-) Perhaps that would do the trick?
If so, I can tidy that up and bring your commit along in there so you'd still get your name as a contributor 😉 👍

I got this error when trying this branch
<?xml version="1.0" encoding="UTF-8"?>
<Error>
  <Code>AccessDenied</Code>
  <Message>There were headers present in the request which were not signed</Message>
  <HeadersNotSigned>x-amz-tagging</HeadersNotSigned>
  <RequestId>8CA589EDB3KB3EKE</RequestId>
  <HostId>/4HjL9q/xBLps/U8Pd9zbkd3ASCEyScl1lcoxi0SM6y80/2BCX9fvJKwi9IFWMrPLhXwRzzEaOY=</HostId>
</Error>
@phulien Can you try again after pulling latest change? Sorry, I'm not at the office right now so don't really have an environment where I can test my changes.
I got the same error with latest change (54fadb5).
😢 Let me see what I can do without making you my personal guinea pig 😅 Give me an hour or so :-)

I have updated the MR. Please check the latest version; it is now much simpler.

jonatanklosko · 2025-01-03T08:44:04Z

FWIW the current version looks good to me, we just need to add the option in the docstring and it also needs a test.

onno-vos-dev · 2025-01-03T08:47:10Z

src/aws_signature.erl

@@ -253,6 +254,7 @@ sign_v4_query_params(AccessKeyID, SecretAccessKey, Region, Service, DateTime, Me
    URIEncodePath = proplists:get_value(uri_encode_path, Options, true),
    TimeToLive = proplists:get_value(ttl, Options, 86400),
    SessionToken = proplists:get_value(session_token, Options, undefined),
+    Tag = proplists:get_value(tag, Options, undefined),


I would prefer plural of tag (so tags) both as the key in the Options as well as the variable name :-)

Sure, I will update Tags, document and add a test case. Thank for reviewing!

onno-vos-dev · 2025-01-03T08:47:50Z

FWIW the current version looks good to me, we just need to add the option in the docstring and it also needs a test.

Agreed 👍 Add a test and I'd be happy 👍

phulien · 2025-01-03T10:33:23Z

@onno-vos-dev please double-check.

onno-vos-dev reviewed Jan 3, 2025

View reviewed changes

jonatanklosko reviewed Jan 3, 2025

View reviewed changes

onno-vos-dev reviewed Jan 3, 2025

View reviewed changes

Support generate presigned URL with tagging

f608b12

phulien force-pushed the main branch from 331cfb6 to f608b12 Compare January 3, 2025 10:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support generate presigned URL with tagging #29

Support generate presigned URL with tagging #29

phulien commented Dec 31, 2024

onno-vos-dev commented Jan 3, 2025 •

edited

Loading

onno-vos-dev Jan 3, 2025 •

edited

Loading

phulien Jan 3, 2025

phulien Jan 3, 2025

jonatanklosko Jan 3, 2025

phulien Jan 3, 2025

jonatanklosko Jan 3, 2025

phulien Jan 3, 2025

jonatanklosko Jan 3, 2025

phulien Jan 3, 2025

jonatanklosko Jan 3, 2025

phulien Jan 3, 2025

jonatanklosko Jan 3, 2025

onno-vos-dev commented Jan 3, 2025 •

edited

Loading

phulien commented Jan 3, 2025

onno-vos-dev commented Jan 3, 2025

phulien commented Jan 3, 2025

onno-vos-dev commented Jan 3, 2025

jonatanklosko commented Jan 3, 2025

phulien commented Jan 3, 2025

jonatanklosko commented Jan 3, 2025

onno-vos-dev Jan 3, 2025

phulien Jan 3, 2025

onno-vos-dev commented Jan 3, 2025

phulien commented Jan 3, 2025

	hackney_url:urlencode(<<"host;x-amz-tagging">>)
	<<"host%3x-amz-tagging">>

Support generate presigned URL with tagging #29

Are you sure you want to change the base?

Support generate presigned URL with tagging #29

Conversation

phulien commented Dec 31, 2024

onno-vos-dev commented Jan 3, 2025 • edited Loading

onno-vos-dev Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onno-vos-dev commented Jan 3, 2025 • edited Loading

phulien commented Jan 3, 2025

onno-vos-dev commented Jan 3, 2025

phulien commented Jan 3, 2025

onno-vos-dev commented Jan 3, 2025

jonatanklosko commented Jan 3, 2025

phulien commented Jan 3, 2025

jonatanklosko commented Jan 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onno-vos-dev commented Jan 3, 2025

phulien commented Jan 3, 2025

onno-vos-dev commented Jan 3, 2025 •

edited

Loading

onno-vos-dev Jan 3, 2025 •

edited

Loading

onno-vos-dev commented Jan 3, 2025 •

edited

Loading