MDEV-25177: Indicate user during DEB-package postint if SystemD 'ProtectSystem' or 'ProtectHome' will cause problems

[illuusio]

• The Jira issue number for this PR is: MDEV-25177

Description

SystemD parameter ProtectSystem prevents writing to '/usr', '/boot' and '/efi' (also /etc). If for example datadir if located '/usr/lib/mariadb' then MariaDB will just die away without much notice to user. Same goes with 'ProtectHome' that hides '/home', '/root' and '/run/user'

How can this PR be tested?

Create /etc/my.cnf with 'datadir'-parameter or other config with incorrect location and install deb-package it should print warning.

Basing the PR against the correct MariaDB version

• This is a new feature and the PR is based against the latest MariaDB development branch

[CLAassistant]

All committers have signed the CLA.

[ottok]

Hi!

Thanks for making a Pull Request. I am happy to review all changes in path debian/*.

A couple of questions:

1. Why is this change on the 10.7 branch? Is this change related to some other change in 10.7, perhaps some modifications to the systemd service files? If so, please reference those commits in the commit message.
2. Why is this change in the Debian postinstall script? What makes this Debian-specific? I would have assumed systemd issues apply to all Linux distros, be it deb or rpm. Maybe this piece of warning should be part of the mariadb-install-db script instead? Or a warning in the mariadbd binary itself, which could emit in logs when it is started up and it check for the envronment to be sane? The root cause is that the server would fail silently if users make an incompatible setting in /etc/my.. – why not fix the root cause and make the server more verbose in such a scenario?
3. I would have expected couple of inline comments on line 58. When you add a new code block it would be good practice to explain what the code block does, so that anybody making changes to that file later on would know why the code block is there and what it does (and can compare comments to code and fix the code in places where the code is wrong and does not do what the comments claim it should do).
4. The git commit subject line is too long and there is no git commit message body. Please follow best git practices when making commits.

Example git commit:

MDEV-12345: Capitalized, short (50 chars or less) summary

More detailed explanatory text, if necessary. Wrap it to about 72
characters or so. In some contexts, the first line is treated as the
subject of an email and the rest of the text as the body. The blank
line separating the summary from the body is critical (unless you omit
the body entirely); tools like rebase will confuse you if you run the
two together.

Write your commit message in the imperative: "Fix bug" and not "Fixed bug"
or "Fixes bug." This convention matches up with commit messages generated
by commands like git merge and git revert.

Further paragraphs come after blank lines.

• Bullet points are okay, too

• Typically a hyphen or asterisk is used for the bullet, followed by a
  single space, with blank lines in between, but conventions vary here

• Use a hanging indent

(from https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project)

[illuusio]

1. Why is this change on the 10.7 branch

What should be the correct branch as 10.7 is default one which comes first? Main/Master?

[ottok]

Answering now my own question:

Why is this change on the 10.7 branch? Is this change related to some other change in 10.7, perhaps some modifications to the systemd service files? If so, please reference those commits in the commit message.

This PR is related to ProtectSystem and ProtectHome, which apparenlty are used in the MariaDB systemd files:

± git grep Protect -- support-files/
support-files/mariadb.service.in:ProtectSystem=full
support-files/mariadb.service.in:ProtectHome=true
support-files/[email protected]:#   ProtectHome=false
support-files/[email protected]:ProtectSystem=full
support-files/[email protected]:ProtectHome=true

Based on git blame seems it was introduced in July 2016 53e7fcc and has been in MariaDB since tag mariadb-10.1.16.

Thus, from MariaDB 10.1.16 onwards users might have a situation where they start the server but they expect something (datadir?) to be read from /home/<user>/.., yet the MariaDB Server does not see the files at all, as the directories /home/, /root, and /run/user are made inaccessible and empty by systemd.

As the "bug" exists since 10.1, the bug fix should go there. However 10.1 is no longer maintained, so the correct version to fix this on is the oldest still maintained version. So target your fix on branch 10.2.

The exception is of course if the bugfix is considered risky. Then it should go only in the latest development branch 10.7.

The fix is not Debian-specific and I feel that making this in the Debian postinstallation scripts is not the correct way to solve it. There must be some distro-agnostic place to do it, right?

[grooverdan]

Otto's right, this should be against 10.2.

I haven't seen a deb/rpm common place or a good way to hook this (baring putting the script in CMake or an external helper which I'm less keen on).

mariadb-install-db/mysql_install_db.sh is a good idea on a place to emit an additional warning.

mariadbd change plausible, but large number of errno=13 checks under env NOTIFY_SOCKET (assumes Type=notify that mariadb has always used) to make the message verbose. Could make a few changes on common ones but that could be a separate work.

[grooverdan]

/usr|/usr/*|/boot|/boot/*|/efi|/efi/*) otherwise /bootleg matches. We want to match the path, and things under it. Same with /home

[grooverdan]

systemD -> systemd per https://systemd.io/

Why not create the overlay file if one doesn't exist with one or moreReadWritePaths?

[grooverdan]

If we do this, do we need to systemctl daemon-reload or has that happened already?

[grooverdan]

stdout or stderr? https://www.debian.org/doc/debian-policy/ch-maintainerscripts.html#introduction-to-package-maintainer-scripts doesn't specify. Is there a conversion?

[grooverdan]

"parameter" -> "directives" (https://www.freedesktop.org/software/systemd/man/systemd.directives.html)

[grooverdan]

If ReadWritePaths is sufficient there's not need to recommend these right?

[grooverdan]

Otherwise ${conf_problem} can be used in an echo.

[grooverdan]

technically you can detect changes from the mysql_datadir defaults in your loop.

[illuusio]

@ottok and @grooverdan It would be nice to have systemd StartPre script which is universal but it seems to be that way every distro wants it's own. I don't if it would be worth of effort to try make that kind of script and try to get at least biggest on board.

[illuusio]

mariadb-install-db/mysql_install_db.sh is a good idea on a place to emit an additional warning.

I didn't check but this is run every time MariaDB is launched then it would ideal to have another round of check.. as you configure your database before starting database. So would it be like this should we a helper script that is called on mysql_install_db.sh and DEB postint. Then it could be used from RPM installing %post. But I feel awkward to add new script on stable series..

[grooverdan]

@ottok and @grooverdan It would be nice to have systemd StartPre script which is universal but it seems to be that way every distro wants it's own. I don't if it would be worth of effort to try make that kind of script and try to get at least biggest on board.

StartPre has a significant number of benefits. Its largely Debian that uses it right? What else it it used for? Can distro specifics be factored out a little and the script made generic?

[illuusio]

Can distro specifics be factored out a little and the script made generic?

I'll take a look how they make it in openSUSE, Fedora and Arch as they lead the way. At least openSUSE has similar script as debian run script for StartPre. But if upstream changes way it handles thing they should adapt.

[ottok]

@ottok and @grooverdan It would be nice to have systemd StartPre script which is universal but it seems to be that way every distro wants it's own. I don't if it would be worth of effort to try make that kind of script and try to get at least biggest on board.

What do you mean every distro? There is just one systemd file and that is used in all the packages published by MariaDB.org:

± git grep StartPre support-files/*
support-files/mariadb.service.in:ExecStartPre=/bin/sh -c "systemctl unset-environment _WSREP_START_POSITION"
support-files/mariadb.service.in:ExecStartPre=/bin/sh -c "[ ! -e @bindir@/galera_recovery ] && VAR= || \
support-files/mariadb.service.in:# ExecStartPre=@scriptdir@/mysql_install_db -u mysql
support-files/mariadb.service.in:# ExecStartPre=sync
support-files/mariadb.service.in:# ExecStartPre=sysctl -q -w vm.drop_caches=3
support-files/[email protected]:ExecStartPre=/bin/sh -c "systemctl unset-environment _WSREP_START_POSITION%I"
support-files/[email protected]:ExecStartPre=/bin/sh -c "[ ! -e @bindir@/galera_recovery ] && VAR= || \
support-files/[email protected]:#ExecStartPre=/bin/sh -c "[ ! -e @bindir@/galera_recovery ] && VAR= || \
support-files/[email protected]:# ExecStartPre=@scriptdir@/mysql_install_db -u mysql
support-files/[email protected]:# ExecStartPre=sync
support-files/[email protected]:# ExecStartPre=sysctl -q -w vm.drop_caches=3
support-files/use_galera_new_cluster.conf:ExecStartPre=

Doing this check in StartPre would indeed be an appropriate place, since the "phenomenon" is due to systemd, and thus the warning should only apply to systemd that have systemd (i.e. not apply to kFreeBSD nor to MariaDB running in containers).

[illuusio]

Doing this check in StartPre would indeed be an appropriate place, since the "phenomenon" is due to systemd, and thus the warning should only apply to systemd that have systemd (i.e. not apply to kFreeBSD nor to MariaDB running in containers).

Yes I'm feeling that too that the way to go. I didn't know (I was studying Fedora MariaDB packaging) that one can have more than one 'ExecStartPre'-commands. OpenSUSE and Fedora have patched and own service files and Arch does like most of times uses upstream stuff with very few patches.
So providing script at libexec which make more verbose startup errors and successions in future would be easiest to get it adapted.

Can I rebase this pull request to another branch without making new one.

[ottok]

Yes I'm feeling that too that the way to go. I didn't know (I was studying Fedora MariaDB packaging) that one can have more than one 'ExecStartPre'-commands. OpenSUSE and Fedora have patched and own service files and Arch does like most of times uses upstream stuff with very few patches.

Please be specific. What do you refer to with Fedora MariaDB packaging? URL? Filename?

If you refer to the downstream packaging done at https://src.fedoraproject.org/rpms/mariadb/tree/rawhide note that it has its own spec file and service files and the PR you are making here at upstream MariaDB.org does thus not affect mentioned files at downstream Fedora.

Can I rebase this pull request to another branch without making new one.

Yes you can, just git rebase -i 10.2 on your own computer, force push that here and then Edit the PR to also mark the Pull Request to point to 10.2.

[illuusio]

@ottok I looked

• https://build.opensuse.org/package/show/server:database/mariadb
• https://github.com/archlinux/svntogit-packages/tree/packages/mariadb/trunk
• https://src.fedoraproject.org/rpms/mariadb/tree/rawhide

[illuusio]

@grooverdan and @ottok Now rebasing has happen and I'll update installing this script with CMake and add it to ExecStartPre in some point of time. Script needs more love like add that systemctl cat that show it things are already altered. This just base point that we can agree if this is the way to go.

[ottok]

Looks sensible to me! 👍

[grooverdan]

Its a good start. Keep going.

[grooverdan]

There are a small number of https://mariadb.com/kb/en/server-system-variables that are read-only (do these need ReadOnlyPaths=?). These are probably easier to enumerate than the writable ones. i.e. basedir, plugin_dir, file_key_management_filename, secure_file_priv please double check this list.

[grooverdan]

What you can do to determine if there is a problem is the bash [ -w "${conf_val}" ] to determine if writeable.

I tested with:

$ sudo systemctl status bob.service
× bob.service
     Loaded: loaded (/etc/systemd/system/bob.service; static)
     Active: failed (Result: exit-code) since Thu 2021-09-23 12:26:36 AEST; 8s ago
    Process: 161902 ExecStartPre=sh -c [ -w /home/dan ] && echo I can write (code=exited, status=1/FAILURE)
        CPU: 10ms

Sep 23 12:26:36 localhost.localdomain systemd[1]: Starting bob.service...
Sep 23 12:26:36 localhost.localdomain systemd[1]: bob.service: Control process exited, code=exited, status=1/FAILURE
Sep 23 12:26:36 localhost.localdomain systemd[1]: bob.service: Failed with result 'exit-code'.
Sep 23 12:26:36 localhost.localdomain systemd[1]: Failed to start bob.service.

$ cat /etc/systemd/system/bob.service
[Service]
ProtectHome=true

ExecStartPre=sh -c '[ -w /home/dan ] && echo I can write'
ExecStart=sleep 2

[illuusio]

@grooverdan Now this should be reviewed and figure out how to use this script in [email protected] file

[ottok]

Is this ready for re-review?

[illuusio]

Is this ready for re-review?

Yes it should be

[ottok]

There are 12 commits. This should be just one commit with a good commit title and body. I didn't review the commits, just looked at the overall 4 files changed.

As discussed, having one distro-independent file to check for systemd settings in a unified way is a good idea.

How about the file naming, did you think that mariadb-execstartpre-check would only be used for systemd purposes and this one check, or might it be extended in the future to do more checks that might be relevant to do before a systemd service startup?

The old init system had in Debian packaging the debian/additions/debian-start.inc.sh file which did various checks. I don't know if anybody feels the need to have those same checks in systemd in the future.

The inline comments make sense and the code AFAIK seems to do what they say, so this looks good to me. I am requesting changes only because the PR is not in a mergeable state.

[illuusio]

How about the file naming, did you think that mariadb-execstartpre-check would only be used for systemd purposes and this one check, or might it be extended in the future to do more checks that might be relevant to do before a systemd service startup?

The old init system had in Debian packaging the debian/additions/debian-start.inc.sh file which did various checks. I don't know if anybody feels the need to have those same checks in systemd in the future.

I think in future checks should contain at least checking supported config parameters (Bash is not just best language for that) and thank you for pointing out there already checks which I wasn't aware of.
My point of view is this just first kind of RFC like thing to see if every distro incorporate this or patch it out from service file. When this is in someday I'll incorporate more check from debian/additions/debian-start.inc.sh as they seem to be relevant with another PR(s).
I don't know would it be overkill to have some mariadb-execstartpre-check.d-directory which would have additional checks for distros or do they never be upstreamed in approach like that?

[grooverdan]

I started to enhance this further, however in testing I don't think the systemd hooks provide the same environemnt under ExecStartPre/ExecCondition as ExecStart. As we only can have 1 ExecStart (as its type=notify)

[ottok]

Related downstream efforts in progress:

• !29: Enable all systemd service files like upstream does
• #1028271 MariaDB systemd: start using sysusers.d
• #1028272 MariaDB systemd: start using tmpfiles.d

[illuusio]

How those are handled when you don't have systemd available?

[illuusio]

I started to enhance this further, however in testing I don't think the systemd hooks provide the same environemnt under ExecStartPre/ExecCondition as ExecStart. As we only can have 1 ExecStart (as its type=notify)

I believe we could have more than one ExecStart or run script before try to launch MariaDB in same line.. it's ugly but pragtical.