3
\$\begingroup\$

Can you please comment on the script below? It backs up my local password database to a remote repository if a change is detected. It works as intended. I'd like some comments in terms of syntax, security, portability, readability, etc.

#!/bin/bash

# Compares local and remote copies of the keepass db. If there are any diff, the local replaces remote, as local is the
# master.
# KeepassXC tends to make some meta-data changes (DB preferences, last opened group...)
# which will be picked up by this script. Therefore, a sync might happen even if no entry has been
# added/modified/deleted
#
# This script is run periodically by cron (crontab -l to view the schedule). Below shows it runs Mondays at 10am
# 0 10 * * * /home/notfound/bin/backupKeepassdb.sh

# It requires:
# - bash as shell (bash initialises $HOSTANME)
# - ts from moreutils package for timestamps in the logs
#
# It should be placed in the bin directory of the user so that it automatically appears in $PATH
#
# Usage:
#    backupKeepassDB.sh

log () {
   echo $1 | ts '[%F %H:%M:%.S]' >> /home/notfound/Logs/backupkeepassdb.log
}

log_and_mail () {
   log "$2"
   echo "$2" | mailx -s "$HOSTNAME - $(basename "$0") - $1" $notification_recipient
}

log_and_mail_and_exit () {
   log_and_mail "$1" "$2"
   exit
}

clone_remote_repo_or_exit () {
   cd $temp_dir

   export GIT_SSH_COMMAND="SSH_AUTH_SOCK='/run/user/1000/keyring/ssh' ssh -i $repo_identity_file_path -o IdentitiesOnly=yes -F /dev/null"
   git clone [email protected]:notfound/notfound.git &> /dev/null
   if [ "$?" != 0 ]; then
       log_and_mail_and_exit "$email_subject_failure" "Failed to clone remote repository"
   fi
}

check_db_is_readable_or_exit () {
   if [ ! -f "$1" ]; then
       log_and_mail_and_exit "$email_subject_failure" "$1 not found or not readable"
   fi
}

push_to_remote () {
   rm -rf "$remote_keepassdb_path"
   cp "$local_keepassdb_path" "$local_repository_path"
   cd "$local_repository_path"
   git add . &> /dev/null
   git commit -m "Update from $HOSTNAME" &> /dev/null
   git push origin main &> /dev/null
}

temp_dir=`mktemp -d`
local_keepassdb_path=/home/notfound/Documents/Secret/Passwords/KeepassXC/Passwords.kdbx
local_repository_path=$temp_dir/backup
remote_keepassdb_path=$local_repository_path/Passwords.kdbx
[email protected]
repo_identity_file_path=/home/notfound/.ssh/notfoundToGitlab_id_ed25519
email_subject_failure="Failed Password backup"

log "Starting Password db backup"
clone_remote_repo_or_exit
check_db_is_readable_or_exit "$local_keepassdb_path"
check_db_is_readable_or_exit "$remote_keepassdb_path"
remote_db_hash=($(sha256sum $remote_keepassdb_path))
local_db_hash=($(sha256sum $local_keepassdb_path))

if [ "$remote_db_hash" != "$local_db_hash" ]; then
    (push_to_remote &&
      log_and_mail "Successfully Updated Remote Keepass DB" "Local Keepass DB different from Remote. Remote has been updated.") ||
      log_and_mail_and_exit "$email_subject_failure" "Failed to push remote repository"
else
    log "Local Keepass DB and Remote Keepass DB are identical. No update needed"
fi
New contributor
Sandrew Cheru is a new contributor to this site. Take care in asking for clarification, commenting, and answering. Check out our Code of Conduct.
\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

Testing $? is a common antipattern, seen here:

   git clone [email protected]:notfound/notfound.git &> /dev/null
   if [ "$?" != 0 ]

That can be replaced by

if ! git clone [email protected]:notfound/notfound.git &> /dev/null

Or even by git clone … ||


There's a severe shortage of error checking. For example:

   cd "$local_repository_path"
   git add . &> /dev/null
   git commit -m "Update from $HOSTNAME" &> /dev/null

If any of these commands fail, does it really make sense to execute the remainder? Consider joining with && and/or adding || exit to specific commands.


Instead of comparing SHA-256 hashes, why not simply compare content with cmp? That's more efficient, and completely eliminates the (admittedly tiny) chance of hash collisions.


Instead of sending errors by email, why not write them to standard error stream? We can then use this script (e.g. from cron) in the usual way and let those environments choose how to deal with the error stream. That way, we do one thing well, rather than adding multiple responsibilities to the code.


I have no comment on the advisability of copying sensitive data to hosts you don't control. But it's not something I would do.

\$\endgroup\$
1
\$\begingroup\$

Remote commands

Some SSH servers permit the execution of remote shell commands. So, rather than clone the repository locally, you could execute the sha256sum command remotely, provided that the server allows it. This would be a better approach when dealing with somewhat large files.

Keepass files are normally very small, so there is no penalty for doing more backups than necessary.

History

Also, it should be noted that Keepass (or at least KeepassXC, the fork that I use) can keep a history of password changes, and also has a recycle bin for deleted entries. So I find limited value in a git workflow.

Security

Using Gitlab to back up sensitive data is worrying. In theory, the file should be useless in the wrong hands, as long as it is protected by strong password, but it is still highly sensitive.

Personally, I use Rclone for backups, with an appropriate hosting plan. Because there are plenty of other files that should be backed up too.

I suggest that you clean up the temp dir when you are done. The trap command can be used for this purpose. On Linux systems, the files present in the /tmp directory are normally accessible to other users, maybe in read-only mode at least depending on the permissions. So I find it more appropriate to use your home directory as a base directory.

Mail

Crontab can send mail if the MAILTO variable is set, see: https://www.cyberciti.biz/faq/linux-unix-crontab-change-mailto-settings/

shellcheck

I have run shellcheck on the script, there is a lot of useful comments. The first is:

^-- SC1128 (error): The shebang must be on the first line. Delete blanks and move comments.

I would encourage you to use shellcheck to review and improve your scripts, and learn a few things.

Error handling

You should check the exit codes of the commands being invoked, especially git push... to make sure that the process indeed succeeded.

After all, the most critical feature for a backup application is reliability. There could be network issues when the script runs, or other unexpected issues not handled by your script.

Regardless of what your script reports, there is no proof that the backup even took place. The /dev/null redirection may discard useful output, which is essential for troubleshooting unattended scripts.

Ironically, you do some checks when cloning the repo, but the push is more critical.

In any event, your script will probably always return an exit code of 0, because the last executed command succeeds ie log_and_mail_and_exit. So it does not reliably provide us with an exit code we could check at the end of the process.

SSH authentication

I am not the expert here, but I assume that your private key may be protected by a passphrase, otherwise SSH_AUTH_SOCK should not be required.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.