search

bus1 / dbus-broker

Linux D-Bus Message Broker
https://github.com/bus1/dbus-broker/wiki
Apache License 2.0
677 stars 79 forks source link

Update Systemd security settings #345

Open rahulsundaram opened 7 months ago

rahulsundaram commented 7 months ago

Based on the discussions in https://src.fedoraproject.org/rpms/dbus-broker/pull-request/11, I am proposing this change here. As part of https://fedoraproject.org/wiki/Changes/SystemdSecurityHardening which has been approved for Fedora 40, I am working on updating Systemd services to add additional hardening settings, please review this PR and let me know if you have any feedback

https://www.freedesktop.org/software/systemd/man/latest/systemd.exec.html has detailed information on each of these settings including the version of Systemd where they were introduced.

dvdhrm commented 7 months ago

Thanks for opening this PR! I think it looks good. I have to look into each individual key again, but on first glance this is all good.

dbus-broker-launch uses NSS services to resolve user-names (e.g., getpwnam(3)). These will pull in nss-plugins into the running process. Most modern plugins will communicate with a standalone service, so they will likely work fine under these restrictions. However, I have not fully audited this.

I think I am fine with trying to apply these, but we need to document that this will affect nss-plugins, and we need to make sure to allow opening any UDS-sockets, since that is what most of these use for communication.

rahulsundaram commented 7 months ago

Thanks for the review. I cleaned up the commit message a bit. Let me know if you would want to merge this as is or if there is an ask from me here on running any tests etc.

rahulsundaram commented 7 months ago

I have made changes requested but also added similar settings to the user service. While it is not part of the scope of change proposal I am working on, it seems reasonable to knock it out while we are here. If you want to split it out or deal with later, let me know

dvdhrm commented 5 months ago

I am very hesitant to merge this, given that I have not tested this and no runtime tests for the individual keys are included in this PR. This can easily lead to errors based on nss-modules on specific client machines, or other non-standard configuration setups.

Where do the individual keys of this PR come from? Did you verify they can be safely applied to dbus-broker? What is the rationale behind these keys? How did you evaluate which keys to apply, and which don't?

To merge this, I would have to survey each individual key, evaluate whether they can be safely applied to dbus-broker and document how this can possibly affect user-configurations. I currently do not see this happening anytime soon.

If you want to move this forward, can you elaborate on the individual keys why you think they are safe, possibly split this PR to merge less controversial keys first, or even providing tests for common setups to show that they do not break.