3
\$\begingroup\$

In order to make cert creation with easy-rsa as practical for others, this script was created and it works so far. The questions are:

  1. where can it be improved?
  2. are there flaws that are not obvious to the author?

The commentary is a bit extensive, but whomever will come across this in the future should be able to understand what's being done no matter how advanced.

#!/bin/bash
##
## This script automates the complete process of creating new easy-rsa  client certificates.
## It includes:     1. Mounting a block device.
##          2. Generating certificates using individual timestamps.
##          3. Copying the created certs to the mounted block device.
##
##
#
# First we look for the usb device at sdc1 and mount it. If not present tell the pebcac the it should insert one.


# all the predifined variables will end up beneath

myself=auto_rsa_gen.sh # tells the script who it is

export easy_rsa=../rsa_testing # tells the script where it is and exports this information for use in child processes


now=$(date +"%F")


##
# Show a list of available drives and ask the user for the dev he wants to mount
echo "These \n$(ls /dev | grep sd) \nare the drives available. Please tell me which one you want to mount \n(hint: its usually the one having just the  one additional entry. Like sdc1):"

# read user input (blkdev is the variable that will hold the user inuput for further processing)
read blkdev

##
# I. Mount
# check if de given device exists
if [ -b /dev/"$blkdev" ]
        then # if it exists mount it to /media
            sudo mount /dev/$blkdev /media
            echo "\nDevice $blkdev has been mounted!"
        else # if it doesn't exist
            echo "Device doesn't appear to exist! Restarting"
            sh "$myself" # restart the script
        fi

# Ask for an identifier to be used in the wrapper for easy-rsa cert creation
echo "\nPlease provide an identifier for the certs that will be created \n(The identifier will be appended to the current date of the machine):"

# read user input (ident is the variable that will hold the user input for further processing)
read ident

# Ask for ne number of certs to be created
echo "\nHow many certificates do you want me to create?"
# read user input (cnum is the variable that will hold the user input for further processing)
read cnum

##
# II. Wrapper for easy-rsa cert creation
#

# this is a while loop in case there was no input for ident. It only stop when the user hast put in something other than nothing
while [ -z $ident ]
    do
    echo "\nYou did not supply an identifier. Please do so or i will terminate."
    read ident # read user input again
done

# change directory to the easy-rsa directory
cd $easy_rsa

# tell user what you are going to do
echo "\nCreating $cnum certificates with ${now}_$ident."

# load vars file
. ./vars

##
# II.a Generation
# for loop that creates $cnum certificates and appends an incrementing number to the end of every
# file name

for i in $(seq -f %03g $cnum)
    do
        ${easy_rsa}/./build-batch ${now}_${ident}_$i

done

##
# II.b Copying
# for loop that searches for all certs created with this run and copies them to /media where $blkdev is mounted
find ${easy_rsa}/keys/ -name ${now}_${ident}*.crt -o -name ${now}_${ident}*.key > /media/${now}_${ident}
for f in $(find ${easy_rsa}/keys/ -name ${now}_${ident}*.crt -o -name ${now}_${ident}*.key )
    do
        cp $f /media

done
echo "\nCopied \n$(find ${easy_rsa}/keys/ -name ${now}_${ident}*.crt -o -name ${now}_${ident}*.key) \nto /media"

# unmounting $blkdev at /media
umount /media
echo "\nUmounted $blkdev at /media. Exiting! Bye!"ere
\$\endgroup\$
1
\$\begingroup\$

This

myself=auto_rsa_gen.sh # tells the script who it is

is better done like this:

myself="$0"

or, if you want just filename, then

myself="$(basename "$0")"

because it will give you the actual name of the script, instead of being hard-coded. This also opens a nice renaming possibilities if you use symlinks.

This part:

echo "These \n$(ls /dev | grep sd) \nare the drives available. Please tell me which one you want to mount \n(hint: its usually the one having just the  one additional entry. Like sdc1):"

# read user input (blkdev is the variable that will hold the user inuput for further processing)
read blkdev

##
# I. Mount
# check if de given device exists
if [ -b /dev/"$blkdev" ]
        then # if it exists mount it to /media
            sudo mount /dev/$blkdev /media
            echo "\nDevice $blkdev has been mounted!"
        else # if it doesn't exist
            echo "Device doesn't appear to exist! Restarting"
            sh "$myself" # restart the script
        fi

I think is better put in a while loop, instead of restarting a script, like this:

while true; do
  echo "Pick device, blah, blah,... Press Ctrl+C to abort."
  read blkdev
  if [ ! -b /dev/"$blkdev" ]; then
    echo "The device doesn't appear to exist!"
    continue;
  fi
  sudo mount /dev/"$blkdev" /media
  echo "\nDevice '$blkdev' has been mounted!"
done

You might also want to check if the mount command succeeded.

Also, /media is a standard directory for dynamic mounts. That means that your script will hide mountpoints of mounted CDs, DVDs, USB keys,... while it is running, which may be very inconvenient in some situations (for example, if you have some copying done in the background).

Btw, echo 'Line 1\nLine 2' outputs Line 1\nLine 2 instead of two lines (GNU bash, version 4.2.45, Fedora 19). The correct way for this is

echo -e 'Line 1\nLine 2'

I assume you have -e aliased, or on your distribution it is on by default, or something like that. However, it is not portable, so I suggest you add -e where needed.

I also have a bit of a problem with

cd $easy_rsa

These are my concerns:

  1. It has a relative path to the subdirectory. This will hardly produce satisfactory results if your script is invoked from a directory other than you intended it to be invoked from. Maybe this value should either be prompted (with read), or picked up from the command line?

  2. You're not checking that the directory exists. You could force the directory creation (mkdir -p "$easy_rsa"), and - on top of that - check that $0 is true (i.e., equal to 0) afterwards.

In the last line

echo "\nUmounted $blkdev at /media. Exiting! Bye!"ere

This ere in the end seems to be here by an error.

I also have one general remark. Instead of writing echo $something, cd $dir, and similar commands, I suggest you use echo "$something", cd "$dir", etc. There is no point in thinking if one of these may contain space and, when editing your script later on, thinking about fixing this if you do include a space. It's much easier and safer to always behave as if a variable can contain a space and just include the quotation marks.

Also, without the quotation marks, you're in trouble with all the variables that get their values from the user. The example for this is your $blkdev variable which you sometimes enclose in the quotation marks, but sometimes you do not (I added them in the mount command in my loop example above).

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

I assume you are running this script on Linux.

Prompting the user for a device to mount and unmount is a bit unusual, not to mention user unfriendly and error prone. What is your intention — to find a USB memory stick? If so, you could mitigate some of the danger by limiting the user's choices.

  • You might only present USB block devices as options (Look for /dev/disk/by-path/*-usb-*.)
  • You could look for a device with a particular volume label. (Look in /dev/disk/by-label/NAME.)
  • You could check whether the device is on removable media.

Here's one way to list removable media and their partitions:

for blkdev in /sys/block/* ; do
    if [ "`cat $blkdev/removable`" = 1 ]; then
        for devnum in `cat "$blkdev"/dev "$blkdev"/*/dev 2>/dev/null` ; do
            for node in /dev/* ; do
                [ "`stat -c %t:%T "$node"`" = "$devnum" ] && echo "$node"
            done
        done
    fi
done

Instead of mounting the device at /media, I suggest creating a temporary directory with mktemp -d and mounting it there. Linux desktop environments often use subdirectories of /media as places where devices are automatically mounted on insertion.


Restarting the script with sh "$myself" is problematic in several ways:

  • You've hard-coded the name of the script, so it will break if anyone renames the script. You could use $0 instead.
  • You've assumed that auto_rsa_gen.sh is in the current directory.
  • The first run is done in Bash due to the shebang line, but subsequent runs are in sh (usually Bash in Bourne-shell compatibility mode).
  • You didn't use exec, so execution of the outer script will continue with the wrong device after the inner script completes successfully with the right device.

The straightforward solution, which is to use a while loop, would have avoided all of those pitfalls.


The validation of $ident does not happen after the prompt, but is interrupted by the prompt for $cnum. That's confusing to the user and to the programmer.


You should echo the confirmation of copying within the loop:

cp "$f" /media/ && echo "Copied $f to /media" \
                || echo "Failed to copy $f to /media"

This is better because:

  • You avoid running find twice, which is more efficient.
  • You avoid running find twice, which is more maintainable.
  • The confirmation message only appears if copying succeeded.
  • It acts as a progress indicator.

It is idiomatic to put the do at the end of the same line as while and for, like this:

while condition ; do
    stuff
done

for i in $list ; do
    stuff_with "$i"
done

Similarly, then follows on the same line as if:

if $condition ; then
    something
else
    something_else
fi

Your indentation will make more sense if you follow these conventions.

\$\endgroup\$

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.