Re: ncat - UNIX-domain sockets support

[Tomas Hozza <thozza () redhat com>]

Hi,

After 11 days there is so far no response to my Ncat UNIX domain
sockets implementation. I would like to ask anybody to have a look
on my code and review it.

Nevertheless I would like to ask if the UNIX domain sockets
functionality can be included in next Ncat release, or if there
are any other things regarding the implementation that need to be
solved.

Thank you.

Regards,
Tomas Hozza


----- Original Message -----
> Hi David.
>
> Thank you very much for reviewing my patches. I did some refactoring
> and added/changed thinks you've requested.
>
> > This code looks quite solid, thanks for it. I have two requests for
> > this
> > patch:
> >  1. Reduce code duplication in ncat_listen.c.
>
> I removed duplicated sections of code and used existing functions
> ncat_listen_stream and ncat_listen_dgram.
>
> >  2. Add tests for local sockets in test/ncat-test.pl.
>
> I added three tests for UNIX domain sockets implementation in Ncat.
>
> > nsock_connect_unixsock_internal should be static. Also, it looks
> > like
> > more common code can be moved into nsock_connect_internal.
>
> I removed the nsock_connect_unixsock_internal and one other function
> for
> making sockets and moved more common code to the existing functions.
>  
> > ncat_listen_unixsock_stream and ncat_listen_unixsock_dgram look
> > like
> > mostly copy-pastes from ncat_listen_stream and ncat_listen_dgram.
> > We
> > can't accept that for reasons of maintenance. These need to be
> > integrated in the existing loops; this may mean building new
> > supporting
> > abstractions.
>
> They are gone! I used existing functions.
>
> > Is there any reason not to handle both SOCK_STREAM and SOCK_DGRAM
> > Unix
> > domain sockets in the same loop? I don't know if the same reasoning
> > applies as for separating ncat_listen_stream and ncat_listen_dgram.
> > (I'm
> > not sure why ncat_listen_stream and ncat_listen_dgram are separate
> > either.)
>
> As stated earlier, I removed ncat_listen_stream and
> ncat_listen_dgram.
> SOCK_STREAM and SOCK_DGRAM sockets cannot be handled in the same
> loop,
> because STREAM sockets are with connection while the DGRAM sockets
> are
> connectionless.
>
> > > -      nsock_trace(ms, "Callback: %s %s %sfor EID %li [%s:%d]",
> > > -                  nse_type2str(nse->type),
> > > nse_status2str(nse->status), errstr,
> > > -                  nse->id, inet_ntop_ez(&nsi->peer,
> > > nsi->peerlen),
> > > nsi_peerport(nsi));
> > > +#if HAVE_SYS_UN_H
> > > +      if (nsi->peer.ss_family == AF_UNIX)
> > > +        nsock_trace(ms, "Callback: %s %s %sfor EID %li [%s]",
> > > +                    nse_type2str(nse->type),
> > > nse_status2str(nse->status), errstr,
> > > +                    nse->id, get_unixsock_path(&nsi->peer));
> > > +      else
> > > +#endif
> > > +        nsock_trace(ms, "Callback: %s %s %sfor EID %li [%s:%d]",
> > > +                    nse_type2str(nse->type),
> > > nse_status2str(nse->status), errstr,
> > > +                    nse->id, inet_ntop_ez(&nsi->peer,
> > > nsi->peerlen), nsi_peerport(nsi));
> >
> > Here and in many other places it looks like you need an abstraction
> > around inet_ntop_ez and get_unixsock_path rather than duplicating
> > the
> > nsock_trace call.
>
> I moved the abstraction to new get_hostaddr_string() function.
>
> > > +        if (o.chat)
> > > +            bye("Chat option not supported when using
> > > UNIX-domain
> > > sockets.");
> > > +#ifdef HAVE_OPENSSL
> > > +        if (o.ssl)
> > > +            bye("SSL option not supported when using UNIX-domain
> > > sockets.");
> > > +#endif
> > > +        if (o.nodns)
> > > +            bye("Not using DNS option with UNIX-domain sockets
> > > doesn't make sense.");
> > > +        if (o.broker)
> > > +            bye("Connection brokering not supported when using
> > > UNIX-domain sockets.");
> >
> > Do all these things intrinsically not work with Unix domain
> > sockets?
> > Or
> > is it just a side effect of not sharing the same loop. I see no
> > reason
> > to forbid o.nodns: that option may have effects beyond just looking
> > up a
> > connect target.
>
> Well, to be honest I'm not sure if they won't work with UNIX sockets.
> I removed some of them and included only ones I think they won't
> work.
>
> It would be great if you (David) or other people would have a look on
> my new patches and give me some feedback.
>
> Thanks!
>
> Regards,
> Tomas Hozza
_______________________________________________
Sent through the nmap-dev mailing list
http://cgi.insecure.org/mailman/listinfo/nmap-dev
Archived at http://seclists.org/nmap-dev/