or_connection_t: replace real_addr with canonical_orport.

Instead of replacing connection_t.{addr,port} with a canonical
orport, and tracking the truth in real_addr, we now leave
connection_t.addr alone, and put the canonical address in
canonical_orport.

Closes #40042

Closes #33898

5 files changed, 69 insertions, 89 deletions

diff --git a/src/core/mainloop/connection.c b/src/core/mainloop/connection.c
index 90fd35b527..b3c5e6f51c 100644
--- a/src/core/mainloop/connection.c
+++ b/src/core/mainloop/connection.c
@@ -434,14 +434,6 @@ connection_describe_peer_internal(const connection_t *conn,
   } else if (conn->type == CONN_TYPE_OR) {
     /* For OR connections, we have a lot to do. */
     const or_connection_t *or_conn = CONST_TO_OR_CONN(conn);
-    /* we report 'real_addr' as the address we're talking with, if it's set.
-     *
-     * TODO: Eventually we should have 'addr' always mean the address on the
-     * internet, and have a separate 'canonical_addr' field.
-     */
-    if (!tor_addr_is_null(&or_conn->real_addr)) {
-      addr = &or_conn->real_addr;
-    }
     /* We report the IDs we're talking to... */
     if (fast_digest_is_zero(or_conn->identity_digest)) {
       // This could be a client, so scrub it.  No identity to report.
@@ -453,13 +445,17 @@ connection_describe_peer_internal(const connection_t *conn,
       tor_snprintf(extra_buf, sizeof(extra_buf),
                    " ID=%s", id_buf);
     }
-    if (! tor_addr_eq(addr, &conn->addr) && !scrub) {
+    if (! scrub && (! tor_addr_eq(addr, &or_conn->canonical_orport.addr) ||
+                    conn->port != or_conn->canonical_orport.port)) {
       /* We report canonical address, if it's different */
       char canonical_addr_buf[TOR_ADDR_BUF_LEN];
-      if (tor_addr_to_str(canonical_addr_buf, &conn->addr,
+      if (tor_addr_to_str(canonical_addr_buf, &or_conn->canonical_orport.addr,
                           sizeof(canonical_addr_buf), 1)) {
-        strlcat(extra_buf, " canonical_addr=", sizeof(extra_buf));
-        strlcat(extra_buf, canonical_addr_buf, sizeof(extra_buf));
+        tor_snprintf(extra_buf+strlen(extra_buf),
+                     sizeof(extra_buf)-strlen(extra_buf),
+                     " canonical_addr=%s:%"PRIu16,
+                     canonical_addr_buf,
+                     or_conn->canonical_orport.port);
       }
     }
   } else if (conn->type == CONN_TYPE_EXIT) {
@@ -570,6 +566,7 @@ or_connection_new(int type, int socket_family)
   tor_assert(type == CONN_TYPE_OR || type == CONN_TYPE_EXT_OR);
   connection_init(now, TO_CONN(or_conn), type, socket_family);
 
+  tor_addr_make_unspec(&or_conn->canonical_orport.addr);
   connection_or_set_canonical(or_conn, 0);
 
   if (type == CONN_TYPE_EXT_OR)
@@ -2252,16 +2249,7 @@ connection_connect_log_client_use_ip_version(const connection_t *conn)
                          ? fascist_firewall_prefer_ipv6_orport(options)
                          : fascist_firewall_prefer_ipv6_dirport(options));
   tor_addr_t real_addr;
-  tor_addr_make_null(&real_addr, AF_UNSPEC);
-
-  /* OR conns keep the original address in real_addr, as addr gets overwritten
-   * with the descriptor address */
-  if (conn->type == CONN_TYPE_OR) {
-    const or_connection_t *or_conn = CONST_TO_OR_CONN(conn);
-    tor_addr_copy(&real_addr, &or_conn->real_addr);
-  } else if (conn->type == CONN_TYPE_DIR) {
-    tor_addr_copy(&real_addr, &conn->addr);
-  }
+  tor_addr_copy(&real_addr, &conn->addr);
 
   /* Check if we broke a mandatory address family restriction */
   if ((must_ipv4 && tor_addr_family(&real_addr) == AF_INET6)
diff --git a/src/core/or/channeltls.c b/src/core/or/channeltls.c
index 90049a7e5c..7ec977be3c 100644
--- a/src/core/or/channeltls.c
+++ b/src/core/or/channeltls.c
@@ -548,14 +548,8 @@ channel_tls_get_remote_addr_method(const channel_t *chan,
     return 0;
   }
 
-  if (! tor_addr_is_null(&tlschan->conn->real_addr)) {
-    /* They want the real address, and real_addr is set. */
-    tor_addr_copy(addr_out, &(tlschan->conn->real_addr));
-  } else {
-    /* We'll have to give them the nominal address, which hopefully has
-    * not been overwritten yet. */
-    tor_addr_copy(addr_out, &TO_CONN(tlschan->conn)->addr);
-  }
+  /* They want the real address, so give it to them. */
+  tor_addr_copy(addr_out, &TO_CONN(tlschan->conn)->addr);
 
   return 1;
 }
@@ -692,6 +686,16 @@ channel_tls_matches_extend_info_method(channel_t *chan,
     return 0;
   }
 
+  const tor_addr_port_t *orport = &tlschan->conn->canonical_orport;
+  // If the canonical address is set, then we'll allow matches based on that.
+  if (! tor_addr_is_unspec(&orport->addr)) {
+    if (extend_info_has_orport(extend_info, &orport->addr, orport->port)) {
+      return 1;
+    }
+  }
+
+  // We also want to match if the true address and port are listed in the
+  // extend info.
   return extend_info_has_orport(extend_info,
                                 &TO_CONN(tlschan->conn)->addr,
                                 TO_CONN(tlschan->conn)->port);
@@ -722,8 +726,8 @@ channel_tls_matches_target_method(channel_t *chan,
     return 0;
   }
 
-  /* real_addr is the address this connection came from.
-   * base_.addr is updated by connection_or_init_conn_from_address()
+  /* addr is the address this connection came from.
+   * canonical_orport is updated by connection_or_init_conn_from_address()
    * to be the address in the descriptor. It may be tempting to
    * allow either address to be allowed, but if we did so, it would
    * enable someone who steals a relay's keys to covertly impersonate/MITM it
@@ -734,7 +738,7 @@ channel_tls_matches_target_method(channel_t *chan,
    * An adversary who has stolen a relay's keys could also post a fake relay
    * descriptor, but that attack is easier to detect.
    */
-  return tor_addr_eq(&(tlschan->conn->real_addr), target);
+  return tor_addr_eq(&TO_CONN(tlschan->conn)->addr, target);
 }
 
 /**
@@ -1883,7 +1887,7 @@ channel_tls_process_netinfo_cell(cell_t *cell, channel_tls_t *chan)
      * might be doing something funny, but nobody else is doing a MITM
      * on the relay's TCP.
      */
-    if (tor_addr_eq(&addr, &(chan->conn->real_addr))) {
+    if (tor_addr_eq(&addr, &TO_CONN(chan->conn)->addr)) {
       connection_or_set_canonical(chan->conn, 1);
       break;
     }
@@ -1921,7 +1925,7 @@ channel_tls_process_netinfo_cell(cell_t *cell, channel_tls_t *chan)
    * we were unable to resolve it previously. The endpoint address is passed
    * in order to make sure to never consider an address that is the same as
    * our endpoint. */
-  relay_address_new_suggestion(&my_apparent_addr, &chan->conn->real_addr,
+  relay_address_new_suggestion(&my_apparent_addr, &TO_CONN(chan->conn)->addr,
                                identity_digest);
 
   if (! chan->conn->handshake_state->sent_netinfo) {
diff --git a/src/core/or/connection_or.c b/src/core/or/connection_or.c
index 91dcbe695b..0795521be0 100644
--- a/src/core/or/connection_or.c
+++ b/src/core/or/connection_or.c
@@ -893,7 +893,9 @@ connection_or_init_conn_from_address(or_connection_t *conn,
 
   conn->base_.port = port;
   tor_addr_copy(&conn->base_.addr, addr);
-  tor_addr_copy(&conn->real_addr, addr);
+  if (! conn->base_.address) {
+    conn->base_.address = tor_strdup(fmt_addr(addr));
+  }
 
   connection_or_check_canonicity(conn, started_here);
 }
@@ -905,9 +907,10 @@ connection_or_init_conn_from_address(or_connection_t *conn,
 static void
 connection_or_check_canonicity(or_connection_t *conn, int started_here)
 {
+  (void) started_here;
+
   const char *id_digest = conn->identity_digest;
   const ed25519_public_key_t *ed_id = NULL;
-  const tor_addr_t *addr = &conn->real_addr;
   if (conn->chan)
     ed_id = & TLS_CHAN_TO_BASE(conn->chan)->ed25519_identity;
 
@@ -936,34 +939,17 @@ connection_or_check_canonicity(or_connection_t *conn, int started_here)
     } else {
       node_ap = &node_ipv6_ap;
     }
-    if (!started_here) {
-      /* Override the addr/port, so our log messages will make sense.
-       * This is dangerous, since if we ever try looking up a conn by
-       * its actual addr/port, we won't remember. Careful! */
-      /* XXXX arma: this is stupid, and it's the reason we need real_addr
-       * to track is_canonical properly.  What requires it? */
-      /* XXXX <arma> i believe the reason we did this, originally, is because
-       * we wanted to log what OR a connection was to, and if we logged the
-       * right IP address and port 56244, that wouldn't be as helpful. now we
-       * log the "right" port too, so we know if it's moria1 or moria2.
-       */
-      /* See #33898 for a ticket that resolves this technical debt. */
-      tor_addr_copy(&conn->base_.addr, &node_ap->addr);
-      conn->base_.port = node_ap->port;
-    }
+    /* Remember the canonical addr/port so our log messages will make
+       sense. */
+    tor_addr_port_copy(&conn->canonical_orport, node_ap);
     tor_free(conn->nickname);
     conn->nickname = tor_strdup(node_get_nickname(r));
-    tor_free(conn->base_.address);
-    conn->base_.address = tor_addr_to_str_dup(&node_ap->addr);
   } else {
     tor_free(conn->nickname);
     conn->nickname = tor_malloc(HEX_DIGEST_LEN+2);
     conn->nickname[0] = '$';
     base16_encode(conn->nickname+1, HEX_DIGEST_LEN+1,
                   conn->identity_digest, DIGEST_LEN);
-
-    tor_free(conn->base_.address);
-    conn->base_.address = tor_addr_to_str_dup(addr);
   }
 
   /*
@@ -1144,8 +1130,8 @@ connection_or_group_set_badness_(smartlist_t *group, int force)
                  (int)(now - or_conn->base_.timestamp_created),
                  best->base_.s, (int)(now - best->base_.timestamp_created));
         connection_or_mark_bad_for_new_circs(or_conn);
-      } else if (!tor_addr_compare(&or_conn->real_addr,
-                                   &best->real_addr, CMP_EXACT)) {
+      } else if (tor_addr_eq(&TO_CONN(or_conn)->addr,
+                             &TO_CONN(best)->addr)) {
         log_info(LD_OR,
                  "Marking %s unsuitable for new circuits: "
                  "(fd "TOR_SOCKET_T_FORMAT", %d secs old).  We have a better "
@@ -1275,7 +1261,7 @@ static or_connect_failure_entry_t *
 or_connect_failure_new(const or_connection_t *or_conn)
 {
   or_connect_failure_entry_t *ocf = tor_malloc_zero(sizeof(*ocf));
-  or_connect_failure_init(or_conn->identity_digest, &or_conn->real_addr,
+  or_connect_failure_init(or_conn->identity_digest, &TO_CONN(or_conn)->addr,
                           TO_CONN(or_conn)->port, ocf);
   return ocf;
 }
@@ -1653,8 +1639,8 @@ connection_tls_start_handshake,(or_connection_t *conn, int receiving))
     log_warn(LD_BUG,"tor_tls_new failed. Closing.");
     return -1;
   }
-  tor_tls_set_logged_address(conn->tls, // XXX client and relay?
-      escaped_safe_str(conn->base_.address));
+  tor_tls_set_logged_address(conn->tls,
+                             connection_describe_peer(TO_CONN(conn)));
 
   connection_start_reading(TO_CONN(conn));
   log_debug(LD_HANDSHAKE,"starting TLS handshake on fd "TOR_SOCKET_T_FORMAT,
@@ -1801,18 +1787,15 @@ connection_or_check_valid_tls_handshake(or_connection_t *conn,
   crypto_pk_t *identity_rcvd=NULL;
   const or_options_t *options = get_options();
   int severity = server_mode(options) ? LOG_PROTOCOL_WARN : LOG_WARN;
-  const char *safe_address =
-    started_here ? conn->base_.address :
-                   safe_str_client(conn->base_.address);
   const char *conn_type = started_here ? "outgoing" : "incoming";
   int has_cert = 0;
 
   check_no_tls_errors();
   has_cert = tor_tls_peer_has_cert(conn->tls);
   if (started_here && !has_cert) {
-    log_info(LD_HANDSHAKE,"Tried connecting to router at %s:%d, but it didn't "
+    log_info(LD_HANDSHAKE,"Tried connecting to router at %s, but it didn't "
              "send a cert! Closing.",
-             safe_address, conn->base_.port);
+             connection_describe_peer(TO_CONN(conn)));
     return -1;
   } else if (!has_cert) {
     log_debug(LD_HANDSHAKE,"Got incoming connection with no certificate. "
@@ -1824,9 +1807,9 @@ connection_or_check_valid_tls_handshake(or_connection_t *conn,
     int v = tor_tls_verify(started_here?severity:LOG_INFO,
                            conn->tls, &identity_rcvd);
     if (started_here && v<0) {
-      log_fn(severity,LD_HANDSHAKE,"Tried connecting to router at %s:%d: It"
+      log_fn(severity,LD_HANDSHAKE,"Tried connecting to router at %s: It"
              " has a cert but it's invalid. Closing.",
-             safe_address, conn->base_.port);
+             connection_describe_peer(TO_CONN(conn)));
         return -1;
     } else if (v<0) {
       log_info(LD_HANDSHAKE,"Incoming connection gave us an invalid cert "
@@ -1834,7 +1817,8 @@ connection_or_check_valid_tls_handshake(or_connection_t *conn,
     } else {
       log_debug(LD_HANDSHAKE,
                 "The certificate seems to be valid on %s connection "
-                "with %s:%d", conn_type, safe_address, conn->base_.port);
+                "with %s", conn_type,
+                connection_describe_peer(TO_CONN(conn)));
     }
     check_no_tls_errors();
   }
@@ -2027,9 +2011,14 @@ connection_or_client_learned_peer_id(or_connection_t *conn,
     /* If we learned an identity for this connection, then we might have
      * just discovered it to be canonical. */
     connection_or_check_canonicity(conn, conn->handshake_state->started_here);
+    if (conn->tls)
+      tor_tls_set_logged_address(conn->tls,
+                                 connection_describe_peer(TO_CONN(conn)));
   }
 
   if (authdir_mode_tests_reachability(options)) {
+    // We don't want to use canonical_orport here -- we want the address
+    // that we really used.
     dirserv_orconn_tls_done(&conn->base_.addr, conn->base_.port,
                             (const char*)rsa_peer_id, ed_peer_id);
   }
@@ -2507,11 +2496,9 @@ connection_or_send_netinfo,(or_connection_t *conn))
     netinfo_cell_set_timestamp(netinfo_cell, (uint32_t)now);
 
   /* Their address. */
-  const tor_addr_t *remote_tor_addr =
-    !tor_addr_is_null(&conn->real_addr) ? &conn->real_addr : &conn->base_.addr;
-  /* We use &conn->real_addr below, unless it hasn't yet been set. If it
-   * hasn't yet been set, we know that base_.addr hasn't been tampered with
-   * yet either. */
+  const tor_addr_t *remote_tor_addr = &TO_CONN(conn)->addr;
+  /* We can safely use TO_CONN(conn)->addr here, since we no longer replace
+   * it with a canonical address. */
   netinfo_addr_t *their_addr = netinfo_addr_from_tor_addr(remote_tor_addr);
 
   netinfo_cell_set_other_addr(netinfo_cell, their_addr);
diff --git a/src/core/or/dos.c b/src/core/or/dos.c
index 5f99280030..41bf303ffe 100644
--- a/src/core/or/dos.c
+++ b/src/core/or/dos.c
@@ -584,7 +584,7 @@ dos_geoip_entry_about_to_free(const clientmap_entry_t *geoip_ent)
   SMARTLIST_FOREACH_BEGIN(get_connection_array(), connection_t *, conn) {
     if (conn->type == CONN_TYPE_OR) {
       or_connection_t *or_conn = TO_OR_CONN(conn);
-      if (!tor_addr_compare(&geoip_ent->addr, &or_conn->real_addr,
+      if (!tor_addr_compare(&geoip_ent->addr, &TO_CONN(or_conn)->addr,
                             CMP_EXACT)) {
         or_conn->tracked_for_dos_mitigation = 0;
       }
@@ -696,12 +696,12 @@ dos_new_client_conn(or_connection_t *or_conn, const char *transport_name)
    * reason to do so is because network reentry is possible where a client
    * connection comes from an Exit node. Even when we'll fix reentry, this is
    * a robust defense to keep in place. */
-  if (nodelist_probably_contains_address(&or_conn->real_addr)) {
+  if (nodelist_probably_contains_address(&TO_CONN(or_conn)->addr)) {
     goto end;
   }
 
   /* We are only interested in client connection from the geoip cache. */
-  entry = geoip_lookup_client(&or_conn->real_addr, transport_name,
+  entry = geoip_lookup_client(&TO_CONN(or_conn)->addr, transport_name,
                               GEOIP_CLIENT_CONNECT);
   if (BUG(entry == NULL)) {
     /* Should never happen because we note down the address in the geoip
@@ -712,7 +712,7 @@ dos_new_client_conn(or_connection_t *or_conn, const char *transport_name)
   entry->dos_stats.concurrent_count++;
   or_conn->tracked_for_dos_mitigation = 1;
   log_debug(LD_DOS, "Client address %s has now %u concurrent connections.",
-            fmt_addr(&or_conn->real_addr),
+            fmt_addr(&TO_CONN(or_conn)->addr),
             entry->dos_stats.concurrent_count);
 
  end:
@@ -735,7 +735,7 @@ dos_close_client_conn(const or_connection_t *or_conn)
   }
 
   /* We are only interested in client connection from the geoip cache. */
-  entry = geoip_lookup_client(&or_conn->real_addr, NULL,
+  entry = geoip_lookup_client(&TO_CONN(or_conn)->addr, NULL,
                               GEOIP_CLIENT_CONNECT);
   if (entry == NULL) {
     /* This can happen because we can close a connection before the channel
@@ -753,7 +753,7 @@ dos_close_client_conn(const or_connection_t *or_conn)
   entry->dos_stats.concurrent_count--;
   log_debug(LD_DOS, "Client address %s has lost a connection. Concurrent "
                     "connections are now at %u",
-            fmt_addr(&or_conn->real_addr),
+            fmt_addr(&TO_CONN(or_conn)->addr),
             entry->dos_stats.concurrent_count);
 
  end:
diff --git a/src/core/or/or_connection_st.h b/src/core/or/or_connection_st.h
index 2507f90803..8e012a6b85 100644
--- a/src/core/or/or_connection_st.h
+++ b/src/core/or/or_connection_st.h
@@ -50,17 +50,18 @@ struct or_connection_t {
   channel_tls_t *chan;
 
   /**
-   * The actual address (as modified by any proxies) that this connection
-   * came from or went to.  (See connection_t.addr for caveats.)
+   * The "canonical" address and port for this relay's ORPort, if this is
+   * a known relay.
    *
-   * TECHNICAL DEBT:
+   * An ORPort is "canonical" in this sense only if it is the same ORPort
+   * that is listed for this identity in the consensus we have.
    *
-   * This field shouldn't really exist.  We need it because our code
-   * overwrites conenction_t.addr with the "canonical address" of the OR we
-   * are talking to, taken from the descriptor of the authenticated OR.
-   * That's a bad choice.
+   * This field may be set on outbound connections for _any_ relay, and on
+   * inbound connections after authentication.  If we don't know the relay's
+   * identity, or if we don't have the relay's identity in our consensus, we
+   * leave this address as UNSPEC.
    **/
-  tor_addr_t real_addr;
+  tor_addr_port_t canonical_orport;
 
   /** Should this connection be used for extending circuits to the server
    * matching the <b>identity_digest</b> field?  Set to true if we're pretty