Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

New 'etcupdate' sub command #660

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

New 'etcupdate' sub command #660

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c3b20ed
New 'etcupdate' sub command
Rodrigo-NH Jan 6, 2024
7031fb3
Fix 'directory' nomenclature
Rodrigo-NH Jan 17, 2024
98866dc
correct path description of /etc directory in log generation
Rodrigo-NH Jan 17, 2024
2dcb30c
Fix English
Rodrigo-NH Jan 30, 2024
98706c0
Diff makes sense the other way round
Rodrigo-NH Jan 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions usr/local/bin/bastille
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ bastille_perms_check() {
bastille_perms_check

## version
BASTILLE_VERSION="0.10.20231125"
BASTILLE_VERSION=3a4ebc63bb84b66d456713e608be86e4cba3b637

usage() {
cat << EOF
Expand Down Expand Up @@ -147,7 +147,7 @@ version|-v|--version)
help|-h|--help)
usage
;;
bootstrap|create|destroy|export|import|list|rdr|restart|setup|start|update|upgrade|verify)
bootstrap|create|destroy|export|import|list|rdr|restart|setup|start|update|upgrade|verify|etcupdate)
# Nothing "extra" to do for these commands. -- cwells
;;
clone|config|cmd|console|convert|cp|edit|htop|limits|mount|pkg|rcp|rename|service|stop|sysrc|tags|template|top|umount|zfs)
Expand Down
254 changes: 254 additions & 0 deletions usr/local/share/bastille/etcupdate.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,254 @@
#!/bin/sh
#
# Copyright (c) 2018-2023, Rodrigo Nascimento Hernandez <rodrigomdev@gmail.com>
# All rights reserved.
#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions are met:
#
# * Redistributions of source code must retain the above copyright notice, this
# list of conditions and the following disclaimer.
#
# * Redistributions in binary form must reproduce the above copyright notice,
# this list of conditions and the following disclaimer in the documentation
# and/or other materials provided with the distribution.
#
# * Neither the name of the copyright holder nor the names of its
# contributors may be used to endorse or promote products derived from
# this software without specific prior written permission.
#
# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
# DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE
# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
# SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
# CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

. /usr/local/share/bastille/common.sh
. /usr/local/etc/bastille/bastille.conf

usage() {
# Update /etc directory while keeping user changes
error_notify "Usage: bastille etcupdate [option(s)(optional)] [jailname] [oldrelease] [newrelease]"

cat << EOF
Options:

-D | --dryrun -- Do a dry run. Output actions to stdout but without making changes.
-Q | --quiet -- Do not output actions to stdout

EOF
exit 1
}


executeconditional() {
if [ $DRY_RUN -eq "0" ]; then
eval "$@"
fi
}

C1_C6_conditions() {
Copy link

@jimbobmcgee jimbobmcgee Jul 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, the C1-C8 naming convention makes it difficult to read what is going on, which might make for maintenance difficulty. Perhaps prefer functions and variables that actually describe the operation, rather than the opaque, e.g. user_added (I think that's C1), upgrade_added (C2?), for readability.

filelistjail=$(find "${jail_etc}" -mindepth 1 -type f)
for jailfile in ${filelistjail}
Copy link

@jimbobmcgee jimbobmcgee Jul 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pattern of for X in $(find...) is risky if filenames contain whitespace or control chars, but the "usual" mechanisms to sanely deal with arbitrary filenames are typically bash-specific (e.g. find ... -print0 | while IFS= read -r -d''; do ... done).

Generally, in POSIX shells, the only supported way (I know of) is to find -exec sh -c '...' + or find -print0 | xargs -0 sh -c '...', which would introduce a subshell and affect availability/scope of your variables.

Perhaps, this may be better written as...

search_jail_etc_dir () {
  [ -n "$1" ] || return 1
  [ -d "$1" ] || return 1

  for jailfile in "${1}"/*; do
    if [ -d "${jailfile}" ]; then
      search_jail_etc_dir "${jailfile}"
    else
      process_jail_etc_file "${jailfile}"
    fi
  done
}

process_jail_etc_file () {
  ...
}

search_jail_etc_dir "${jail_etc}"

This should ensure that filenames are properly quoted, so even if they contain garbage, they will still be handled correctly.

do
filepart=$(echo "${jailfile}" | awk -F 'etc/' '{print $NF}')
newbasefile="${new_basedir}/etc/${filepart}"
currentbasefile="${current_basedir}/etc/${filepart}"

if [ ! -f "${currentbasefile}" ]; then
if [ ! -f "${newbasefile}" ]; then
C1=$((C1+1))
C1ct="$C1ct${jailfile}\n"
Copy link

@jimbobmcgee jimbobmcgee Jul 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even considering the readability of C1 (as above), the suffix ct does not make sense. I believe this is a file list, built up for display purposes, but ct could easily be taken to mean count.

Perhaps $user_added_list and $user_added_count would make it more explicit?

else
C2=$((C2+1))
C2ct="$C2ct${jailfile}\n"
fi
fi

if [ -f "${currentbasefile}" ]; then
diffr=$(diff -u "${jailfile}" "${currentbasefile}")
Copy link

@jimbobmcgee jimbobmcgee Jul 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the output of diff -u is important, consider using temp files to capture it rather than variables. Variables can have whitespace folding and length issues, which might lead to unexpected corruption.

Also, if diff3 is available (i.e. if command -v diff3 >/dev/null 1>&2; then ... fi), perhaps use a threeway diff to generate auto-updated files, e.g. diff3 -A "${newbasefile}" "${currentbasefile}" "${jailfile}" (I think that's the right way around for those file args; it may also/alternatively need -m). This should be able to better differentiate between changes that were made by the end-user and changes that were introduced by the update. I think this is what freebsd-update does for the "normal" install/upgrade procedure, and it might be good to do something similar here.

Perhaps use diffdir="$(mktemp -d -t "${TMPDIR:-/tmp}/etcupdate.XXXXXX")" to make a temporary directory, then write your diffs into the temp dir. Let the end-user decide whether to use your version or the existing version, then move the desired version out of the directory into the correct place. Use trap "[ -d '${diffdir}' ] && rm -rf -- '${diffdir}'" EXIT INT TERM to clean up all unused temp files at the end of the run.

Also, perhaps allow end-user to invoke vi (or "${VISUAL:-${EDITOR:-vi}}" for style points) on the updated file, so they can make manual edits to each file prior to acceptance. Especially, if doing in-place diffs with diff3, you can use the exit code to determine if a diff conflict has happened ([ "$?" -eq 1 ]), and prompt them to manually resolve.

if [ -z "${diffr}" ]; then
if [ ! -f "${newbasefile}" ]; then
C3=$((C3+1))
C3ct="$C3ct${jailfile}\n"
cmd="rm -rf ${jailfile}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend -- between switches and file list; if someone happens to place a file called -x into /etc, rm would see this as an argument, not a file. e.g. rm -f -- ${jailfile}

If $jailfile is always a file (constrained by find -type f in L56), does it need to be recursive -r?

Possible issue with whitespace in filename, if filename is folded into eval'd command string? Since you are using eval "$@" in executeconditional, couldn't this simply be executeconditional rm -f -- "${jailfile}"

executeconditional "$cmd"
else
C4=$((C4+1))
C4ct="$C4ct${jailfile}\n"
cmd="cp -p ${newbasefile} ${jailfile}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As with above, could this be executeconditional cp -p -- "${newbasefile}" "${jailfile}"

executeconditional "$cmd"
# Copy keeping permissions
fi
else
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, there have been changes to the jail's /etc files compared to the currentbasefile, so there will be a diff for C5 or C6. Rather than have a list of diffs to revisit later (the conservative approach is to leave them alone), it would be great to have the option enter an interactive mode to approve/modify the diff here.

diffs="${diffs}${diffr}"
diffs="${diffs}\n==========================================================================================================\n\n"
if [ -f "${newbasefile}" ]; then
C5=$((C5+1))
C5ct="$C5ct${jailfile}\n"
else
C6=$((C6+1))
C6ct="$C6ct${jailfile}\n"
fi
fi
fi
done
}

# Creates missing directories from UPGRADEVERSION in the jail preserving original permissions
C7_conditions() {
dirlistrelease=$(find "${new_basedir}/etc" -mindepth 1 -type d)
for dirpath in ${dirlistrelease}
do
dirpathnf=$(echo "${dirpath}" | awk -F '/etc' '{print $NF}')
jailpath="${bastille_jail_base}/root/etc${dirpathnf}"
Rodrigo-NH marked this conversation as resolved.
Show resolved Hide resolved
if [ ! -d "${jailpath}" ]; then
C7=$((C7+1))
cmd="mkdir ${jailpath}"
Copy link

@jimbobmcgee jimbobmcgee Jul 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As with above, could this be executeconditional mkdir -p -- "${jailpath}"
(the -p would make any missing intermediate parent dirs; omit it if you don't want this)

executeconditional "$cmd"
dirperm=$(stat -f "%Mp%Lp" "${dirpath}")
cmd="chmod ${dirperm} ${jailpath}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As with above, could this be executeconditional chmod "${dirperm}" -- "${jailpath}"

executeconditional "$cmd"
C7ct="$C7ct${jailpath}\n"
fi
done
}

# Copy missing files from UPGRADEVERSION to the jail preserving original permissions
C8_conditions() {
filelistrelease=$(find "${new_basedir}/etc" -mindepth 1 -type f)
for sourcefile in ${filelistrelease}
do
dirpathnf=$(echo "${sourcefile}" | awk -F '/etc' '{print $NF}')
jailfile="${bastille_jail_base}/root/etc${dirpathnf}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest changing bastille_jail_base to something like bastille_thin_jail because the former makes it sound like a "base" jail, which is what the release jails are in this context. The thin jail is the jail to be operated on.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I agree with you, I named this variable following the same convention found in the codebase, for consistency. Perhaps best left as is until it (if) is reviewed as a whole? I can see pros and cons of both approaches. So.. umm.. I'm not sure.

bastille/usr/local/share/bastille/create.sh

Line 274 in 3a4ebc6

bastille_jail_base="${bastille_jailsdir}/${NAME}/root/.bastille" ## dir

bastille/usr/local/share/bastille/destroy.sh

Line 40 in 3a4ebc6

bastille_jail_base="${bastille_jailsdir}/${TARGET}" ## dir

if [ ! -f "${jailfile}" ]; then
C8=$((C8+1))
cmd="cp -p ${sourcefile} ${jailfile}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As with above, could this be executeconditional cp -p -- "${sourcefile}" "${jailfile}"

executeconditional "$cmd"
C8ct="$C8ct${jailfile}\n"
fi
done
}

formatoutput() {
output="SUMMARY:\n"
txtvar=""
txtname=""
for x in 1 2 3 4 5 6 7 8; do
eval txtvar="\$"C$x
Copy link

@jimbobmcgee jimbobmcgee Jul 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are trying to loop over the passed function arguments here, i.e. $1, $2, etc...

Consider refactoring into a while [ -n "$1" ]; do ... shift; done -- then you can always use $1 and have no limitation on the number of arguments passed:

format_output_summary () {
  printf 'SUMMARY:\n'
  while [ -n "$1" ]; do
    printf '%s = %s\n' "$1" "$2"
    shift; shift
  done
}

format_output_details () { : ... ; } #...todo

format_output () {
  format_output_summary "$C1txt" "$C1" "$C2txt" "$C2" "$C3txt" "$C3" "$C4txt" "$C4" #...etc
  format_output_details "$C1ct" "$C1txt" "$C2ct" "$C2txt" "$C3ct" "$C3txt" "$C4ct" "$C4txt" #...etc
}

eval txtname="\$"C$x"txt"
output="$output${txtname} = ${txtvar}\n"
done

output="${output}\nDETAILS:\n"

for x in 1 2 3 4 5 6 7 8; do
eval txtvar="\$"C$x"ct"
eval txtname="\$"C$x"txt"
output="${output}${txtname}\n${txtvar}"
output="${output}==========================================================================================================\n\n"
done

output="${output}\nDIFF for files of conditions C5 & C6:\n"
output="${output}${diffs}"

printf '%b\n' "${output}"
}

# Handle special-case commands first.
case "$1" in
help|-h|--help)
usage
;;
esac

bastille_root_check

# Handle and parse options
DRY_RUN="0"
QUIET="0"
while [ $# -gt 0 ]; do
case "${1}" in
-D|--dryrun)
DRY_RUN="1"
shift
;;
-Q|--quiet)
QUIET="1"
shift
;;
-*|--*)
error_notify "Unknown Option."
usage
;;
*)
break
;;
esac
done

TARGET="${1}"
COMPAREVERSION="${2}"
UPGRADEVERSION="${3}"
bastille_jail_base="${bastille_jailsdir}/${TARGET}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest changing bastille_jail_base to something like bastille_thin_jail because the former makes it sound like a "base" jail, which is what the release jails are in this context. The thin jail is the jail to be operated on.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#660 (comment)


if [ $# -gt 3 ] || [ $# -lt 3 ]; then
usage
fi

if [ "$(/usr/sbin/jls name | awk "/^${TARGET}$/")" ]; then
error_notify "Jail running."
error_exit "See 'bastille stop ${TARGET}'."
fi

if [ ! -d "${bastille_jail_base}" ]; then
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest changing bastille_jail_base to something like bastille_thin_jail because the former makes it sound like a "base" jail, which is what the release jails are in this context. The thin jail is the jail to be operated on.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#660 (comment)

error_exit "Jail not found."
fi

## check for required releases
if [ ! -d "${bastille_releasesdir}/${COMPAREVERSION}" ] || [ ! -d "${bastille_releasesdir}/${UPGRADEVERSION}" ]; then
error_exit "Releases must be bootstrapped first; see 'bastille bootstrap'."
fi

jail_root="${bastille_jail_base}/root"
jail_etc="${jail_root}/etc"
new_basedir="${bastille_releasesdir}/${UPGRADEVERSION}"
current_basedir="${bastille_releasesdir}/${COMPAREVERSION}"

C1=0
C1txt="Condition C1:\nJail's /etc files that doesn't exist in \
Rodrigo-NH marked this conversation as resolved.
Show resolved Hide resolved
${COMPAREVERSION} and doesn't exist in ${UPGRADEVERSION} Action: keep current files"
Rodrigo-NH marked this conversation as resolved.
Show resolved Hide resolved
C2=0
C2txt="Condition C2:\nJail's /etc files that doesn't exist in \
Rodrigo-NH marked this conversation as resolved.
Show resolved Hide resolved
${COMPAREVERSION} but exist in ${UPGRADEVERSION} Action: keep current files"
C3=0
C3txt="Condition C3:\nJail's /etc files that weren't modified when compared to \
${COMPAREVERSION} but doesn't exist in ${UPGRADEVERSION} Action: delete current files"
Rodrigo-NH marked this conversation as resolved.
Show resolved Hide resolved
C4=0
C4txt="Condition C4:\nJail's /etc files that weren't modified when compared to \
${COMPAREVERSION} and exist in ${UPGRADEVERSION} Action: update/copy the newer files"
C5=0
C5txt="Condition C5:\nJail's /etc files that were modified when compared to \
${COMPAREVERSION} and exist in ${UPGRADEVERSION} Action: keep the current files"
C6=0
C6txt="Condition C6:\nJail's /etc files that were modified when compared to \
${COMPAREVERSION} and doesn't exist in ${UPGRADEVERSION} Action: keep the current files"
Rodrigo-NH marked this conversation as resolved.
Show resolved Hide resolved
C7=0
C7txt="Condition C7:\nCreate directories (with permissions) that exist in \
${UPGRADEVERSION} /etc but doesn't exist in the jail"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change "doesn't" to "don't"

C8=0
C8txt="Condition C8:\nCopy files (with permissions) that exist in \
${UPGRADEVERSION} /etc but doesn't exist in the jail"
Rodrigo-NH marked this conversation as resolved.
Show resolved Hide resolved
diffs=""

C1_C6_conditions
C7_conditions
C8_conditions
if [ $QUIET -eq "0" ]; then
formatoutput
fi