On Sun, 27 Mar 2011 21:54:13 +0000
Stefan Götz <email address hidden> wrote:

> > + * @return             true if rule was valid and output was
> > written,
> > + *                     false otherwise
> 
> [L] Again, when is a rule 'valid'?

Explaining the input format in detail would be reciting the
implementation, I think. It already states that rules set up by
hip_fw_manage_esp_rule() are covered, which is the important info.

> > + * @param now       We consider this the current time.
> > + * @return          Number of rules that were identified with an
> > esp tuple.
> > + *                  Not necessarily the number of tuples updated.
> > + *
> > + * @note This function doesn't clear the packet counters itself.
> 
> [M] Reading and zeroing the packet counters should be done atomically
> with a single command line invocation. Otherwise, a packet might
> arrive in between and the ESP rule would be deleted even though it
> was still active.

Good point. Didn't find a way to zero out individual rules though.

> > + *
> > + * @todo De-uglify this. You may be tempted to statically link in
> > libiptc,
> > + *       and I'd generally approve of it because while it was
> > never meant
> > + *       to be used publicly, quite some projects have relied on
> > it without
> > + *       burning their fingers too badly for a long time now. But
> > on the
> > + *       other hand, there's the impending nftables release that
> > will render
> > + *       all your hard work obsolete anyway. I'd rather suggest
> > waiting for
> > + *       a post-alpha release of libnl_nft before wasting your
> > time...
> > + *       --cmroz, oct 2010
> 
> [M] I find this confusing. First, the @todo says that this
> functionality should not be implemented as is but via libiptc
> instead. Then it says the implementation should stay the way it is.
> So is this really a @todo? Or is the @todo the update to libnl_nft?

The TODO applies to "de-uglify this", everything else is just a hint. Would you degrade it to a @note?

Alternatively, we could also discuss the issue right now and get rid of that paragraph altogether. Controlling iptables via shell may not be pretty, but using libiptc instead would blow up the code considerably in most cases, without real benefit. The only counterexample (that I know of), where libiptc could atually simplify matters, would be the ESP speedups introduced in this branch.

But pulling in a new lib because of that? Sounds like overkill.
Also, I bet there are many scripts out there that depend on the current output format, so the netfilter team will be careful not to change it unnecessarily.

Some months later, I'm not so sure anymore that iptables will be obsoleted in the near future.

> > + */
> > +static unsigned int detect_esp_rule_activity(const char *const cmd,
> > +                                             const time_t now)
> > +{
> > +
> > +    unsigned int ret      = 0;
> > +    bool         chain_ok = false;
> > +    char         bfr[256];
> > +    FILE        *p;
> > +
> > +    if (!(p = popen(cmd, "r"))) {
> > +        HIP_ERROR("popen(\"%s\"): %s\n", cmd, strerror(errno));
> > +        return 0;
> > +    }
> > +
> > +    while (fgets(bfr, sizeof(bfr), p)) {
> > +        if (strncmp(bfr, "Chain", 5) == 0) {
> > +            chain_ok = (strncmp(bfr, "Chain HIPFW-INPUT",   17) ==
> > 0) ||
> > +                       (strncmp(bfr, "Chain HIPFW-OUTPUT",  18) ==
> > 0) ||
> > +                       (strncmp(bfr, "Chain HIPFW-FORWARD", 19) == 
0);
> 
> [L] Would it make sense to let the 'iptables' command list only these
> three chains? That would help with the atomic zeroing of the packet
> counts, too.

That would involve three separate popen() calls, and the current code is no rocket science so I'd rather keep it.

> > === modified file 'firewall/firewall_defines.h'
> > --- firewall/firewall_defines.h	2011-03-22 16:37:54 +0000
> > +++ firewall/firewall_defines.h	2011-03-22 16:37:54 +0000
> > @@ -129,6 +131,7 @@
> >      int                direction;
> >      struct connection *connection;
> >      int                state;
> > +    int                hook;
> 
> [L] Even though this is not done for the other fields, it'd be great
> to get documentation on this new field. What it's purpose is, where
> it is used, etc.

Will do. Even though the "hook" actually names an iptables chain, I'll keep the name for consistency with other places in hipfw.

> > @@ -141,6 +144,8 @@
> >      int          verify_responder;
> >      int          state;
> >      time_t       timestamp;
> > +    /* members needed for iptables setup */
> > +    bool         udp_encap; /**< uses udp-nat? */
> 
> [L] To me as a developer who is not the author of this code, this
> piece of documentation is not very helpful. Please elaborate or
> remove it.

I'll try, but this will probably still make sense to those who know about this HIP extension only.

> > +{
> > +    char bfr[196];
> 
> [M] why 196?

Arbitrary value, I'll document it though.

> > +
> > +    va_list vargs;
> > +    va_start(vargs, command);
> > +
> > +    if (vsnprintf(bfr, sizeof(bfr), command, vargs) <= 0) {
> 
> [M] The return value of vsnprintf() must be checked to make sure that
> 'command' was not truncated due to insufficient buffer space in 'bfr'.
> Blindly executing a potentially truncated command sounds like an
> invitation to a pretty interesting bug party, especially since the
> firewall is running as root, I think.

You're right, thanks.

> [L] all these 'helper' changes could/should be committed separately
> as they are not specific to this branch.

But they would be unused then.