fix mod_s3_upload

[mjohnson9]

A recent update to mod_s3_upload caused it to start crashing with a stack trace similar to below:

registered_name: 'mod_s3_upload_[redacted]'
exception error: no match of right hand side value
                 #{path => <<"GMCN2Q0FY3WVXX0-Identicon.png">>,
                   query =>
                       <<"Content-Type=image%2Fpng&Content-Length=539&X-Amz-Acl=public-read">>}
  in function  aws_util:signed_url/7 (/opt/ejabberd/.ejabberd-modules/sources/ejabberd-contrib/mod_s3_upload/src/aws_util.erl, line 62)
  in call from mod_s3_upload:put_get_url/3 (/opt/ejabberd/.ejabberd-modules/sources/ejabberd-contrib/mod_s3_upload/src/mod_s3_upload.erl, line 414)
  in call from mod_s3_upload:handle_iq/2 (/opt/ejabberd/.ejabberd-modules/sources/ejabberd-contrib/mod_s3_upload/src/mod_s3_upload.erl, line 343)
  in call from mod_s3_upload:handle_info/2 (/opt/ejabberd/.ejabberd-modules/sources/ejabberd-contrib/mod_s3_upload/src/mod_s3_upload.erl, line 224)
  in call from gen_server:try_dispatch/4 (gen_server.erl, line 1123)
  in call from gen_server:handle_msg/6 (gen_server.erl, line 1200)

This PR fixes that. It also includes a couple of security-related patches that prevent escape from the S3 bucket, and make enumeration significantly more difficult.

[jpds]

Tagging @RomanHargrave - only nit I can see is the comment above object_name() will need updating.

Edit: I just deployed this on one of my ejabberd instances and the bucket appears to have been removed from my upload path:

garage_api::generic_server 2001:XXX:XXXX::XXXX (via [::1]:11718) PUT /dc/dca6b9764c9b47a5ab476f812e5ef401/test.jpg?X-Amz-Signature=...
garage_api::generic_server Response: error 404 Not Found, Bucket not found: dc

However, the bucket name at the end of bucket_url should be before the /dc/ bit.

[mjohnson9]

Interesting. I also use a folder-style bucket path, and haven't experienced that. Because object_name doesn't return a value that starts with a /, it should resolve relative to your bucket path. Does your bucket path end with a /? I wonder if a lack of a / suffix might affect how uri_string:resolve/2 functions.

Will incorporate your other feedback this weekend.

[jpds]

I've tested this again, and indeed it works if the bucket_url has the / on the end (I originally removed this from #319 where a double-slash broke routing with my S3 backend).

[mjohnson9]

Ok! I'll add some normalization of the bucket URL this weekend, too.

[sando38]

I had the same issue as described in the initial post. This PR fixes it for me as well. I use minio with bucket_url: https://s3.service.com/bucket/ configured in mod_s3_upload:

[badlop]

Module uri_string was introduced in OTP 21.0, and the functions you use apparently require at least 25.0, so compilation fails with older Erlang versions that ejabberd support.

I propose:

1. Keep the code as it is, don't bother about that problem, instead:
2. Mention in the module README that it requires Erlang/OTP 25.0 or higher (fortunately, all official ejabberd installers use 25.3)
3. I'll add a step in the tests to disable testing of that module with older Erlang versions

[Neustradamus]

To follow.

Linked to:

• https://github.com/processone/ejabberd-contrib/pull/320

[badlop]

@RomanHargrave when you have some spare time, can you review this patch, and confirm its merge? If you don't have time right now, it seems it can be merged as it got several positive feedback.

[mjohnson9]

I'm having to put my work on this on hold for now. I lost the OS partition that I was working from and haven't had the time to recover it. I may circle back to it in the future, but this is hobby work, and I'm not dogfooding it at the moment.

[badlop]

Great! I've updated the documentation and test: OTP 25 or higher is required for this module. This probably is not a problem in modern servers.