From 219f5b4be428388f2b8cc12a7480c4a6922b59ca Mon Sep 17 00:00:00 2001 From: Jordan Petridis Date: Sat, 19 Aug 2023 07:40:30 +0300 Subject: [PATCH 1/2] cmd/initContainer: Be aware of security hardened / or /etc On new builds of GNOME OS [1], the host's / is mounted with 'nodev,...' and those flags are also inherited by /etc because it's not a separate mount point. This leads to the same problem with /etc/machine-id that was seen before with /var/lib/flatpak, /var/lib/systemd/coredump and /var/log/journal [2]. Therefore, use the same approach [2] to handle /etc/machine-id. [1] https://gitlab.gnome.org/GNOME/gnome-build-meta/-/issues/718 [2] Commit 1cc9e07b7c36fe9f https://github.com/containers/toolbox/commit/1cc9e07b7c36fe9f https://github.com/containers/toolbox/pull/1340 https://github.com/containers/toolbox/issues/911 https://github.com/containers/toolbox/pull/1354 Signed-off-by: Jordan Petridis --- src/cmd/initContainer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cmd/initContainer.go b/src/cmd/initContainer.go index 41b825b33..c3424290b 100644 --- a/src/cmd/initContainer.go +++ b/src/cmd/initContainer.go @@ -52,7 +52,7 @@ var ( source string flags string }{ - {"/etc/machine-id", "/run/host/etc/machine-id", "ro"}, + {"/etc/machine-id", "/run/host/etc/machine-id", ""}, {"/run/libvirt", "/run/host/run/libvirt", ""}, {"/run/systemd/journal", "/run/host/run/systemd/journal", ""}, {"/run/systemd/resolve", "/run/host/run/systemd/resolve", ""}, From 6bd7c87932eebc47acef5cba29d3c74d5d759d6f Mon Sep 17 00:00:00 2001 From: Debarshi Ray Date: Tue, 15 Aug 2023 20:57:46 +0200 Subject: [PATCH 2/2] cmd/initContainer: Simplify code by removing a function parameter Until now, configureUsers() was pushing the burden of deciding whether to add a new user or modify an existing one on the callers, even though it can trivially decide itself. Involving the caller loosens the encapsulation of the user configuration logic by spreading it across configureUsers() and it's caller, and adds an extra function parameter that needs to be carefully set and is vulnerable to programmer errors. Fallout from 9ea6fe5852ea8f5225114d825e8e6813e2a3cfea https://github.com/containers/toolbox/pull/1356 --- src/cmd/initContainer.go | 62 ++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 38 deletions(-) diff --git a/src/cmd/initContainer.go b/src/cmd/initContainer.go index c3424290b..c1d8e85ef 100644 --- a/src/cmd/initContainer.go +++ b/src/cmd/initContainer.go @@ -236,24 +236,12 @@ func initContainer(cmd *cobra.Command, args []string) error { } } - if _, err := user.Lookup(initContainerFlags.user); err != nil { - if err := configureUsers(initContainerFlags.uid, - initContainerFlags.user, - initContainerFlags.home, - initContainerFlags.shell, - initContainerFlags.homeLink, - false); err != nil { - return err - } - } else { - if err := configureUsers(initContainerFlags.uid, - initContainerFlags.user, - initContainerFlags.home, - initContainerFlags.shell, - initContainerFlags.homeLink, - true); err != nil { - return err - } + if err := configureUsers(initContainerFlags.uid, + initContainerFlags.user, + initContainerFlags.home, + initContainerFlags.shell, + initContainerFlags.homeLink); err != nil { + return err } if utils.PathExists("/etc/krb5.conf.d") && !utils.PathExists("/etc/krb5.conf.d/kcm_default_ccache") { @@ -386,9 +374,7 @@ func initContainerHelp(cmd *cobra.Command, args []string) { } } -func configureUsers(targetUserUid int, - targetUser, targetUserHome, targetUserShell string, - homeLink, targetUserExists bool) error { +func configureUsers(targetUserUid int, targetUser, targetUserHome, targetUserShell string, homeLink bool) error { if homeLink { if err := redirectPath("/home", "/var/home", true); err != nil { return err @@ -400,45 +386,45 @@ func configureUsers(targetUserUid int, return fmt.Errorf("failed to get group for sudo: %w", err) } - if targetUserExists { - logrus.Debugf("Modifying user %s with UID %d:", targetUser, targetUserUid) + if _, err := user.Lookup(targetUser); err != nil { + logrus.Debugf("Adding user %s with UID %d:", targetUser, targetUserUid) - usermodArgs := []string{ - "--append", + useraddArgs := []string{ "--groups", sudoGroup, - "--home", targetUserHome, + "--home-dir", targetUserHome, + "--no-create-home", "--shell", targetUserShell, "--uid", fmt.Sprint(targetUserUid), targetUser, } - logrus.Debug("usermod") - for _, arg := range usermodArgs { + logrus.Debug("useradd") + for _, arg := range useraddArgs { logrus.Debugf("%s", arg) } - if err := shell.Run("usermod", nil, nil, nil, usermodArgs...); err != nil { - return fmt.Errorf("failed to modify user %s with UID %d: %w", targetUser, targetUserUid, err) + if err := shell.Run("useradd", nil, nil, nil, useraddArgs...); err != nil { + return fmt.Errorf("failed to add user %s with UID %d: %w", targetUser, targetUserUid, err) } } else { - logrus.Debugf("Adding user %s with UID %d:", targetUser, targetUserUid) + logrus.Debugf("Modifying user %s with UID %d:", targetUser, targetUserUid) - useraddArgs := []string{ + usermodArgs := []string{ + "--append", "--groups", sudoGroup, - "--home-dir", targetUserHome, - "--no-create-home", + "--home", targetUserHome, "--shell", targetUserShell, "--uid", fmt.Sprint(targetUserUid), targetUser, } - logrus.Debug("useradd") - for _, arg := range useraddArgs { + logrus.Debug("usermod") + for _, arg := range usermodArgs { logrus.Debugf("%s", arg) } - if err := shell.Run("useradd", nil, nil, nil, useraddArgs...); err != nil { - return fmt.Errorf("failed to add user %s with UID %d: %w", targetUser, targetUserUid, err) + if err := shell.Run("usermod", nil, nil, nil, usermodArgs...); err != nil { + return fmt.Errorf("failed to modify user %s with UID %d: %w", targetUser, targetUserUid, err) } }