Support static websites with index pages

[Gamecock]

Please review the direct and provide suggestions. If this looks good I can add a commit to update the documentation. I've done preliminary testing with the OSS version and V2 & V4 signatures and AccessKey auth. However additional testing is welcome.

Sort of inelegant that I have to append the 'index.html' 3 times, once to update the URL, and then again in the V2 and V4 signatures. I'll take annother look and see if I can avoid that.

Uses the ALLOW_DIRECTORY_LIST flag to decide to return list(true) or index(false).
Returns a 404 if no index.html in that directory.

fixes #13

[Victrid]

Hello.

I've tried your patch and it works nice on requests like example.com/some/path/ to get somebucket.aws/some/path/index.html, which have a trailing slash and function _isDirectory will recognize it as a directory.

But when the requests is like example.com/some/path, the function will not work, as it recognizes with local string processing, which detects strings end with a slash and returns false, and will finally send 404 to clients.

When hosting a local folder with configurations like root /some/path;, nginx would issue a redirect adding a trailing slash as default.

Maybe this could be achieved by transforming the 404 not found from s3 upstream to a 301 redirect with a trailing slash on some conditions (like when no dots after last slash)?

User -> http/some/path  -> s3-gateway -> s3/some/path
   301 http/some/path/  <- s3-gateway <- 404
User -> http/some/path/ -> s3-gateway -> s3/some/path/index.html

If this is possible, hosting static sites with this gateway would be much more robust on different styles of paths.

[Gamecock]

Thanks for testing, and I think that can be done. Need the maintainers to comment on what impacts that would have on the existing behavior.

…

On Sun, Jul 31, 2022, 10:52 AM Jiang Weihao ***@***.***> wrote: Hello. I've tried your patch and it works nice on requests like example.com/some/path/ to get somebucket.aws/some/path/index.html, which have a trailing slash and function _isDirectory will recognize it as a directory. But when the requests is like example.com/some/path, the function will not work, as it recognizes with local string processing, which detects strings end with a slash and returns false, and will finally send 404 to clients. When hosting a local folder with configurations like root /some/path;, nginx would issue a redirect adding a trailing slash as default. Maybe this could be achieved by transforming the 404 not found from s3 upstream to a 301 redirect with a trailing slash on some conditions (like when no dots after last slash)? User -> http/some/path -> s3-gateway -> s3/some/path 301 http/some/path/ <- s3-gateway <- 404 User -> http/some/path/ -> s3-gateway -> s3/some/path/index.html If this is possible, hosting static sites with this gateway would be much more robust on different styles of paths. — Reply to this email directly, view it on GitHub <#42 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABMSCSUL2DTH52IJZVXIJPDVW2HMFANCNFSM546SCNVA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[Gamecock]

@Victrid I can get the logic to trigger the redirect, but I dont seem to be able to change the $s3uri variable. It keeps retrying
/some/path
instead of /some/path/index.html

I thought this should work,
set $s3uri "${s3uri}/index.html";

   Any thoughts, I can share the current state of the branch if you are interested.

[Victrid]

@Gamecock I've opened a PR to your branch implementing this.

It will not change the s3 uri, but let user's browser to perform the redirection. It is performed with nginx itself. On s3 returned 404, it will be internal redirected to @staticPageHandler, and if is not a dir with static site hosting enabled, it will be internal redirected to @trailslash, which will send a 301 to user.

    location @staticPageHandler {
        # Checks if requesting a folder without trailing slash, and return 302 
        # appending a slash to it when using for static site hosting.
        js_content s3gateway.staticPageHandler;
    }


    location @trailslash {
        # 301 to request without slashes
        rewrite ^ $scheme://$http_host$request_uri/ redirect;
    }

[dekobon]

@Gamecock Thank you for your PR! I'm going to start reviewing it now.

[dekobon]

Here are some high level thoughts about the feature.

This feature should be configurable using a flag separate from ALLOW_DIRECTORY_LIST because it is a distinct feature with security implications.

As you no doubt discovered, both any index page feature will interact with the directory list feature. For example, if directory listing is enabled and index pages are enabled, then you want the index page to display only if it is present in the S3 bucket, and when it is not present, you would display the directory listing.

The approach that I see you went with was one where we have mutually exclusive settings, such that you cannot enable index pages AND directory listings. I like this approach because it avoids a bunch of the problems with race conditions you would hit if you tried to enable both.

[dekobon]

I just looked through the code and it is a good PR! Thank you so much for your work. I get a bit excited every time I see a contribution.

The changes needed are:

• Add a setting to enable or disable the feature, with the default being disabled
• Add documentation for the setting and proper use of the feature
• Move the string literal index.html to a top level variable

[Gamecock]

@dekobon If they are two separate settings for ALLOW_DIRECTORY_LIST and RETURN_INDEX_PAGES do you want a runtime error or just ignore one of them?

[dekobon]

Runtime error would be preferable. What do you think?

…

On Wed, Aug 3, 2022 at 6:43 PM Mike Finch ***@***.***> wrote: @dekobon <https://github.com/dekobon> If they are two separate settings for ALLOW_DIRECTORY_LIST and RETURN_INDEX_PAGES do you want a runtime error or just ignore one of them? — Reply to this email directly, view it on GitHub <#42 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADXXGDE7UVZFXRNG45WGB3VXK4UFANCNFSM546SCNVA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- -Elijah

[Gamecock]

Are you OK with including @Victrid's suggestion to redirect with a 301/308 on items that don't have either a file extension or a trailing slash? I'm not checking that it actually exists.

[dekobon]

@Gamecock Perhaps the environment variable that enables index pages should allow a setting that enables redirects. This seems like the kind of thing that you sometimes want, but surely not always.

[Gamecock]

@dekobon The build issue looks like a permissions issue on push. I don't think I can resolve.

[dekobon]

The build succeeded, but PR checkins like this do not have permissions to push a new container image to the repository. There is probably a way to set up the build so it will conditionally skip the container image push step, but I haven't figured it out yet.

[dekobon]

We are almost ready to merge. This is exciting seeing this functionality getting added.

Next, we need to:

• Rebase in the latest master branch into this branch.
• Fix the minor issues highlighted in code review.
• Update the documentation to reflect the new features added.

[dekobon]

This looks really good. I see only a few minor changes.

[dekobon]

Around line 34, we will need to add:

export COMPOSE_COMPATIBILITY=true

Because with the addition of the health check in the docker-compose.yaml, I had to upgrade my Docker Compose version to support it - previously I was using the version provided by Ubuntu's package repository. After upgrading, Docker Compose changes how it names containers and thereby breaking this script.

[Gamecock]

I'll submit s separate PR for windows support to make it cleaner, unless you have an objection.

[dekobon]

I'd put it in this PR because without this environment variable, v2 of docker-compose on Linux fails to work with the test script.

[Gamecock]

Got it.

[dekobon]

Please put the string literal index.html in a top level variable and reference it.

[Gamecock]

I've merged @Victrid 's PR with mine. Still have some cleanup and documentation, will get it in in the next few days.

[Gamecock]

@dekobon I think all the changes you requested have been made.

[dekobon]

Thank you for the contribution!

[sochotnicky]

So - this MR basically reverted the V4 auth fix that was merged in #43

My question is - was that fix breaking something or was the revert a mistake that was unintended?

[Gamecock]

It was not intentional on my part, it looks like a merge conflict not addressed properly. Having said that if it's something that only causes an issue for one client, there should probably be a unit test update also so no one else breaks this by mistake again.

[dekobon]

Yes, that was a complete mistake on my part when doing the merge.

[dekobon]

The change has been re-applied. Thank you for catching the issue.