[Bro-Dev] #830: topic/tunnels

[Bro Tracker]

#830: topic/tunnels
----------------------------+------------------------
  Reporter:  jsiwek         |      Owner:  robin
      Type:  Merge Request  |     Status:  assigned
  Priority:  Normal         |  Milestone:  Bro2.1
 Component:  Bro            |    Version:  git/master
Resolution:                 |   Keywords:
----------------------------+------------------------

Comment (by robin):

 I was about to merge this into master and hence are moving the todos from
 the code to here (these are mostly cosmetically tasks that could be done
 after the merge as well). However, I did *not* merge because of a large
 problem, see next commit I'll add to the ticket.

 Current code in topic/robin/tunnels-merge.

     - The process of creating an EncapsulatingConn and feeding packet
       would benefit from a bit more refactoring. Code like this
       appears a few times (whereever a tunnel is found):

         {{{
             Encapsulation* outer = new Encapsulation(e);
             EncapsulatingConn ec(c, BifEnum::Tunnel::AYIYA);
             outer->Add(ec);

             sessions->DoNextInnerPacket(network_time(), 0, inner, outer);

             delete inner;
             delete outer;

         }}}

       Can we factor that out somehow? Perhaps move it all into the
       DoNextInnerPacketMethod() and rename that one?

     - event.bif: tunnel_changed(): I initially didn't understand the
       last sentence about the tunnel field. I now get it after reading
       the code. Not sure how to improve.

     - Sessions.cc: This is a bigger one.The ip_tunnels map needs to do
       state management to automatically discard old entries, like
       those not used for X hours. Can be done with a timer per index.

     - NetSessions::DoNextInnerPacket: Would it make sense use
       network_time in the non-hdr case?

     - TunnelEncapsulation: I find the class name Encapsulation
       misleading as it's really set chain of encapsulations. Rename to
       EncapsulationChain or EncapsulationStack?

     - Encapsulation::Encapsulation(const Encapsulation* other)

       I don't like the ptr constructor. When reading code using that,
       I can't tell what it does with the pointer (i.e., that it
       actually deep-copies the object). Can we use just the reference
       version? (I realize that means more "if ( not null )" at the
       caller end).

     - tunnel/main.bro: I suggest to make the expiration interval
 configurable.

     - tunnel/main.bro: Does the active tabel really need to be
 &synchronized?

     - tunnel/main.bro: tunnel_changed() event: there's something here I
 don't
           understand. Shouldn't c$tunnel already be registered? And
           what if a layer goes away, does that need to be removed
           here? Or is that done separately?  Also, conn/main.bro has a
           tunnel_changed handler at the same priority that *sets*
           c$tunnel.  That's seems undefined behaviour.

     - conn.log: "parents" lacks context, it's hard to say what it
       means if you doesn't already have tunnels in mind. Rename to
      "tunnel_parents" or "tunneled_in" or just "tunnels"?

-- 
Ticket URL: <http://tracker.bro-ids.org/bro/ticket/830#comment:9>
Bro Tracker <http://tracker.bro-ids.org/bro>
Bro Issue Tracker