Page MenuHomeFreeBSD

Add two new options to "ipfw table <NAME> create" to simplify firewall reload
ClosedPublic

Authored by lev on Nov 26 2018, 12:33 PM.

Details

Summary

Now it is very hard to reload (with service ipfw restart and such) firewall which uses tables and have create table NAME commands, as these commands will fail because tables already exists And delete table NAME will fail for first firewall load, as tables are not exist yet.

This patch adds two new options for create table command:

  • missing — this option suppresses EEXISTS error, but check, that existing table has same parameters as new one.
  • or-flush — this options implies missing and additionally flush table if it exists.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

lev created this revision.Nov 26 2018, 12:33 PM
mizhka_gmail.com requested changes to this revision.Nov 26 2018, 11:21 PM
mizhka_gmail.com added a subscriber: mizhka_gmail.com.
mizhka_gmail.com added inline comments.
ipfw/ipfw.8
2121–2139 ↗(On Diff #51113)

Bump date of man doc?

ipfw/tables.c
328–331 ↗(On Diff #51113)

Should be tabs instead of spaces

500 ↗(On Diff #51113)

(flush != 0)
better to follow same style over whole code ;)

This revision now requires changes to proceed.Nov 26 2018, 11:21 PM
lev updated this revision to Diff 51163.Nov 27 2018, 11:54 AM

Address comments by @mizhka_gmail.com

lev marked 3 inline comments as done.Nov 27 2018, 11:55 AM
mizhka_gmail.com accepted this revision.Mar 21 2019, 6:44 AM
mizhka_gmail.com added inline comments.
sbin/ipfw/tables.c
499–502 ↗(On Diff #51163)

double indentation. please use && to simplify code

This revision is now accepted and ready to land.Mar 21 2019, 6:44 AM
lev updated this revision to Diff 55315.Mar 21 2019, 10:44 AM
This revision now requires review to proceed.Mar 21 2019, 10:44 AM
lev marked an inline comment as done.Mar 21 2019, 10:45 AM
ae set the repository for this revision to rS FreeBSD src repository.
0mp requested changes to this revision.Mar 21 2019, 1:47 PM
0mp added a subscriber: 0mp.
0mp added inline comments.
ipfw/ipfw.8
2155 ↗(On Diff #55315)

Just a minor thing, but we usually put a new sentence in a new line.

You may find more style issues in the patch by running igor and mandoc -Tlint on the manpage.

This revision now requires changes to proceed.Mar 21 2019, 1:47 PM
lev marked an inline comment as done.Mar 21 2019, 2:12 PM
lev added inline comments.
lev updated this revision to Diff 55318.
ipfw/ipfw.8
2155 ↗(On Diff #55315)

mandoc -Tlint shows a lot of errors, much more than my changes.

0mp accepted this revision.Mar 21 2019, 2:25 PM

OK from manpages. Please remember to bump the date in the manual page.

ipfw/ipfw.8
2158 ↗(On Diff #55318)

typo? s/tabel/table/

2155 ↗(On Diff #55315)

I wouldn't care about those other errors. Not in this patch at least. :) Thanks!

This revision is now accepted and ready to land.Mar 21 2019, 2:25 PM
lev marked an inline comment as done.Mar 21 2019, 2:30 PM
lev updated this revision to Diff 55320.
This revision now requires review to proceed.Mar 21 2019, 2:30 PM
lev marked an inline comment as done.Mar 21 2019, 2:30 PM
mizhka_gmail.com accepted this revision.Mar 21 2019, 3:09 PM
This revision is now accepted and ready to land.Mar 21 2019, 3:09 PM
ae added a comment.Apr 9 2019, 6:19 PM

I think you can add to the beginning of your ipfw rules script something like this:

ipfw -q flush
ipfw -q table all destroy

And then create needed tables and fill them.

lev added a comment.Apr 10 2019, 11:25 AM
In D18339#426430, @ae wrote:

I think you can add to the beginning of your ipfw rules script something like this:

ipfw -q flush
ipfw -q table all destroy

And then create needed tables and fill them.

But this change is more flexible. I could imagine situation when you don't want to flush existing table but want to be sure that it is exists without external (for example, sh) logic.

ae added a comment.Apr 10 2019, 5:29 PM

Ok. It is more flexible, but produces additional options. I think ipfw(8) is already very complex.
What if we will make "missing"+"flush" behavior as default.
It seems if user wants to create table, it is expected that later this table will be filled. So, if we are creating some table, and it is already exist, we will check that the table has the same configuration and then flush it.
If configuration is different, then we return error. What you think?

lev added a comment.Apr 10 2019, 6:19 PM
In D18339#426772, @ae wrote:

Ok. It is more flexible, but produces additional options. I think ipfw(8) is already very complex.
What if we will make "missing"+"flush" behavior as default.
It seems if user wants to create table, it is expected that later this table will be filled. So, if we are creating some table, and it is already exist, we will check that the table has the same configuration and then flush it.
If configuration is different, then we return error. What you think?

Looks good for me personally (for my goals), but what if user doesn't want flush existing, if s/he wants to be sure, that table is exists, but if it exists already, to save current content? It could be programmed externally to ipfw (with sh or by other means), of course, but stadard service ipfw reload doesn't allow it.
For example firewall could have table with addresses which are blacklisted by some log analyzer ("file2ban"). User want to reload firewall, but don't want to lose accumulated black list. Now (or with "default to flush") user need to ether dump table before restart and load after it or temporary commet-out table creation from firewall config (table creation should be here, because same config is used on system boot!). Both solutions are possible (and they are possible now, without any changes) but very inconvenient, IMHO.

Maybe, we could have or-flush by default and or-preserve as new option. It adds only one new option (instead of two), but allows both scenarios.

Is it okay to ask what the status of this is? Because it looks really useful!

melifaro accepted this revision.Fri, May 24, 10:27 AM
This revision was automatically updated to reflect the committed changes.