Re: [Zenmap-Patch] Reducing Topology Noise

[Daniel Miller <bonsaiviking () gmail com>]

Jay,

Sorry it has taken so long for me to get to this. Overall, I like it. Here
are a few thoughts:

1. differing_length.xml doesn't seem to be different from old to new, and I
think it probably should be. Right now it looks like:

       / b -> X \
.. -> a          c -> ..
       \ X -> X /

but I think a better way would be:

       / b \
.. -> a     X -> c -> ..
       \ X /

Unfortunately, this means a more difficult reworking of your patch. As I
see it, your patch treats an anon hop as collapsible if it has the same
distance and the same "known ancestor" (hop with a real IP that is closer
than this anon hop). To do this sort of collapse, you'd have to also allow
hops with the same distance and the same "known descendant" (hop with a
real IP that is farther than this anon hop). I think you could do this by
entering the same node into invalid_node_cache under 2 different keys:
(pre_hop["ipaddr"], pre_hop_distance) and (post_hop["ipaddr"],
post_hop_distance), if that makes sense. Of course, be careful that
hop_split_at_different_real is not affected!

2. A minor cleanup thing: since you set key = (pre_hop["ipaddr"],
pre_hop_distance), key cannot be None, so take that condition out of the
following if statement. By the same token, the condition "if key is not
None and key in invalid_node_cache:" can never be false, since if "key not
in invalid_node_cache" then the previous block will put it into the cache.
Just make the node assignment an unconditional statement.

Other than that, the patch looks good. You may want to just commit the
patch as it is now (possibly with that small cleanup in 2) and then
consider my suggested change as a separate effort. Thanks!

Dan


On Wed, Jul 9, 2014 at 9:23 AM, Jay Bosamiya <jaybosamiya () gmail com> wrote:

> Hi All!
>
> I've attached an updated the patch with some modifications. Previously,
> the patch would lose info about number of hops since it worked like:
>        / X -> X \
> .. -> a          b -> ..  =>  .. -> a -> X -> b -> ..
>        \ X -> X /
> (where lowercase letters (i.e. a,b) are normal and X are anonymous)
>
> The new patch maintains the info about number of hops. The patch now
> works like:
>        / X -> X \
> .. -> a          b -> ..  =>  .. -> a -> X -> X -> b -> ..
>        \ X -> X /
>
> As for hop_split.xml, It now works as:
>        / X -> b -> ..                 / b -> ..
> .. -> a                => .. -> a -> X
>        \ X -> c -> ..                 \ c -> ..
>
> This change makes sense, since we keep all info about number of hops,
> but there is absolutely no way to distinguish between the two X's.
>
> As for the change I mentioned for anon_hops_at_known.xml in my previous
> mail, I think that this could be something for a future patch due to
> some complications that may arise.
>
> Feedback is welcome as always :)
>
> Note: I have added a few more test cases to the zip
> (hop_split_at_different_anon.xml, hop_split_at_different_real.xml and
> long_anon.xml) which make the changes even more obvious and should help
> review the patch better.
>
> Cheers,
> Jay
>
> On Saturday 21 June 2014 03:47 PM, Jay Bosamiya wrote:
> > Hi All!
> >
> > I've been working on reducing Zenmap's Topology view to reduce noise due
> > to anonymous hops.
> >
> > Basically what it does is this:
> >          / anon_1 \
> > ..-> ip_a          ip_b -> ..  => .. -> ip_a -> anon -> ip_b -> ..
> >          \ anon_2 /
> >
> > A big thanks to Anders Sundman for sending in a patch [1] that tried to
> > do this. Your patch helped a lot though it only solved part of the
> > problem (worked with only single anonymous hops in parallel).
> >
> > The current patch can also handle things like:
> >         / anon_1 -> anon_2 \
> > ..->ip_a                    ip_b->.. => .. ->ip_a -> anon -> ip_b->..
> >         \ anon_3 -> anon_4 /
> >
> > Attached is the patch. Also attached is a zip file containing XMLs to
> > test with (traceroutes with anonymous hops in different combinations).
> >
> >
> > There are a few cases that we need to think about, however, namely
> > "anon_hops_at_known.xml" or "hop_split.xml" (from the zip file attached).
> >
> >
> > For "anon_hops_at_know.xml", I think that the anonymous hop should be
> > removed completely (since 1.1.1.2 fits perfectly instead of the anon).
> > I think that it should work like:
> >          / anon_1 \
> > ..-> ip_a          ip_c -> ..  => .. -> ip_a -> ip_b -> ip_c -> ..
> >          \  ip_b  /
> >
> >
> > For "hop_split.xml", I am not sure what should be done. Currently, it
> > works like:
> >          / anon_1 -> ip_b -> ..
> > ..-> ip_a
> >          \ anon_2 -> ip_c -> ..
> >
> > but I think it would be better if it became:
> >                  / ip_b -> ..
> > ..-> ip_a -> anon
> >                  \ ip_c -> ..
> >
> >
> > I have not implemented the 2 changes since I wanted some feedback before
> > I did so.
> >
> > Cheers,
> > Jay
>
>
> _______________________________________________
> Sent through the dev mailing list
> http://nmap.org/mailman/listinfo/dev
> Archived at http://seclists.org/nmap-dev/
_______________________________________________
Sent through the dev mailing list
http://nmap.org/mailman/listinfo/dev
Archived at http://seclists.org/nmap-dev/