Change in behaviour - latest framework snapshot with DSM7 support

[th0ma7]

Setup

Package Name: synokernel-usbserial
Package Version: PR #4420

NAS Model: DS918+
NAS Architecture: apollolake
DSM version: 6.2.3 Update 3

Expected behavior

Since I've rebased against master which now includes DSM7 preliminary support, my development packages behave differently and fail to start. I've started investigating and didn't noticed much difference besides the permissions now being assigned to sc-synokernel-usbserial on files which wasn't the case before (probably expected on DSM7 but wasn't on DSM6) and not necessarely a blocker neither in theory.

$ ll -d /volume1/@appstore/synokernel-usbserial
drwxr-xr-x 1 sc-synokernel-usbserial synokernel-usbserial 38 Mar 30 20:08 /volume1/@appstore/synokernel-usbserial

Which makes me wonder if it isn't trying to run the service script as user sc-synokernel-usbserial which will necessarily fail (loading kernel modules require root access).

Other logs

New install log:

Tue Mar 30 20:07:36 EDT 2021
===> Step preuninst. USER= GROUP= SHARE_PATH=
Tue Mar 30 20:07:36 EDT 2021
===> Step postuninst. USER= GROUP= SHARE_PATH=
Invoke service_postuninst
Tue Mar 30 20:08:06 EDT 2021
===> Step preinst. USER=synokernel-usbserial GROUP= SHARE_PATH=
Tue Mar 30 20:08:08 EDT 2021
===> Step postinst. USER=synokernel-usbserial GROUP= SHARE_PATH=
Invoke service_postinst
Granting 'sc-synokernel-usbserial' unix ownership on /volume1/@appstore/synokernel-usbserial/var

Old install log:

Tue Mar 30 20:06:06 EDT 2021
===> Step preinst. USER= GROUP= SHARE_PATH=
Tue Mar 30 20:06:07 EDT 2021
===> Step postinst. USER= GROUP= SHARE_PATH=
Invoke service_postinst

[th0ma7]

Confirmed, it is trying to run the package as root from my own startup logs.
Normally I log everything under /tmp and moved the previous file and obtained the following:

th0ma7-nas:/tmp$ ll -d *usbserial*
-rw-r--r-- 1 sc-synokernel-usbserial synokernel-usbserial     772 Mar 30 20:25 synocli-kernelmodule-synokernel-usbserial.log
-rw-r--r-- 1 root                    root                 3000258 Mar 30 20:07 synocli-kernelmodule-synokernel-usbserial.log-OLD

And looking under the conf directory there is now a privilege file such as:

th0ma7-nas:/var/packages/synokernel-usbserial/conf$ cat privilege
{
  "defaults": {
    "run-as": "package"
  },
  "username": "sc-synokernel-usbserial",
  "ctrl-script": [
    {
      "action": "preinst",
      "run-as": "root"
    },
    {
      "action": "postinst",
      "run-as": "root"
    },
    {
      "action": "preuninst",
      "run-as": "root"
    },
    {
      "action": "postuninst",
      "run-as": "root"
    },
    {
      "action": "preupgrade",
      "run-as": "root"
    },
    {
      "action": "postupgrade",
      "run-as": "root"
    }
  ]
}

Switching "run-as": "root" solved the issue.
Still, at service setup the file ownership and creation must then be reworked as this will break packages.
My guess is that privilege file should not be created by default under < DSM-7.

[BenjV]

If you need root for this package you could add "run-as' root to the start action:
Example of a privilege file who runs all scripts as root (so also the start, stop and status of the start-stop-status script):

{
  "defaults": {
    "run-as": "package"
  },
  "username": "sc-synokernel-usbserial",
  "ctrl-script": [
    {
      "action": "preinst",
      "run-as": "root"
    },
    {
      "action": "postinst",
      "run-as": "root"
    },
    {
      "action": "preuninst",
      "run-as": "root"
    },
    {
      "action": "postuninst",
      "run-as": "root"
    },
    {
      "action": "preupgrade",
      "run-as": "root"
    },
    {
      "action": "postupgrade",
      "run-as": "root"
    },
    {
      "action": "start",
      "run-as": "root"
    },
    {
      "action": "stop",
      "run-as": "root"
    },
    {
      "action": "status",
      "run-as": "root"
    }
  ]
}

I think it would be wise to run all stop scripts as root so it will be able to forcefully kill hanging processes.

[th0ma7]

If you need root for this package you could add "run-as' root to the start action

@BenjV that's the issue, I can certainly add a privilege config file (and thnx for the tip btw for the root tip). Although from a framework point of view this is a regression: we cannot rebuild DSM6 packages without affecting its behavior in comparison to previously. I would suggest that the code be adapted so that if DSM <= 6 + ! privilege file = do nothing. This way the original behavior is kept and will allow rebuilding package which will behave like they previously did.

I think it would be wise to run all stop scripts as root so it will be able to forcefully kill hanging processes.

Will that work on DSM7? Because I thought it was enforcing package user by default always?

[BenjV]

If the privilege file has this on board everything will run as the package user except what you expliciet give "run-as : root", that is the purpose of the privilege file also on DSM 7.

  "defaults": {
    "run-as": "package"
  }

So in my example you could just remove the run-as : root from the start option and the actual package will run-as : "package" and all other scripts will run-as "root".

There are two options in my opinion.

1. Create a setting option in the package building process to be able for a package developer to choose between the package running as package (the default) or via a setting create a privilege file that will have the start option running as root,and remove the su ${EFF_USER} -s /bin/sh -c option from the service setup script.
2. Keep the su ${EFF_USER} -s /bin/sh -c option in the service setup script and use my complete example as the privilege file and for package that need to run as root remove the su ${EFF_USER} -s /bin/sh -c option from the service setup script.

Needless to state that option 2 is the simplest solution.

And you don't have to worry about backward compatibility, DSM 6 (and even DSM 5) is acting exactly the same as DSM 7 on the privilege file.

[th0ma7]

@BenjV re-reading your post.
Agree with proposals above. I particularly like the idea of a SPK_RUNAS variable where option is package by default. Could even be extended to SPK_INSTALLAS but not sure its worth it.

I also agree to disagree. For DSM<7 where there is no conf/privilege file, the framework should keep the previous behavior and not add one by default. I foresee issue down that path if this is not addressed.

[BenjV]

The conf/privilege is introduced in DSM 5 by Synology so I will work without any problem.

I am not sure but I think as of DSM 7 conf/privilege are required, so unless you want to maintain different packages for DSM 5/6 and DSM 7 there is no choice.
All current package will not run on DSM 7

[th0ma7]

Lets hope all packages eventually gets migrated over to DSM7, and once done all will be good. Although we both know this takes a while and in the meantime we do have to republish a few packages from time to time (ex: https://github.com/SynoCommunity/spkrepo/issues/70). As-is, package rebuilds may require being adapted otherwise they may break due to the framework change.

I actually ask myself how I'll proceed with upgrading considering I only have one unit at home... sigh I wished Synology would be more grateful to SynoCommunity developpers...

[hgy59]

IMHO now is time to redesign all packages to be future compatible with DSM7. As I started to redesign the installation logging (#4539) we should start to redesign the package privilege configuration.

1. All packages must use a privilege file
2. Without an explicit conf/privilege file the default shall be "defaults": { "run-as": "package" }
3. Remove all su ${EFF_USER} -s /bin/sh -c from package installation code

If we do not start such redesigns now, we will have packages that are hard to maintain for years, as there are still packages that do not use the generic service installer introduced more than 5 years ago for DSM6.

And I prefer explicit conf/privilege files over adding a lot of Makefile variables for each and every action type.

[Safihre]

Aah the good old days, we used to define (require?) the privilege file!
67862e0#diff-7f48ae17b1db68c7f640b90bf8284a365e61661881f19409a07b9f32bda0e38a
Are there really cases where you want to run specific steps as root, but others not?
So isn't it just easier to add 1 Makefile variable to override all of them?

[th0ma7]

@hgy59

If we do not start such redesigns now, we will have packages that are hard to maintain for years, as there are still packages that do not use the generic service installer introduced more than 5 years ago for DSM6.

Agreed with the redesign with the following two caveats:

1. Always be in state where we can rebuild packages throughout the redesign process. We cannot halt package publishing during this period. Currently things are in really good shape (an awesome job was done btw) although there will be a few corner cases to be found as we progress.
2. I propose we create a new permanent legacy or deprecated branch, pre-DSM7 merge (snapshot prior to PR Add DSM 7 Support #4395), for older DSM. This would allow us to remove support <= DSM5 in order to further clean-up the code while progressing on the redesign where we see fit. There where previous discussions on this a while ago on PR Isolate DSM5 branch #3917 with a few ideas discussed that may be helpful.

[th0ma7]

Are there really cases where you want to run specific steps as root, but others not?
So isn't it just easier to add 1 Makefile variable to override all of them?

@Safihre it's a really good question.
I notice the following from migrating from no privilege to using one while updating the package: backup configuration copies, previously owned by root becomes managed by sc-package user leading to issues of copying the file over the previous in /tmp or moving the file back to its final destination at recovery stage where it stays owned by root wheras all other files are now sc-package user associated... And this ends-up screwing-up during the subsequent upgrade...

So, I don't know yet but yes, there "may be" cases to play with the user depending of the action being performed, at least from a migration standpoint.

[BenjV]

If you use the methode of saving files as stated in the development manual for upgrading packages, then package center will chown the files to the correct owner at the end op the upgrading process.

[th0ma7]

If you use the methode of saving files as stated in the development manual for upgrading packages, then package center will chown the files to the correct owner at the end op the upgrading process.

@BenjV can you please point me to that, I tried finding it but may have overlooked. Also, its exactly my point: rebuilding + publishing current packages with no changes may lead to such issue. So we definitively need to be careful for any updates being published.

[BenjV]

Developers manual for DSM 6.2 on page 100

According to the privilege specification, package center chown files under /var/packages/${package}/target. (The setuid and
setgid bit will be cleared)

Developers manual for DSM 7 on page 67 chapter "Script Environment Variables"

SYNOPKG_TEMP_UPGRADE_FOLDER: The temporary directory when the package is upgrading. You can move the
files from the previous version of the package to it in preupgrade script and move them back in postupgrade.

But to be very sure the best thing to do is in the "post install" and "post upgrade" do a chown of the complete /var folder of the package.
I have a DS116 for testing purpose, so the moment DSM 7 will be available I will happy to test packages to be sure everything works ok.

[th0ma7]

While testing something else I quickly looked at the amount of packages which does a mv or cp command in conjunction with TMP_DIR or /tmp and found 37x packages that will require attention (as only 2x of them do a chown which looks unrelated):

$ grep -e mv -e cp */src/* 2>/dev/null| grep -e TMP_DIR -e "/tmp" | cut -f1 -d: | sort -u
bicbucstriim/src/installer.sh
boxbackup-client/src/installer.sh
cops/src/installer.sh
darkstat/src/installer.sh
debian-chroot/src/installer.sh
domoticz/src/installer.sh
ejabberd/src/installer.sh
fengoffice/src/installer.sh
ffsync/src/installer.sh
gentoo-chroot/src/installer.sh
git-server/src/installer.sh
haproxy/src/installer.sh
hassio/src/service-setup.sh
horde/src/installer.sh
htpcmanager/src/installer.sh
irssi/src/installer.sh
jappix/src/installer.sh
lirc/src/installer.sh
mantisbt/src/installer.sh
maraschino/src/installer.sh
memcached/src/installer.sh
monit/src/installer.sh
mutt/src/service-setup.sh
nzbmegasearch/src/installer.sh
octoprint/src/installer.sh
owncloud/src/installer.sh
plexivity/src/installer.sh
roundcube/src/installer.sh
rutorrent/src/service-setup.sh
salt-master/src/installer.sh
saltpad/src/installer.sh
selfoss/src/installer.sh
shairport-sync/src/installer.sh
squidguard/src/installer.sh
subliminal/src/installer.sh
tt-rss/src/installer.sh
wallabag/src/service-setup.sh

From this list only 15x of them do have a privilege file:

$ for spk in $(grep -e mv -e cp */src/* 2>/dev/null| grep -e TMP_DIR -e "/tmp" | cut -f1 -d/ | sort -u); do ls -1 $spk/src/conf/privilege 2>/dev/null; done
boxbackup-client/src/conf/privilege
domoticz/src/conf/privilege
ejabberd/src/conf/privilege
ffsync/src/conf/privilege
git-server/src/conf/privilege
haproxy/src/conf/privilege
htpcmanager/src/conf/privilege
maraschino/src/conf/privilege
memcached/src/conf/privilege
nzbmegasearch/src/conf/privilege
octoprint/src/conf/privilege
plexivity/src/conf/privilege
saltpad/src/conf/privilege
squidguard/src/conf/privilege
subliminal/src/conf/privilege

Perhaps we should define ${SYNOPKG_TEMP_UPGRADE_FOLDER} right away on DSM-6.2, adapt packages in using that and applying a chown right away and be in a readiness state for package upgrades.