Reworking options handling and default settings to be more flexible #2

merged 21 commits into from Jan 28, 2014

2 participants


No description provided.


Something strange happens here with the indentation. Maybe you use tabs instead of spaces after ")", but before "{ print ..."? Could you please correct it and use tabs ONLY in lines prefixes and never use tabs in the middle of lines?..


Could you please replace the style:

if (something) { code }
else { code }


if (something) {
} else {

? Because the whole source code should be guided by the same coding style...


Typo - "opvenvz".


Seems to me $CONF variable is assigned, but is never used. What is $CONF for?..

For config file support, which I've not finished/added yet. Though I'm questioning doing so, as I hate to drag in Config::IniFiles as a requirement.

You're right, such requirement is weird, because simple INI file parsing is quite easy to implement in Perl. BTW, are you sure you really need this config? Vzfirewal's main idea is that it is kept extremely simple.

If there are command-line options to set things, the config file wouldn't strictly be needed, no. As for simplicity, I agree it should do the best job it can at running sanely with no configuration. There are things in the current setup which, while simple, need to be improved (eg. allowing all icmp types and forcing ssh open to the world). It's probably more a user preference whether you would change values in a single conf file (say ICMP_TYPES="3 4 11 12 8") or specify long cli options (--icmp-types="3 4 11 12 8"); I figured just support both ways.

I'll work on getting the cli options working on those and wait on your comment/opinion on having a conf file or not.

Having implemented the command-line options, I will say I do want a config file. I now have a handle to change the default (I'm setting --failsafe-addrs to our admin subnet), but you have to remember to do that every time you run vzfirewall, and so does every other admin that runs it. My --failsafe-addrs settings keep getting reset to the defaults (less secure) when another admin here runs vzfirewall. So I want a config file so I can specify what the system default settings should be.


Why vzfirewall.rules is placed to e.g. /etc/vz/ (dirname('/etc/vz/conf'))? It is a system-wide file which is not related to OpenVZ at all, so it should not reside in /etc/vz.

vzfirewall.rules would be a default file to store iptables rules in if no other locations are found on the system and nothing is explicitly specified; as you cannot/would not use vzfirewall on a system not running openvz, using the location of other openvz config files by default seemed pretty safe, but can certainly be changed. Would you prefer /etc/vzfirewall/vzfirewall.rules as the default?

Not quite. The file with iptables rules is related not to openvz, but to a service like iptables-persistent in Ubuntu or Debian - (RHEL also has such service - So the target file should be the same as iptables-persistent (or similar) uses. Vzfirewall should interact with such file, else the rules will not be applied on the next machine boot. I meant that.

So maybe the location should first check /etc/sysconfig/iptables, then /etc/iptables/rules.v4, and only then - /etc/vz/vzfirewall.rules (in the latest case - with a warning saying that the rules will not be persisted on the next boot time)?

Yes, that's the intent, and close to what it does. It searches two known locations (that you mention) to try to figure out what environment it is in, as well as using a fallback location (vzfirewall.rules) rather than failing to run. You can easily add a line in startup scripts so vzfirewall.rules is read at boot up; or maybe it's better to fail with an error saying to specify --rules?

I'll move vzfirewall.rules to be the last location searched, and add a warning if you're in verbose mode.


Great! I've made a little code review (comments are tied to line numbers), could you please correct the changes and push to your repo?.. The pull request will be updated automatically if I am not wrong.


looks like I needed to rebase from github first... oops.


That last commit wraps up most of the actual functionality I have in mind to implement in the short term, pending the decision of whether or not to support a config file.

I might add debian packaging files if time allows.


A bit suspicious line:

if ($opt{'openvz-conf-dir'}) {
    $conf{'openvz-conf-dir'} = $opt{dir};

Did you mean

$conf{'openvz-conf-dir'} = $opt{'openvz-conf-dir'};


sure enough .. now fixed.


Why mandatory logging with no support of turning on/off this feature?.. I mean this commit: 372d2fa (I don't know if it's right, maybe if you explain this decision, it will look OK).

Don't all admins log access violations, and correlate/review the logs regularly? ;-) I hadn't even considered an on/off switch, but can sure add one in. I would tend to leave the logging of ssh access to the hardware node in either case, but I suppose if someone has a real reason they don't want any record of what's being blocked, we could let them disable it.

With incessant port scans going on, we don't use those to see who's testing the config, but at times it's invaluable help in catching why some service isn't working (ie. need to allow something in the firewall).


Jesse, overall - great work! A couple of more comments (maybe) from my side, and I'll merge it into the upstream. :-) BTW large pull requests are evil: it's quite hard to review it. I'm not sure GitHub allows to create a number of small pull requests instead of a big one.

@DmitryKoterov DmitryKoterov commented on the diff Jan 22, 2014
@@ -0,0 +1,19 @@
+# vzfirewall.conf: vzfirewall system configuration
DmitryKoterov Jan 22, 2014 Owner

Please add here a comment like:

Default location of this file: /etc/...


but at times it's invaluable help in catching why some service isn't working (ie. need to allow something in the firewall)

1. So is it for a debugging purpose or for a production purpose? If it is for debugging, I vote to leave it turned off by default (but allow to turn the option on in the config file). If it is for a production purpose, then I don't understand it quite: 99.99% of such logs are useless, because they are generated by worms' port scanners. (Frankly, during last 5 years I never turned on logging in iptables, because else I got immediately spoofed with lots of useless info.) Why waste resources then? BTW SSH port on a hardware note is practically always opened, and because it is opened, SSH logs connection attempts by itself in /var/log/auth.log. So why log it in addition by vzfirewall?
2. Where are these logs saved at by default? /var/log/syslog?

@jnorell jnorell closed this Jan 27, 2014
@jnorell jnorell reopened this Jan 27, 2014
@DmitryKoterov DmitryKoterov merged commit 6c804e0 into DmitryKoterov:master Jan 28, 2014

Splendid! Thanks for your patience.


Jesse, may I convert tabs to spaces now and commit? Will it conflict with any of your not-yet-pushed changes?


Go for it, I don't have anything else underway. Also, if you have preferred .vimrc settings I can switch to whatever.


Done. Please pull.
Tab size is 4 spaces (it's your defaults, I suppose).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment