Add a uci config file for matter and run matter as root - #51
Conversation
* Add a matter uci config file with one config variable (wifi_iface) * Matter could not access ubus --> running as root for now Signed-off-by: Frederik Van Bogaert <frederik.vanbogaert@mind.be>
|
Frederik Van Bogaert seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Code Review
This pull request adds rpcd as a dependency, installs a new UCI configuration file, and comments out the user and group parameters in the init script, causing the daemon to run as root. The review feedback highlights a security regression where the daemon runs as root instead of the unprivileged matter user; it is recommended to restore the unprivileged user and update the ACL permissions instead. Additionally, it is suggested to rename the matter_uci.defaults file to avoid confusion, as it is installed as a configuration file rather than a first-boot defaults script.
| #procd_set_param user matter | ||
| #procd_set_param group matter |
There was a problem hiding this comment.
Running the daemon as root instead of the unprivileged matter user is a significant security regression. The reason the daemon could not access ubus is likely because the matter user lacks the necessary permissions in service/matter-netman/files/matter_acl.json to access the uci ubus object (provided by rpcd, which was added as a dependency in this PR) or other required ubus objects.
Instead of running as root, we should keep the daemon running as the matter user and update service/matter-netman/files/matter_acl.json to grant the necessary ubus permissions. For example, to allow reading UCI configurations, you can add the uci object with the get method to the ACL file:
"access": {
"otbr": {
"methods": ["*"]
},
"uci": {
"methods": ["get"]
}
}| #procd_set_param user matter | |
| #procd_set_param group matter | |
| procd_set_param user matter | |
| procd_set_param group matter |
There was a problem hiding this comment.
I think instead of running as root you should be able to just add the relevant objects to service/matter-netman/files/matter_acl.json to give the matter user access.
| $(INSTALL_DATA) ./files/bootstrap.sh $(1)/usr/share/matter/ | ||
| $(INSTALL_DATA) ./files/matter_acl.json $(1)/usr/share/acl.d | ||
| $(INSTALL_BIN) ./files/matter.init $(1)/etc/init.d/matter | ||
| $(INSTALL_DATA) ./files/matter_uci.defaults $(1)/etc/config/matter |
There was a problem hiding this comment.
The source file is named matter_uci.defaults, but it is installed directly as a UCI configuration file to /etc/config/matter. In OpenWrt, .defaults files typically refer to uci-defaults scripts (which are shell scripts executed on first boot). To avoid confusion, the source file should be renamed to matter.config or simply matter in the repository.
$(INSTALL_DATA) ./files/matter.config $(1)/etc/config/matter
There was a problem hiding this comment.
I think calling it matter.defaults is fine, have a look at otbr-agent.defaults for an example. They need to be installed into /etc/uci-defaults/; I think what you're doing here will overwrite the config file on a package upgrade.
Note: This PR also includes the change of #50 since we need it and it hasn't been merged yet