Don't expose HTTP API for secure clusters #6408
Conversation
|
cc @jacobtomlinson @quasiben (for awareness) |
| @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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| 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. |
There was a problem hiding this comment.
The API is disabled if tls is disabled. This sentence is the other way round, isn't it?
There was a problem hiding this comment.
I believe the API is enabled if mTLS is disabled -> API is disabled if mTLS is enabled:
distributed/distributed/scheduler.py
Lines 2954 to 2957 in 3d7ea1a
|
slight off-topic: Is HTTPS something that would help? Is this something we can easily do? Are there reasons why we wouldn't want to do HTTPS? (We should create a follow up ticket if the answer is yes, we want to do this) |
There was a problem hiding this comment.
Thanks for raising this @gjoseph92. I agree that we should disable the HTTP API when mTLS is enabled. I would also be happy to just disable the HTTP API by default and make it opt-in.
I'm not especially excited about splitting HTTP routes into "secure" and "insecure" like this. It feels like we are leaking implementation details to the user here. We should definitely add some auth options to the API in the future, so then we would be left with a stray "insecure" config option that no longer makes sense.
Yeah, I was hesitant about that too. @jacobtomlinson @Matt711 would you prefer just leaving |
Yeah that's totally fine as we can still enable it in config on platforms where auth is handled outside of distributed. Thanks for picking this up! |
|
Sounds good. @jacobtomlinson would someone on your side be able to pick that up today, or should I? (I'd want to see a test confirming that the API is not enabled with the default config to avoid regressions FYI.) We're hoping to get the release out today. I'll close out this PR then. |
|
My guess is Jacob is out for the day (kind of late for him). Matt was an intern, whose last day was Friday. Think we are going to struggle to come up with someone to fix it with short notice (today). If we have some more time, that might be more doable. Edit: NVM appears this is already being addressed here ( dask/community#245 (comment) ) |
Closes #6407
I'd like to come up with a better name than
insecure-routesfor the config, since that implies that the others are secure (I don't know if that's something we really guarantee?)cc @Matt711 @jakirkham
pre-commit run --all-files