Call for testing: firewall protocol support

[Ondrej Zajicek]

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).

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).

> +<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?

> +
> +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.

> +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.


> +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.


-- 
Elen sila lumenn' omentielvo

Ondrej 'SanTiago' Zajicek (email: santiago at crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://trubka.network.cz/pipermail/bird-users/attachments/20111212/0af45ff4/attachment-0001.asc>