RimBlock
15-04-2010, 04:39
Hi Gregory,
I see the script is coming on by leaps and bounds.
I personally prefer a collection of small funtional scripts to one large one and was just suggesting having a config file which can be sources at the begining of each individual script. The technique allows for possible reuse of the seperate functional scripts for other 'projects'. Different people, different preferences . How about putting the code chunks in to functions called from the case statement. A good overview here. Makes it easier to read and easier to amend than a solid block of code .
You beat me to the WHITELIST . Whilst playing with setting my host firewall rules last night it occured that a whitelist would be good to prevent you locking yourself out. Glad to see you had the same thought.
Going through the code there is one part that screams "Danger Will Robinson!...".
It is the woot lock section;
Not sure why the ls command but I would guess you are redirecting STOUT to a log file or running from a terminal session.
It is the killing the PID in the wootlock file. Why this sets alarm bells ringing is due to the fact that the script may have previously exited abnormally and left a wootlock file floating around containing a PID which may have been reused by another process after the script died. Net effect, you could end up killing a process you have no wish to or if someone gets in to the system and is able to edit the wootlock file, they can get your script to kill your entire server (although I am sure file permissions and running the script as a user rather than root should prevent that).
Why not take the output from the PS command to feed the kill and pref. a straight kill rather than a -9. Move to the -9 if you have a hang and the straight kill fails rather than defaulting to it. Using the PS ensures, if the grep is targeted enough, that you will kill a running process associated with the script and not just any old PID left in a file. The file can still be used as a flag to denote a check for the pid is needed. I personally love flag files / db flags in tables and 'sweep' scripts / procs as a good control method.
The next question would be, why is it being killed. Would it not be better to allow it to complete. How about having a datestamp and a count in the wootlock file. You could check the wootfile for the date/time and count. pause for 10 seconds then check again. If it has not changed then kill it. If it is changing then let it be and report it is already processing.
Better still, why not set a lock file at the start of the script regardless of the function you are asking the script to perform and rather than just exiting when finished, go to an exit function which takes the exit value and removes the script lock file before translating the exit code and reporting exit reason to a log file and then exits with the status code. The exit function is generic and can be used by all the code chunks / functions. Again, to get the best effect, you would need to log to a log file from within the script
Adding a LOG2FILE variable and setting it to 1 or 0 (yes or no) then having an 'if' check and redirecting STDOUT and STDERR to a LOGFILE (pref with a date stamp in the filename) by doing;
If unset then it STDOUT and STDERR can just go to the console/terminal.
You could even have another check for the size of the ERR_LOG_FILE and have it automatically email you if there is a script error.
Ok, last thought. You could also have a DONOTRUN variable which denotes that the script should not be run and an if statement to exit if this is set at the beginning of the script. This would lock out the script at script level rather than having to amend the crontab to stop it running. You could go more granular and have variables for locking out script functions so you could lock out the 'ban' function but keep the 'checkiptables' function active.
Might also be worth thinking about stepping back and putting together a proper design on paper etc rather than correcting and fixing and expanding 'on the fly' which if you are anything like me and the majority of programmers out there is whayt you will be doing. Most programmers hate paperwork including me .
Pheww. Lots to read and ether work with or ignore .
Cheers
RB
I see the script is coming on by leaps and bounds.
I personally prefer a collection of small funtional scripts to one large one and was just suggesting having a config file which can be sources at the begining of each individual script. The technique allows for possible reuse of the seperate functional scripts for other 'projects'. Different people, different preferences . How about putting the code chunks in to functions called from the case statement. A good overview here. Makes it easier to read and easier to amend than a solid block of code .
You beat me to the WHITELIST . Whilst playing with setting my host firewall rules last night it occured that a whitelist would be good to prevent you locking yourself out. Glad to see you had the same thought.
Going through the code there is one part that screams "Danger Will Robinson!...".
It is the woot lock section;
if [ -e "$WOOTLOCK" ]; then
#w00t may already be running, terminate process
ls "$WOOTLOCK"
line=`$CAT "$WOOTLOCK"`
if [ "$line" != "" ]; then
$KILL -9 "$line" >> /dev/nul
$RM $WOOTLOCK
fi
fi
It is the killing the PID in the wootlock file. Why this sets alarm bells ringing is due to the fact that the script may have previously exited abnormally and left a wootlock file floating around containing a PID which may have been reused by another process after the script died. Net effect, you could end up killing a process you have no wish to or if someone gets in to the system and is able to edit the wootlock file, they can get your script to kill your entire server (although I am sure file permissions and running the script as a user rather than root should prevent that).
Why not take the output from the PS command to feed the kill and pref. a straight kill rather than a -9. Move to the -9 if you have a hang and the straight kill fails rather than defaulting to it. Using the PS ensures, if the grep is targeted enough, that you will kill a running process associated with the script and not just any old PID left in a file. The file can still be used as a flag to denote a check for the pid is needed. I personally love flag files / db flags in tables and 'sweep' scripts / procs as a good control method.
The next question would be, why is it being killed. Would it not be better to allow it to complete. How about having a datestamp and a count in the wootlock file. You could check the wootfile for the date/time and count. pause for 10 seconds then check again. If it has not changed then kill it. If it is changing then let it be and report it is already processing.
Better still, why not set a lock file at the start of the script regardless of the function you are asking the script to perform and rather than just exiting when finished, go to an exit function which takes the exit value and removes the script lock file before translating the exit code and reporting exit reason to a log file and then exits with the status code. The exit function is generic and can be used by all the code chunks / functions. Again, to get the best effect, you would need to log to a log file from within the script
Adding a LOG2FILE variable and setting it to 1 or 0 (yes or no) then having an 'if' check and redirecting STDOUT and STDERR to a LOGFILE (pref with a date stamp in the filename) by doing;
exec 1>> "$LOG_FILE" #Redirect STDOUT to a logfile
exec 2>> "$ERR_LOG_FILE" #Redirect STDERR to a logfile
You could even have another check for the size of the ERR_LOG_FILE and have it automatically email you if there is a script error.
Ok, last thought. You could also have a DONOTRUN variable which denotes that the script should not be run and an if statement to exit if this is set at the beginning of the script. This would lock out the script at script level rather than having to amend the crontab to stop it running. You could go more granular and have variables for locking out script functions so you could lock out the 'ban' function but keep the 'checkiptables' function active.
Might also be worth thinking about stepping back and putting together a proper design on paper etc rather than correcting and fixing and expanding 'on the fly' which if you are anything like me and the majority of programmers out there is whayt you will be doing. Most programmers hate paperwork including me .
Pheww. Lots to read and ether work with or ignore .
Cheers
RB