Opened 6 months ago

Last modified 5 months ago

#33898 assigned defect

Stop modifying addr on connections, and delete real_addr

Reported by: teor Owned by: nickm
Priority: High Milestone: Tor: 0.4.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: ipv6, technical-debt, prop311 044-can
Cc: Actual Points:
Parent ID: #33048 Points: 1
Reviewer: Sponsor: Sponsor55-can

Description (last modified by teor)

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.

Child Tickets

Change History (14)

comment:1 Changed 6 months ago by teor

Of all the tickets I've assigned to you so far, this is the most urgent and important one. It could save us all a lot of confusion (and debugging time) in future.

It would be really useful to have it done in the next few weeks, before we ask relay operators to test IPv6 reachability self-checks.

comment:2 Changed 6 months ago by teor

#33899 contains a tweak to the existing code, to consider IPv6 connections canonical. It also replaces the addr with the canonical IPv6 address, rather than an IPv4 address.

comment:3 Changed 6 months ago by teor

In #33899, nickm says:

I'm inclined to rename node_ap in connection_or_check_canonicity to something like canonical_ap, but that can wait till #33898.

comment:4 Changed 6 months ago by teor

Description: modified (diff)

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

comment:5 Changed 6 months ago by teor

Description: modified (diff)

comment:6 Changed 6 months ago by arma

Yeah, I can try to explain how we got here, and then folks can decide if they have a better place we can get to.

When you receive a connection from a relay, it never comes from the relay's IP address and ORPort. At best, it comes from the relay's IP address and some high-numbered port. So if you rely on the address and port of the incoming connection to be able to learn which relay it is, there will at best be ambiguity in the cases where an IP address runs more than one relay ("which one is the one that connected to you?"), and at worst the connection came from a different IP address than is listed in the relay's descriptor, for example because the other side sets OutboundBindAddress, or because they *don't* set it but their default route goes out through a different IP address, or because you do some port forwarding thing on your side so it looks to you like connections come from your forwarder.

Also there's the (hopefully less common) case where somebody is trying to do a person-in-the-middle attack where they ask the other side to extend to your identity but their address, and then they forward the connection to you. Or they ask you to connect to some remote relay but at a nonstandard address and port, and then they redirect that connection to the relay.

Ok. Given that context, here are the rules we followed back when I wrote this part:

  • If the connection is to (or from) a known relay, then addr and port will tell you which relay it is.
  • Else (not to/from a known relay), addr and port will be whatever you tried to connect to, or whatever TCP told you for the incoming connection.
  • real_addr will always be whatever-you-tried-to-connect-to or whatever-TCP-told-you. In the case where we overwrote addr and port because it's a known relay, there is no concept of real_port, i.e. that information is discarded.
  • address will always be a string version of addr. We keep it entirely so we don't have to keep recreating it every time we want to write addr somewhere.

For extra context, the above conventions predate the "canonical" flag, and also predate the DoS subsystem (which rightly looks at real_addr rather than addr).

I agree that these rules start to get ugly once relays have multiple canonical addresses.

comment:7 Changed 6 months ago by teor

Part of the problem here is a naming issue. It's not clear what "real" means, particularly when the other address fields doesn't have any qualifiers.

And it's even less clear when we should be using each address field.

One quick win here, would be to give each of the existing fields a distinct, qualified name. The name should clearly describe its purpose or contents.

I agree that these rules start to get ugly once relays have multiple canonical addresses.

I don't think it's as bad as you think.

Here are some general principles:

  • Each connection is either IPv4 or IPv6. So any substitute address should be from the corresponding family.
  • Any logs should contain addresses from the relevant family, or ideally, all available addresses.
  • If an address is only intended for logging, it can be in any convenient format. For example, "IPv4:port and [IPv6]:port". If we use an unparseable format like this, it will be easier to see coding bugs that use the wrong address.

comment:8 Changed 6 months ago by nickm

Here is some more messy stuff to remember: the "address" and "addr" fields have a different uses on other connections types. For exit connections, addr is used to hold the target address for a while, I think.

I think my first step here is to go through literally all the code that touches these fields.

comment:9 Changed 6 months ago by nickm

Okay, I went through all the code that sets or uses addr, real_addr, or address.

It is a labyrinth! These fields are used and set in subtly different ways depending on the connection.

For most exit connections, the "address" starts out as what we were asked to connect to, and then later the "addr" field is made into whatever we got from DNS lookup. The "address" field is used as a key for the DNS cache too.

For linked exit connections, the "addr" appears to be taken from the address of the channel, so it can be copied into the linked dirconn.

For hidden service exit connections, the "addr" is set to the local port that we're trying to connect to.

For incoming connectnions, addr is initialized from the real remote sockaddr. Unless it's on a unix socket.

For listeners, addr is set to the address we're listening on, and address is used for unix sockets to reconnect to that address if we need to rebind later on.

For OR connections, addr is changed when we make an outgoing connection in connection_or_init_conn_from_address -- we might be using a different address than we decided initially to connect to if we have a descriptor for a node. Addr is also set by connection_or_check_canonicity when we are receiving an incoming connection.

For directory connections, if there is a Forwarded-For header, its contents replace the address field in http_set_address_origin(). This is used to send X-Your-Address-Is, and to annotate descriptors.

For entry connections, "address" and "addr" are address that _made_ the request. This also goes for UDP dns queries and controller resolve requests.

comment:10 Changed 6 months ago by nickm

So what do we do at this point? I think we need a long term plan for these fields and a plan for the OR fields specifically.

As a long-term plan, I think we should move towards disabling "addr" as meaning anything other than "the network address we are connected to, possibly through a local proxy". If we retain "address", it should be display-only. Possibly we should rename these fields to avoid confusion.

We should have a "display peer for logging" function that we use instead of addr/port/address in log messages.

We'll need additional fields for the following purposes:

  • Canonical address of peer, to use in OR connections.
  • Requested address and resolved address, to use in exit connections.
  • Address to re-bind to, for listeners.
  • Best guess at peer's real internet address, for tunneled directory connections, to be used for X-Your-Address-Is.


So what parts of this do we do now?

First I should solicit comment on this plan. Then, I should open a ticket or three about this. Then, we should document all of this in the code, and create the "describe this peer" function. Then finally we should adjust OR connections so that they use the fields in the intended manner.

comment:11 Changed 6 months ago by dgoulet

Sounds legit to have the strategy of pushing down the address for the common use case in let say connection_t (if any). Then, specialize as we go up the hierarchy tree.

To try to comment on the "what part we do now", maybe identifying if the low level connection_t could have an address field that is common to all other objects. If yes, do that change, if not, specialize the "address" concept in each higher level objects?

comment:12 in reply to:  10 Changed 6 months ago by teor

Replying to nickm:

So what do we do at this point? I think we need a long term plan for these fields and a plan for the OR fields specifically.

As a long-term plan, I think we should move towards disabling "addr" as meaning anything other than "the network address we are connected to, possibly through a local proxy". If we retain "address", it should be display-only. Possibly we should rename these fields to avoid confusion.

I think renaming is a good idea: a lot of the confusion with these fields comes from their generic names.

We should have a "display peer for logging" function that we use instead of addr/port/address in log messages.

+1

We'll need additional fields for the following purposes:

  • Canonical address of peer, to use in OR connections.
  • Requested address and resolved address, to use in exit connections.
  • Address to re-bind to, for listeners.
  • Best guess at peer's real internet address, for tunneled directory connections, to be used for X-Your-Address-Is.


So what parts of this do we do now?

First I should solicit comment on this plan. Then, I should open a ticket or three about this. Then, we should document all of this in the code, and create the "describe this peer" function. Then finally we should adjust OR connections so that they use the fields in the intended manner.

I think it's important to do the parts related to the Sponsor 55 work, to avoid subtle bugs. But it's not necessarily required work, so we should be conscious of the amount of time in the grant.

Here are the related parts of your list above, and the relevant changes and proposals:

  • Canonical address of peer, to use in OR connections.

Used to perform IPv6 extends in proposal 311.

  • Address to re-bind to, for listeners.

Used to automatically open IPv6 ORPorts in proposal 312.

  • Best guess at peer's real internet address, for tunneled directory connections, to be used for X-Your-Address-Is.

Optionally used to detect IPv6 addresses in proposal 312. This change might be out of scope because:

  • we don't guess our own IPv6 address using IPv6 addresses from remote relays, or
  • we use the addresses in NETINFO cells instead.

comment:13 Changed 5 months ago by nickm

Keywords: 044-can added

comment:14 Changed 5 months ago by nickm

Milestone: Tor: 0.4.4.x-finalTor: 0.4.5.x-final
Note: See TracTickets for help on using tickets.