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

🐛fix CSP frame-ancestors for dynamic-sidecar services #2977

Merged
merged 5 commits into from
Apr 12, 2022

Conversation

GitHK
Copy link
Contributor

@GitHK GitHK commented Apr 8, 2022

What do these changes do?

Services like JupyterLab when running inside an iframe might also try to display an iframe (for example to display the Voila preview).

Related issue/s

How to test

Checklist

@GitHK GitHK requested review from sanderegg and pcrespov as code owners April 8, 2022 11:39
@GitHK GitHK requested a review from mguidon April 8, 2022 11:40
@GitHK GitHK self-assigned this Apr 8, 2022
@GitHK GitHK added a:dynamic-sidecar dynamic-sidecar service a:director-v2 issue related with the director-v2 service labels Apr 8, 2022
@GitHK GitHK added this to the a-mile-stone milestone Apr 8, 2022
@codecov
Copy link

codecov bot commented Apr 8, 2022

Codecov Report

Merging #2977 (51cb117) into master (d2d1348) will increase coverage by 0.2%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2977     +/-   ##
========================================
+ Coverage    80.4%   80.7%   +0.2%     
========================================
  Files         326     558    +232     
  Lines       16358   25636   +9278     
  Branches     2044    3392   +1348     
========================================
+ Hits        13161   20699   +7538     
- Misses       2723    4169   +1446     
- Partials      474     768    +294     
Flag Coverage Δ
integrationtests 65.7% <ø> (+<0.1%) ⬆️
unittests 75.9% <ø> (+1.9%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ules/dynamic_sidecar/docker_service_specs/proxy.py 100.0% <ø> (ø)
...ector_v2/modules/comp_scheduler/background_task.py 83.3% <0.0%> (-8.4%) ⬇️
...mcore_service_webserver/garbage_collector_utils.py 79.4% <0.0%> (-3.0%) ⬇️
...rector_v2/modules/comp_scheduler/base_scheduler.py 86.7% <0.0%> (-1.9%) ⬇️
...e_service_director_v2/modules/dask_clients_pool.py 92.4% <0.0%> (-1.6%) ⬇️
...imcore_service_webserver/garbage_collector_core.py 68.2% <0.0%> (-0.7%) ⬇️
...ice-library/src/servicelib/aiohttp/rest_routing.py 78.0% <0.0%> (ø)
...ces/director/src/simcore_service_director/utils.py 100.0% <0.0%> (ø)
...ages/models-library/src/models_library/clusters.py 100.0% <0.0%> (ø)
...dels-library/src/models_library/app_diagnostics.py 100.0% <0.0%> (ø)
... and 281 more

@GitHK GitHK changed the title 🐛fix CSP frame-ancestors with iframes inside dynamic-sidecar launched services 🐛fix CSP frame-ancestors for dynamic-sidecar services Apr 8, 2022
@@ -52,7 +52,7 @@ def get_dynamic_proxy_spec(
"swarm_stack_name": dynamic_sidecar_settings.SWARM_STACK_NAME,
"traefik.docker.network": swarm_network_name,
"traefik.enable": "true",
f"traefik.http.middlewares.{scheduler_data.proxy_service_name}-security-headers.headers.customresponseheaders.Content-Security-Policy": f"frame-ancestors {scheduler_data.request_dns}",
f"traefik.http.middlewares.{scheduler_data.proxy_service_name}-security-headers.headers.customresponseheaders.Content-Security-Policy": f"frame-ancestors {scheduler_data.request_dns} {scheduler_data.node_uuid}.services.{scheduler_data.request_dns}",
Copy link
Member

@pcrespov pcrespov Apr 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you need commas between these two values?

I see other entries of traefik that requires that ... for instance

f"traefik.http.middlewares.{scheduler_data.proxy_service_name}-security-headers.headers.accessControlAllowOriginList": ",".join(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: scheduler_data.node_uuid}.services.{scheduler_data.request_dns} is the entrypoint of a dynamic services, right? I see that the project id is not used for that. Does this assume that nodes have universal identifiers? i.e. two projects cannot include a node with the same NodeID as dynamic service ..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is exactly the entrypoint and we are saying to CSP that it can start iframes, to allow for iframe in iframes.

Yes, that is the same assumption as the legacy services!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GitHK @sanderegg OK, just for the record:
it would be good to identify dynamic services with something more than the node UUID (e.g. project_uuid/node_uuid)

We would like to guarantee the node-uuid uniquenes only within the scope of its project. That would help us a lot on:

  • simplifies duplication of projects
  • simplifies caching in metamodling iterations
  • ...

See #2133 for more details

Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, we need to discuss whether the project_id should be added. But I guess this can done in a separate PR

@GitHK GitHK requested a review from pcrespov April 11, 2022 09:51
@GitHK GitHK merged commit d7ea6e8 into ITISFoundation:master Apr 12, 2022
@GitHK GitHK deleted the fix-csp-policy branch April 12, 2022 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:director-v2 issue related with the director-v2 service a:dynamic-sidecar dynamic-sidecar service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants