Re: [PATCH 1/2] iptables: utils: Add bash completion

[Mart Frauenlob <mart.frauenlob@chello.at>]

On 02.03.2016 13:54, Pablo Neira Ayuso wrote:
> On Wed, Mar 02, 2016 at 01:24:01PM +0100, Mart Frauenlob wrote:
>> On 02.03.2016 12:34, Pablo Neira Ayuso wrote:
>>> On Thu, Feb 25, 2016 at 04:06:53PM +0100, Mart Frauenlob wrote:
>>>>   utils/iptables_bash_completion/README.md |  290 ++++
>>>>   utils/iptables_bash_completion/iptables  | 2426 ++++++++++++++++++++++++++++++
>>>
>>> This is fair good amount of work, but this is also quite a bit of
>>> new shell code to be maintained in our trees.
>>>
>>> Moreover, I was told that, in the specific case of debian, there is a
>>> package (bash-completion) where you place this.
>>>
>>> So sorry, I'm not applying this.

[...]

>> I think this is a good piece of work, probably the best featured bash
>> completion ever, and I'd wish to get it distributed.
>> My abilities in that matter are limited.
>>
>> Of course I'd keep fixing bugs and add new iptables features (though I think
>> there are not many to expect in the future, right?).
>> If someone submits a bug report, I'll take care of it if anyhow possible.
>> Just need to be notified by CC or so.
>
> Then, my suggestion is to simplify this script in order to reduce the
> maintainance burden.

Excuse me I'm unsure what maintenance burden you are expecting.
 From my point of view I don't see it.
Bug reports for this for sure have low priority.
Jozsef has my ipset completion in his tree for long time and afaik only 
one false bug report was sent (he just applied my patch to add it to build).
The script is well structured, it should not be hard to fix.
Adding new extensions or options of them is in most cases just an entry 
in the matches/targets array.
If an option requires input, then one entry at the option request 
section is needed:
elif [[ $prev = --new-option ]]; then return 0

And if completion on the value is possible:
elif [[ $prev = --new-option ]]; then complete_new_option_value ...

It's a little bit more work if an extension is protocol depending, but 
that's most likely just an entry in an extglob i.e. 
(tcp|upd|icmp|new_ext) in the get_match/target_opts() functions.
More complex are overlapping option names (hopefully that will never 
happen again in the future).

I can put more documentation into the source, to make it easier to follow.
As said I'll help on fixing bugs.

> One idea is to push into iptables some infrastructure so the script
> can inquire iptables on available options. This would be simple C code
> to be places on every extension to print the options. Then, add a tool
> like iptables-completion that you can use to inquire what is possible
> to get as options. Thus, we get a generic script that inquires
> iptables, instead of having them all hardcoded into the script.
>
> Would you explore this? I know Giuseppe Longo and Eric Leblond are
> looking into this for nft following a similar approach.

Maybe, see below.
But is it worth it? Correct me if I got the wrong impression.
nft will be/is the successor of iptables and most work in iptables will 
go into iptables-translate.
So why not just take the "classical" approach here and add that fancy 
featured completion and put the focus on the new tools?
I'm willing to help there (mainly on the shell side), if wanted.

Now for the new infrastructure:
Retrieving just the options is a fairly small part of my completion 
code. There are many other things I take into account. I try to list 
them all here (hopefully not missing one).
If any of these information is missing, I think it needs to be stored 
hard-coded.

- list of all extensions available - need to know if match or target
- table in which it is valid
- valid for ipv4 or ipv6 or both
- if extension is only valid for certain protocols
- does the core or extension option allow inversion
- which extension options are mandatory
- which core or extension options are mutually exclusive
- which extension options are only valid for certain protocols
- if out of a selection of extension options one is needed (i.e. recent 
match: --rcheck, --update, --remove)
- if an extension option is (in)valid depending on another of its 
options values (i.e. statistic match with --probability and --mode = random)
- core and extension options aliases (i.e. --destination-port, --dport)
- if it is valid to use a match or match option multiple times

Is all this info stored somewhere within iptables?

Also, the reply from he new iptables-completion tool may still need 
parsing, option and option alias de-duping (if not all will be done in C).
A parameter to show long or short or both form of options (I have an 
option in my completion that lets the user choose) would be needed.

So I think for it to be efficient in reducing shell code, I'd need to 
tell the new tool what information I have retrieved from the iptables 
command line. So it can based on that send back completion information. 
i.e. iptables -t filter -p tcp -m[TAB] - is what I have.
The optimal reply would be: all matches that are present on the system, 
are valid in the filter table and are valid for the tcp protocol.
If ! -p tcp is specified, I'd of course expect a different reply.

For non option completion I don't see much of a chance for code 
reduction. Unless for some exceptions like REJECT --reject-type (here 
the protocol is of interest), or -m icmp --icmp-type, or AUDIT --type 
xyz (in that case completion code is so tiny, an external program call 
is not worth it at all) ... where the extension knows very well about 
the expected values. If that kind of information (based on the 
parameters used) can be sent back, that could be useful.

Retrieving builtin chains and retrieving user defined chains would be 
nice to have. Saves the parsing of the output of iptables -S.
Another thing may be protocols/service names. As iptables knows the 
names, it may be possible to return them for completion, so not to parse 
the files from /etc/{services,protocols} with the shell.

If you think this can be done with affordable effort I'm willing to 
help. But mainly on the shell side, as I'm no C programmer. Maybe 
replicating code to other extensions, if someone does a template, is 
possible for me.

Best regards,

Mart