Call for testing: firewall protocol support

[Alexander V. Chernikov]

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.



-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-Add-firewall-support-v2.patch
URL: <http://trubka.network.cz/pipermail/bird-users/attachments/20111223/5c48bb9e/attachment-0001.ksh>