Azure / iot-identity-service

Source of the Azure IoT Identity Service and related services.
MIT License
37 stars 46 forks source link

Killall is expected by postrm but not required by debian/control #578

Closed gri6507 closed 9 months ago

gri6507 commented 9 months ago

Host OS: Debian 11 amd64, minimalistic install.

When I try to purge aziot-edge with autoremove, which also purges aziot-identity-service, I get the following output.

apt purge --autoremove aziot-edge
Reading package lists... Done
Building dependency tree
Reading state information... Done
The following packages will be REMOVED:
  aziot-edge* aziot-identity-service*
0 upgraded, 0 newly installed, 2 to remove and 0 not upgraded.
After this operation, 40.8 MB disk space will be freed.
Do you want to continue? [Y/n] y
(Reading database ... 21574 files and directories currently installed.)
Removing aziot-edge (1.4.27-1) ...
Removing aziot-identity-service (1.4.7-1) ...
Processing triggers for libc-bin (2.24-11+deb9u4) ...
Processing triggers for man-db (2.7.6.1-2) ...
(Reading database ... 21493 files and directories currently installed.)
Purging configuration files for aziot-edge (1.4.27-1) ...
/var/lib/dpkg/info/aziot-edge.postrm: 7: /var/lib/dpkg/info/aziot-edge.postrm: killall: not found
userdel: group iotedge is the primary group of another user and is not removed.
Purging configuration files for aziot-identity-service (1.4.7-1) ...
/var/lib/dpkg/info/aziot-identity-service.postrm: 26: /var/lib/dpkg/info/aziot-identity-service.postrm: killall: not found
/var/lib/dpkg/info/aziot-identity-service.postrm: 27: /var/lib/dpkg/info/aziot-identity-service.postrm: killall: not found
/var/lib/dpkg/info/aziot-identity-service.postrm: 28: /var/lib/dpkg/info/aziot-identity-service.postrm: killall: not found
/var/lib/dpkg/info/aziot-identity-service.postrm: 29: /var/lib/dpkg/info/aziot-identity-service.postrm: killall: not found

On all Debian/Ubuntu systems, killall is provided by psmisc package. This should be listed as a Depends in https://github.com/Azure/iot-identity-service/blob/main/contrib/debian/control#L13C1-L13C44

arsing commented 9 months ago

Yes.

Needs to be fixed in iotedge too since its postrm does the same thing. (It shouldn't rely on pulling it in via a-i-s's deps.)

arsing commented 9 months ago

It doesn't help actually. psmisc is purged before aziot-identity-service is.

$ apt purge --autoremove aziot-identity-service

...

The following packages will be REMOVED:
  aziot-identity-service* libkmod2* libtss2-esys-3.0.2-0* libtss2-mu0* libtss2-rc0* libtss2-sys1* libtss2-tcti-cmd0* libtss2-tcti-device0* libtss2-tcti-mssim0* libtss2-tcti-swtpm0*
  libtss2-tctildr0* psmisc* tpm-udev* udev*
0 upgraded, 0 newly installed, 14 to remove and 0 not upgraded.

...

Processing triggers for libc-bin (2.31-13+deb11u7) ...
(Reading database ... 6677 files and directories currently installed.)
Purging configuration files for psmisc (23.4-2) ...
Purging configuration files for aziot-identity-service (1.4.7-1) ...
/var/lib/dpkg/info/aziot-identity-service.postrm: 26: killall: not found
/var/lib/dpkg/info/aziot-identity-service.postrm: 27: killall: not found
/var/lib/dpkg/info/aziot-identity-service.postrm: 28: killall: not found
/var/lib/dpkg/info/aziot-identity-service.postrm: 29: killall: not found
Purging configuration files for udev (247.3-7+deb11u4) ...
arsing commented 9 months ago

Indeed, https://www.debian.org/doc/debian-policy/ch-relationships.html#binary-dependencies-depends-recommends-suggests-enhances-pre-depends says:

Finally, the Depends field should be used if the depended-on package is needed by the postrm script to fully clean up after the package removal. There is no guarantee that package dependencies will be available when postrm is run, but the depended-on package is more likely to be available if the package declares a dependency (particularly in the case of postrm remove). The postrm script must gracefully skip actions that require a dependency if that dependency isn’t available.

So it is good to add the Depends, but it's not going to solve the original problem.

We do run it with || true, and realistically it's not going to kill anything because the systemd units will be stopped anyway, so it should be okay to ignore.

gri6507 commented 9 months ago

lintian reports a number of errors and warnings for the DEBs (see full list below). One of those is for use of killall, albeit for a different reason. Between the lintian concern and the fact that postrm purge cannot rely on control file's Depends specification, perhaps a better approach is to move away from using killall altogether?

$ lintian /tmp/q/aziot-edge_1.4.27-1_amd64.deb
E: aziot-edge: bogus-mail-host-in-debian-changelog "Azure IoT Edge Devs" <> (for version 1.4.27-1) [usr/share/doc/aziot-edge/changelog.Debian.gz:1]
E: aziot-edge: depends-on-essential-package-without-using-version Depends: hostname
E: aziot-edge: depends-on-essential-package-without-using-version Depends: sed
E: aziot-edge: malformed-contact Maintainer Azure
E: aziot-edge: missing-dependency-on-libc needed by usr/bin/iotedge and 1 others
W: aziot-edge: command-with-path-in-maintainer-script /usr/sbin/userdel (plain script) [postrm:22]
W: aziot-edge: command-with-path-in-maintainer-script /usr/sbin/userdel (plain script) [postrm:25]
W: aziot-edge: command-with-path-in-maintainer-script /usr/sbin/userdel (plain script) [postrm:26]
W: aziot-edge: copyright-without-copyright-notice
W: aziot-edge: debian-changelog-has-wrong-day-of-week 2020-12-01 was a Tuesday [usr/share/doc/aziot-edge/changelog.Debian.gz:1]
W: aziot-edge: initial-upload-closes-no-bugs [usr/share/doc/aziot-edge/changelog.Debian.gz:1]
W: aziot-edge: killall-is-dangerous [postrm:7]
W: aziot-edge: maintainer-script-empty [prerm]
W: aziot-edge: priority-extra-is-replaced-by-priority-optional
$ lintian /tmp/q/aziot-identity-service_1.4.7-1_amd64.deb
E: aziot-identity-service: bogus-mail-host-in-debian-changelog "Azure IoT Edge Devs" <> (for version 1.4.7-1) [usr/share/doc/aziot-identity-service/changelog.Debian.gz:1]
E: aziot-identity-service: malformed-contact Maintainer Azure
E: aziot-identity-service: sharedobject-in-library-directory-missing-soname [usr/lib/libaziot_keys.so]
W: aziot-identity-service: command-with-path-in-maintainer-script /usr/bin/getent (plain script) [preinst:21]
W: aziot-identity-service: command-with-path-in-maintainer-script /usr/bin/getent (plain script) [preinst:24]
W: aziot-identity-service: command-with-path-in-maintainer-script /usr/bin/getent (plain script) [preinst:29]
W: aziot-identity-service: command-with-path-in-maintainer-script ... use "--tag-display-limit 0" to see all (or pipe to a file/program)
W: aziot-identity-service: copyright-without-copyright-notice
W: aziot-identity-service: extended-description-line-too-long line 1
W: aziot-identity-service: extended-description-line-too-long line 10
W: aziot-identity-service: extended-description-line-too-long line 11
W: aziot-identity-service: extended-description-line-too-long ... use "--tag-display-limit 0" to see all (or pipe to a file/program)
W: aziot-identity-service: initial-upload-closes-no-bugs [usr/share/doc/aziot-identity-service/changelog.Debian.gz:1]
W: aziot-identity-service: killall-is-dangerous [postrm:26]
W: aziot-identity-service: killall-is-dangerous [postrm:27]
W: aziot-identity-service: killall-is-dangerous [postrm:28]
W: aziot-identity-service: killall-is-dangerous ... use "--tag-display-limit 0" to see all (or pipe to a file/program)
W: aziot-identity-service: no-manual-page [usr/bin/aziotctl]
W: aziot-identity-service: possible-unindented-list-in-extended-description line 5
arsing commented 9 months ago

One of those is for use of killall, albeit for a different reason.

Server is down but I checked the archive.org copy.

The maintainer script seems to call killall. Since the program terminates processes by name, it may accidentally affect unrelated processes.

Most uses of killall should use invoke-rc.d instead.

Our use is killall -u $USER, not a process name, so it doesn't apply.

The suggestion to use invoke-rc.d is just saying "stop your services using the service manager", which we don't need to do since that is already handled by the debhelper snippet to stop the systemd services, like I said in my last comment. Specifically the prerm snippet does deb-systemd-invoke stop 'aziot-certd.socket' 'aziot-identityd.socket' 'aziot-keyd.socket' 'aziot-tpmd.socket' >/dev/null || true which in turn will cause the corresponding service units to stop because they each have Requires= on their socket units.

perhaps a better approach is to move away from using killall altogether?

It was added as a last resort in case stopping the services fails. I don't know why that would happen, and gordonwang0 is out of office right now, so I'd rather not remove it.