Hi Marco,
On Sun, Mar 19, 2023 at 03:01:20AM +0100, Marco d'Itri wrote:
> It is expected that /etc/shells can be edited by system administrators,
> I have been doing that forever in my career as a professional system
> administrator and until now I was not even aware of these programs from
> debianutils.
That applies to configuration files in general, right? However,
configuration files have an owner from a packaging point of view.
> Hence my reasoning that having convert-etc-shells modify the file would
> not be harmful, and so far I am not aware of any practical problem that
> this has ever caused.
If convert-etc-shells were some administrative tool not to be run by
maintainer scripts, that would actually be correct.
> I also see that you wrote update-shells in 2021, but convert-etc-shells
> was added to usrmerge in 2016.
update-shells is an attempt at fixing long-standing bugs in add-shell
and remove-shell. Prior to update-shells, those were the canonical tools
to modify /etc/shells by packages and except for usrmerge, everyone else
used those interfaces. Of course, those interfaces are not up to the
task posed by usrmerge, so using them wasn't really an option. However,
cooperating with debianutils would have been.
> Right. But both update-shells and usr-is-merged are new to bookworm, and
> I remember that having the /usr/ paths in /etc/shells is not usually
> needed, so this explains why nobody has reported actual problems so far.
Yeah, it popped up as a reproducibility issue now.
> (Also, would you mind moving /var/lib/shells.state to /var/lib/misc/?)
Thank you for suggesting this. I agree that that choice of path is
better. When opening that can of worms, I would like to figure out
whether there are even better places.
update-shells is meant to be run by maintainer scripts only. If an
administrator were to run it without changes to shells.d, the expected
behaviour is noop. Thus, I am wondering whether something below /usr
would be a better choice wrt. hermetic /usr. I think the major question
here is what should happen if /etc/shells is deleted. If it should be
populated with shells by update-shells, then its state file also needs
to be deleted. This would be a reason for the location in /var, which
would likekly be discarded together with /etc. If however, we see
update-shells purely as a packaging tool, then something below /usr
could be better (in a similar vein as we consider moving the dpkg
database to /usr). Would you be able to help with finding an answer to
this question?
Then I wonder what severity that change in location should bear. Is it
something we want to do during freeze? Is it worth the effort or more
like a time travel fix? In any case, I think this is a separate issue.
Would you clone it if you care deeply enough?
I also noticed one other flaw in my proposal: Running convert-etc-shells
as part of update-shells would cause /usr variants of shells to be
re-added after having been removed by administrators. So the
convert-etc-shells should be a one time conversion action instead and
only happen on the first run of update-shells after a /usr-merge. I
think this can be achieved by adding a flag-value to shells.state.
I've prepared an update for debianutils and tested it in the following
cases:
* Installation on a pre-merged chroot -> /usr/bin/sh is added to
/etc/shells.
* Installation on a chroot merged by usrmerge -> no difference
* Installation on an unmerged system. Manual merge without
convert-etc-shells. Manual update-shells. -> Looks the same as after
convert-etc-shells.
Does anyone see any bugs?
Helmut
diff --minimal -Nru debianutils-5.7/debian/changelog
debianutils-5.7/debian/changelog
--- debianutils-5.7/debian/changelog 2022-11-02 17:31:14.000000000 +0100
+++ debianutils-5.7/debian/changelog 2023-03-19 15:00:09.000000000 +0100
@@ -1,3 +1,10 @@
+debianutils (5.7-0.5) UNRELEASED; urgency=medium
+
+ * Non-maintainer upload.
+ * Absorb usrmerge's convert-etc-shells into update-shells.
+
+ -- Helmut Grohne <hel...@subdivi.de> Sun, 19 Mar 2023 15:00:09 +0100
+
debianutils (5.7-0.4) unstable; urgency=medium
* Non-maintainer upload
diff --minimal -Nru debianutils-5.7/debian/patches/absorb-convert-etc-shells
debianutils-5.7/debian/patches/absorb-convert-etc-shells
--- debianutils-5.7/debian/patches/absorb-convert-etc-shells 1970-01-01
01:00:00.000000000 +0100
+++ debianutils-5.7/debian/patches/absorb-convert-etc-shells 2023-03-19
15:00:09.000000000 +0100
@@ -0,0 +1,112 @@
+Absorb the script convert-etc-shells from usrmerge to obtain reproducible
+behaviour. usrmerge will stop running convert-etc-shells and instead trigger
+the shells update in debianutils.
+
+--- a/update-shells
++++ b/update-shells
+@@ -1,11 +1,15 @@
+ #!/bin/sh
+ # SPDX-License-Identifier: GPL-2.0-or-later
+ # Copyright 2021 Helmut Grohne <hel...@subdivi.de>
+-
++#
+ # A "hashset" is a shell variable containing a sequence of elements separated
+ # and surrounded by hash (#) characters. None of the elements may contain a
+ # hash character. The character is thus chosen, because it initiates a comment
+ # in /etc/shells. All variables ending in _SHELLS in this file are hashsets.
++#
++# The state file contains the concatenated shells from shells.d. It optionally
++# contains a first line "usr-is-merged" if /usr variants of user shells have
++# been added.
+
+ set -e
+
+@@ -93,6 +97,13 @@
+ done < "$STATE_FILE"
+ fi
+
++PERFORM_USR_MERGE=no
++if ! hashset_contains "$STATE_SHELLS" usr-is-merged &&
++ test -h /bin && test -h /sbin; then
++ PERFORM_USR_MERGE=yes
++ STATE_SHELLS="#usr-is-merged$STATE_SHELLS"
++fi
++
+ cleanup() {
+ rm -f "$NEW_ETC_FILE" "$NEW_STATE_FILE"
+ }
+@@ -100,20 +111,52 @@
+
+ : > "$NEW_ETC_FILE"
+ ETC_SHELLS='#'
++USRMERGE_SHELLS='#'
+ while IFS= read -r line; do
+ shell=${line%%#*}
+- # copy all comment lines, packaged shells and local additions
+- if [ -z "$shell" ] ||
+- hashset_contains "$PKG_SHELLS" "$shell" ||
+- ! hashset_contains "$STATE_SHELLS" "$shell"; then
++ if [ -z "$shell" ]; then
++ # copy comments
++ echo "$line" >> "$NEW_ETC_FILE"
++ elif hashset_contains "$PKG_SHELLS" "$shell"; then
++ # copy packaged shells
+ echo "$line" >> "$NEW_ETC_FILE"
+ ETC_SHELLS="$ETC_SHELLS$shell#"
++ elif ! hashset_contains "$STATE_SHELLS" "$shell"; then
++ # copy local additions
++ echo "$line" >> "$NEW_ETC_FILE"
++ ETC_SHELLS="$ETC_SHELLS$shell#"
++ # If we perform a /usr-merge upgrade, add the corresponding
++ # /usr path. This is what convert-etc-shells from usrmerge
++ # formerly did.
++ if test "$PERFORM_USR_MERGE" = yes &&
++ test "${shell#/bin/}" != "${shell#/sbin/}"; then
++ USRMERGE_SHELLS="$USRMERGE_SHELLS/usr$shell#"
++ fi
+ else
+ log "removing shell $shell"
+ fi
+ done < "$SOURCE_ETC_FILE"
+
++if test "$PERFORM_USR_MERGE" = yes; then
++ saved_IFS=$IFS
++ IFS='#'
++ set -f
++ # shellcheck disable=SC2086 # word splitting intended, globbing disabled
++ set -- ${USRMERGE_SHELLS###}
++ set +f
++ IFS=$saved_IFS
++ for shell; do
++ if ! hashset_contains "$ETC_SHELLS" "$shell"; then
++ echo "$shell" >> "$NEW_ETC_FILE"
++ log "adding /usr-merged shell $shell"
++ fi
++ done
++fi
++
+ : > "$NEW_STATE_FILE"
++if hashset_contains "$STATE_SHELLS" usr-is-merged; then
++ echo "usr-is-merged" >> "$NEW_STATE_FILE"
++fi
+ saved_IFS=$IFS
+ IFS='#'
+ set -f
+@@ -133,13 +176,13 @@
+
+ if [ "$NOACT" = 0 ]; then
+ if [ -e "$STATE_FILE" ]; then
+- chmod --reference="${STATE_FILE}" "${NEW_STATE_FILE}" || chmod
$(stat -c %a "${STATE_FILE}") "${NEW_STATE_FILE}"
+- chown --reference="${STATE_FILE}" "${NEW_STATE_FILE}" || chown
$(stat -c %U "${STATE_FILE}") "${NEW_STATE_FILE}"
++ chmod --reference="${STATE_FILE}" "${NEW_STATE_FILE}" || chmod
"$(stat -c %a "${STATE_FILE}")" "${NEW_STATE_FILE}"
++ chown --reference="${STATE_FILE}" "${NEW_STATE_FILE}" || chown
"$(stat -c %U "${STATE_FILE}")" "${NEW_STATE_FILE}"
+ else
+ chmod 0644 "$NEW_STATE_FILE"
+ fi
+- chmod --reference="${SOURCE_ETC_FILE}" "${NEW_ETC_FILE}" || chmod
$(stat -c %a "${SOURCE_ETC_FILE}") "${NEW_ETC_FILE}"
+- chown --reference="${SOURCE_ETC_FILE}" "${NEW_ETC_FILE}" || chown
$(stat -c %U "${SOURCE_ETC_FILE}") "${NEW_ETC_FILE}"
++ chmod --reference="${SOURCE_ETC_FILE}" "${NEW_ETC_FILE}" || chmod
"$(stat -c %a "${SOURCE_ETC_FILE}")" "${NEW_ETC_FILE}"
++ chown --reference="${SOURCE_ETC_FILE}" "${NEW_ETC_FILE}" || chown
"$(stat -c %U "${SOURCE_ETC_FILE}")" "${NEW_ETC_FILE}"
+ sync -d "$NEW_ETC_FILE" "$NEW_STATE_FILE"
+ mv -Z "${NEW_ETC_FILE}" "${TARGET_ETC_FILE}" || mv "${NEW_ETC_FILE}"
"${TARGET_ETC_FILE}"
+ sync "$TARGET_ETC_FILE"
diff --minimal -Nru debianutils-5.7/debian/patches/series
debianutils-5.7/debian/patches/series
--- debianutils-5.7/debian/patches/series 2022-07-27 08:17:42.000000000
+0200
+++ debianutils-5.7/debian/patches/series 2023-03-19 15:00:09.000000000
+0100
@@ -1 +1,2 @@
dpkg-root
+absorb-convert-etc-shells
