HiveBrain v1.2.0
Get Started
← Back to all entries
patternbashMinor

Automated owncloud installation script

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
automatedinstallationscriptowncloud

Problem

Here is a shell script which installs and configure owncloud on a Debian server. It also installs apache2 and MySQL as dependencies and fail2ban with a specific rule for owncloud. Apache2 is also configured to use a self signed SSL certificate.

The script seems to work properly (everything is installed and works fine) my question is about the possible security issues that could be created:

  • Is the ssl certificate correctly issued and installed?



  • Is there a potential issue with the handling of mysql?



  • Is there something else that I should see?



The official Github repo of this file is here.

```
#!/bin/bash

#Some bash script optimization for robustness (More information: www.davidpashley.com/articles/writing-robust-shell-scripts/)
#Break if the script uses unset variables
set -o nounset
#Break if a command has a non-true return value
set -o errexit

echo_info () {
echo "--------------------------------------"
echo "| INFO: $1"
echo "--------------------------------------"
}

echo_error () {
echo "--------------------------------------"
echo "| ERROR: $1"
echo "--------------------------------------"
}

is_installed () {
#Check if a package is installed (More information: https://askubuntu.com/questions/319307/reliably-check-if-a-package-is-installed-or-not)
if dpkg --get-selections | grep -q "^$1[[:space:]]*install$" >/dev/null; then
echo_error "$1 is already installed"
exit 1
fi
}

check_not_installed () {

echo_info "Check if some package is already installed. If this is the case the script stops because it could not be performed without risc."

is_installed apache2
is_installed mysql-server-5.5
is_installed owncloud
is_installed fail2ban

}

check_root () {
if [ "$(id -u)" != "0" ]; then
echo_error "The script must be called as root user!"
exit 1
fi
}

install_owncloud () {

cd /tmp

#Automatic installation of owncloud (More information: https://softwar

Solution

Naming

Some of the function names could be improved, for example this one:

is_installed () {
    #Check if a package is installed (More information: https://askubuntu.com/questions/319307/reliably-check-if-a-package-is-installed-or-not)
    if dpkg --get-selections | grep -q "^$1[[:space:]]*install$" >/dev/null; then
        echo_error "$1 is already installed"
        exit 1
    fi
}


I expect a function named is_X to behave like a boolean,
so that I can build conditional statements with it.
But this is not that kind of function,
it validates something or else exits the program.
I'd name this verify_not_installed or similar.

Other minor issues

if dpkg --get-selections | grep -q "^$1[[:space:]]*install$" >/dev/null; then


You don't need to redirect the output of grep -q, the -q flag already takes care of that.

echo_error "$1 is already installed"
exit 1


Every time you echo_error, you also exit 1.
So you could move the exit 1 inside the function,
and then probably rename it (I usually call it fatal).

echo 'deb http://download.owncloud.org/download/repositories/8.2/Debian_8.0/ /' >> /etc/apt/sources.list.d/owncloud.list

wget -nv https://download.owncloud.org/download/repositories/8.2/Debian_8.0/Release.key -O Release.key


The base URL is common in both of these commands,
so I would put it in a variable to make it easy to change later if needed.

mysql=`which mysql`


Backticks are archaic, use $(...) instead.

Q1="CREATE DATABASE IF NOT EXISTS $ocDb;"
Q2="GRANT USAGE ON *.* TO $ocDbUser@localhost IDENTIFIED BY '$ocDbUserPw';"
Q3="GRANT ALL PRIVILEGES ON $ocDb.* TO $ocDbUser@localhost;"
Q4="FLUSH PRIVILEGES;"
SQL="${Q1}${Q2}${Q3}${Q4}"


This is a tedious and fragile way to construct a longer string.
You could pass a here-document directly to mysql:

mysql -uroot -p$mysqlRootPw <<EOF
CREATE DATABASE IF NOT EXISTS $ocDb;
GRANT USAGE ON *.* TO $ocDbUser@localhost IDENTIFIED BY '$ocDbUserPw';
...
EOF


sed -i "s/php_value upload_max_filesize .*/php_value upload_max_filesize $maxFileSize/" /var/www/owncloud/.htaccess
sed -i "s/php_value post_max_size .*/php_value post_max_size $maxFileSize/" /var/www/owncloud/.htaccess
sed -i "s/php_value memory_limit .*/php_value memory_limit $maxFileSize/" /var/www/owncloud/.htaccess


This overwrites the /var/www/owncloud/.htaccess three times.
You can perform multiple replacements in a single sed call,
and overwrite the file only once:

sed -i \
  -e "s/php_value upload_max_filesize .*/php_value upload_max_filesize $maxFileSize/" \
  -e "s/php_value post_max_size .*/php_value post_max_size $maxFileSize/" \
  -e "s/php_value memory_limit .*/php_value memory_limit $maxFileSize/" \
  /var/www/owncloud/.htaccess


echo -e "[Definition]\nfailregex={\"app\":\"core\",\"message\":\"Login failed: user '.*' , wrong password, IP:\",\"level\":2,\"time\":\".*\"}\n          {\"app\":\"core\",\"message\":\"Login failed: '.*' \(Remote IP: '', X-Forwarded-For: '.*'\)\",\"level\":2,\"time\":\".*\"}\n          {\"reqId\":\".*\",\"remoteAddr\":\"\",\"app\":\"core\",\"message\":\"Login failed: .*\",\"level\":2,\"time\":\".*\"}" > /etc/fail2ban/filter.d/owncloud.conf


This extremely long line is unreadable.
Consider using a here-document instead:

cat  /etc/fail2ban/filter.d/owncloud.conf
[Definition]
failregex={"app":"core","message":"Login failed: user '.*' , wrong password, IP:","level":2,"time":".*"}
          {"app":"core","message":"Login failed: '.*' \(Remote IP: '', X-Forwarded-For: '.*'\)","level":2,"time":".*"}
          {"reqId":".*","remoteAddr":"","app":"core","message":"Login failed: .*","level":2,"time":".*"}
EOF

Code Snippets

is_installed () {
    #Check if a package is installed (More information: https://askubuntu.com/questions/319307/reliably-check-if-a-package-is-installed-or-not)
    if dpkg --get-selections | grep -q "^$1[[:space:]]*install$" >/dev/null; then
        echo_error "$1 is already installed"
        exit 1
    fi
}
if dpkg --get-selections | grep -q "^$1[[:space:]]*install$" >/dev/null; then
echo_error "$1 is already installed"
exit 1
echo 'deb http://download.owncloud.org/download/repositories/8.2/Debian_8.0/ /' >> /etc/apt/sources.list.d/owncloud.list

wget -nv https://download.owncloud.org/download/repositories/8.2/Debian_8.0/Release.key -O Release.key
mysql=`which mysql`

Context

StackExchange Code Review Q#117956, answer score: 2

Revisions (0)

No revisions yet.