From 70feff2c2d57696ea9b4e3741f721fac6ebf3be3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Lesimple?= Date: Tue, 26 Jan 2021 16:23:02 +0000 Subject: [PATCH] enh: install: use in-place overwrite for sudoers files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes a race condition in sudo where it would log a log of error messages to syslog if used while we're running the install script: files around sudoers.d/ are then moved around, and it'll yell for each file it previously listed if the file no longer exists when it tries to stat() it. It also deprecates the --no-wait flag of the install script, as now the sudoers.d/ directory will always have integrity at all times. Signed-off-by: Stéphane Lesimple --- bin/admin/install | 74 +++++++++++++++++++++++---------- bin/admin/osh-sync-watcher.sh | 11 ++--- bin/sudogen/generate-sudoers.sh | 8 ++++ lib/shell/functions.inc | 1 + 4 files changed, 66 insertions(+), 28 deletions(-) diff --git a/bin/admin/install b/bin/admin/install index 03efac4..20e0ff9 100755 --- a/bin/admin/install +++ b/bin/admin/install @@ -50,7 +50,6 @@ set_default_options() opt[syslog-ng]=1 opt[overwrite-syslog-ng]=1 opt[migration-grant-aclkeeper-to-gatekeepers]=0 - opt[wait]=1 opt[check-ttyrec]=1 opt[install-fake-ttyrec]=0 } @@ -98,6 +97,7 @@ while [ -n "$1" ]; do elif [ "$1" = "--upgrade" ]; then set_default_options else + # "--[no]-wait" is no longer used, but to keep compatibility, we keep it here (ignored) foundoption=0 for allowedopt in modify-banner modify-sshd-config modify-ssh-config modify-motd modify-umask \ modify-pam-lastlog remove-weak-moduli regen-hostkeys overwrite-logrotate overwrite-cron \ @@ -193,7 +193,6 @@ Usage: --[no-]syslog-ng put our syslog-ng config files in syslog-ng.d/ [N, U] --[no-]overwrite-syslog-ng overwrite possibly existing files in system syslog-ng.d/ with our templates [N, U] - --[no-]wait wait for 3 seconds to avoid race with master/slave sync daemon [N, U, M] --[no-]check-ttyrec verify that the ttyrec installed version is compatible with our code [N, U, M] --[no-]install-fake-ttyrec install a fake ttyrec binary if ttyrec is not present; useful mainly for tests, or if you *really* don't want to use the real ttyrec @@ -227,15 +226,6 @@ if [ "$OS_FAMILY" = FreeBSD ]; then fi fi -if [ "${opt[wait]}" = 1 ]; then - action_doing "Touching lockfile to suspend sync, and waiting 3 seconds to ensure it has been picked up..." - # shellcheck disable=SC2064 - trap "rm -f $LOCKFILE" EXIT HUP INT - touch "$LOCKFILE" - sleep 3 - action_done -fi - if [ "${opt[install-fake-ttyrec]}" = 1 ]; then action_doing "Installing fake ttyrec (use this only for tests!)" if [ ! -e "/usr/bin/ttyrec" ] && [ ! -e "/usr/local/bin/ttyrec" ]; then @@ -522,24 +512,66 @@ if [ "$nothing" = 0 ]; then action_na fi - # delete all bastion sudoers file (pattern osh-*) - action_doing "Remove obsolete sudoers files" - find "$SUDOERS_DIR/" -name "osh-*" -type f -delete - action_done + # List all sudoers.d files matching osh-*. Then, each time we regenerate one below, + # we'll delete it from the list. This way, when we're done, and if any file name remains, + # it means we should delete it because it's obsolete. This way of upgrading in place + # tries to avoid a race condition in sudo where it stat()s all the sudoers.d files, then + # opens them one by one, if some files have disappeared in the meantime, + # it'll log an error to syslog. + action_doing "Listing pre-existing sudoers.d files before in-place regeneration" + oldsudoers=$(mktemp) + find "$SUDOERS_DIR/" -name "osh-*" -type f > "$oldsudoers" + nbfiles=$(wc -l < "$oldsudoers") + action_done "Found $nbfiles sudoers.d files" # copy new sudoers files action_doing "Copy sudoers files to $SUDOERS_DIR" for file in "$basedir/etc/sudoers.d"/osh-*; do - action_detail "$file" - install -o "$UID0" -g "$GID0" -m 0440 "$file" "$SUDOERS_DIR/" + filename=$(basename "$file") + sed_compat "/\/$filename$/d" "$oldsudoers" + action_detail "... copying $filename" + # don't use install(1) because it doesn't overwrite in place (it unlinks then copies data) + # this can lead to a race condition if somebody uses sudo exactly at this moment, where it'll + # log a bunch of errors because files it has listed from sudoers.d have disappeared + # use a .tmp file while we're setting the proper perms, files containing '.' are ignored by sudo + if ! cp "$file" "$SUDOERS_DIR/$filename.tmp"; then + action_error "Failed copying $file to $SUDOERS_DIR/$filename.tmp" + continue + fi + if ! chown "$UID0":"$GID0" "$SUDOERS_DIR/$filename.tmp"; then + action_error "Failed chowing $SUDOERS_DIR/$filename.tmp to $UID0:$GID0" + # attempt to continue + fi + if ! chmod 0440 "$SUDOERS_DIR/$filename.tmp"; then + action_error "Failed chmoding $SUDOERS_DIR/$filename.tmp to 0440" + # attempt to continue + fi + # then overwrite in place + if ! mv -f "$SUDOERS_DIR/$filename.tmp" "$SUDOERS_DIR/$filename"; then + action_error "Failed to move $SUDOERS_DIR/$filename.tmp to $SUDOERS_DIR/$filename" + fi done action_done # regenerate all group sudoers files - "$basedir/bin/sudogen/generate-sudoers.sh" create group + OLD_SUDOERS="$oldsudoers" "$basedir/bin/sudogen/generate-sudoers.sh" create group # regenerate all accounts sudoers files - "$basedir/bin/sudogen/generate-sudoers.sh" create account + OLD_SUDOERS="$oldsudoers" "$basedir/bin/sudogen/generate-sudoers.sh" create account + + action_doing "Removing obsolete sudoers.d files if any..." + nbtoremove=$(wc -l < "$oldsudoers") + if [ "$nbtoremove" = 0 ]; then + action_na + else + for toremove in $(< "$oldsudoers") + do + action_detail "removing $toremove" + rm -f "$toremove" + done + action_done "removed $nbtoremove obsolete files" + fi + rm -f "$oldsudoers" # create the bastionsync account (needed for master/slave) action_doing "Creating the bastionsync account" @@ -1334,11 +1366,11 @@ fi if [ "${opt[check-ttyrec]}" = 1 ] ; then action_doing "Checking ttyrec version" if ! command -v ttyrec >/dev/null 2>&1; then - action_error "ttyrec is not installed, the bastion will not work! Please either install ovh-ttyrec (/opt/bastion/bin/admin/install-ttyrec.sh) or run this script a second time with \`$0 --nothing --no-wait --install-fake-ttyrec'" + action_error "ttyrec is not installed, the bastion will not work! Please either install ovh-ttyrec (/opt/bastion/bin/admin/install-ttyrec.sh) or run this script a second time with \`$0 --nothing --install-fake-ttyrec'" else ttyrec_version=$(ttyrec -V 2>/dev/null | grep -Eo 'ttyrec v[0-9.]+' | cut -c9-) if [ -z "$ttyrec_version" ]; then - action_error "Incompatible ttyrec version installed, the bastion will not work! Please either install ovh-ttyrec (/opt/bastion/bin/admin/install-ttyrec.sh) or run this script again with \`$0 --nothing --no-wait --install-fake-ttyrec'" + action_error "Incompatible ttyrec version installed, the bastion will not work! Please either install ovh-ttyrec (/opt/bastion/bin/admin/install-ttyrec.sh) or run this script again with \`$0 --nothing --install-fake-ttyrec'" else action_detail "found v$ttyrec_version" action_detail "expected v$TTYREC_VERSION_NEEDED" diff --git a/bin/admin/osh-sync-watcher.sh b/bin/admin/osh-sync-watcher.sh index c21d54e..a6dd571 100755 --- a/bin/admin/osh-sync-watcher.sh +++ b/bin/admin/osh-sync-watcher.sh @@ -120,13 +120,10 @@ do else remoteport=22 fi - if [ -e "$LOCKFILE" ] && [ $(( $(date +%s) - $(stat -c %Y "$LOCKFILE") )) -le 300 ]; then - _log "$remote: [Server $(($+1))/$remotehostslen - Step 1/3] syncing needed data postponed for next run (upgrade lockfile present)" - else - _log "$remote: [Server $((i+1))/$remotehostslen - Step 1/3] syncing needed data..." - rsync -vaA --numeric-ids --delete --filter "merge $rsyncfilterfile" --rsh "$rshcmd -p $remoteport" / "$remoteuser@$remote:/" - _log "$remote: [Server $((i+1))/$remotehostslen - Step 1/3] sync ended with return value $?" - fi + + _log "$remote: [Server $((i+1))/$remotehostslen - Step 1/3] syncing needed data..." + rsync -vaA --numeric-ids --delete --filter "merge $rsyncfilterfile" --rsh "$rshcmd -p $remoteport" / "$remoteuser@$remote:/" + _log "$remote: [Server $((i+1))/$remotehostslen - Step 1/3] sync ended with return value $?" _log "$remote: [Server $((i+1))/$remotehostslen - Step 2/3] syncing lastlog files from master to slave, only if master version is newer..." rsync -vaA --numeric-ids --update --include '/' --include '/home/' --include '/home/*/' --include '/home/*/lastlog' --exclude='*' --rsh "$rshcmd -p $remoteport" / "$remoteuser@$remote:/" diff --git a/bin/sudogen/generate-sudoers.sh b/bin/sudogen/generate-sudoers.sh index a2c0b00..8c8c04b 100755 --- a/bin/sudogen/generate-sudoers.sh +++ b/bin/sudogen/generate-sudoers.sh @@ -77,6 +77,10 @@ manage_account_sudoers() } > "${dst}.tmp" # then move the file to its final name (potentially overwriting a previous file of the same name) mv -f "${dst}.tmp" "$dst" + # if we have a OLD_SUDOERS file defined, remove the filename from it + if [ -n "$OLD_SUDOERS" ]; then + sed_compat "/$(basename "$dst")$/d" "$OLD_SUDOERS" + fi return 0 } @@ -133,6 +137,10 @@ manage_group_sudoers() } > "${dst}.tmp" # then move the file to its final name (potentially overwriting a previous file of the same name) mv -f "${dst}.tmp" "$dst" + # if we have a OLD_SUDOERS file defined, remove the filename from it + if [ -n "$OLD_SUDOERS" ]; then + sed_compat "/$(basename "$dst")$/d" "$OLD_SUDOERS" + fi return 0 } diff --git a/lib/shell/functions.inc b/lib/shell/functions.inc index 6d3258c..83f114a 100644 --- a/lib/shell/functions.inc +++ b/lib/shell/functions.inc @@ -37,6 +37,7 @@ LINUX_DISTRO=$(echo "$LINUX_DISTRO" | tr '[:upper:]' '[:lower:]' | tr -d ' ') DISTRO_VERSION_MAJOR=$(echo "$DISTRO_VERSION" | grep -Eo '^[0-9]+' || true) [ -z "$DISTRO_LIKE" ] && DISTRO_LIKE="$LINUX_DISTRO" +# no longer needed, but keep if for a few versions so that non-restarted daemons are still happy # shellcheck disable=SC2034 LOCKFILE=/var/run/bastion-upgrade.lock