Create Apache2 virtualhost, Mysql db & user. CLI tool and GUI on top of it

Question

Review for optimization, code standards, missed validation.

What's in this version:

• Rewritten, instead of large complex script - few tools with each own task.
• Solid CLI tool and GUI script on top of it.
• Apply all previous code reviews on new code.
• Some improvements from myself
• Input arguments as usual and pass users/passwords to mysql script as environment vars for improved security.
• Man page written(not for review)

Github

virtualhost-cli.sh - main cli tool, the only one script what need root

#!/usr/bin/env bash

set -e

cd "${0%/*}"

source virtualhost.inc.sh

parseargs "$@"
validate
parse
#connect

hostfile="${config[a2ensite]}${config[subdomain]}.conf"
siteconf="${config[apachesites]}${hostfile}"

(cat >"$siteconf" <<EOF
<VirtualHost ${config[virtualhost]}:${config[virtualport]}>
  ServerAdmin ${config[serveradmin]}
    DocumentRoot ${config[webroot]}
    ServerName  ${config[domain]}
    ServerAlias ${config[domain]}
    <Directory "${config[webroot]}">
      AllowOverride All
      Require local
    </Directory>
    # Available loglevels: trace8, ..., trace1, debug, info, notice, warn,
    # error, crit, alert, emerg.
    LogLevel error
</VirtualHost>
# vim: syntax=apache ts=4 sw=4 sts=4 sr noet
EOF
) || die "May run as root or give $siteconf writable permissions to current user"

(
  mkdir -p "${config[webroot]}"
  chown ${config[webmaster]}:${config[webgroup]} "${config[webroot]}"
  chmod u=rwX,g=rXs,o= "${config[webroot]}"
  chown root:root "$siteconf"
  chmod u=rw,g=r,o=r "$siteconf"
  a2ensite "${hostfile}"
  systemctl reload apache2
) || die "Run as root"

die "Config file saved and enabled at ${siteconf}" "Notice" 0

virtualhost.inc.sh - lib used by all other scripts

#check if function exists
if ! type die &>/dev/null;then
  die() {
    echo "${2:-Error}: $1" >&2
    exit ${3:-1}
  }
fi
[[ "${BASH_VERSINFO:-0}" -ge 4 ]] || die "Bash version 4 or above required"

# defaults
declare -A config=()
config[webmaster]="$(id -un)"   # user who access web files. group is www-data
config[webgroup]="www-data"     # apache2 web group, does't need to be webmaster group. SGID set for folder.
config[webroot]='${homedir}/Web/${subdomain}'
config[domain]="localhost"      # domain for creating subdomains
config[virtualhost]="*"         # ip of virtualhost in case server listen on many interfaces or "*" for all
config[virtualport]="80"        # port of virtualhost. apache2 must listen on that ip:port
config[serveradmin]="webmaster@localhost" # admin email
config[a2ensite]="050-"         # short prefix for virtualhost config file
config[apachesites]="/etc/apache2/sites-available/" # virtualhosts config folder

declare -A mysql=() # mysql script read values from env

have_command() {
  type -p "$1" >/dev/null
}

try() {
  have_command "$1" && "$@"
}

if_match() {
  [[ "$1" =~ $2 ]]
}

validate() {
  [[ -z $1 && -z "${config[subdomain]}" ]] && die "--subdomain required"
  [[ "${config[webmaster]}" == "root" ]] && die "--webmaster should not be root"
  id "${config[webmaster]}" >& /dev/null || die "--webmaster user '${config[webmaster]}' not found"
  getent group "${config[webgroup]}" >& /dev/null
  [[ $? -ne 0 ]] && die "Group ${config[webgroup]} not exists"
  have_command apache2 || die "apache2 not found"
  [[ -d ${config[apachesites]} ]] || die "apache2 config folder not found"

  (LANG=C; if_match "${config[domain]}" "^[a-zA-Z]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9\.])*$") || \
    die "Bad domain"
  [[ -z "$1" ]] && ((LANG=C; if_match "${config[subdomain]}" "^[a-zA-Z]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?$") || \
    die "Bad subdomain")
  (LANG=C; if_match "${config[serveradmin]}" \
    "^[a-z0-9!#\$%&'*+/=?^_\`{|}~-]+(\.[a-z0-9!#$%&'*+/=?^_\`{|}~-]+)*@([a-z0-9]([a-z0-9-]*[a-z0-9])?\.)*[a-z0-9]([a-z0-9-]*[a-z0-9])?\$" \
    ) || die "Bad admin email"

  octet="(25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9]?[0-9])"
  (if_match "${config[virtualhost]}" "^$octet\\.$octet\\.$octet\\.$octet$") || \
    (if_match "${config[virtualhost]}" "^[a-zA-Z]([a-zA-Z0-9-]{0,61}[a-zA-Z0-9\.])*$") || \
    (if_match "${config[virtualhost]}" "^\*$") || \
    die "Bad virtualhost"
  (if_match "${config[virtualport]}" "^[1-9][0-9]+$") || die "Bad virtualport"
}

tolowercase() {
  echo "${1}" | tr '[:upper:]' '[:lower:]'
}

parse() {
  config[webroot]=$(echo "${config[webroot]}" | \
  homedir=$( getent passwd "${config[webmaster]}" | cut -d: -f6 ) \
  webmaster="${config[webmaster]}" \
  subdomain="${config[subdomain]}" \
  domain="${config[subdomain]}.${config[domain]}" \
  envsubst '${homedir},${webmaster},${subdomain},${domain}')

  config[domain]="${config[subdomain]}.${config[domain]}"
  config[domain]=$(tolowercase "${config[domain]}")
  config[subdomain]=$(tolowercase "${config[subdomain]}")
  config[virtualhost]=$(tolowercase "${config[virtualhost]}")
}

# check if apache listening on defined host:port
connect() {
  (systemctl status apache2) &>/dev/null || return
  local host="${config[virtualhost]}"
  [[ "$host" == "*" ]] && host="localhost"
  ret=0
  msg=$(netcat -vz "$host" "${config[virtualport]}" 2>&1) || ret=$? && true
  [[ $ret -ne 0 ]] && die "$msg"
}

# load all allowed arguments into $config array
parseargs() {
  (getopt --test > /dev/null) || true
  [[ "$?" -gt 4 ]] && die 'I’m sorry, `getopt --test` failed in this environment.'
  OPTIONS=""
  LONGOPTS="help,webmaster:,webgroup:,webroot:,domain:,subdomain:,virtualhost:,virtualport:,serveradmin:"
  ! PARSED=$(getopt --options=$OPTIONS --longoptions=$LONGOPTS --name "$0" -- "$@")
  if [[ ${PIPESTATUS[0]} -ne 0 ]]; then
    # e.g. return value is 1
    # then getopt has complained about wrong arguments to stdout
    exit 2
  fi
  # read getopt’s output this way to handle the quoting right:
  eval set -- "$PARSED"
  while true; do
    case "$1" in
      --help)
        man -P cat ./virtualhost.1
        exit 0
        ;;
      --)
        shift
        break
        ;;
      *)
        index=${1#--} # limited to LONGOPTS
        config[$index]=$2
        shift 2
        ;;
    esac
  done

}

validate_mysql() {
  for key in adminuser database user;do
    (LANG=C; if_match "${mysql[$key]}" "^[a-zA-Z][a-zA-Z0-9_-]*$") || die "bad mysql $key"
  done
}
escape_mysql() {
  for key in adminpasswd passwd;do
    printf -v var "%q" "${mysql[$key]}"
    mysql[$key]=$var
  done
}

virtualhost-yad.sh - GUI tool for CLI scripts

#!/usr/bin/env bash

set -e

cd "${0%/*}"

die() {
  echo "${2:-Error}: $1" >&2
  xmessage -buttons Ok:0 -nearmouse "${2:-Error}: $1" -timeout 10
  exit ${3:-1}
}

source virtualhost.inc.sh

in_terminal() {
  [ -t 0 ]
}

die() {
  local msg="${2:-Error}: $1"
  echo "$msg" >&2
  in_terminal && exit ${3:-1}
  try notify-send "$msg" && exit ${3:-1}
  try yad --info --text="$msg" && exit ${3:-1}
  try xmessage -buttons Ok:0 -nearmouse "$msg" -timeout 10 && exit ${3:-1}
  exit ${3:-1}
}

have_command yad || die "yad package required. 'sudo apt install yad'"

user_info() {
  yad --title="Virtualhost" --window-icon="${2:-error}" --info --text="$1" --timeout="${3:-15}" --button="Ok:0" --center
}

parseargs "$@"

while true; do
  formbutton=0
  formoutput=$(yad --form --field="Subdomain" --field="Domain" --field="Web master username" \
    --field="Apache group" --field='Webroot' \
    --field='Webroot variables - ${homedir}(of webmaster) ${subdomain} ${webmaster} ${domain}:LBL' \
    --field="Virtualhost ip or domain" \
    --field="Virtualhost port" --field="Server admin email" \
    --field="Create mysql user&db:CHK" \
    --field="Mysql admin user" --field="Mysql admin password" \
    --field="Create database" \
    --field="Create mysql user" --field="Create mysql password" \
    --button="Cancel:5" --button="Save defaults:2" --button="Create:0" \
    --title="Create apache virtualhost" \
    --text='Subdomain are case sensetive for Webroot folder ${subdomain} variable' \
    --focus-field=1 --center --window-icon="preferences-system" --width=600 \
    "${config[subdomain]}" "${config[domain]}" "${config[webmaster]}" "${config[webgroup]}" \
    "${config[webroot]}" "test" "${config[virtualhost]}" "${config[virtualport]}" \
    "${config[serveradmin]}" true \
    "${mysql[adminuser]}" "${mysql[adminpasswd]}" \
    "${mysql[database]}" "${mysql[user]}" "${mysql[passwd]}" \
    ) || formbutton="$?" && true
  # Cancel(5) or close window(other code)
  [[ "$formbutton" -ne 0 && "$formbutton" -ne 2 && "$formbutton" -ne 1 ]] && die "Cancel"

  IFS='|' read -r -a form <<< "$formoutput"

  pos=0
  for key in subdomain domain webmaster webgroup webroot nothing virtualhost virtualport serveradmin;do
    config[$key]="${form[$pos]}"
    let pos=pos+1
  done

  usemysql=
  [[ "${form[9]}" -eq "TRUE" ]] && usemysql=1

  pos=10
  for key in adminuser adminpasswd database user passwd;do
    mysql[$key]="${form[$pos]}"
    let pos=pos+1
  done

  vres=0
  # subdomain can't be default option, skip it
  [[ "$formbutton" -eq 2 ]] && skipsubdomain=1 || skipsubdomain=
  # validate input, continue or show error and return to form
  valoutput=$(validate $skipsubdomain 2>&1) || vres=$? && true
  [[ "$vres" -ne 0 ]] && user_info "$valoutput" && continue

  clires=0
  if [[ "$formbutton" -ne 2 ]]; then
    cmd="pkexec `pwd`/virtualhost-cli.sh"
    [[ "$formbutton" -eq 2 ]] && cmd="./virtualhost-install.sh"
    clioutput=$($cmd --subdomain "${config[subdomain]}" \
    --domain "${config[domain]}" --webmaster "${config[webmaster]}" \
    --webgroup "${config[webgroup]}" --webroot "${config[webroot]}" \
    --virtualhost "${config[virtualhost]}" --virtualport "${config[virtualport]}" \
    --serveradmin "${config[serveradmin]}" 2>&1) || clires=$? && true
    [[ "$clioutput" ]] && user_info "$clioutput" || true
    [[ "$clires" -ne 0 ]] && continue
    # mysql
    if [[ "$usemysql" ]]; then
      mysqlres=0
      mysqloutput=$(adminuser="${mysql[adminuser]}" adminpwd="${mysql[adminpasswd]}" \
      database="${mysql[database]}" mysqluser="${mysql[user]}" \
      mysqlpasswd="${mysql[passwd]}" ./virtualhost-mysql.sh \
      --subdomain "${config[subdomain]}" 2>&1) || mysqlres=$? && true
      [[ "$mysqloutput" ]] && user_info "$mysqloutput" || true
      [[ "$mysqlres" -ne 0 ]] && continue
      break
    fi
    break
  fi
done

virtualhost-mysql.sh - create db and user, values passed by environment variables to not appear in ps output - for security.

#!/usr/bin/env bash

set -e

cd "${0%/*}"

source virtualhost.inc.sh

parseargs "$@" # only --subdomain used
# read values from env
subdomain=$(tolowercase "${config[subdomain]}")
mysql[adminuser]="${adminuser:-root}"
mysql[adminpasswd]="${adminpwd}"
mysql[database]="${mysqldatabase:-${subdomain}}"
mysql[user]="${mysqluser:-${subdomain:-$(id -un)}}"
mysql[passwd]="${mysqlpasswd}"

validate_mysql
escape_mysql

mysqlcreate=$(cat <<EOF
CREATE USER '${mysql[user]}'@'localhost' IDENTIFIED BY '${mysql[passwd]}';
GRANT USAGE ON *.* TO '${mysql[user]}'@'localhost';
CREATE DATABASE IF NOT EXISTS \`${mysql[database]}\` CHARACTER SET utf8 COLLATE utf8_general_ci;
GRANT ALL PRIVILEGES ON \`${mysql[database]}\`.* TO '${mysql[user]}'@'localhost';
FLUSH PRIVILEGES;
EOF
)

mysql --user="${mysql[adminuser]}" --password="${mysql[adminpasswd]}" <<<$mysqlcreate

virtualhost-install.sh - install desktop shortcut with arguments defined as defaults for GUI tool. Can be executed from "virtualhost-yad.sh" with values from form. Not for review as simple and similar.

Answer 1

Naming

In larger scripts like this, naming is even more important than usual, to help the reader understand the elements of the program easily.

Looking at this at the beginning and I'm already puzzled:

parseargs "$@"
validate
parse
#connect

Although it's pretty obvious what parseargs will do, it's not so obvious how parse is different. And what is validate going to do that parseargs doesn't already do? Why doesn't it do everything needed?

And what is #connect? Looking at the implementation I see it checks if a virtual host + port is up. A more verbose, descriptive name would be better for this function.

The other functions are not so easy to fix, we need to look at an organizational issue first.

---

parseargs populates a config array. This may be subjective, the name "config" makes me thing of some sort of configuration file, but in this case it's about command line arguments. I would rename it to args.

In fact in this program config is used for multiple purposes: store defaults, and then parsed arguments. I would find it easier to understand to separate these two (define defaults), and have an additional step that combines them into params.

Program organization

As hinted earlier, I would have expected parseargs to encapsulate everything needed before the program is ready to execute its main job.

I see that the different scripts have slightly different needs:

• virtualhost-cli.sh calls parseargs, validate and parse
• virtualhost-yad.sh calls just parseargs
• virtualhost-mysql.sh calls parseargs and validate_mysql

I see that parseargs is a common element, but it's hard to see what is included in it and what is not, and how much of what it does is shared. In fact virtualhost-mysql.sh only needs it to parse --subdomain.

Consider this alternative usage:

In virtualhost-cli.sh:

parseargs_opts="help,webmaster:,webgroup:,webroot:,domain:,subdomain:,virtualhost:,virtualport:,serveradmin:"
parseargs "$@"

In virtualhost-yad.sh:

parseargs_opts="help,webmaster:,webgroup:,webroot:,domain:,subdomain:,virtualhost:,virtualport:,serveradmin:"
parseargs "$@"

In virtualhost-mysql.sh:

parseargs_opts="help,subdomain:"
parseargs "$@"

This way it would be much more clear what parseargs does, where its responsibilities begin and end. Yes there is some duplication for two of the scripts, but I think the clarity this alternative organization brings makes it well worth it.

---

Another program organization smell is that validate_mysql and escape_mysql are defined in a shared script, but only used by virtualhost-mysql.sh. It would be better to define these functions in the same script where they are used.

Sloppy error messages

Early in the script there's this:

(cat >"$siteconf" <<EOF
<VirtualHost ${config[virtualhost]}:${config[virtualport]}>
  ServerAdmin ${config[serveradmin]}
  ...
</VirtualHost>
EOF
) || die "May run as root or give $siteconf writable permissions to current user"

And then this:

(
  mkdir -p "${config[webroot]}"
  chown ${config[webmaster]}:${config[webgroup]} "${config[webroot]}"
  chmod u=rwX,g=rXs,o= "${config[webroot]}"
  ...
) || die "Run as root"

I don't like the error messages in die "..." because I don't think you can know for sure that they are always appropriate. In compound operations like this, I would phrase the error message after the big-picture task it was trying to perform and failed, and not speculate about what the solution might be.

Btw, if the second snippet requires root, then it would be better to check the user's identity at the very beginning of the script and fail fast.

Don't reference undefined variables

It seems to me that virtualhost-yad.sh references ${mysql[...]} before those values are set, in the long while true; do loop body. If this is not a bug, well it's hard to read and confusing.

Safety

It's a good practice to declare variables used within functions as local, to avoid unintended side effects.

---

I'm a bit puzzled by this statement:

virtualhost-mysql.sh - create db and user, values passed by environment variables to not appear in ps output - for security.

And then in the script there's this line:

mysql --user="${mysql[adminuser]}" --password="${mysql[adminpasswd]}" <<<$mysqlcreate

The admin user and password are exposed!

Effective Bash

I see (...) a lot of sub-shells in the script, far more than really necessary. Although the performance penalty is likely negligible in this script, it's better to not get used to bad habits.

In many places in (if_match ...) || ... you could simply drop the parentheses.

Or where you write (LANG=C; if_match ...) || die "..." I think you could write LANG=C if_match ... || die "...".

For a more complex example, instead of:

(cat >"$siteconf" <<EOF
...
EOF
) || die "..."

You could write:

cat >"$siteconf" <<EOF || die "..."
...
EOF

Here, you could replace the (...) with { ... } and lose nothing, but avoid the creation of a sub-shell:

(
  mkdir -p "${config[webroot]}"
  chown ${config[webmaster]}:${config[webgroup]} "${config[webroot]}"
  chmod u=rwX,g=rXs,o= "${config[webroot]}"
  chown root:root "$siteconf"
  chmod u=rw,g=r,o=r "$siteconf"
  a2ensite "${hostfile}"
  systemctl reload apache2
) || die "Run as root"

---

Instead of echo ... | tr ..., use tr ... <<< "...".

Instead of cmd <<< $var, write cmd <<< "$var".

Instead of let pos=pos+1, I prefer the simpler ((pos++)).

Since the script requires Bash 4, you could take advantage of native lowercasing with ${1,,} instead of tr '[:upper:]' '[:lower:]' <<< "$1".

The repeated definitions of the die function are confusing, especially in virtualhost-yad.sh.

The implementations of the die functions look unnecessary complex. Most of the time it's only used with a single message argument. It would be better to keep it simple, and for the rare cases where you need slightly different behavior, either just write out the different behavior, or define another function.