throwcatch: Super easy, SFTP-based backup tool

Question

I've been working on a project for fun, it has reached a place of pretty decent stability and I would like to hear any and all criticisms you can find.

The project basically enables admins to create users on a hosting server that is then set up to receive files from external sources. These external sources could represent any number of servers needing to backup any number of files.

The most controversial aspect of the script(s) is that all authentication is done based on username and IP alone to verify users. On the other hand, users are only allowed to upload files, and only to their designated home folders. - And since the data is transfered using SFTP, the connection is encrypted.

Repo can be found here: https://github.com/superDuperCyberTechno/throwcatch

Anyway, this is the code I'm most worried about:

#!/bin/bash

function valid_ip()
{
    local  ip=$1
    local  stat=1

    if [[ $ip =~ ^[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}$ ]]; then
        OIFS=$IFS
        IFS='.'
        ip=($ip)
        IFS=$OIFS
        [[ ${ip[0]} -le 255 && ${ip[1]} -le 255 \
            && ${ip[2]} -le 255 && ${ip[3]} -le 255 ]]
        stat=$?
    fi
    return $stat
}

if [ "$1" = "--remove" ];then
    #create the catchers array to keep track of the users
    catchers=()
    #create an index var
    index=0

    #read the sshd_config file for entries
    while IFS= read -r line
    do
        ((index++))
        catchers[$index]=${line:16}
        echo "$index. ${line:16}"
    done < <(grep "#throwcatchuser" /etc/ssh/sshd_config)

    if [[ $index -eq 0 ]];then
        echo "No users to remove."
        exit 1
    fi

    while [[ ! $catcher ]] || [[ $catcher -lt 1 ]] || [[ $catcher -gt $index ]];do
        read -p 'Choose a catcher from the list to delete: ' catcher
    done

    #fetch the username
    username="${catchers[$catcher]%%@*}"
    #delete tghe user from /etc/passwd
    userdel $username
    #remove the users folder
    rm -rf /home/$username

    #remove the entry in the sshd_config file
    sed -i "/\#throwcatchuser ${catchers[$catcher]}/,/\#\/throwcatchuser ${catchers[$catcher]}/d" /etc/ssh/sshd_config

    #restart sshd
    service sshd restart

    echo "${catchers[$catcher]} removed..."

    exit 1
fi

# this is effectively how to tell if this is the first run
if ! grep -q '^ssh$' /etc/securetty;then
    printf "\nssh" >> /etc/securetty
    cp /etc/ssh/sshd_config /etc/ssh/sshd_config_ORIG
fi

#prompt for thrower username
read -p $'What\'s the name of your new thrower?\n' name
while [[ ! $name || $(id -u $name > /dev/null 2>&1; echo $?) -eq 0 ]];do
    read -p $'Please choose a valid name...\n' name
done

#prompt for thrower ip
read -p $'What\'s the IP address of your new thrower?\n' ip
while ! valid_ip $ip;do
    read -p $'Please provide a valid IP address...\n' ip
done

#check if the user already exists, if it does, kill the process
exists=0
while IFS= read -r line
do
    ((exists++))
done < <(grep "#throwcatchuser $name@$ip" /etc/ssh/sshd_config)

if [[ $exists -gt 0 ]];then
    echo "$name@$ip already exists"
    exit 1
fi

#add the user
useradd $name
#set the passwd to nothing
passwd -d $name
#make the home folder
mkdir /home/$name
#make the catches folder
mkdir /home/$name/catches
#set up the rights for the catches folder
chown $name:$name /home/$name/catches
chmod 700 /home/$name/catches

echo "" >> /etc/ssh/sshd_config
echo "#throwcatchuser $name@$ip" >> /etc/ssh/sshd_config
echo "#IF YOU DELETE THE ABOVE LINE, catch WON'T BE ABLE TO REMOVE THE USER FROM THIS CONFIG FILE" >> /etc/ssh/sshd_config
echo "Match User $name Address $ip" >> /etc/ssh/sshd_config
echo "PasswordAuthentication yes" >> /etc/ssh/sshd_config
echo "PermitEmptyPasswords yes" >> /etc/ssh/sshd_config
echo "PubkeyAuthentication no" >> /etc/ssh/sshd_config
echo "ChrootDirectory /home/$name" >> /etc/ssh/sshd_config
echo "ForceCommand internal-sftp -p open,close,write,stat,realpath,fsetstat" >> /etc/ssh/sshd_config
echo "PermitTunnel no" >> /etc/ssh/sshd_config
echo "AllowAgentForwarding no" >> /etc/ssh/sshd_config
echo "AllowTcpForwarding no" >> /etc/ssh/sshd_config
echo "x11Forwarding no" >> /etc/ssh/sshd_config
echo "#IF YOU DELETE THE BELOW LINE, catch WON'T BE ABLE TO REMOVE THE USER FROM THIS CONFIG FILE" >> /etc/ssh/sshd_config
echo "#/throwcatchuser $name@$ip" >> /etc/ssh/sshd_config

service sshd restart

Answer 1

Exit code

When the --remove parameter is present, the script always exits with 1, regardless of successful removal or failure. This seems inappropriate, because non-zero exit code should be used to signal an error.

Function declaration

The recommended POSIX-compliant function declaration style is this:

fun() {

Use exit codes directly

The stat variable in the valid_ip function is unnecessary. You could use the exit code directly, like this:

valid_ip() {
    local  ip=$1

    if ...; then
        # ...
        [[ ${ip[0]} -le 255 && ${ip[1]} -le 255 \
            && ${ip[2]} -le 255 && ${ip[3]} -le 255 ]]
    else
        return 1
    fi
}

An even better example is here:

while [[ ! $name || $(id -u $name > /dev/null 2>&1; echo $?) -eq 0 ]];do

This could be written as:

while [[ ! $name ]] || id -u "$name" &> /dev/null; do

I also simplified the redirection, and double-quoted the $name when used as a program argument.

Setting variable for one command only

Instead of this:

OIFS=$IFS
IFS='.'
ip=($ip)
IFS=$OIFS

You can set a variable for one command only by prefixing it, like this:

IFS=. ip=($ip)

Avoid duplication

I find this condition tedious to write due to the duplicated logic:

[[ ${ip[0]} -le 255 && ${ip[1]} -le 255 \
    && ${ip[2]} -le 255 && ${ip[3]} -le 255 ]]

I would prefer to eliminate the duplication using a loop:

for v in "${ip[@]}"; do
    [[ $v -le 255 ]] || return 1
done

Appending multiple lines to a file

Instead of this:

echo "" >> /etc/ssh/sshd_config
...
echo "#/throwcatchuser $name@$ip" >> /etc/ssh/sshd_config

It would be simpler to use a here document:

cat << EOF >> /etc/ssh/sshd_config

#IF YOU DELETE THE ABOVE LINE, catch WON'T BE ABLE TO REMOVE THE USER FROM THIS CONFIG FILE
Match User $name Address $ip
...
AllowAgentForwarding no
AllowTcpForwarding no
x11Forwarding no
#IF YOU DELETE THE BELOW LINE, catch WON'T BE ABLE TO REMOVE THE USER FROM THIS CONFIG FILE
#/throwcatchuser $name@$ip
EOF