Skip to content
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

DietPi-Software | Sonarr/Radarr/Lidarr: Several enhancements #3349

Merged
merged 12 commits into from
Feb 25, 2020

Conversation

MichaIng
Copy link
Owner

@MichaIng MichaIng commented Jan 19, 2020

Status: Ready

  • Changelog
  • Add info to online docs

Reference: #3347 (comment)

Commit list/description:

  • DietPi-Software | Sonarr/Radarr/Lidarr: Enhance reinstall by skipping download and install of new binaries when install dir already exist. Previously the new archive was always downloaded and merged into the old one, which could lead to obsolete files and missing migration steps. Print an info that updates should be done via internal updater, which is generally safer. If the instance is broken, hence internal updater cannot work, the install dir must be removed manually first.
  • DietPi-Software | Sonarr/Radarr/Lidarr: Create run user with "dietpi" as primary group and skip creation of group with same name. This resolves potential issues where supplementary groups are overwritten by the systemd units "Group=" setting.
  • DietPi-Software | Sonarr/Radarr/Lidarr: Enable automated service restart after internal updater has finished. Prior to this, internal updates led to a service stop, which then needed to be restarted manually.
  • DietPi-Software | Sonarr/Radarr/Lidarr: Harden service permissions by allowing R/W access only to install dir, log dir and /mnt. The whole /mnt dir is granted since this is the most common dir where users might place downloads, if outside of /mnt/dietpi_userdata. UNIX permissions must match as well, since ReadWritePaths only adds exclusions to ProtectSystem=strict but does not add additional permissions. This change must be communicated since user might currently use custom dirs outside the permitted ones. Those can easily be added to the ReadWritePaths list via dietpi-services GUI service edit.
  • DietPi-Software | Sonarr/Radarr/Lidarr: Enhance uninstall by cleanly disable and remove the systemd unit with all drop-in configs.

+ DietPi-Software | Sonarr/Radarr/Lidarr: Enhance reinstall by skipping download and install of new binaries when install dir already exist. Previously new binaries are downloaded but moving them in place failed. Print an info that updates should be done via internal updater, which is generally safer due to possible migration steps. If the instance is broken, hence internal updater cannot work, the install dir must be removed manually first.
+ DietPi-Software | Sonarr/Radarr/Lidarr: Create run user with "dietpi" as primary group and skip creation of group with same name. This resolves potential issues where supplementary groups are overwritten by the systemd units "Group=" setting.
+ DietPi-Software | Sonarr/Radarr/Lidarr: Enable automated service restart after internal updater has finished. Prior to this, internal updates led to a service stop, which then needed to be restarted manually.
+ DietPi-Software | Sonarr/Radarr/Lidarr: Harden service permissions by allowing R/W access only to install dir, log dir and /mnt. The whole /mnt dir is granted since this is the most common dir where users might place downloads, if outside of /mnt/dietpi_userdata. UNIX permissions must match as well, since ReadWritePaths only adds exclusions to ProtectSystem=strict but does not add additional permissions. This change must be communicated since user might currently use custom dirs outside the permitted ones. Those can easily be added to the ReadWritePaths list via dietpi-services GUI service edit.
+ DietPi-Software | Sonarr/Radarr/Lidarr: Enhance uninstall by cleanly disable and remove the systemd unit with all drop-in configs.
@MichaIng MichaIng added this to the v6.29 milestone Jan 19, 2020
+ CHANGELOG | Sonarr/Radarr/Lidarr: Several enhancements
+ DietPi-Software | Sonarr/Radarr/Lidarr: Internal updater requires R/W access to /tmp
@MichaIng MichaIng requested review from Fourdee and Joulinar January 29, 2020 14:38
@MichaIng
Copy link
Owner Author

MichaIng commented Jan 29, 2020

@Fourdee @Joulinar
Do you guys have a suggestion which tradeoff between security and comfort is best here?

I would generally like to add hardenings to our systemd services, the following is very strict:

ProtectSystem=strict # Deny write permissions to EVERYTHING
ProtectHome=true # Deny even read permissions to /home + /root + /run/user
PrivateDevices=true # Deny even read to all physical devices (/dev/null /dev/zero /dev/random and similar is allowed)
ProtectKernelTunables=true # Deny write access to most in /proc and /sys
ProtectControlGroups=true # Deny write access to /sys/fs/cgroup
ReadWritePaths=/opt/NzbDrone /mnt /var/log/sonarr /tmp # Re-allow read access to listed dirs

So practically everything but /opt/NzbDrone /mnt /var/log/sonarr /tmp is read-only, /home + /root + /run/user + physical /dev is even non-readable. This protects the system against harm, bugs, accidents, unintended usage and such.

I left whole /mnt in R/W, since many users do not leave their download/media dirs in /mnt/dietpi_userdata, but move them to other external drives and network mounts etc. Note that ReadWritePaths= never "adds" permissions, it practically just excludes the listed dirs from ProtectSystem=. Hence regular UNIX user/group permissions are still relevant for those, to have read/write/nothing permissions.

So while this should not break usage for ~95% of users, I know that some mount drives to /home/<user> or use other mount points and random paths. Those can be added relatively simple to the ReadWritePaths list via dietpi-services > Edit, but yeah, it is an additional step that at least needs to be communicated clearly.
Accessing/mounting things from/to /home is very bad practice anyway (for "system" services) and caused already a lot of non-DietPi-specific bug reports, difficult to debug, so it is probably even good to have users running into issues with this at an earlier stage. E.g. MariaDB service has ProtectHome=true by default, hence when users move/mount userdata into their home, it cannot start, even that dir/file permissions match.

PrivateTmp= is another flag that is added to more and more systemd units by default, to isolate their tmp data. For Sonarr/Radarr/Lidarr this would currently break DietPi-Arr_to_RAM, sadly. But it can be added when moving tmpfs databases to /run, e.g. However this is another topic with more work and testing required.

Some feedback/ideas about this whole service isolation topic would be great, as I want to have some consistency across all our services, hence add hardening flags to all of them, which should have an acceptable trade-off between security and usability/comfort.

+ CHANGELOG | Wording
@Joulinar
Copy link
Collaborator

Hi,
sorry for the delay but I was thinking on it for a while.

In general I like the approach to protect and harden the system. It's a good step to protect the system from config issues and other things. But yeah it will restrict users to the given file systems. So a good and clear communication is needed before switching to this model. To restrict r/w possibilities on the image, is something similar we are using on my company. There we provide as well very strict images. Sometimes this leads to quite a lot of discussions, if customer trying to install some software that requires a specific path which is write protected. So it would require good documentation which mount point could be used to install own software (not in dietpi catalog).

Next to that, what will happen with user who already use areas we are going to protect? Will their software stop working or do they need to migrate? Or will there be an option for advanced users to disable the protection and keep on running with the old model? Probably this should be active for fresh installed images only, to avoid breaking systems (not sure if this is even possible to separate).

If your estimation is right and it will work for 95% of users, it will be a good step. Because you never will find a solution that fits for everybody.

@MichaIng
Copy link
Owner Author

@Joulinar

Sometimes this leads to quite a lot of discussions, if customer trying to install some software that requires a specific path which is write protected.

In our case it is only the specific service (e.g. Sonarr, installed via dietpi-software), which has this limited permissions, everything outside of it is not affeted, hence custom installed software still has permissions according to UNIX user/group/permissions and the how the service is implemented. So all mount points can be used exactly the same way.

Next to that, what will happen with user who already use areas we are going to protect? Will their software stop working or do they need to migrate? Or will there be an option for advanced users to disable the protection and keep on running with the old model? Probably this should be active for fresh installed images only, to avoid breaking systems (not sure if this is even possible to separate).

Jep, I didn't add a forced reinstall, hence existing installs will stay unaffected. But there are reinstalls done on patches for earlier versions. We definitly need to add a prompt at least, and give easy to follow instructions about how to allow R/W access to custom paths. Same needs to be done in the docs for fresh installs. Probably it is best to simply prompt all Sonarr/Radarr/Lidarr users on v6.29 update about the new hardenings and link to online docs for detailed instructions. Those will become relevant only when doing a reinstall manually or update from old versions, but users do reinstalls sometimes to fix broken instances and for other reasons, hence it does not hurt to add custom R/W paths right from the beginning, if needed.

If your estimation is right and it will work for 95% of users, it will be a good step. Because you never will find a solution that fits for everybody.

Jep, whenever we automate something, we do choices/apply defaults, hence it will never suite everyone 100%. However as long as things work the same way they always did, users know about it and learned already how to deal/change. But when doing changes, this must be done more sensitive, thought through and communicated well.

@Joulinar
Copy link
Collaborator

Jep, whenever we automate something, we do choices/apply defaults, hence it will never suite everyone 100%. However as long as things work the same way they always did, users know about it and learned already how to deal/change. But when doing changes, this must be done more sensitive, thought through and communicated well.

fully agreed

@Joulinar
Copy link
Collaborator

Joulinar commented Feb 9, 2020

@MichaIng
btw do I need to push any button to move ahead with this pull request?

@MichaIng
Copy link
Owner Author

MichaIng commented Feb 9, 2020

@Joulinar
Nope not required. I'm just concentrating on image creation currently, will come back to this PR afterwards.

@MichaIng MichaIng linked an issue Feb 9, 2020 that may be closed by this pull request
+ DietPi-Patch | Sonarr/Radarr/Lidarr: Inform user about permission changes: #3349
+ CHANGELOG | Sonarr/Radarr/Lidarr: Those require write permissions to /tmp as well, which is granted via systemd unit already
+ DietPi-Software | Sonarr/Radarr/Lidarr: Remove sonarr/radarr/lidarr groups as well on uninstall. This is required for pre-v6.29 installs in case a reinstall changed the users primary group to "dietpi" as intended.
+ DietPi-Patch | Typo + add empty line as visual enhancement
+ DietPi-Patch | Wording clarification
+ DietPi-Patch | Typo
@MichaIng MichaIng merged commit 9939ef9 into dev Feb 25, 2020
@MichaIng MichaIng deleted the arr_enhancements branch February 25, 2020 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Two questions.
2 participants