3
\$\begingroup\$

Here is my second version of a bash program I'm writing, as per @hjpotter92 I've updated and cleaned it since last:

This script is run directly after a fresh install of Debian, to do:

  • sets up LS_COLORS====> colors in /bin/ls output

  • sets up Portsentry (security for ssh and defence for ports)

  • sets up syntax highlighting in nano,

  • sets up iptables,

  • sets up ssh,

  • sets up custom bashrc files

  • creates users on the system if needed,

  • checks if user has a password set and sets it if not,

  • installs non-free firmware and sets up apt with virtualbox deb file and multimedia deb.

This is from my previous post↓↓↓

You have a lot of stub code in the script. Clean it up; remove unused statements/declarations/tasks and post another question with the updated code.

QUESTIONS:

Can you give me pointers on what could be better or different?

Do you have any ideas about new/other features for a setup program for a developer on Debian Stretch(9)?

Here is the code:

#!/bin/bash -x

# redirect all errors to a file
exec 2>debianConfigVersion3.1ERRORS.txt
##################################################################################################### exec 3>cpSuccessCodes.txt ## 

SCRIPTNAME=$(basename "$0")

if [ "$UID" != 0 ]
    then
    echo "This program should be run as root, exiting! now....."
    exit 1
fi

if [ "$#" -eq 0 ]
then
    echo "RUN AS ROOT...Usage if you want to create users:...$SCRIPTNAME USER_1 USER_2 USER_3 etc."
    echo "If you create users they will be set with a semi strong password which you need to change later as root with passwd"
    echo
    echo
    echo "#################### ↓↓↓↓↓↓↓↓↓↓↓ OR ↓↓↓↓↓↓↓↓↓↓ #############################"
    echo
    echo
    echo "RUN AS ROOT...Usage without creating users: $SCRIPTNAME"
    echo
    sleep 10

fi

echo "Here starts the party!"
echo "Setting up server..........please wait!!!!!"
sleep 3

### ↓↓↓↓ NEXT TIME USE "declare VARIABLE" ↓↓↓↓↓↓↓↓↓↓ #####
OAUTH_TOKEN=d6637f7ccf109a0171a2f55d21b6ca43ff053616
CURRENTDIR=/tmp/svaka
BASHRC=.bashrc
NANORC=.nanorc
BASHRCROOT=.bashrcroot
SOURCE=sources.list
SSHD_CONFIG=sshd_config
#-----------------------------------------------------------------------↓↓
export DEBIAN_FRONTEND=noninteractive
#-----------------------------------------------------------------------↑↑

if grep "Port 22" /etc/ssh/sshd_config
then
    echo -n "Please select/provide the port-number for ssh in iptables and sshd_config:"
    read port ### when using the "-p" option then the value is stored in $REPLY
    PORT=$port
fi
############################### make all files writable, executable and readable in the working directory#########
if /bin/chmod -R 777 "$CURRENTDIR"
then
    continue
else
    echo "chmod CURRENTDIR failed"
    sleep 3
    exit 127
fi

################ Creating new users #####################1

checkIfUser()
{
    for name in "$@"
    do
        if /usr/bin/id -u "$name" #>/dev/null 2>&1
        then
            echo "User: $name exists....setting up now!"
        else
            echo "User: $name does not exists....creating now!"
            /usr/sbin/useradd -m -s /bin/bash "$name" #>/dev/null 2>&1
        fi
    done
}

###########################################################################3
################# GET USERS ON THE SYSTEM ###################################

prepare_USERS()
{
    checkIfUser "$@"
    /usr/bin/awk -F: '$3 >= 1000 { print $1 }' /etc/passwd > "$CURRENTDIR"/USERS.txt

    /bin/chmod 777 "$CURRENTDIR"/USERS.txt
    if [[ ! -f "$CURRENTDIR"/USERS.txt && ! -w "$CURRENTDIR"/USERS.txt ]]
    then
        echo "USERS.txt doesn't exist or is not writable..exiting!"
        exit 127
    fi
    for user in "$@"
    do
        echo "$user" >> /tmp/svaka/USERS.txt || { echo "writing to USERS.txt failed"; exit 127; }
    done
}
###########################################################################33
################33 user passwords2
userPass()
{
    if [[ ! -f "$CURRENTDIR"/USERS.txt && ! -w "$CURRENTDIR"/USERS.txt ]]
    then
        echo "USERS.txt doesn't exist or is not writable..exiting!"
        exit 127
    fi
    while read i
    do
        if [ "$i" = root ]
        then
            continue
        fi
        if [[ $(/usr/bin/passwd --status "$i" | /usr/bin/awk '{print $2}') = NP ]] || [[ $(/usr/bin/passwd --status "$i" | /usr/bin/awk '{print $2}') = L ]] 
        then
            echo "$i doesn't have a password."
            echo "Changing password for $i:"
            echo $i:$i"YOURSTRONGPASSWORDHERE12345Áá" | /usr/sbin/chpasswd
            if [ "$?" = 0 ]
                then
                echo "Password for user $i changed successfully"
                sleep 5
            fi
        fi
    done < "$CURRENTDIR"/USERS.txt
}

################################################ setting up iptables ####################3
setUPiptables()
{
    #if ! grep -e '-A INPUT -p tcp --dport 80 -j ACCEPT' /etc/iptables.test.rules
    if [[ `/sbin/iptables-save | grep '^\-' | wc -l` > 0 ]]
    then
        echo "Iptables already set, skipping..........!"
    else
        if [ "$PORT" = "" ]
        then
            echo "Port not set for iptables exiting"
            echo -n "Setting port now, insert portnumber: "
            read port
            PORT=$port
        fi
        if [ ! -f /etc/iptables.test.rules ]
        then
            /usr/bin/touch /etc/iptables.test.rules
        else
            /bin/cat /dev/null > /etc/iptables.test.rules
        fi

        /bin/cat << EOT >> /etc/iptables.test.rules
        *filter

        # Allows all loopback (lo0) traffic and drop all traffic to 127/8 that doesn't use lo0
        -A INPUT -i lo -j ACCEPT
        -A INPUT ! -i lo -d 127.0.0.0/8 -j REJECT

        # Accepts all established inbound connections
        -A INPUT -m state --state ESTABLISHED,RELATED -j ACCEPT

        # Allows all outbound traffic
        # You could modify this to only allow certain traffic
        -A OUTPUT -j ACCEPT

        # Allows HTTP and HTTPS connections from anywhere (the normal ports for websites)
        -A INPUT -p tcp --dport 80 -j ACCEPT
        -A INPUT -p tcp --dport 443 -j ACCEPT

        # Allows SSH connections
        # The --dport number is the same as in /etc/ssh/sshd_config
        -A INPUT -p tcp -m state --state NEW --dport $PORT -j ACCEPT

        # Now you should read up on iptables rules and consider whether ssh access
        # for everyone is really desired. Most likely you will only allow access from certain IPs.

        # Allow ping
        #  note that blocking other types of icmp packets is considered a bad idea by some
        #  remove -m icmp --icmp-type 8 from this line to allow all kinds of icmp:
        #  https://security.stackexchange.com/questions/22711
        -A INPUT -p icmp -m icmp --icmp-type 8 -j ACCEPT

        # log iptables denied calls (access via dmesg command)
        -A INPUT -m limit --limit 5/min -j LOG --log-prefix "iptables denied: " --log-level 7

        # Reject all other inbound - default deny unless explicitly allowed policy:
        -A INPUT -j REJECT
        -A FORWARD -j REJECT

        COMMIT
EOT
        sed "s/^[ \t]*//" -i /etc/iptables.test.rules ## remove tabs and spaces
        /sbin/iptables-restore < /etc/iptables.test.rules || { echo "iptables-restore failed"; exit 127; }
        /sbin/iptables-save > /etc/iptables.up.rules || { echo "iptables-save failed"; exit 127; }
        /usr/bin/printf "#!/bin/bash\n/sbin/iptables-restore < /etc/iptables.up.rules" > /etc/network/if-pre-up.d/iptables ## create a script to run iptables on startup
        /bin/chmod +x /etc/network/if-pre-up.d/iptables || { echo "chmod +x failed"; exit 127; }
    fi
}

###################################################33 sshd_config4
setUPsshd()
{
    if grep "Port $PORT" /etc/ssh/sshd_config
    then
        echo "sshd already set, skipping!"
    else

        if [ "$PORT" = "" ]
        then
            echo "Port not set"
            exit 12
        fi
        users=""
        /bin/cp -f "$CURRENTDIR"/sshd_config /etc/ssh/sshd_config
        sed -i "s/Port 34504/Port $PORT/" /etc/ssh/sshd_config
        for user in `awk -F: '$3 >= 1000 { print $1 }' /etc/passwd`
        do
            users+="${user} "
        done
        if grep "AllowUsers" /etc/ssh/sshd_config
        then
            sed -i "/AllowUsers/c\AllowUsers $users" /etc/ssh/sshd_config
        else
            sed -i "6 a \
            AllowUsers $users" /etc/ssh/sshd_config
        fi

        /bin/chmod 644 /etc/ssh/sshd_config
        /etc/init.d/ssh restart
    fi
}

#################################################3333 Remove or comment out DVD/cd line from sources.list5
editSources()
{
    if grep '^# *deb cdrom:\[Debian' /etc/apt/sources.list
    then
        echo "cd already commented out, skipping!"
    else
        sed -i '/deb cdrom:\[Debian GNU\/Linux/s/^/#/' /etc/apt/sources.list
    fi
}

####################################################33 update system6

updateSystem()
{
    /usr/bin/apt update && /usr/bin/apt upgrade -y
}


###############################################################7
############################# check if programs installed and/or install
checkPrograms()
{
    if [ ! -x /usr/bin/git ] || [ ! -x /usr/bin/wget ] || [ ! -x /usr/bin/curl ] || [ ! -x /usr/bin/gcc ] || [ ! -x /usr/bin/make ]
    then
        echo "Some tools with which to work with data not found installing now......................"
        /usr/bin/apt install -y git wget curl gcc make
    fi
}

#####################################################3 update sources.list8
updateSources()
{
    if grep "deb http://www.deb-multimedia.org" /etc/apt/sources.list
    then
        echo "Sources are setup already, skipping!"
    else
        sudo /bin/cp -f "$CURRENTDIR"/"$SOURCE" /etc/apt/sources.list || { echo "sudo cp failed"; exit 127; }
        /bin/chmod 644 /etc/apt/sources.list
        /usr/bin/wget http://www.deb-multimedia.org/pool/main/d/deb-multimedia-keyring/deb-multimedia-keyring_2016.8.1_all.deb || { echo "wget failed"; exit 127; }
        /usr/bin/dpkg -i deb-multimedia-keyring_2016.8.1_all.deb
        /usr/bin/wget -q https://www.virtualbox.org/download/oracle_vbox_2016.asc -O- | sudo apt-key add -
        updateSystem || { echo "update system failed"; exit 127; }
        /usr/bin/apt install -y vlc vlc-data browser-plugin-vlc mplayer youtube-dl libdvdcss2 libdvdnav4 libdvdread4 smplayer mencoder build-essential
        sleep 3
        updateSystem || { echo "update system failed"; exit 127; }
        sleep 3
    fi
}

###############################################33  SETUP PORTSENTRY ############################################################
##############################################3                     ############################################################33

setup_portsentry()
{
    if  ! grep -q '^TCP_PORTS="1,7,9,11,15,70,79' /etc/portsentry/portsentry.conf || [[ ! -f /etc/portsentry/portsentry.conf ]]
    then
        /usr/bin/apt install -y portsentry logcheck
        /bin/cp -f "$CURRENTDIR"/portsentry.conf /etc/portsentry/portsentry.conf || { echo "cp portsentry failed"; exit 127; }
        /usr/sbin/service portsentry restart || { echo "service portsentry restart failed"; exit 127; }
    fi
}

###############################################################################################################################33
#####################################################3 run methods here↓   ###################################################3
#####################################################                      ###################################################
prepare_USERS "$@"
userPass "$@"
setUPiptables
setUPsshd
editSources
updateSystem
setup_portsentry
checkPrograms
updateSources
###########################################################################################################            #####3##
##############################################################################################################3Methods
##########################################3 Disable login www #########
passwd -l www-data
#################################### firmware
apt install -y firmware-linux-nonfree firmware-linux
sleep 5
################ NANO SYNTAX-HIGHLIGHTING #####################3
if [ ! -d "$CURRENTDIR"/nanorc  ]
then
    if [ "$UID" != 0 ]
    then
        echo "This program should be run as root, goodbye!"
        exit 127

    else
        echo "Doing user: $USER....please, wait\!"
        /usr/bin/git clone https://$OAUTH_TOKEN:x-auth-basic@github.com/gnihtemoSgnihtemos/nanorc || { echo "git failed"; exit 127; }
        cd "$CURRENTDIR"/nanorc || { echo "cd failed"; exit 127; }
        /usr/bin/make install-global || { echo "make failed"; exit 127; }
        /bin/cp -f "$CURRENTDIR/$NANORC" /etc/nanorc >&3 || { echo "cp failed"; exit 127; }
        /bin/chown root:root /etc/nanorc || { echo "chown failed"; exit 127; }
        /bin/chmod 644 /etc/nanorc || { echo "chmod failed"; exit 127; }
        if [ "$?" = 0 ]
        then
            echo "Implementing a custom nanorc file succeeded!"
        else
            echo "Nano setup DID NOT SUCCEED\!"
            exit 127
        fi
        echo "Finished setting up nano!"
    fi
fi

################ LS_COLORS SETTINGS #############################
if ! grep 'eval $(dircolors -b $HOME/.dircolors)' /root/.bashrc
then
    echo "Setting root bashrc file....please wait!!!!"
    if /bin/cp -f "$CURRENTDIR/$BASHRCROOT" "$HOME"/.bashrc
    then
        echo "Root bashrc copy succeeded!"
    else
        echo "Root bashrc cp failed, exiting now!"
        exit 127
    fi
    /bin/chown root:root "$HOME/.bashrc" || { echo "chown failed"; exit 127; }
    /bin/chmod 644 "$HOME/.bashrc" || { echo "failed to chmod"; exit 127; }
    /usr/bin/wget https://raw.github.com/trapd00r/LS_COLORS/master/LS_COLORS -O "$HOME"/.dircolors || { echo "wget failed"; exit 127; }
    echo 'eval $(dircolors -b $HOME/.dircolors)' >> "$HOME"/.bashrc
fi
while read user
do
    if [ "$user" = root ]
    then
        continue
    fi

    sudo -i -u "$user" user="$user" CURRENTDIR="$CURRENTDIR" BASHRC="$BASHRC" bash <<'EOF'
    if grep 'eval $(dircolors -b $HOME/.dircolors)' "$HOME"/.bashrc
    then
        :
    else
        echo "Setting users=Bashrc files!"
        if /bin/cp -f "$CURRENTDIR"/"$BASHRC" "$HOME/.bashrc"
        then
            echo "Copy for $user (bashrc) succeeded!"
            sleep 3
        else
            echo "Couldn't cp .bashrc for user $user"
            exit 127
        fi
        /bin/chown $user:$user "$HOME/.bashrc" || { echo "chown failed"; exit 127; }
        /bin/chmod 644 "$HOME/.bashrc" || { echo "chmod failed"; exit 127; }
        /usr/bin/wget https://raw.github.com/trapd00r/LS_COLORS/master/LS_COLORS -O "$HOME"/.dircolors || { echo "wget failed"; exit 127; }
        echo 'eval $(dircolors -b $HOME/.dircolors)' >> "$HOME"/.bashrc
    fi
EOF
done < "$CURRENTDIR"/USERS.txt

echo "Finished setting up your system!"
cd ~/ || { echo "cd ~/ failed"; exit 155; }
rm -rf /tmp/svaka || { echo "Failed to remove the install directory!!!!!!!!"; exit 155; }

I've checked the program into https://www.shellcheck.net/

\$\endgroup\$
2
\$\begingroup\$

It's good you're taking advices and improving your script. There's still much work to do, so keep it up!

Never do chmod 777

I haven't seen a single valid use case of permission 777 in recent memory. And there's no valid use case of it in this script. Don't take permission values lightly. Use precisely the permission bits you really need.

Don't specify absolute path of common commands

Unless you have a good specific reason to do otherwise, don't specify the absolute path of commands such as /usr/bin/awk and /bin/chmod. Let the shell find awk and chmod in PATH.

Using absolute paths reduces the portability and usability of scripts. This script will only work if I have those binaries at those exact locations.

Don't litter the filesystem with log files

Because of this:

# redirect all errors to a file
exec 2>debianConfigVersion3.1ERRORS.txt

A log file will be created in the current working directory of the user calling the script. If you call this script from many different working directories, all of those directories will have such file. This is not good behavior from scripts.

If you want to collect logs from the script, it would be better to use a dedicated directory that is independent from the current working directory of the calling user.

Also, the comment is a bit misleading. It redirects stderr, which is not necessary errors. In this example, the output due to the -x flag goes there, but I wouldn't call that "errors".

Avoid unnecessary sleep

echo "Setting up server..........please wait!!!!!"
sleep 3

After the echo here, a user may assume that the script is busy working. But it's not, it's just sleeping! I don't see the point of this sleep.

In fact all sleep commands in this script look unnecessary and rather annoying, from a user's point of view.

Unclear logic

The motivation of this code is not clear to me:

if grep "Port 22" /etc/ssh/sshd_config
then
    echo -n "Please select/provide the port-number for ssh in iptables and sshd_config:"
    read port ### when using the "-p" option then the value is stored in $REPLY
    PORT=$port
fi

Why ask the user for a port number if /etc/ssh/sshd_config contains "Port 22"?

What is even the importance of /etc/ssh/sshd_config containing "Port 22"? A line such as # Port 2222 would match, and then what? Why should that affect the decisions made by the script?

The global PORT variable is used and modified in multiple places in the program, and it's hard to follow what happens to it.

I have a couple of tips to clean this up:

  • Since we're talking specifically about SSHD port, call the variable sshd_port, not just PORT. (And avoid uppercase variable names, which are intended for system variables only, such as PATH.)
  • Do you really need to support non-default SSHD port? Probably not. In that case, just set sshd_port=22 at the top of the script, and do not ask the user to enter it. Nice and simple.

Just for the record, the original code would have been better written like this:

if grep -q "Port 22" /etc/ssh/sshd_config
then
    read -p "Please select/provide the port-number for ssh in iptables and sshd_config:" PORT
fi

The improvements:

  • Adding the -q flag for grep make the search terminate immediately when a match is found, and it also suppresses unnecessary output
  • It's possible to read directly into the variable PORT

But as I said earlier, this is not the most important problem here. If you simplify your logic, I think this piece of code can completely disappear.

Overengineering

I jumped into reviewing the script from top to bottom. Now I see that was a mistake, it would have been better to get an overall view first. Because the biggest problem is not all the above stuff, but that the script is overengineered: it contains a lot of stuff that's probably unnecessary.

So my first and foremost suggestion is to start by trimming it down:

  • Drop features you don't really need

  • Simplify as much as possible

    • Is there a good reason to support non-default SSHD port?
    • In setUPsshd, why append AllowUsers after line 6? Why not simply the end of the file?
    • Why use absolute paths instead of simple command names?
    • Why do if [[ `/sbin/iptables-save | grep '^\-' | wc -l` > 0 ]] when you already know better techniques exist: if grep -q '^-' /sbin/iptables-save
    • ... The above are just examples. Question everything that looks complicated.

Next, review the function and variable names. For example:

  • the name checkIfUser doesn't describe well what it does. (The comment "Creating new users" does -> that should have been the function name.)
  • the name CURRENTDIR is really poor. You probably meant workdir.
  • the name i is very poor to represent usernames
  • ... and so on, I suggest to review all

Next:

  • Avoid code duplication. Extract common logic to functions. Each function with a single purpose, and with a good name that describes that purpose.
  • Use consistent techniques: for example you used if grep ... in most places, but sometimes you used a different, far worse technique. Use the better technique, consistently everywhere.

Finally, there are some obvious quality issues such as sometimes using "$CURRENTDIR"/USERS.txt and other times using /tmp/svaka/USERS.txt.

Bugs

In setUPsshd this copies from "$CURRENTDIR"/sshd_config:

/bin/cp -f "$CURRENTDIR"/sshd_config /etc/ssh/sshd_config

But I don't see evidence that such file exists. CURRENTDIR is set to /tmp/svaka at the beginning of the script, and this directory is deleted at the end after every run, so in all likelihood, the referenced source file probably doesn't exist.


In prepare_USERS, you extract usernames into the "$CURRENTDIR"/USERS.txt, and then append some more usernames to it. This appending step will probably append usernames that are already there, resulting in duplicates.

\$\endgroup\$
  • 1
    \$\begingroup\$ Wow detailed answer.thanks, the file a the end what you call bugs is in the working dir and comes with the script as all the other files it copies but I have a lot of work in head of me \$\endgroup\$ – somethingSomething Oct 19 '18 at 11:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.