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.shlog () { 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.kdbxlocal_repository_path=$temp_dir/backupremote_keepassdb_path=$local_repository_path/Passwords.kdbx[email protected]repo_identity_file_path=/home/notfound/.ssh/notfoundToGitlab_id_ed25519email_subject_failure="Failed Password backup"log "Starting Password db backup"clone_remote_repo_or_exitcheck_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"fi3 Answers3
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/nullOr even bygit 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 withcmp? 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. fromcron) 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.
Remote commands
Some SSH servers permit the execution ofremote shell commands. So, rather than clone the repositorylocally, you could execute thesha256sum 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 ahistory 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 youclean up the temp dir when you are done. Thetrap 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.
Crontab can send mail if theMAILTO 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 theexit codes of the commands being invoked, especiallygit push... to make sure that the process indeed succeeded.
After all, the most critical feature for a backup application isreliability. 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 thelast executed command succeeds ielog_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, otherwiseSSH_AUTH_SOCK should not be required.
quotes
log () { echo $1 | ts ...You want"$1" there, and at some other unquoted places.
shellcheck
You didn't use it to lint this code, and you should.
repeated constant
The/home/notfound directory is important to this script,repeated several times.It might change, for example when another team member does some testing.Consider defining a variable for it, or just use${HOME}.
Public API
log_and_mail () { log "$2" ...Give us a hint, please, about the formal parameters -- a comment, or:
log_and_mail () { subj="${1:-email subject}" msg="${2:-email message}" log "${msg}" ...trivial
log_and_mail_and_exit () {Certainly this does what it says.But maybe caller could be responsible for invokingexit?It seems like we have very little value-add here.
boolean OR
ifs are nice enough,but consider taking advantage of the OR operator:
git clone ... || log_... ... test -f "$1" || log_...gratuitous-r
rm -rf "$remote_keepassdb_path"Near as I can tell,Passwords.kdbx is a file rather than a folder.So no need to recurse.
The worry is that after maintenance perhaps the variable doesn't quitehold what you intended, andrm -rf runs amok where it shouldn't.I'm just trying to limit the blast radius.
clarity
You have a bunch of..._path variables,which are a mix of folders and files.Consider using a..._dir suffix on some of them.
suppressing errors and error messages
cd "$local_repository_path" git add . &> /dev/nullSome folks are fond ofset -e to bail on first error,and some folks aren't.We're not using that setting here.
Suppose thecd failed.We can't plausibly continue.Standard idiom would be
cd "$local_repository_path" || exit 1Or verbosely log it or whatever.Just don't go on to thegit add.
Now suppose thecd worked great,but we're not within a repo.It seems a little dicey to suppress stderr here.Better to stick with justgit add . > /dev/null,and similarly forpush andcommit.Or filter "expected" diagnostics, e.g.git add . 2>&1 | egrep -v 'already added' or whatever you anticipate.
BTW, maybe that repo has no branches, and duringdebugging we nevergit checkout SHA1 some hash commit?In the FS wecd foo; in version control wemight want togit checkout main before add, commit, push.
If multiple hosts push, then maybe do apull before thepush?
local diff testing
Consider consultinggit status, and suppressing redundantnetworkpush operations when there's nothing new to push.
You mustlog in to answer this question.
Explore related questions
See similar questions with these tags.


