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
Add support for listening on a UNIX socket instead of IP #116259
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @DataGhost
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
if socket_path is not None: | ||
# They didn't find a way to put this in aiohttp yet so we have to do it here | ||
# https://github.com/aio-libs/aiohttp/issues/4155#issuecomment-643509809 | ||
if self.socket_permissions is not None: | ||
try: | ||
os.chmod(socket_path, self.socket_permissions) | ||
except OSError as error: | ||
_LOGGER.error( | ||
"Failed to change permissions on %s: %s", socket_path, error | ||
) | ||
if self.socket_user is not None or self.socket_group is not None: | ||
try: | ||
shutil.chown( | ||
socket_path, | ||
self.socket_user or -1, | ||
self.socket_group or -1, | ||
) | ||
except OSError as error: | ||
_LOGGER.error( | ||
"Failed to change user/group on %s: %s", socket_path, error | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does blocking I/O in the event loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes that's a pretty silly thing to just forget.
…g calls outside event loop
Proposed change
I'm running HA in a Docker container on Linux with a reverse proxy setup through Apache that takes care of outside connectivity. Binding HA to 127.0.0.1 works in not having the instance directly accessible from outside the machine, but it will still be accessible from the machine itself bypassing the proxy. I'm personally moving as many localhost-only services as possible to UNIX sockets so they won't need a port and can be locked down to only the relevant users/groups having access to them. As a bonus there's usually a little bit of a performance improvement as well, as the network stack isn't used and the packets don't need to traverse the firewall either, however this impact will be negligible on most/all HA instances I believe.
Type of change
Additional information
Aiohttp already supports this easily by using a
UnixSite
instead of aTCPSite
. That is exactly what I've done. Since the socket creation is affected by the activeumask
, it generally results in a socket with 755 permissions allowing effectively only the user to connect, which could be undesirable depending on the setup (e.g. shared group between HA and the proxy), so the effective permissions after creation are configurable.I have no idea what tests to make for this, if any. It looks like the TCP client isn't tested either. The
aiohttp.test_utils.TestClient
doesn't take into account any possible HA configuration changes in terms of bound interfaces and port but talks more or less directly to the app, seemingly bypassing most of the network stack. Tests still pass if I block traffic to the port configured in the respective tests, and in the socket case even if I set the socket permissions to 000 so I don't think it's easy to test this in the same style as the rest. I have at least verified that this is working for me.A sample configuration would be:
For me, a more natural configuration would be
however so far I haven't really found this extra nesting level being used for things other than defining multiple similar entities, so I left it as a flat structure for now. That seems to be the preferred way.
Checklist
ruff format homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: