Skip to content
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

[8.x] Use user provided url in AwsTemporaryUrl method #36480

Merged
merged 1 commit into from
Mar 7, 2021

Conversation

jnoordsij
Copy link
Contributor

The getAwsUrl method allows overriding the base domain by a custom value set in the config, which is particulary useful for the Digital Ocean Spaces CDN. With this CDN, you can access files via a provided CDN endpoint or even use a custom subdomain; with the custom url option this can be easily setup in Laravel config.

However, the getAwsTemporaryUrl does not do the same things, hence only publicly visible files can profit from this. In this PR, the same behavior is added to this method.

I am not sure wether this is considered a breaking change; currently the behavior seems inconsistent and if your getAwsUrl is working with a custom set config url, I think nothing should break.

@taylorotwell
Copy link
Member

Were you able to test this with an actual DigitalOcean Spaces account? I thought the HMAC signature in the URL contained the signature of the entire URL - so changing the base portion of the URL seems like it would break that signature?

@jnoordsij
Copy link
Contributor Author

jnoordsij commented Mar 6, 2021

You seem to be right about the URL somehow being incorporated into the signature; if I generate a temporary URL through the DO Spaces UI it generates both a "path-style" and "virtual-hosted" style file url, which have different signatures.

However, the second one allows replacing the base domain with either the DO CDN domain or even my custom subdomain, so somehow they seem to allow for that on their side.

All URL patterns working with same signature:
https://{my-bucket}.ams3.digitaloceanspaces.com/<file&signature>
https://{my-bucket}.ams3.cdn.digitaloceanspaces.com/<file&signature>
https://{my-custom-subdomain}/<file&signature>

Path-style URL which has a different signature
https://ams3.digitaloceanspaces.com/{my-bucket}/<file&different signature>

@jnoordsij
Copy link
Contributor Author

Note: I found Digital Ocean advertises this method in their documentation:

You can use presigned URLs with the Spaces CDN. To do so, configure your SDK or S3 tool to use the non-CDN endpoint, generate a presigned URL for a GetObject request, then modify the hostname in the URL to be the CDN hostname (<space-name>.<region>.cdn.digitaloceanspaces.com, unless the Space uses a custom hostname).

But this does raise the question wether or not this will work for other providers.

@taylorotwell taylorotwell merged commit 883713b into laravel:8.x Mar 7, 2021
@jnoordsij jnoordsij deleted the override-temporary-url branch March 8, 2021 09:16
@driesvints
Copy link
Member

@jnoordsij FYI: I merged 8.x into master and had to resolve some merge conflicts. This functionality was ported to the AWS adapter specifically. If you want this to be available for DO or other adapters feel free to send in a PR.

@jnoordsij
Copy link
Contributor Author

@jnoordsij FYI: I merged 8.x into master and had to resolve some merge conflicts. This functionality was ported to the AWS adapter specifically. If you want this to be available for DO or other adapters feel free to send in a PR.

Thanks! DO Spaces is S3 based, so it should be working perfectly fine with that adapter.

@dwightwatson
Copy link
Contributor

I believe this change has broken my implementation in production, so I've rolled back to Laravel 8.31.0.

I'm using AWS S3 and my configuration includes a url so that my public content is stored my subdomain instead of a bucket URL. Previously generating a link to a file directly would use my custom url, but generating a temporary link would use {$bucket}.s3.ap-southest-2.amazonaws.com/*.

Now, the generated link is using my custom URL and resulting in a SignatureDoesNotMatch error being returned from AWS - the referenced file cannot be downloaded (omitted some details from the screenshot):

image

My assumption is that AWS's presigned URL includes the host in the signing, so swapping the host out breaks the signature. Alternatively the custom host may need to be passed into the presign request (if that's an option?).

@driesvints
Copy link
Member

Ping @jnoordsij

@driesvints
Copy link
Member

@dwightwatson are you rolling your own custom implementation or are you just using Laravel's base implementation? If you look at @jnoordsij's examples then this should cover subdomains?

@jnoordsij
Copy link
Contributor Author

@dwightwatson can you tell a little more about your setup? Is your subdomain just something that has a CNAME to the bucket (similar to custom subdomains DO Spaces) and if yes, can you point me to the Amazon docs how you set this up?

If you store public content on another filesystem, you might be better off using two different drivers instead of overriding parts of the s3 adapter.

@dwightwatson
Copy link
Contributor

Using Laravel's base code, nothing on top.

I'm using a subdomain of my main app. Looking at it now in Route 53 I realise it's actually aliased to a CloudFront CDN in front of the bucket, not aliased to the bucket itself. This was so anything served publicly would be cached. Reading above it doesn't sound like this should matter though - if the signature is the same regardless of the host?

Previously this worked both technically, but also because the temporary URL pointed directly to S3 and not through my subdomain I had confidence it wasn't being cached by my CDN (because this is private content that I only want available on-demand).


I appreciate the intention of the PR, but find it odd that it broke the way it did.

@dwightwatson
Copy link
Contributor

Reading through it - if the signature is only covering the host it really shouldn't have made a difference here (regardless of the exact set up of my subdomain). I was under the same assumption as Taylor above that the signature covers the entire URL. I wonder if this is a subtle difference between AWS's and DO's signing methods?

@ashgibson
Copy link

This broke my production site as well. I have had to update my env file and comment out AWS_URL to fix the issue.

I have an Attachment.php model and use this to generate a redirect url

return Storage::temporaryUrl($this->file_name, now()->addMinutes(5), [
            'ResponseContentType' => $this->original_mime_type,
            'ResponseContentDisposition' => 'attachment; filename="' . utf8_encode($this->original_name) . '"',
        ]);

My generated url was {subdomain}/{bucketname}/{path}, it is now {awsdomain}/{bucketname}/{path}

@dwightwatson
Copy link
Contributor

The more I look into this I think this change is incompatible with AWS S3 - it's presigned URL expects to be the AWS URL. This should probably be removed from the S3 driver and moved to a DO driver or the end-user's app as needed.

@jnoordsij
Copy link
Contributor Author

Hmmm I fear then that actually the host is (or rather should be) used in signing for AWS S3, and swapping the host afterwards should break things. But DO probably has something in their backend replacing the actual host (wether it is the normal endpoint, default cdn or custom subdomain) with the default endpoint.

If I try presigning the url for DO with the actual host I'm using (the custom subdomain), the generated URL doesn't actually work. I think the best course of action would be changing the config key used for replacing host in temporary URLs, so that by default it doesn't override existing setups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants