| Message ID | 20231009151729.2223963-8-arbab@linux.ibm.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | package/petitboot: misc fixes/enhancement | expand |
On 09/10/2023 17:17, Reza Arbab wrote: > Run the petitboot UI as an unprivileged user. This requires using the > agetty package instead of the busybox getty utility, running the initial > pb-console helper at user login rather than directly. That sounds counterproductive though? It means you have to log in before the boot menu is displayed? Or perhaps I misunderstand the statement here. It's also not clear why it would need agetty instead of busybox getty. This doesn't sound like something that should be done by default. > If sudo is installed, with a sudoers policy allowing petituser to > perform sudo with no password (or a blank password), the "drop to shell" > feature of petitboot will automatically become a root shell. It seems to me that the logical thing to do would be to drop into an actual getty, which asks for a login and password. > > Signed-off-by: Reza Arbab <arbab@linux.ibm.com> > --- > package/petitboot/Config.in | 1 + > package/petitboot/S15pb-discover | 4 +++- > package/petitboot/pb-console | 6 ++++-- > package/petitboot/petitboot.mk | 12 ++++++++++++ > package/petitboot/shell_config | 24 ++++++++++++++++++++++++ > package/petitboot/shell_profile | 2 ++ > 6 files changed, 46 insertions(+), 3 deletions(-) > create mode 100644 package/petitboot/shell_config > create mode 100644 package/petitboot/shell_profile > > diff --git a/package/petitboot/Config.in b/package/petitboot/Config.in > index 5f1d91e77ecb..0f965e71e628 100644 > --- a/package/petitboot/Config.in > +++ b/package/petitboot/Config.in > @@ -16,6 +16,7 @@ config BR2_PACKAGE_PETITBOOT > select BR2_PACKAGE_KEXEC_LITE if ( BR2_powerpc || BR2_powerpc64 || BR2_powerpc64le ) > select BR2_PACKAGE_NVME if ( BR2_powerpc || BR2_powerpc64 || BR2_powerpc64le ) > select BR2_PACKAGE_POWERPC_UTILS if ( BR2_powerpc || BR2_powerpc64 || BR2_powerpc64le ) > + select BR2_PACKAGE_UTIL_LINUX_AGETTY > help > Petitboot is a small kexec-based bootloader > > diff --git a/package/petitboot/S15pb-discover b/package/petitboot/S15pb-discover > index 71ab62d99859..a37e33521f1a 100644 > --- a/package/petitboot/S15pb-discover > +++ b/package/petitboot/S15pb-discover > @@ -12,7 +12,9 @@ fi > > start() { > printf 'Starting %s: ' "$DAEMON" > - mkdir -p /var/log/petitboot > + # shellcheck disable=SC2174 # only apply -m to deepest dir > + mkdir -p -m 0775 /var/log/petitboot > + chown root:petitgroup /var/log/petitboot Why is it owned by root and not petituser? > > # shellcheck disable=SC2086 # we need the word splitting > start-stop-daemon -S -q -b -m -p "$PIDFILE" -x "/usr/sbin/$DAEMON" \ > diff --git a/package/petitboot/pb-console b/package/petitboot/pb-console > index 407ff3b30232..eea40163d02f 100644 > --- a/package/petitboot/pb-console > +++ b/package/petitboot/pb-console > @@ -3,14 +3,16 @@ > DAEMON="pb-console" > > PB_CONSOLE_PORT=${2:-"console"} > -PB_CONSOLE_ARGS="--getty --detach -- -n -i 0 $PB_CONSOLE_PORT linux" > +PB_CONSOLE_ARGS="--getty=/sbin/agetty --detach -- -a petituser -n -i $PB_CONSOLE_PORT linux" > > # shellcheck source=/dev/null > [ -r "/etc/default/petitboot" ] && . "/etc/default/petitboot" > > start() { > printf 'Starting %s on %s: ' "$DAEMON" "$PB_CONSOLE_PORT" > - mkdir -p /var/log/petitboot > + # shellcheck disable=SC2174 # only apply -m to deepest dir > + mkdir -p -m 0775 /var/log/petitboot > + chown root:petitgroup /var/log/petitboot > > # shellcheck disable=SC2086 # we need the word splitting > start-stop-daemon -S -q -x "/usr/libexec/petitboot/$DAEMON" \ > diff --git a/package/petitboot/petitboot.mk b/package/petitboot/petitboot.mk > index ff87f3498734..5b517eb3b1a6 100644 > --- a/package/petitboot/petitboot.mk > +++ b/package/petitboot/petitboot.mk > @@ -71,6 +71,10 @@ define PETITBOOT_POST_INSTALL > $(TARGET_DIR)/usr/sbin/kexec-restart > $(INSTALL) -D -m 0755 $(PETITBOOT_PKGDIR)/pb-console \ > $(TARGET_DIR)/etc/init.d/pb-console > + $(INSTALL) -D -m 0755 $(PETITBOOT_PKGDIR)/shell_config \ > + $(TARGET_DIR)/home/petituser/.shrc > + $(INSTALL) -D -m 0755 $(PETITBOOT_PKGDIR)/shell_profile \ > + $(TARGET_DIR)/home/petituser/.profile > > mkdir -p $(TARGET_DIR)/etc/udev/rules.d > (for port in $(PETITBOOT_GETTY_PORT); do \ > @@ -84,4 +88,12 @@ endef > > PETITBOOT_POST_INSTALL_TARGET_HOOKS += PETITBOOT_POST_INSTALL > > +define PETITBOOT_USERS > + petituser -1 petitgroup -1 * /home/petituser /bin/sh - petitboot user Are petitgroup and petituser standard names? If not, we normally use the package name as username and group name, i.e. petitboot -1 petitboot -1 ... Also, does this user really need a home directory and a shell? It really should be a system user, no? It's only when it falls into the shell that you need an actual shell... > +endef > + > +define PETITBOOT_PERMISSIONS > + /var/petitboot d 775 root petitgroup - - - - - What is /var/petitboot used for? > +endef > + > $(eval $(autotools-package)) > diff --git a/package/petitboot/shell_config b/package/petitboot/shell_config > new file mode 100644 > index 000000000000..b10b95baae6c > --- /dev/null > +++ b/package/petitboot/shell_config > @@ -0,0 +1,24 @@ > +#!/bin/sh > + > +try_sudo() { > + [ -x "$(command -v sudo)" ] || return > + sudo -K > + echo | sudo -S /bin/true >/dev/null 2>&1 || return > + > + echo "No password required, running as root." > + sudo -i > + sudo -K > + exit > +} > + > +reset > + > +echo "Exiting petitboot. Type 'exit' to return." > +echo "You may run 'pb-sos' to gather diagnostic data." > + > +if [ "$(id -u)" != "0" ]; then > + try_sudo > + export PS1='$ ' > +else > + export PS1='# ' > +fi > diff --git a/package/petitboot/shell_profile b/package/petitboot/shell_profile > new file mode 100644 > index 000000000000..1ca5e6364dba > --- /dev/null > +++ b/package/petitboot/shell_profile > @@ -0,0 +1,2 @@ > +export ENV="/home/petituser/.shrc" This needs a bit more explanation. Is ENV something that is used by pb-console? How is it evaluated? Marked as Changes Requested. Regards, Arnout > +exec /usr/libexec/petitboot/pb-console
On Sun, Nov 05, 2023 at 07:26:16PM +0100, Arnout Vandecappelle wrote: >On 09/10/2023 17:17, Reza Arbab wrote: >>Run the petitboot UI as an unprivileged user. This requires using the >>agetty package instead of the busybox getty utility, running the initial >>pb-console helper at user login rather than directly. > > That sounds counterproductive though? It means you have to log in >before the boot menu is displayed? Or perhaps I misunderstand the >statement here. > > It's also not clear why it would need agetty instead of busybox getty. Sorry, I didn't explain it very well. The chain goes like this: 1. /etc/init.d/pb-console start console 2. /usr/libexec/petitboot/pb-console --getty --detach -- -n -i 0 console linux 3. /sbin/getty -l/usr/libexec/petitboot/pb-console -n -i 0 console linux 4. /usr/libexec/petitboot/pb-console 5. /usr/sbin/petiboot-nc After the change: 1. /etc/init.d/pb-console start console 2. /usr/libexec/petitboot/pb-console --getty=/sbin/agetty --detach -- -a petituser -n -i console linux 3. /sbin/agetty -a petituser -n -i console linux 4. /home/petituser/.profile 5. /usr/libexec/petitboot/pb-console 6. /usr/sbin/petiboot-nc Because we're using the -a (autologin) feature of agetty, everything from (4) down is running as petituser. If you select "drop to shell" from the menu, you then also do 7. /home/petituser/.shrc (the $ENV set in .profile) 8. /bin/sh >>If sudo is installed, with a sudoers policy allowing petituser to >>perform sudo with no password (or a blank password), the "drop to shell" >>feature of petitboot will automatically become a root shell. > > It seems to me that the logical thing to do would be to drop into an >actual getty, which asks for a login and password. At this point we're already running getty and (auto)logged in as petituser. The sudo integration allos system-specific flexibility in if/how the the shell may be elevated. As petituser, the shell has permission to collect diagnostics/logs by running pb-sos. >>--- a/package/petitboot/S15pb-discover >>+++ b/package/petitboot/S15pb-discover >>@@ -12,7 +12,9 @@ fi >> start() { >> printf 'Starting %s: ' "$DAEMON" >>- mkdir -p /var/log/petitboot >>+ # shellcheck disable=SC2174 # only apply -m to deepest dir >>+ mkdir -p -m 0775 /var/log/petitboot >>+ chown root:petitgroup /var/log/petitboot > > Why is it owned by root and not petituser? I don't see any reason why it couldn't be owned by petituser. >>@@ -84,4 +88,12 @@ endef >> PETITBOOT_POST_INSTALL_TARGET_HOOKS += PETITBOOT_POST_INSTALL >>+define PETITBOOT_USERS >>+ petituser -1 petitgroup -1 * /home/petituser /bin/sh - petitboot user > > Are petitgroup and petituser standard names? If not, we normally use >the package name as username and group name, i.e. > > petitboot -1 petitboot -1 ... I think we could change petituser to petitboot, but there is a hardcoded reference to petitgroup in the code: discover/discover-server.c: group = getgrnam("petitgroup"); >>+define PETITBOOT_PERMISSIONS >>+ /var/petitboot d 775 root petitgroup - - - - - > > What is /var/petitboot used for? That is where pb-discover mounts devices, looking for boot sources: /var/petitboot/mnt/dev/nvme0n1p1/grub2/grub.cfg /var/petitboot/mnt/dev/nvme0n1p1/vmlinux
On 09/11/2023 17:16, Reza Arbab wrote: > On Sun, Nov 05, 2023 at 07:26:16PM +0100, Arnout Vandecappelle wrote: >> On 09/10/2023 17:17, Reza Arbab wrote: >>> Run the petitboot UI as an unprivileged user. This requires using the >>> agetty package instead of the busybox getty utility, running the initial >>> pb-console helper at user login rather than directly. >> >> That sounds counterproductive though? It means you have to log in before the >> boot menu is displayed? Or perhaps I misunderstand the statement here. >> >> It's also not clear why it would need agetty instead of busybox getty. > > Sorry, I didn't explain it very well. The chain goes like this: > > 1. /etc/init.d/pb-console start console > 2. /usr/libexec/petitboot/pb-console --getty --detach -- -n -i 0 console linux > 3. /sbin/getty -l/usr/libexec/petitboot/pb-console -n -i 0 console linux > 4. /usr/libexec/petitboot/pb-console > 5. /usr/sbin/petiboot-nc > > After the change: > > 1. /etc/init.d/pb-console start console > 2. /usr/libexec/petitboot/pb-console --getty=/sbin/agetty --detach -- -a > petituser -n -i console linux > 3. /sbin/agetty -a petituser -n -i console linux How did the -l option disappear here? Is that due to --getty= instead of --getty? Anyway, the only reason to use agetty is for the -a option. Can't we achieve the same by using su? I.e. create a wrapper script around pb-console that does something like exec su -s /usr/libexec/petitboot/pb-console petituser > 4. /home/petituser/.profile Something I didn't touch in my earlier review: I don't think .profile is the right place to set this up. Either petituser is a normal user - in that case logging in through some other means (e.g. ssh, sudo) should give a normal shell. I assume you can tell pb-console which "shell" to use in the "drop to shell" case. > 5. /usr/libexec/petitboot/pb-console > 6. /usr/sbin/petiboot-nc > > Because we're using the -a (autologin) feature of agetty, everything from (4) > down is running as petituser. If you select "drop to shell" from the menu, you > then also do > > 7. /home/petituser/.shrc (the $ENV set in .profile) > 8. /bin/sh > >>> If sudo is installed, with a sudoers policy allowing petituser to >>> perform sudo with no password (or a blank password), the "drop to shell" >>> feature of petitboot will automatically become a root shell. >> >> It seems to me that the logical thing to do would be to drop into an actual >> getty, which asks for a login and password. > > At this point we're already running getty and (auto)logged in as > petituser. The sudo integration allos system-specific flexibility in if/how the > the shell may be elevated. As petituser, the shell has permission to collect > diagnostics/logs by running pb-sos. I'm anyway wrong - getty or login are not setuid, they need to be executed as root, so petituser can't start them. >>> --- a/package/petitboot/S15pb-discover >>> +++ b/package/petitboot/S15pb-discover >>> @@ -12,7 +12,9 @@ fi >>> start() { >>> printf 'Starting %s: ' "$DAEMON" >>> - mkdir -p /var/log/petitboot >>> + # shellcheck disable=SC2174 # only apply -m to deepest dir >>> + mkdir -p -m 0775 /var/log/petitboot >>> + chown root:petitgroup /var/log/petitboot >> >> Why is it owned by root and not petituser? > > I don't see any reason why it couldn't be owned by petituser. > >>> @@ -84,4 +88,12 @@ endef >>> PETITBOOT_POST_INSTALL_TARGET_HOOKS += PETITBOOT_POST_INSTALL >>> +define PETITBOOT_USERS >>> + petituser -1 petitgroup -1 * /home/petituser /bin/sh - petitboot user >> >> Are petitgroup and petituser standard names? If not, we normally use the >> package name as username and group name, i.e. >> >> petitboot -1 petitboot -1 ... > > I think we could change petituser to petitboot, but there is a hardcoded > reference to petitgroup in the code: > > discover/discover-server.c: group = getgrnam("petitgroup"); Sounds like petitgroup is at least a standard name, and in that case petituser fits nicely (and I assume it is already used _somewhere_ at least). So OK to keep petituser/petitgroup. Please say something about this in the commit message (we want the history to show why we chose "petituser" rather than the more typical "petitboot" for the username). >>> +define PETITBOOT_PERMISSIONS >>> + /var/petitboot d 775 root petitgroup - - - - - Given the information below, why is it in petitgroup? If pb-discover is just going to create directories there on which it can mount things, it's not very useful for pb-console to have write access to it, right? Unless pb-console needs to delete those directories or rename them? >> >> What is /var/petitboot used for? > > That is where pb-discover mounts devices, looking for boot sources: > > /var/petitboot/mnt/dev/nvme0n1p1/grub2/grub.cfg > /var/petitboot/mnt/dev/nvme0n1p1/vmlinux Could you mention this in the commit message as well? The commit message is going to grow pretty large, but that's fine. Regards, Arnout
On Fri, Nov 10, 2023 at 10:01:25AM +0100, Arnout Vandecappelle wrote: >On 09/11/2023 17:16, Reza Arbab wrote: >>On Sun, Nov 05, 2023 at 07:26:16PM +0100, Arnout Vandecappelle wrote: >>>It's also not clear why it would need agetty instead of busybox getty. >> >>Sorry, I didn't explain it very well. The chain goes like this: >> >>1. /etc/init.d/pb-console start console >>2. /usr/libexec/petitboot/pb-console --getty --detach -- -n -i 0 console linux >>3. /sbin/getty -l/usr/libexec/petitboot/pb-console -n -i 0 console linux >>4. /usr/libexec/petitboot/pb-console >>5. /usr/sbin/petiboot-nc >> >>After the change: >> >>1. /etc/init.d/pb-console start console >>2. /usr/libexec/petitboot/pb-console --getty=/sbin/agetty --detach -- -a petituser -n -i console linux >>3. /sbin/agetty -a petituser -n -i console linux > > How did the -l option disappear here? Is that due to --getty= instead of --getty? Not because of --getty= exactly, but you've got the right idea. The script does not add -l if it sees -a. > Anyway, the only reason to use agetty is for the -a option. Can't we >achieve the same by using su? I.e. create a wrapper script around >pb-console that does something like > >exec su -s /usr/libexec/petitboot/pb-console petituser It's tricky. "pb-console --getty" runs getty which runs pb-console again, so at least that first invocation has be done as root. Instead of a wrapper, it can be made to work by adding this at the right place in pb-console: # If we're root, rerun as petituser if [ "$(id -u)" = "0" ]; then exec su -s "$0" petituser $@ fi I'm not sure if patching pb-console is preferable to depending on "agetty -a" and .profile? >>4. /home/petituser/.profile > > Something I didn't touch in my earlier review: I don't think .profile >is the right place to set this up. Either petituser is a normal user - >in that case logging in through some other means (e.g. ssh, sudo) >should give a normal shell. In .profile, I can do if [ "$PPID" = "1" ]; then exec /usr/libexec/petitboot/pb-console fi so it only runs when petituser is logging in by getty/agetty and not through ssh or sudo. >>>>+define PETITBOOT_PERMISSIONS >>>>+ /var/petitboot d 775 root petitgroup - - - - - > > Given the information below, why is it in petitgroup? If pb-discover >is just going to create directories there on which it can mount >things, it's not very useful for pb-console to have write access to >it, right? Unless pb-console needs to delete those directories or >rename them? You're right, I think /var/petitboot is really only used by pb-discover (root), so we can probably drop this. I'll revise and split all this stuff into more than one patch so it's easier to understand when looking through the git log.
diff --git a/package/petitboot/Config.in b/package/petitboot/Config.in index 5f1d91e77ecb..0f965e71e628 100644 --- a/package/petitboot/Config.in +++ b/package/petitboot/Config.in @@ -16,6 +16,7 @@ config BR2_PACKAGE_PETITBOOT select BR2_PACKAGE_KEXEC_LITE if ( BR2_powerpc || BR2_powerpc64 || BR2_powerpc64le ) select BR2_PACKAGE_NVME if ( BR2_powerpc || BR2_powerpc64 || BR2_powerpc64le ) select BR2_PACKAGE_POWERPC_UTILS if ( BR2_powerpc || BR2_powerpc64 || BR2_powerpc64le ) + select BR2_PACKAGE_UTIL_LINUX_AGETTY help Petitboot is a small kexec-based bootloader diff --git a/package/petitboot/S15pb-discover b/package/petitboot/S15pb-discover index 71ab62d99859..a37e33521f1a 100644 --- a/package/petitboot/S15pb-discover +++ b/package/petitboot/S15pb-discover @@ -12,7 +12,9 @@ fi start() { printf 'Starting %s: ' "$DAEMON" - mkdir -p /var/log/petitboot + # shellcheck disable=SC2174 # only apply -m to deepest dir + mkdir -p -m 0775 /var/log/petitboot + chown root:petitgroup /var/log/petitboot # shellcheck disable=SC2086 # we need the word splitting start-stop-daemon -S -q -b -m -p "$PIDFILE" -x "/usr/sbin/$DAEMON" \ diff --git a/package/petitboot/pb-console b/package/petitboot/pb-console index 407ff3b30232..eea40163d02f 100644 --- a/package/petitboot/pb-console +++ b/package/petitboot/pb-console @@ -3,14 +3,16 @@ DAEMON="pb-console" PB_CONSOLE_PORT=${2:-"console"} -PB_CONSOLE_ARGS="--getty --detach -- -n -i 0 $PB_CONSOLE_PORT linux" +PB_CONSOLE_ARGS="--getty=/sbin/agetty --detach -- -a petituser -n -i $PB_CONSOLE_PORT linux" # shellcheck source=/dev/null [ -r "/etc/default/petitboot" ] && . "/etc/default/petitboot" start() { printf 'Starting %s on %s: ' "$DAEMON" "$PB_CONSOLE_PORT" - mkdir -p /var/log/petitboot + # shellcheck disable=SC2174 # only apply -m to deepest dir + mkdir -p -m 0775 /var/log/petitboot + chown root:petitgroup /var/log/petitboot # shellcheck disable=SC2086 # we need the word splitting start-stop-daemon -S -q -x "/usr/libexec/petitboot/$DAEMON" \ diff --git a/package/petitboot/petitboot.mk b/package/petitboot/petitboot.mk index ff87f3498734..5b517eb3b1a6 100644 --- a/package/petitboot/petitboot.mk +++ b/package/petitboot/petitboot.mk @@ -71,6 +71,10 @@ define PETITBOOT_POST_INSTALL $(TARGET_DIR)/usr/sbin/kexec-restart $(INSTALL) -D -m 0755 $(PETITBOOT_PKGDIR)/pb-console \ $(TARGET_DIR)/etc/init.d/pb-console + $(INSTALL) -D -m 0755 $(PETITBOOT_PKGDIR)/shell_config \ + $(TARGET_DIR)/home/petituser/.shrc + $(INSTALL) -D -m 0755 $(PETITBOOT_PKGDIR)/shell_profile \ + $(TARGET_DIR)/home/petituser/.profile mkdir -p $(TARGET_DIR)/etc/udev/rules.d (for port in $(PETITBOOT_GETTY_PORT); do \ @@ -84,4 +88,12 @@ endef PETITBOOT_POST_INSTALL_TARGET_HOOKS += PETITBOOT_POST_INSTALL +define PETITBOOT_USERS + petituser -1 petitgroup -1 * /home/petituser /bin/sh - petitboot user +endef + +define PETITBOOT_PERMISSIONS + /var/petitboot d 775 root petitgroup - - - - - +endef + $(eval $(autotools-package)) diff --git a/package/petitboot/shell_config b/package/petitboot/shell_config new file mode 100644 index 000000000000..b10b95baae6c --- /dev/null +++ b/package/petitboot/shell_config @@ -0,0 +1,24 @@ +#!/bin/sh + +try_sudo() { + [ -x "$(command -v sudo)" ] || return + sudo -K + echo | sudo -S /bin/true >/dev/null 2>&1 || return + + echo "No password required, running as root." + sudo -i + sudo -K + exit +} + +reset + +echo "Exiting petitboot. Type 'exit' to return." +echo "You may run 'pb-sos' to gather diagnostic data." + +if [ "$(id -u)" != "0" ]; then + try_sudo + export PS1='$ ' +else + export PS1='# ' +fi diff --git a/package/petitboot/shell_profile b/package/petitboot/shell_profile new file mode 100644 index 000000000000..1ca5e6364dba --- /dev/null +++ b/package/petitboot/shell_profile @@ -0,0 +1,2 @@ +export ENV="/home/petituser/.shrc" +exec /usr/libexec/petitboot/pb-console
Run the petitboot UI as an unprivileged user. This requires using the agetty package instead of the busybox getty utility, running the initial pb-console helper at user login rather than directly. If sudo is installed, with a sudoers policy allowing petituser to perform sudo with no password (or a blank password), the "drop to shell" feature of petitboot will automatically become a root shell. Signed-off-by: Reza Arbab <arbab@linux.ibm.com> --- package/petitboot/Config.in | 1 + package/petitboot/S15pb-discover | 4 +++- package/petitboot/pb-console | 6 ++++-- package/petitboot/petitboot.mk | 12 ++++++++++++ package/petitboot/shell_config | 24 ++++++++++++++++++++++++ package/petitboot/shell_profile | 2 ++ 6 files changed, 46 insertions(+), 3 deletions(-) create mode 100644 package/petitboot/shell_config create mode 100644 package/petitboot/shell_profile