Script to check for failed logins and then trawl users' .bash_history for keywords

Question

I have created a script to check for failed logins and then trawl User .bash_history for keywords. There is also processing on the .bash_history files to convert epoch to human-readable timestamps. I've run my script through shellcheck.net and the only real thing that it's complaining about are my For loops. It's saying they should be while loops instead, and I'm struggling to convert them.

Here's the script in its entirety:

#! /bin/bash
#
printf "\n"
echo -e "\e[93m*** Failed Login Checker ***\e[0m"
sleep 2
grep -E 'Failed password|invalid' /var/log/secure* | grep "sshd\[" | uniq
read -r -p "Press [Enter] key to continue..."
printf "\n"
echo -e "\e[93m*** BASH History Checker - script initiated ***\e[0m"
sleep 2
HOLDINGDIR=/root/check_history
if [ ! -d $HOLDINGDIR ];
then
 mkdir $HOLDINGDIR
fi
for i in $( find /home -name .bash_history -type f );
do
 cp "$i" $HOLDINGDIR/$( ls -la "$i" | awk '{print3ドル}' )_history
done
echo -e "\e[93mAll BASH History files copied - now processing date format...\e[0m"
for k in $( find $HOLDINGDIR -name '*_history' -type f );
do
 echo "Processing file: " "$k"
 sed -i -E 's/^#([0-9]+).*$/date -d @1円/e' "$k"
 chmod 755 "$k"
done
printf "\n"
echo -e "\e[93mAll BASH History files timestamps converted\e[0m"
printf "\n"
read -r -e -p "Enter search term: " -i "sudo su" search
echo "You typed: " "$search"
sleep 3
printf "\n"
for m in $( find $HOLDINGDIR -name '*_history' -type f );
do
 printf "\n"
 FILENAME=$m
 echo "-----------------------------------------------------------"
 echo "History file for: " "$FILENAME" | awk -F/ '{print4ドル}'
 echo "-----------------------------------------------------------"
 grep -F -B1 -i "$search" "$m"
 read -r -p "Press [Enter] key for next file..."
done

• Grajdeanu Alex: I've never heard someone complaining about using for loops instead of while loops (except some edge cases) :) Vice-versa, yea !
• Toby Speight: @Dex'ter: The advantage of while/read is that you don't have to wait for the producer process to complete before your loop can start, and you don't have the memory overhead of storing all the results in the shell. In other words, the while loop is much more pipelinable.

Answer 1

Don't embed terminal escapes in your strings

You don't know what terminal will be displaying your output - or even whether output will be going to a terminal. The standard tool to get the appropriate strings (if they exist) is tput. You'll want something like:

sgr0="$(tput sgr0)"
sgr93="$(tput sgr 93)"

which you can then use in your strings

echo -e isn't portable

Since you're using printf, you can conveniently use that when you don't want a newline:

printf "\n%s" "$sgr93*** Failed Login Checker ***$sgr0"

Pointless sleeping

sleep 2
read -r -p "Press [Enter] key to continue..."

These server only two purposes - annoy the user, and make it harder to use in a pipeline.

Unquoted variable expansion

Although the value you've given $HOLDINGDIR is currently safe to expand without quotes, I recommend you quote its expansion anyway, to give you the freedom to change it later (for example, you might want its value to incorporate $HOME instead of hard-coding /root).

No check that mkdir succeeded

If $HOLDINGDIR already exists as a regular file, or its parent directory doesn't exist or can't be written, then it won't be created, and we don't check that it succeeded. The easy way to deal with this is to make any failed command abort the script:

set -x

Unbounded find

Do you really mean to search recursively for .bash_history files? It probably makes more sense to look in each user's home directory only. And not all home directories are necessarily under /home. I'd be more inclined to iterate over awk -F: '{ print 6ドル "/.bash_history" }' /etc/passwd instead:

awk -F: '{ print 1,ドル 6ドル "/.bash_history" }' /etc/passwd \
 | while read u f
 do if test -e "$f"
 then cp -v "$f" "$HOLDINGDIR/${u}_history"
 fi
 done

Replace copy+edit with a pipeline

You copy files and then process them with sed. It's much better to simply have sed take its input from the original file and send its output to the new file, so instead of the cp above we can just:

sed -e 's/^#([0-9]+).*$/date -d @1円/e' "$f" >"$HOLDINGDIR/${u}_history"

and remove the following for loop.

And drop the chmod 755: you don't really want to make the transformed history files executable and readable by everybody.

Interactive reading of search term

read -r -e -p "Enter search term: " -i "sudo su" search

Again, this makes it hard to run from anything other than a terminal. And if you mistype it, you have to start right from the beginning, seeking out all the history files again. Instead, you could provide the terms as positional arguments, then you can

for search in "$@"
do grep -FiH -B1 "$search" "$HOLDINGDIR"/*_history
done

Even better, you could search for all the search terms together, remembering that grep -F accepts a newline-separated list of search terms:

grep -FiH -B1 "$(printf '%s\n' "$@")" "$HOLDINGDIR"/*_history

Modified program

#!/bin/bash
set -e
if [ "$#" = 0 ]
then
 echo "No search terms specified!" >&2
 exit 1
fi
HOLDINGDIR=/root/check_history
test -d "$HOLDINGDIR" || mkdir "$HOLDINGDIR"
awk -F: '{ print 1,ドル 6ドル "/.bash_history" }' /etc/passwd \
 | while read u f
 do if test -e "$f"
 then sed -e 's/^#([0-9]+).*$/date -d @1円 "+#%c"/e' "$f" >"$HOLDINGDIR/${u}_history"
 fi
 done
grep -FiH -B1 "$(printf '%s\n' "$@")" "$HOLDINGDIR"/*_history

• 200_success: echo is a Bash builtin, so the behaviour of echo -e in a #!/bin/bash script is predictable.
• Toby Speight: @200, that's true. But it does increase the barrier to porting, so I still claim it's worth avoiding.
• machinist: Thanks to everyone who replied, especially @TobySpeight :) P.S. the sleeps and key press waits are in there so that the user has a more steady, controlled output coming at them; no other reason. P.P.S. I couldn't trawl /etc/passwd for Home directories as all my users login via Active Directory, i.e. no entries in /etc/passwd