[RFC nft] ct expr: make directional keys work

[RFC nft] ct expr: make directional keys work

[Florian Westphal]

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

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.

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?

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

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

ct packets both > 100: also looks wrong to me

ct packets original + ct packets reply > 100:
Looks like the most nft-esque solution to me, but would need new EXPR_ADD.

Thoughts?

Thanks,
Florian

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

[Pablo Neira Ayuso]

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

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

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> 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?

Yes, no s/r conflicts.

I'll send what I have later today or tomorrow.

> So I think with such syntax the grammar would need to be upgraded to
> something like:
> 
> ct_key                  :       SADDR   ORIGINAL        { $$ = ...; }
>                         |       SADDR   REPLY           { $$ = ...; }

I could give that a try too, at the moments its basically

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


> ct_direction            :       DIRECTION       STRING
>                         {

and no new keywords (reuses the symbolic type in src/ct.c)

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

Yes, thats right.  For consisteny it does make sense to require an
explicit L4 protocol, so I guess I'll keep it as-is for now.

We can improve it later to allow 'magic' derivation of the dependeny
if we really want to.

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

agree

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

Ok, I'll do the summing-up in nft_ct.c then.

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

I'll have to check.  I don't want to add any new keywords to the lexer
since that creates more problems for the grammar.

(where we choke because something that should be treated/parsed as a
 plain string is then cosidered a keyword...)

I think I'll have something presentable by tomorrow.

Thanks.

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

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > 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
> 
> I'll have to check.  I don't want to add any new keywords to the lexer
> since that creates more problems for the grammar.

Drats, it doesn't work.
conflicts: 1 shift/reduce

'ct direction' is already a valid expression, it loads
CTINFO2DIR(ctinfo).

I'll send what I have in a second, but its not great either.

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

[Pablo Neira Ayuso]

On Fri, Dec 18, 2015 at 06:17:07PM +0100, Florian Westphal wrote:
> Florian Westphal <fw@strlen.de> wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > 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
> > 
> > I'll have to check.  I don't want to add any new keywords to the lexer
> > since that creates more problems for the grammar.
> 
> Drats, it doesn't work.
> conflicts: 1 shift/reduce

Right, the parser doesn't know if it has to reduce

ct direction original

as simple expression, or keep shifting for more tokens.

> 'ct direction' is already a valid expression, it loads
> CTINFO2DIR(ctinfo).
> 
> I'll send what I have in a second, but its not great either.

Wait for it.