Re: [tor-bugs] #33898 [Core Tor/Tor]: Stop modifying addr on connections, and delete real_addr

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#33898: Stop modifying addr on connections, and delete real_addr
-------------------------------------------+-------------------------------
 Reporter:  teor                           |          Owner:  nickm
     Type:  defect                         |         Status:  assigned
 Priority:  High                           |      Milestone:  Tor:
                                           |  0.4.4.x-final
Component:  Core Tor/Tor                   |        Version:
 Severity:  Normal                         |     Resolution:
 Keywords:  ipv6, technical-debt, prop311  |  Actual Points:
Parent ID:  #33048                         |         Points:  1
 Reviewer:                                 |        Sponsor:  Sponsor55-can
-------------------------------------------+-------------------------------

Old description:

> In connection_or_check_canonicity(), we overwrite conn.addr with the
> canonical address of the relay in the consensus. That makes accurate
> logging impossible.
>
> And so we also need channel.real_addr, to store the actual address of the
> remote peer. And for some reason, we also have conn.address, a string
> copy of the peer's canonical address and port.
>
> This is... a mess.
>
> Here's the high-level interface I'd like to see:
> * use a function to format a connection or channel addresses for loogging
> * use exactly as many address and port variables as needed in connection
> and channel (no extras!)
> * qualify each address and port variable's name with its purpose
>
> For example, here's one possible design:
> * delete addr, port, address, and real_addr
> * add remote_ap, a tor_addr_port_t that is the remote address and port of
> the TCP connection to the remote peer
> * implement connection_describe(), which:
>   * formats remote_ap,
>   * formats is_canonical (and any other useful info), and
>   * calls node_describe() to format the canonical IPv4 and IPv6 addresses
> and ports of the remote peer.
>
> We may also need separate fields for reverse proxied addresses, see the
> comment at:
> https://github.com/torproject/tor/blob/7517e1b5d31aada1f594c2594737a231d9d8e116/src/core/or/channeltls.c#L1339
>
> If we need separate variables or functions for channels, we can use a
> similar design. (But ideally, re-using as many functions and variables as
> possible.)
>
> This is important for Sponsor 55: getting accurate connection information
> will make diagnosing bugs much easier.

New description:

 In connection_or_check_canonicity(), we overwrite conn.addr with the
 canonical address of the relay in the consensus. That makes accurate
 logging impossible.

 And so we also need channel.real_addr, to store the actual address of the
 remote peer. And for some reason, we also have conn.address, a string copy
 of the peer's canonical address and port.

 See:
 https://github.com/torproject/tor/blob/7f9eaec538b7d01e0d1b130dc4cf2ec634252d46/src/core/or/connection_or.c#L920

 This is... a mess.

 Here's the high-level interface I'd like to see:
 * use a function to format a connection or channel addresses for loogging
 * use exactly as many address and port variables as needed in connection
 and channel (no extras!)
 * qualify each address and port variable's name with its purpose

 For example, here's one possible design:
 * delete addr, port, address, and real_addr
 * add remote_ap, a tor_addr_port_t that is the remote address and port of
 the TCP connection to the remote peer
 * implement connection_describe(), which:
   * formats remote_ap,
   * formats is_canonical (and any other useful info), and
   * calls node_describe() to format the canonical IPv4 and IPv6 addresses
 and ports of the remote peer.

 We may also need separate fields for reverse proxied addresses, see the
 comment at:
 https://github.com/torproject/tor/blob/7517e1b5d31aada1f594c2594737a231d9d8e116/src/core/or/channeltls.c#L1339

 If we need separate variables or functions for channels, we can use a
 similar design. (But ideally, re-using as many functions and variables as
 possible.)

 This is important for Sponsor 55: getting accurate connection information
 will make diagnosing bugs much easier.

--

Comment (by teor):

 Add link to a comment that explains why we have real_addr:
 https://github.com/torproject/tor/blob/7f9eaec538b7d01e0d1b130dc4cf2ec634252d46/src/core/or/connection_or.c#L920

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

_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs