search

crownstone / crownstone-home-assistant

Beta version of the Crownstone integration for Home Assistant
6 stars 3 forks source link

USB dongle not found #5

Closed RSpiertz closed 3 years ago

RSpiertz commented 3 years ago

I have HA running in a docker container (nothing special, just FROM homeassistant/home-assistant). The dongle is passed in as /dev/crownstone by docker compose, but not found by the crownstone HACS plugin. No ports are found at all by list_ports.comports() in crownstone_uart/core/uart/UartManager.py. I've fixed it for now by changing self.uart_instance.initialize_usb() to self.uart_instance.initialize_usb('/dev/crownstone') in custom_components/crownstone/helpers.py, but that's of course not a long term solution. If you need more info to debug this, let me know!

mrquincle commented 3 years ago

We've been requested by HA not to list the serial ports ourselves. It has been recommended to indeed have the user manually input the device they need to use. This option has of course to become available in the GUI as well. Thanks for reporting!

RicArch97 commented 3 years ago

Thanks @RSpiertz !

The integration has since been reworked for the new PR to HA core, with the idea to manually select the port from a list of comports found. The plan is to move these changes over to HACS as well in the upcoming weeks.

Seeing your comments about docker however makes me realize their should be an option to manually type a serial path as well. Is /dev/crownstone a static port as in that this will never change even if the usb is unplugged en plugged in again? And is /dev/serial/by-id also exposed in a docker container? We currently use the symlink in /dev/serial/by-id to always connect to the correct serial port.

RSpiertz commented 3 years ago

Seeing your comments about docker however makes me realize their should be an option to manually type a serial path as well. Is /dev/crownstone a static port as in that this will never change even if the usb is unplugged en plugged in again? And is /dev/serial/by-id also exposed in a docker container? We currently use the symlink in /dev/serial/by-id to always connect to the correct serial port.

For me /dev/crownstone is indeed static, added using an udev entry on the host and passed into the container by docker compose. The directory /dev/serial doesn't exist in my docker container. I could pass it in, but there are also devices in there that Home Assistant doesn't need, so it wouldn't be a good idea from a security pov.

mrquincle commented 3 years ago

Device path I'd also recommend to allow people to define a path without limiting them to a drop-down.

Boot process The boot process is not entirely deterministic, so you can't use assume that /dev/ttyUSB0 and /dev/ttyUSB1 will always ordered that way if you have multiple dongles. Or /dev/ttyACM0 and /dev/ttyACM1. For that reason, you will need some mechanism in place. That's why they invented /dev/serial/by-id. Not using it would be silly. :-)

Security perspective Some remarks:

Additional info for the user It's probably good to explain to the user how to set a udev rule or point to a place for more information.

RicArch97 commented 3 years ago

Device path I'd also recommend to allow people to define a path without limiting them to a drop-down.

I will add this soon to the PR (these changes will also be ported to the hacs version)

Additional info for the user It's probably good to explain to the user how to set a udev rule or point to a place for more information.

Yes, we should probably provide this information for docker users, HassOS users would not need this

RSpiertz commented 3 years ago

The boot process is not entirely deterministic, so you can't use assume that /dev/ttyUSB0 and /dev/ttyUSB1 will always ordered that way if you have multiple dongles. Or /dev/ttyACM0 and /dev/ttyACM1. For that reason, you will need some mechanism in place. That's why they invented /dev/serial/by-id. Not using it would be silly. :-)

Agreed on not using /dev/tty* as it can change, but I like /dev/crownstone a lot more than /dev/serial/by-id/usb-Silicon_Labs_CP2104_USB_to_UART_Bridge_Controller_016788BD-if00-port0 :)

  • By passing --device you'll run into issues on plugging the dongle in and out.

I didn't check whether docker compose uses this method of passing (wouldn't know what else), but just tested it and plugging it out and back in doesn't cause any problem.

  • I am not so convinced about /dev/serial (what particular security risk is that, but see below).

HA doesn't need access to all devices in /dev/serial (in my case for example the zigbee stick nor more importantly the crypto stick), closing down that attack vector is better for security and makes the principle of least privilege happy.

  • The symlinks in /dev/serial are created on the fly, so not sure if they truly don't exist.

They don't in my running docker container, the whole of /dev/serial doesn't exist.

It's probably good to explain to the user how to set a udev rule or point to a place for more information.

In case you'd like an example, this is the one I use: ACTION=="add", ATTRS{idVendor}=="10c4", ATTRS{idProduct}=="ea60", SYMLINK+="crownstone"

RicArch97 commented 3 years ago

Agreed on not using /dev/tty* as it can change, but I like /dev/crownstone a lot more than /dev/serial/by-id/usb-Silicon_Labs_CP2104_USB_to_UART_Bridge_Controller_016788BD-if00-port0 :)

The integration shows a list of the /dev/tty* ports to select from in the setup, and saves the linked /dev/serial/by-id of the selected port, then in the entry setup the link is read backwards to get current /dev/tty* port that the USB is on, to pass it to crownstone-uart. This is for non-docker usage. Seeing docker already handles that part we can just add a manual option so you can put in /dev/crownstone and we should be good to go

mrquincle commented 3 years ago

HA doesn't need access to all devices in /dev/serial (in my case for example the zigbee stick nor more importantly the crypto stick), closing down that attack vector is better for security and makes the principle of least privilege happy.

Interesting, a crypto stick! I guess people were running a docker on dedicated hardware, but this sounds like your actual system. I understand in that case!

The zigbee stick seems like HA would benefit to have access to. :-D

They don't in my running docker container, the whole of /dev/serial doesn't exist.

The folder /dev/serial might not exist because you have your own udev rule?

It's the kernel drivers that are responsible for the device node creation. The kernel communicates with udev (the device manager) through uevent messages. Then udev can dynamically manage device nodes in /dev, including permissions, symlinks, etc. through .rules scripts. In e.g. /lib/udev/rules.d/60-serial.rules on Debian-derived systems, you'll see rules like:

IMPORT{builtin}="path_id"
ENV{ID_PATH}=="?*", ENV{.ID_PORT}=="", SYMLINK+="serial/by-path/$env{ID_PATH}"
ENV{ID_PATH}=="?*", ENV{.ID_PORT}=="?*", SYMLINK+="serial/by-path/$env{ID_PATH}-port$env{.ID_PORT}"

The folder is also deleted automatically when all serial devices have been plugged out. Hence, I'm still not entirely convinced that this mechanism doesn't happen in docker. ;-)

If hot-plugging doesn't lead to issues, that's perfect. If it does, you now know to look for cgroup.allow. :-)

RSpiertz commented 3 years ago

Interesting, a crypto stick! I guess people were running a docker on dedicated hardware, but this sounds like your actual system. I understand in that case!

It's normally not in my home server indeed, but it has come in handy at times :)

The zigbee stick seems like HA would benefit to have access to. :-D

There's a deconz container running next to the HA one, so no direct stick access needed.

The folder /dev/serial might not exist because you have your own udev rule?

That could be indeed!

If hot-plugging doesn't lead to issues, that's perfect. If it does, you now know to look for cgroup.allow. :-)

Thanks, that's going to be my next option if the current one does cause problems down the road :)

RicArch97 commented 3 years ago

The integration has been updated with an option to manually type the port for the Crownstone USB.

If you want, you can add all the files from that PR as a custom component and test it. That PR is currently only limited to one platform, the HACS version still needs an overhaul :)

mrquincle commented 3 years ago

@RSpiertz does it work? :-)

RSpiertz commented 3 years ago

I'm getting "The custom integration 'crownstone' does not have a valid version key (None) in the manifest file and was blocked from loading." when adding the code from the PR as a custom component. I'm afraid I don't have time to look into it further at the moment.

RicArch97 commented 3 years ago

I'm getting "The custom integration 'crownstone' does not have a valid version key (None) in the manifest file and was blocked from loading." when adding the code from the PR as a custom component. I'm afraid I don't have time to look into it further at the moment.

Custom components need a version in the manifest indeed. Core components don't, that's why it's not in the manifest of that PR. You could simply add "version" : "1" for example to make it work. See this for an example.

Note that the PR is currently going through changes because of ongoing review, you could simply wait for the core integration to go live, or the next version of our integration on HACS, which will include all the new changes currently being made as well.

RSpiertz commented 3 years ago

It seems to work! I've reverted to the HACS component after quick testing though, as this one doesn't support presence and energy monitoring, or at least, it didn't create entities for them.

RicArch97 commented 3 years ago

Good to hear! The PR does indeed not support presence/energy monitoring, as it is limited to one platform per PR. Thanks a lot for testing!

RicArch97 commented 3 years ago

Added manual configuration in release 2.0.0.