On 12.12.2011 05:10, Ondrej Zajicek wrote:

On Sun, Dec 11, 2011 at 05:25:08PM +0400, Alexander V. Chernikov wrote:

...

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1

Hello list!

This patch adds 'firewall' protocol permitting prefixes announced to this protocol to be put in configured firewall table with optional value.

Nice!

I have one major comment. It is a good idea to have a generic protocol 'firewall'? From the code, it seems like an almost empty shell to me, which just complicates design and adding fwtype-specific features and config options. I would just use separate protocols (ipfw, pf, ipset). Well, there are at least 3 protocols instead of one. This bumps maximum number of protocols/protocol attributes significantly which is not good for multiprotocol patches (and hardens config portability for filter writers). It is also the same design bird is using for kernel proto, for example.

Some minor comments below:

...

Supported firewalls: IPFW, PF, * Optional value support: IPFW, *

Sample configuration:

protocol bgp { .. import filter { fw_value = 42; accept; } # Set firewall optional value for each prefix }

protocol firewall { fwtype ipfw; fwtable "2"; export all; flush always; # do flush both on startup and shutdown };

Tested on FreeBSD 8.X, PF should work on Open/NetBSD, too.

[*] I can add support for ipset on demand. However I can't understand how it can be [effectively] used without some kind of radix/rbtree backend (according to docs).

Really? I also looked on ipset (but i cannot find any useful docs) and the kernel interface seemed to be similar to pf (i.e. commands to add/remove prefixes from a set).

I have set up debian with ipset 6.10 so I plan to add all these various table types to bird before this year ends.

...

+<descrip> + <tag>fwtype pf|ipfw</tag> Select firewall type. + <tag>fwtable<m/name/</tag> Specifies firewall table name.

...

+ <tag>flush on startup|shutdown</tag>Perform table flush on protocol startup or shutdown. + <tag>flush always</tag>Perform table flush on protocol startup and shutdown.

Flush should be probably default, otherwise users get error messages after BIRD restart. It would also be more consistent with Kernel protocol behavior. Perhaps also use 'keep' keyword instead of 'flush', like in Kernel proto?

Okay, flush by default which is reasonable, and { keep on start, keep on shutdown, keep (for both)} ?

...

+ +static void +firewall_rt_notify(struct proto *P, rtable *src_table, net *n, rte *new, rte *old, ea_list *attrs) +{ ... + if (old&& new&& p->fw->fw_replace) + { + if (!p->fw->fw_replace(p->fwdata, n, prefix_data)) + log(L_ERR "Replacing prefix %I/%d with data '%S' failed", n->n.prefix, n->n.pxlen, prefix_data); + return; + } + + if (old) + if (!p->fw->fw_del(p->fwdata, n)) + log(L_ERR "Removing prefix %I/%d failed", n->n.prefix, n->n.pxlen); + + if (new) + if (!p->fw->fw_add(p->fwdata, n, prefix_data)) + log(L_ERR "Adding prefix %I/%d with data '%s' failed", n->n.prefix, n->n.pxlen, prefix_data);

These error messages should definitely be rate limited (see log_rl()). Also you have an error message in both the generic code and the fwtype-specific backend.

Fixed, thanks.

...

+static int +firewall_start(struct proto *P) +{ + struct firewall_proto *p = (struct firewall_proto *) P; + struct firewall_config *c = (struct firewall_config *)P->cf; + void *fwdata; + + if ((fwdata = p->fw->fw_init(P, c->fwtable)) == NULL) + return PS_DOWN; + + p->fwdata = fwdata; + + /* Flush table if needed */ + if ((c->flush_start)&& (p->fw->fw_flush)) + if (!p->fw->fw_flush(fwdata)) + { + log(L_ERR "flush failed for table %s", c->fwtable); + return PS_DOWN;

I guess that protocol start() function is not supposed to return PS_DOWN (just PS_START or PS_UP).

Fromt the code it seems that returning PS_DOWN would cause some problems in the state machine.

Okay. So I should hang in PS_START in this case (and periodically try to do init, as this is done in l3vpn) ?

...

+static void +firewall_get_status(struct proto *P, byte *buf) +{ + struct firewall_config *c = (struct firewall_config *) P->cf; + + bsprintf(buf, "Table [%s]", c ? c->fwtable : "none");

This is unnecessary, c should be always non-NULL.

...

+void +ipfw_fw_shutdown(void *_priv) +{ + struct ipfw_priv *priv = _priv; + + if (--ipfw_instance_count == 0) + { + DBG("Closing ipfw socket %d\n", ipfw_fd); + close(ipfw_fd); + ipfw_fd = -1; + } + + mb_free(priv); +}

This is unnecessary. After shutdown, everything from protocol pool is freed.

It's hard to get rid of some habits :)

Btw, moving integer attributes to unsigned helps passing IPv4 addresses as attribute.

On 22.12.2011 06:25, Pawel Tyll wrote:

Hi Alexander,

...

This patch adds 'firewall' protocol permitting prefixes announced to this protocol to be put in configured firewall table with optional value. Thanks for implementing this. It seems to be working fine so far.

Is there some simple filter function that will convert an IP address to integer? If not, then maybe it's worthwhile to allow import filter { fw_value = gw; accept; }; to work. Something like: Such function should be added to filters API, yes. Unfortunately filters code is a bit hard to understand, but I'll try to look at it another time

# ipfw table 67 add 127.0.0.1 127.0.0.1 # ipfw table 67 list 127.0.0.1/32 2130706433 # ipfw -i table 67 list 127.0.0.1/32 127.0.0.1

Again, thanks and great work :)

PS. Get ready to be bothered about ng_eiface! ;>