[tor-bugs] #5083 [Obfsproxy]: Implement heartbeat message in obfsproxy

[Tor Bug Tracker & Wiki]

#5083: Implement heartbeat message in obfsproxy
-------------------------+--------------------------------------------------
 Reporter:  karsten      |          Owner:  asn         
     Type:  enhancement  |         Status:  needs_review
 Priority:  normal       |      Milestone:              
Component:  Obfsproxy    |        Version:              
 Keywords:               |         Parent:              
   Points:               |   Actualpoints:              
-------------------------+--------------------------------------------------

Comment(by karsten):

 Replying to [comment:8 asn]:
 > Replying to [comment:7 karsten]:
 > > Replying to [comment:6 asn]:
 > > > c) Are unique IPs and connections information useful/meaningful in
 the case of obfsproxy clients?
 > >
 > > They're the most basic statistics that we have about obfsproxy usage.
 I'd prefer having a GeoIP database in obfsproxy and resolve both
 connections and unique addresses to country codes.  But that requires more
 coding, reviewing, and time that we don't have.
 > >
 >
 > Hm, when I said 'obfsproxy clients', I meant 'obfsproxy configured as a
 client'. We don't seem to count outgoing connections when we are in SOCKS
 mode (since we use `open_outbound_hostname()` there), so an obfsproxy
 configured with:
 > {{{
 > obfsproxy obfs2 socks 127.0.0.1:9999
 > }}}
 > will probably say "we saw 0 connections from 0 unique addresses." all
 the time.

 You're right.  We shouldn't say that as a client.

 > I think, we should only call `status_note_connection()` if we are a
 server, and we should only mention connection information in the heartbeat
 message if we are a server.
 >
 > Karsten, if you cba to do this, I think me or Nick can handle it.

 To be honest, I don't know where in obfsproxy I'd learn whether we're a
 client or a server.  If you don't mind too much, would you fix that?  Your
 suggestion to only call status_note_connection() as a server and not
 mention connection information as a client sounds perfectly fine to me.

 > BTW, there is something fishy with the address copying in
 `status_note_connection()` it seems like you are using `strcpy()` and try
 to copy the whole `addrport` which is bigger than `p - addrport` bytes
 since it also contains the TCP port.
 > Maybe something like this:
 > {{{
 > delim = strchr(s, ':');
 > if (!delim)
 >    return; /* bug of conn->peername? */
 > tor_assert(delim >= s);
 > addr = xstrndup(s, delim-s);
 > }}}
 > could work.
 >
 > If you are bored, Nick or me can probably handle this too!

 Right, the fishy address copying was indeed fishy.  That should now be
 fixed in the new status_note_connection().

 Thanks again for the very helpful review!

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/5083#comment:10>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online