Pitching a script to find improvements and suggestions: Firewall and Network Auditor
 

Hello all,

Recently I had to write a script to gather and audit some network info from Linux servers. I would appreciate suggestions and improvement tweaks and further inclusions to make the script more robust.

- Bellamkonda Sudhakar

The script itself:

That's a lot of code to comment on and it looks pretty tidy; two reasons why nobody has commented?

As said, it's tidy code but some things you might like to consider:

• $( ... ) is recommended over ` ... ` for reasons explained here.
• [[ ... ]] is recommended over [ ... ] for reasons explained here.
• A common but not universal convention is to use upper case variable names for environment variables and lower case names for the rest.
• Some of the command output string manipulation looks as if it could be simplified. For example
  
  Code:

  ---

  ip -d link | grep ^[0-9] | awk '{ print $2 }' | sed 's/://' | grep -vi "lo" | grep -vc "eth"

  ---

  ... can be simplified to
  
  Code:

  ---

  ip -d link | grep -c '^[0-9]*: eth'

  ---

• Errors conventionally result in a message to stderr and a non-zero return code. If you regard it as an error, this code
  
  Code:

  ---

  if [ $UID -ne 0 ]
then
    echo
    echo "No root previleges"
    exit 0
fi

  ---

  could be changed to
  
  Code:

  ---

  if [ $UID -ne 0 ]
then
    echo "No root privileges" >&2
    exit 1
fi

  ---

• The single command line option could more conveniently be handled using a case statement, here including an error trap for an invalid option and assuming usage is moved to a function which gives verbose output if called with -v (and otherwise could suggest using the -h option):
  
  Code:

  ---

  case $1 in
    -a )
        echo 'Unserviced option -a' >&2
        exit 1
        ;;
    -c )
      cleanup
      echo "PID files removed"
      exit 0
      ;;
    -h )
      usage -v
      ;;
    * )
      echo "Invalid option $1" >&2
      usage
      exit 1
esac

  ---

  More complex command line parsing, facilitating trapping invalid extra arguments is best handled by getopts.
• Purely a matter of style, no right and no wrong way; it is more compact to use
  
  Code:

  ---

  if [[ <test expression> ]];then
    <commands>
fi

  ---

• Again purely a matter of style, no right and no wrong way; you may prefer, instead of
  
  Code:

  ---

  if [ $IP4E ]; then IPV="IPV4"; fi

  ---

  to use
  
  Code:

  ---

  [[ $IP4E ]] && IPV="IPV4"

  ---

  ... but beware; [[ <test expression> ]] && <commands 1> || <commands 2> can behave unexpectedly (if <commands 1> set a non zero return code, <commands 2> is executed).
• If strings do not contain anything to be substituted by bash, they may be in single quotes, not double (for example "IPV4" above).
• Aliases can make what are intended to be external commands behave unexpectedly. ls is often aliased and so troublesome. Safest for scripts to unset all aliases with unalias -a
• In case the script's name is changed, the usage message could use ${0##*/} for its name.
• The usage message could be done with a single echo (but is not then so easy to indent prettily):
  
  Code:

  ---

  echo '
Usage: netadtchk [-a | -h | -c]
    -a All
    -h Help. Prints this usage message and exits
    -c Clear PID files"
    If no option is given, prints the summary information to the stdout and into the /tmp/netadtchk/$AUDITFL file
'

  ---

  Note: the -a option is not serviced in the script.
• Spelling: privilege and summary

I apologize - I need to learn quite q few things. Bear with me .

Still reading script - and catkin's insight was good. I am unsure about command

I mean script if script is already meant for *nix why would you need that - that too if you intend to use it just once. I think a static value would have been good enough.


Following variable seems unused

Moved: This thread is more suitable in the Programming forum since you're only interested in robustness (I would first look at existing functionality tools like GNU Tiger, LSAT or OpenVAS provide) and has been moved accordingly to help your thread/question get the exposure it deserves.

• You have a lot of unquoted variables in your script. While this is ok as long as their contents don't contain spaces or other reserved characters, if for some reason this should change, then you're going to have trouble.
  
  You should get into the habit of always quoting variable expansions by default, then you only have to worry about those few times when they need to be unquoted.
  
  
• You make four individual calls to "ip". It might be more efficient if you call "id -d addr" only once and store its entire output into a variable. Then you could simply parse that for all the values you want.
  
  
• As catkin pointed out, you have lots of useless uses of grep and related tools. If your command uses sed or awk, then you already have grep-like ability built in and generally shouldn't need it. Also, don't forget that grep and sed have -e options allowing you to apply multiple expressions at once.
  
  Finally, always quote your expressions to avoid conflicts with shell grammar.
  
  
  Code:

  ---

  #NMSR=`grep nameserver /etc/resolv.conf | grep -v ^# | head -1 | awk '{ print $2 } '`
NMSR=$( awk '/nameserver/ { print $2 ; exit }' /etc/resolv.conf )

#DFGW=`ip route | grep default | sed 's/default\|proto\|static//g'`
DFGW=$( sed ' /default/ s/default\|proto\|static//g' < <( ip route ) )

  ---

• BTW, the -1 syntax for head/tail is deprecated; you should use -n 1 instead.
  
  
• Notice too in the above that process substitution can be used instead of a pipe.
  
  
• As the guys at Greg's Wiki say: "expr is a relic of ancient Rome. Do not wield it". Bash has integer math ability built in.
  
  
  Code:

  ---

  #TNIC=`expr $HNIC + $VNIC + $LNIC`
TNIC=$(( $HNIC + $VNIC + $LNIC ))

  ---

• The < redirector works stand-alone inside a command substitution, so bash can import a file into a variable without using cat. And please use $(), as catkin mentioned. It's so much more readable and flexible.
  
  
  Code:

  ---

  #IPFW=`cat /proc/sys/net/ipv4/ip_forward`
#IPMQ=`cat /proc/sys/net/ipv4/ip_dynaddr`
IPFW=$( < /proc/sys/net/ipv4/ip_forward )
IPMQ=$( < /proc/sys/net/ipv4/ip_dynaddr )

  ---

• catkin stated that [[..]] is better than [..], but also, ((..)) is better than either when doing numerical evaluations.
  
  
  Code:

  ---

  #if [ $UID -ne 0 ]
if (( UID != 0 ))
#or even just...
if (( UID ))

  ---

I could go on, but this post is getting to be a bit tiring to work on, so I think I'll stop here. ;)

I believe the above is plenty enough for you to digest at the moment, so my only addition is a personal view.
The use of a function is generally tied to reducing the need to repeat the same lines of code several times.
You have functions like create_workspace which ultimately are only required at one point in the entire script,
so I probably would not have worried about creating a function for this example.

Look forward to your next revision :)

Another reason for a function is to make the code more comprehensible; it has been suggested that no section of code should be more than X lines where X is what is visible on a screen (a lot bigger than it used to be!). Like all such "rules" it is a judgement call.

Its just me - I usually get impatient when OP forgets to respond/acknowledge.

that's a big WOW, Thanks a lot, in fact you can say that my BASH scripting knowledge is a bit of an ancient relic :) and I have used some of the suggestions in other scripts that i have been writing. I guess that I will rewrite the script and revise it with all there in mind.

I appreciate all you guys and especially Catkin and David the H.

@asimba : so that i can use this script in different *nix other than linux, i.e. freebsd etc. hopefully.

Thanks a lot.
Sudhagud
(Till the revised script :) )