add dynamic check of origin for CORS #7904

bentsku · 2023-03-18T20:36:02Z

This is a follow up from #7554, from an idea proposed by @whummer (in this comment) , with dynamic handling of CORS origin for web resource served by LocalStack.
This is not mergeable as is, as it still has some logging regarding performance, but I'd like to have some comments / opinions.

Scenario:

LocalStack is serving a resource meant to be accessed from the browser (CloudFront or S3 website).

But this resource needs to access LocalStack resources as well. Right now, the only way is to set the environment variables of LocalStack to allow the Origin (let’s say, http://<bucket>.s3-website.localhost.localstack.cloud:4566).

Fix

This PR adds a dynamic check of the origin, to validate if it follows some regex matching 2 service domains known for serving content accessible from a user browser (thus enforcing CORS). We can then add a regex for every resource subdomain in that situation.

The performance right now is around 4µs on my local machine when a request would be either rejected or matching s3-website or cloudfront. The performance would not be affected for most requests, as we either don't match CORS because of no Origin header, or the request is normally allowed.

\cc @whummer @dfangl @alexrashed

coveralls · 2023-03-18T21:23:27Z

Coverage: 81.747% (-0.007%) from 81.753% when pulling 711510b on allow-dynamic-internal-cors into 00d3a59 on master.

github-actions · 2023-03-18T22:23:41Z

LocalStack integration with Pro

      3 files ±  0       3 suites ±0 1h 33m 40s ⏱️ + 1m 9s
1 832 tests +18 1 441 ✔️ +12 391 💤 +  6 0 ❌ ±0
2 560 runs +28 1 808 ✔️ +13 752 💤 +15 0 ❌ ±0

Results for commit a693385. ± Comparison against base commit 5949082.

♻️ This comment has been updated with latest results.

whummer

Cool! Nice approach that can further streamline the CORS UX. 🎉

Was able to replicate with the following steps:

create a CloudFront distribution, as per our docs:

$ awslocal s3 mb s3://test
$ echo 'Hello World' > /tmp/hello.txt
$ awslocal s3 cp /tmp/hello.txt s3://test/hello.txt --acl public-read

$ domain=$(awslocal cloudfront create-distribution --origin-domain-name test.s3.amazonaws.com | jq -r '.Distribution.DomainName')
$ echo $domain
ce6554a5.cloudfront.localhost.localstack.cloud

create a simple index.html file that connects to the enpoint:

<html>
<script>
fetch('http://ce6554a5.cloudfront.localhost.localstack.cloud:4566/hello.txt');
</script>
</html>

upload the file to a bucket and expose it as an S3 website:

awslocal s3 cp /tmp/index.html s3://test/index.html --content-type text/html
awslocal s3api put-bucket-website --bucket test --website-configuration 'ErrorDocument={Key=index.html},IndexDocument={Suffix=index.html}'

open http://test.s3-website.localhost.localstack.cloud:4566/index.html in the browser, observe the Network console (request to http://ce6554a5.cloudfront.localhost.localstack.cloud:4566/hello.txt)

Result:

returns a 403 error on current master 🟥
returns 200 and expected file content with the changes in this PR ✅

To me, this achieves a decent balance between improving convenience (works out of the box without extra configs), while not sacrificing security (requests still only allowed from *.localhost.localstack.cloud pages in the browser). 👍 As usual with CORS or security-critical configurations, would be good to get a second pair of eyes on it, though.. 👀 /cc @dfangl @alexrashed @thrau

localstack/aws/handlers/cors.py

whummer · 2023-03-20T12:43:23Z

localstack/aws/handlers/cors.py

    for protocol in {"http", "https"}:
-        for port in _ports:
+        for port in _get_allowed_cors_ports():


nit: we could also use _ALLOWED_INTERNAL_PORTS here, for consistency. (although the performance impact is probably negligible).

That's what I thought and did at first, but then it would be generated only once, and in your other tests where you monkey patch ALLOWED_CORS_ORIGINS, you'd have to monkey patch _ALLOWED_INTERNAL_PORTS before. I thought it'd make your tests a bit more complicated.

localstack/tests/unit/test_cors.py

Lines 18 to 21 in be7178d

# test allowed origins for extended config (HTTPS edge port 443, HTTP edge port 4566)

monkeypatch.setattr(config, "EDGE_PORT", 443)

monkeypatch.setattr(config, "EDGE_PORT_HTTP", 4566)

origins = cors._get_allowed_cors_origins()

tests/unit/test_cors.py

bentsku · 2023-03-20T19:01:38Z

Thanks @whummer for creating a reproducible sample!

Just a note:

(requests still only allowed from *.localhost.localstack.cloud pages in the browser).

The requests are only allowed from *.s3-website.localhost.localstack.cloud and *.cloudfront.localhost.localstack.cloud for now.
Also, the webpage served by those 2 "dynamic origins" would be able to not only access cloudfront, but also for example the SQS API, or any API using the default LocalStack configuration.
Just giving precision for the second pair of eyes 👀

thrau

I think this is fantastic 🎉 I don't see a big issue right now 👍

thrau · 2023-03-25T13:16:41Z

localstack/aws/handlers/cors.py

@@ -166,11 +196,25 @@ def is_cors_origin_allowed(headers: Headers) -> bool:
        return True

    @staticmethod
+    @log_duration(min_ms=0)


nit: could we remove this again?

thrau · 2023-03-25T13:21:04Z

localstack/aws/handlers/cors.py

 from ...constants import LOCALHOST, LOCALHOST_HOSTNAME, PATH_USER_REQUEST
+from ...utils.bootstrap import log_duration


nit: ideally we don't use relative imports of modules that are outside the scope of the component we are building. basically: relative imports are used when things belong together in a component, absolute imports are used when importing code that represents a dependency to another component

Right, I'm not sure why there are dynamic imports here, I can change them 👍

bentsku temporarily deployed to localstack-ext-tests March 18, 2023 20:36 — with GitHub Actions Inactive

bentsku temporarily deployed to localstack-ext-tests March 18, 2023 20:39 — with GitHub Actions Inactive

bentsku marked this pull request as ready for review March 18, 2023 22:54

bentsku requested a review from thrau as a code owner March 18, 2023 22:55

bentsku requested review from whummer and thrau and removed request for thrau March 18, 2023 22:55

whummer approved these changes Mar 20, 2023

View reviewed changes

bentsku temporarily deployed to localstack-ext-tests March 21, 2023 16:37 — with GitHub Actions Inactive

bentsku changed the title ~~wip: add dynamic check of origin for CORS~~ add dynamic check of origin for CORS Mar 21, 2023

thrau requested review from dfangl and removed request for thrau March 21, 2023 17:36

bentsku force-pushed the allow-dynamic-internal-cors branch from a693385 to c87decd Compare March 24, 2023 22:31

bentsku added 3 commits March 25, 2023 13:47

add dynamic check of origin for cors

22009af

fix

06315bd

fix nits

f61cd88

thrau force-pushed the allow-dynamic-internal-cors branch from c87decd to f61cd88 Compare March 25, 2023 12:47

thrau approved these changes Mar 25, 2023

View reviewed changes

fix nits

711510b

whummer approved these changes Mar 29, 2023

View reviewed changes

whummer merged commit 987a4d7 into master Mar 29, 2023

whummer deleted the allow-dynamic-internal-cors branch March 29, 2023 12:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

add dynamic check of origin for CORS #7904

add dynamic check of origin for CORS #7904

Uh oh!

bentsku commented Mar 18, 2023 •

edited

Loading

Uh oh!

coveralls commented Mar 18, 2023 •

edited

Loading

Uh oh!

github-actions bot commented Mar 18, 2023 •

edited

Loading

Uh oh!

whummer left a comment •

edited

Loading

Uh oh!

Uh oh!

whummer Mar 20, 2023

Uh oh!

bentsku Mar 20, 2023

Uh oh!

Uh oh!

bentsku commented Mar 20, 2023 •

edited

Loading

Uh oh!

thrau left a comment

Uh oh!

thrau Mar 25, 2023

Uh oh!

thrau Mar 25, 2023

Uh oh!

bentsku Mar 25, 2023 •

edited

Loading

Uh oh!

Uh oh!

	# test allowed origins for extended config (HTTPS edge port 443, HTTP edge port 4566)
	monkeypatch.setattr(config, "EDGE_PORT", 443)
	monkeypatch.setattr(config, "EDGE_PORT_HTTP", 4566)
	origins = cors._get_allowed_cors_origins()

		from ...constants import LOCALHOST, LOCALHOST_HOSTNAME, PATH_USER_REQUEST
		from ...utils.bootstrap import log_duration

Uh oh!

add dynamic check of origin for CORS #7904

add dynamic check of origin for CORS #7904

Uh oh!

Conversation

bentsku commented Mar 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Scenario:

Fix

Uh oh!

coveralls commented Mar 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LocalStack integration with Pro

Uh oh!

whummer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

whummer Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

bentsku Mar 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bentsku commented Mar 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thrau left a comment

Choose a reason for hiding this comment

Uh oh!

thrau Mar 25, 2023

Choose a reason for hiding this comment

Uh oh!

thrau Mar 25, 2023

Choose a reason for hiding this comment

Uh oh!

bentsku Mar 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bentsku commented Mar 18, 2023 •

edited

Loading

coveralls commented Mar 18, 2023 •

edited

Loading

github-actions bot commented Mar 18, 2023 •

edited

Loading

whummer left a comment •

edited

Loading

bentsku commented Mar 20, 2023 •

edited

Loading

bentsku Mar 25, 2023 •

edited

Loading