Code review comment for lp:~midauth-pisa-devs/hipl/midauth-firewall

On Tue, Oct 25, 2011 at 02:28:06PM +0200, René Hummen wrote:
> On 20.10.2011, at 14:15, Diego Biurrun wrote:
> > On Wed, Oct 19, 2011 at 01:17:30PM +0000, René Hummen wrote:
> >
> >> --- Makefile.am 2011-10-17 18:14:10 +0000
> >> +++ Makefile.am 2011-10-19 13:13:48 +0000
> >> @@ -253,7 +256,7 @@
> >>
> >> -test_check_firewall_LDFLAGS = -ldl
> >> +test_check_firewall_LDFLAGS = -ldl -z muldefs
> >
> > We have solved this differently elsewhere by #including the .c file to
> > unit-test static functions.
>
> This has been discussed previously on the list:
> http://www.freelists.org/post/hipl-dev/Mock-functions-for-hipl-unit-tests

But we currently don't favor that solution. If that needs changing,
it's a topic for a separate merge request and would need to be changed
in all places.

> >> @@ -1526,14 +1565,28 @@
> >>
> >> } else if (esp_info && seq && ack) {
> >> - if (!tuple) {
> >> + if (tuple && tuple->direction == ORIGINAL_DIR) {
> >> + other_dir = &tuple->connection->reply;
> >> + } else if (tuple) {
> >> + other_dir = &tuple->connection->original;
> >> + } else {
> >> HIP_ERROR("Insufficient stored state to process UPDATE\n");
> >> return 0;
> >> }
> >
> > tuple is checked twice.
>
> Yes, but I need to be sure that tuple exists when accessing its
> members in the body of the else if.

So? That does not imply you need to check "tuple" twice, just nest the
ifs, witness:

  if (tuple) {
      if (tuple->direction == ORIGINAL_DIR) {
          other_dir = &tuple->connection->reply;
      } else {
          other_dir = &tuple->connection->original;
  } else {
     HIP_ERROR("Insufficient stored state to process UPDATE\n");
     return 0;
 }

> >> - if (!hip_fw_verify_packet(common, tuple)) {
> >> + if (!hipfw_midauth_verify_challenge(ctx, other_dir->midauth_nonce)) {
> >> + HIP_ERROR("failed to verify midauth challenge\n");
> >> + return 0;
> >> + }
> >
> > 0 is returned in case of error?
>
> Yes, firewall/conntrack.c does not use the same semantics for return
> values compared to the rest of the code.

Then fix conntrack.c and don't make the code consistently inconsistent,
cf. the boyscout rule.

> >> @@ -1543,14 +1596,29 @@
> >> } else if (ack) {
> >> - if (!tuple) {
> >> + if (tuple && tuple->direction == ORIGINAL_DIR) {
> >> + other_dir = &tuple->connection->reply;
> >> + } else if (tuple) {
> >> + other_dir = &tuple->connection->original;
> >> + } else {
> >> HIP_ERROR("Insufficient stored state to process UPDATE\n");
> >> return 0;
> >> }
> >
> > another redundant check
> >
> > Oh, this code seems to be completely duplicated.
>
> Yes, but I didn't see the need for a new function as this code is
> specific for updates and only used twice.

There is no such thing as "only" used twice, code duplication
is code duplication is code duplication :)

> >> --- firewall/firewall.c 2011-08-16 07:49:25 +0000
> >> +++ firewall/firewall.c 2011-10-19 13:13:48 +0000
> >> @@ -76,12 +76,13 @@
> >> #include "lib/core/prefix.h"
> >> #include "lib/core/util.h"
> >> #include "hipd/hipd.h"
> >> -#include "config.h"
> >> #include "cache.h"
> >> #include "common_types.h"
> >> +#include "config.h"
> >> #include "conntrack.h"
> >> #include "esp_prot_api.h"
> >> #include "esp_prot_conntrack.h"
> >> +#include "firewall.h"
> >> #include "firewall_control.h"
> >> #include "firewall_defines.h"
> >> #include "helpers.h"
> >> @@ -90,9 +91,9 @@
> >> #include "pisa.h"
> >> #include "port_bindings.h"
> >> #include "reinject.h"
> >> +#include "rewrite.h"
> >> #include "rule_management.h"
> >> #include "user_ipsec_api.h"
> >> -#include "firewall.h"
> >
> > The ordering is actually done on purpose. config.h is not in the same
> > directory, same as the other #includes with directory prefixes and
> > firewall.h directly "belongs" to firewall.c, so it made sense at the
> > time to put it last and list all dependencies before. I have this
> > feeling in the back of my head that the latter solved some real issue.
>
> 'make alltests' runs successfully. So, what exactly is your suggestion?

My suggestion is not to change the #include ordering.

> >> @@ -74,6 +81,8 @@
> >> int modified;
> >> };
> >>
> >> +#include "midauth.h"
> >
> > Why the peculiar placement of this #include?
>
> There is a cyclic redundancy between firewall/midauth.h (requires
> definition of struct hip_fw_context) and firewall/firewall_defines.h
> (requires definition of MIDAUTH_DEFAULT_NONCE_LENGTH). This helps
> mitigating the problem.

So fix the cyclic redundancy, don't add more workarounds.

> >> +int hipfw_midauth_verify_challenge(const struct hip_fw_context *const ctx,
> >> + const uint8_t midauth_nonce[MIDAUTH_DEFAULT_NONCE_LENGTH])
> >
> > missing doxy
>
> Hmm...I can see the doxygen here:
> http://bazaar.launchpad.net/~midauth-pisa-devs/hipl/midauth-firewall/view/head:/firewall/midauth.c#L220

Doh, that must have been a brainfart from my side.

> >> + if (!found) {
> >> + HIP_ERROR("CHALLENGE_RESPONSE found, but no matching nonce\n");
> >> + return ignore_missing_challenge_response ? 1 : 0;
> >> + }
> >> +
> >> + return found ? 1 : 0;
> >
> > You just checked found, no need to do it again.
>
> I guess that this has been done to ensure correct type conversion from
> bool to int.

Read the code again, it's nonsense, "found" is initialized once and
never set again.

Diego

P.S.: You should keep launchpad in CC.