Re: [RFC nft] ct expr: make directional keys work

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Thu, Dec 17, 2015 at 01:55:33AM +0100, Florian Westphal wrote:
> some ct expressions don't work at the moment since we never set the
> 'direction' attribute, but kernel mandates it.
> 
> The current approach i've been working splits ct keywords into
> two groups, one mandates a 'direction' argument (saddr, protocol),
> others do not (mark for example).
> 
> Would this syntax be acceptable?
> 
> ct saddr original 192.168.0.1

Did you try to fit this in the existing parser to see if it result in
shift/reduce conflicts?

I'm telling this because the relational expression expects a ct
expression on the left hand side, and a value on the right hand side.

So I think with such syntax the grammar would need to be upgraded to
something like:

ct_key                  :       SADDR   ORIGINAL        { $$ = ...; }
                        |       SADDR   REPLY           { $$ = ...; }

Where the $$ would need to encode both the key and direction.

Probably it's easier to fit this into it like this:

ct_expr                 :       CT      ct_key
                        {
                                $$ = ct_expr_alloc(&@$, $1, IP_CT_DIR_MAX);
                        }
                        |       CT      ct_direction       ct_directional_key
                        {
                                $$ = ct_expr_alloc(&@$, $2, $1);
                        }
                        ;

ct_direction            :       DIRECTION       STRING
                        {
                                int dir;

                                dir = ct_direction_lookup($2);
                                if (dir < 0)
                                        display error on wrong direction
                        }
                        ;

ct_directional_key      :       SADDR   { $$ = NFT_CT_SADDR; }
                        ...

> If not, I'd like suggestions on how this should look like instead.
> 
> Since the saddr (and a few other) arguments have unknown size
> (depends on the l3 tracker tuple sizes), its currently filled in
> later depending on NH base (i.e. in nft upstream).
> 
> This means that
> 
> ct proto-dst original ssh
> 
> will NOT work, but
> 
> ct protocol original tcp ct proto-dst original ssh
>
> would.  Is that ok?  I don't see how I could auto-add the dependency in
> such case.

I see, you're proposing to extract the dependency from the service,
but then if I specify 22 instead (numeric value) we cannot extract
anything from there.

> Moreover, while this is currently implemented as a dependency (type set to
> inet_service if PROTO_BASE_TRANSPORT_HDR present) the kernel does just
> fetch a 16 bit quantity from the tuple so there is no real dependency
> -- its just raw data.
> 
> So we could actually allow things like
> 
> ct proto-dst original 22
> 
> and it would match anything that has a 22 in the dst.tuple.all field...
> but -- does that make sense?

I would start simple, ie. bail out and ask the user that the layer 4
protocol needs to be explicitly specified in case the port is
specified, same thing with layer 3. Better to start being a bit more
restrictive and relax this than the other way around I'd say.

> Finally, I'm working on support for packets and byte counters.
> 
> Fetching original or reply directions would 'just work' after
> directional keys are supported, i.e.:
> 
> ct packets original > 100
> 
> But I'm not sure how we should handle the case where someone wants to test
> 'X bytes/packets in total'.

I'd suggest to add NFT_CT_CTR_BOTH, ie. we'll have
NFT_CT_CTR_ORIG, NFT_CT_CTR_REPL and NFT_CT_CTR_BOTH.

Adding a expression to sum things seems too much for this case I think.

> ct packets > 100:
> could be confusing, also not sure how difficult it is
> to allow ct keywords that have an optional direction

I think this should fit into the grammar that I'm proposing above with
no shift/reduce conflicts, ie.

ct packets VALUE
ct direction original packets VALUE
ct direction reply packets VALUE