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

Don't expose HTTP API for secure clusters #6408

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions distributed/distributed-schema.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,15 @@ properties:

These modules will have a ``routes`` keyword that gets added to the main HTTP Server.
This is also a list that can be extended with user defined modules.
These routes should be read-only; they should not be able to affect cluster state.
insecure-routes:
type: array
description: |
A list of modules that will only be added as HTTP routes if unauthenticated client connections are allowed

These routes offer the ability to affect cluster state. Unlike client connections, HTTP routes do not support
authentication. Since the client is the primary way to affect cluster state, it's assumed that if client
connections require authentication, these routes should not be exposed.
allowed-imports:
type: array
description: |
Expand Down
3 changes: 2 additions & 1 deletion distributed/distributed.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,11 @@ distributed:
- distributed.http.scheduler.prometheus
- distributed.http.scheduler.info
- distributed.http.scheduler.json
- distributed.http.scheduler.api
- distributed.http.health
- distributed.http.proxy
- distributed.http.statics
insecure-routes:
- distributed.http.scheduler.api

allowed-imports:
- dask
Expand Down
11 changes: 10 additions & 1 deletion distributed/http/scheduler/tests/test_scheduler_http.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
from dask.sizeof import sizeof

from distributed.utils import is_valid_xml
from distributed.utils_test import gen_cluster, inc, slowinc
from distributed.utils_test import gen_cluster, inc, slowinc, tls_only_security


@gen_cluster(client=True)
Expand Down Expand Up @@ -259,6 +259,15 @@ async def test_api(c, s, a, b):
assert (await resp.text()) == "API V1"


@gen_cluster(client=True, clean_kwargs={"threads": False}, security=tls_only_security())
async def test_api_disabled_if_secure(c, s, a, b):
async with aiohttp.ClientSession() as session:
async with session.get(
"http://localhost:%d/api/v1" % s.http_server.port
) as resp:
assert resp.status == 404
Comment on lines +262 to +268
Copy link
Member

Choose a reason for hiding this comment

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

if there is a typo, the route is actually removed or the version number is bumped, this test would still pass.
You could parametrize over security and ensure that the route is reachable if security is disabled to make sure the test assumption is actually valid

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The above test_api should effectively do that (I just copied that test to write this one).

I also found this testing less thorough than I'd like ideally (would be nice to have a scheduler.py unit test with a dummy route for the insecure-routes being dropped, plus this test here confirming that the API routes were dropped by default). But I noticed there's no testing in scheduler.py for HTTP routes, only these sorts of tests.



@gen_cluster(client=True, clean_kwargs={"threads": False})
async def test_retire_workers(c, s, a, b):
async with aiohttp.ClientSession() as session:
Expand Down
4 changes: 4 additions & 0 deletions distributed/scheduler.py
Original file line number Diff line number Diff line change
Expand Up @@ -2951,6 +2951,10 @@ def __init__(
except ImportError:
show_dashboard = False
http_server_modules.append("distributed.http.scheduler.missing_bokeh")
if not self.security.require_encryption:
http_server_modules.extend(
dask.config.get("distributed.scheduler.http.insecure-routes")
)
routes = get_handlers(
server=self, modules=http_server_modules, prefix=http_prefix
)
Expand Down
8 changes: 6 additions & 2 deletions docs/source/http_services.rst
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,11 @@ Pages and JSON endpoints served by the scheduler
Scheduler API
-------------

Scheduler methods exposed by the API with an example of the request body they take
Scheduler methods exposed by the API with an example of the request body they take.

.. note::
To prevent unauthorized access, the scheduler API is disabled by default if `tls`_ is enabled.
See the ``distributed.http.insecure-routes`` :doc:`config <configuration>` setting.
Comment on lines +58 to +59
Copy link
Member

Choose a reason for hiding this comment

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

The API is disabled if tls is disabled. This sentence is the other way round, isn't it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe the API is enabled if mTLS is disabled -> API is disabled if mTLS is enabled:

if not self.security.require_encryption:
http_server_modules.extend(
dask.config.get("distributed.scheduler.http.insecure-routes")
)


- ``/api/v1/retire_workers`` : retire certain workers on the scheduler

Expand All @@ -63,7 +67,7 @@ Scheduler methods exposed by the API with an example of the request body they ta
}

- ``/api/v1/get_workers`` : get all workers on the scheduler
- ``/api/v1/adaptive_target`` : get the target number of workers based on the scheduler's load
- ``/api/v1/adaptive_target`` : get the target number of workers based on the scheduler's load

Individual bokeh plots
----------------------
Expand Down