hs: Onion services put IPv6 addresses in service descriptors

Rewrite service_intro_point_new() to take a node_t. Since
node_get_link_specifier_smartlist() supports IPv6 link specifiers,
this refactor adds IPv6 addresses to onion service descriptors.

Part of 23576, implements 26992.

changes/bug23576

@@ -0,0 +1,7 @@
+  o Minor features (IPv6, v3 onion services):
+    - Make v3 onion services put IPv6 addresses in service
+      descriptors. Before this change, service descriptors only
+      contained IPv4 addressesd. Implements 26992.
+  o Code simplification and refactoring:
+    - Simplify v3 onion service link specifier handling code.
+      Fixes bug 23576; bugfix on 0.3.2.1-alpha.

src/feature/hs/hs_common.c

@@ -1697,6 +1697,12 @@ hs_get_extend_info_from_lspecs(const smartlist_t *lspecs,
 
   tor_assert(lspecs);
 
+  if (smartlist_len(lspecs) == 0) {
+    log_fn(LOG_PROTOCOL_WARN, LD_REND, "Empty link specifier list.");
+    /* Return NULL. */
+    goto done;
+  }
+
   SMARTLIST_FOREACH_BEGIN(lspecs, const link_specifier_t *, ls) {
     switch (link_specifier_get_ls_type(ls)) {
     case LS_IPV4:
@@ -1730,6 +1736,12 @@ hs_get_extend_info_from_lspecs(const smartlist_t *lspecs,
 
   /* Legacy ID is mandatory, and we require IPv4. */
   if (!have_v4 || !have_legacy_id) {
+    bool both = !have_v4 && !have_legacy_id;
+    log_fn(LOG_PROTOCOL_WARN, LD_REND, "Missing %s%s%s link specifier%s.",
+           !have_v4 ? "IPv4" : "",
+           both ? " and " : "",
+           !have_legacy_id ? "legacy ID" : "",
+           both ? "s" : "");
     goto done;
   }
 
@@ -1748,6 +1760,10 @@ hs_get_extend_info_from_lspecs(const smartlist_t *lspecs,
      * release. */
   } else {
     /* If we can't reach IPv4, return NULL. */
+    log_fn(LOG_PROTOCOL_WARN, LD_REND,
+           "Received an IPv4 link specifier, "
+           "but the address is not reachable: %s:%u",
+           fmt_addr(&addr_v4), port_v4);
     goto done;
   }
 
@@ -1755,7 +1771,7 @@ hs_get_extend_info_from_lspecs(const smartlist_t *lspecs,
 
  validate:
   /* We'll validate now that the address we've picked isn't a private one. If
-   * it is, are we allowing to extend to private address? */
+   * it is, are we allowed to extend to private addresses? */
   if (!extend_info_addr_is_allowed(&addr_v4)) {
     log_fn(LOG_PROTOCOL_WARN, LD_REND,
            "Requested address is private and we are not allowed to extend to "

src/feature/hs/hs_service.c

@@ -426,23 +426,16 @@ service_intro_point_free_void(void *obj)
 }
 
 /* Return a newly allocated service intro point and fully initialized from the
- * given extend_info_t ei if non NULL.
- * If is_legacy is true, we also generate the legacy key.
- * If supports_ed25519_link_handshake_any is true, we add the relay's ed25519
- * key to the link specifiers.
+ * given node_t node, if non NULL.
  *
- * If ei is NULL, returns a hs_service_intro_point_t with an empty link
+ * If node is NULL, returns a hs_service_intro_point_t with an empty link
  * specifier list and no onion key. (This is used for testing.)
  * On any other error, NULL is returned.
  *
- * ei must be an extend_info_t containing an IPv4 address. (We will add supoort
- * for IPv6 in a later release.) When calling extend_info_from_node(), pass
- * 0 in for_direct_connection to make sure ei always has an IPv4 address. */
+ * node must be an node_t with an IPv4 address. */
 STATIC hs_service_intro_point_t *
-service_intro_point_new(const extend_info_t *ei, unsigned int is_legacy,
-                        unsigned int supports_ed25519_link_handshake_any)
+service_intro_point_new(const node_t *node)
 {
-  hs_desc_link_specifier_t *ls;
   hs_service_intro_point_t *ip;
 
   ip = tor_malloc_zero(sizeof(*ip));
@@ -472,12 +465,17 @@ service_intro_point_new(const extend_info_t *ei, unsigned int is_legacy,
   ip->replay_cache = replaycache_new(0, 0);
 
   /* Initialize the base object. We don't need the certificate object. */
-  ip->base.link_specifiers = smartlist_new();
+  ip->base.link_specifiers = node_get_link_specifier_smartlist(node, 0);
+
+  if (node == NULL) {
+    goto done;
+  }
 
   /* Generate the encryption key for this intro point. */
   curve25519_keypair_generate(&ip->enc_key_kp, 0);
-  /* Figure out if this chosen node supports v3 or is legacy only. */
-  if (is_legacy) {
+  /* Figure out if this chosen node supports v3 or is legacy only.
+   * NULL nodes are used in the unit tests. */
+  if (!node_supports_ed25519_hs_intro(node)) {
     ip->base.is_only_legacy = 1;
     /* Legacy mode that is doesn't support v3+ with ed25519 auth key. */
     ip->legacy_key = crypto_pk_new();
@@ -490,40 +488,9 @@ service_intro_point_new(const extend_info_t *ei, unsigned int is_legacy,
     }
   }
 
-  if (ei == NULL) {
-    goto done;
-  }
-
-  /* We'll try to add all link specifiers. Legacy is mandatory.
-   * IPv4 or IPv6 is required, and we always send IPv4. */
-  ls = hs_desc_link_specifier_new(ei, LS_IPV4);
-  /* It is impossible to have an extend info object without a v4. */
-  if (BUG(!ls)) {
-    goto err;
-  }
-  smartlist_add(ip->base.link_specifiers, ls);
-
-  ls = hs_desc_link_specifier_new(ei, LS_LEGACY_ID);
-  /* It is impossible to have an extend info object without an identity
-   * digest. */
-  if (BUG(!ls)) {
-    goto err;
-  }
-  smartlist_add(ip->base.link_specifiers, ls);
-
-  /* ed25519 identity key is optional for intro points. If the node supports
-   * ed25519 link authentication, we include it. */
-  if (supports_ed25519_link_handshake_any) {
-    ls = hs_desc_link_specifier_new(ei, LS_ED25519_ID);
-    if (ls) {
-      smartlist_add(ip->base.link_specifiers, ls);
-    }
-  }
-
-  /* IPv6 is not supported in this release. */
-
-  /* Finally, copy onion key from the extend_info_t object. */
-  memcpy(&ip->onion_key, &ei->curve25519_onion_key, sizeof(ip->onion_key));
+  /* Finally, copy onion key from the node. */
+  memcpy(&ip->onion_key, node_get_curve25519_onion_key(node),
+         sizeof(ip->onion_key));
 
  done:
   return ip;
@@ -2106,7 +2073,6 @@ static hs_service_intro_point_t *
 pick_intro_point(unsigned int direct_conn, smartlist_t *exclude_nodes)
 {
   const node_t *node;
-  extend_info_t *info = NULL;
   hs_service_intro_point_t *ip = NULL;
   /* Normal 3-hop introduction point flags. */
   router_crn_flags_t flags = CRN_NEED_UPTIME | CRN_NEED_DESC;
@@ -2127,43 +2093,17 @@ pick_intro_point(unsigned int direct_conn, smartlist_t *exclude_nodes)
    * we don't want to use that node anymore. */
   smartlist_add(exclude_nodes, (void *) node);
 
-  /* We do this to ease our life but also this call makes appropriate checks
-   * of the node object such as validating ntor support for instance.
-   *
-   * We must provide an extend_info for clients to connect over a 3-hop path,
-   * so we don't pass direct_conn here. */
-  info = extend_info_from_node(node, 0);
-  if (BUG(info == NULL)) {
-    goto err;
-  }
+  /* Create our objects and populate them with the node information. */
+  ip = service_intro_point_new(node);
 
-  /* Let's do a basic sanity check here so that we don't end up advertising the
-   * ed25519 identity key of relays that don't actually support the link
-   * protocol */
-  if (!node_supports_ed25519_link_authentication(node, 0)) {
-    tor_assert_nonfatal(ed25519_public_key_is_zero(&info->ed_identity));
-  } else {
-    /* Make sure we *do* have an ed key if we support the link authentication.
-     * Sending an empty key would result in a failure to extend. */
-    tor_assert_nonfatal(!ed25519_public_key_is_zero(&info->ed_identity));
-  }
-
-  /* Create our objects and populate them with the node information.
-   * We don't care if the intro's link auth is compatible with us, because
-   * we are sending the ed25519 key to a remote client via the descriptor. */
-  ip = service_intro_point_new(info, !node_supports_ed25519_hs_intro(node),
-                               node_supports_ed25519_link_authentication(node,
-                                                                         0));
   if (ip == NULL) {
     goto err;
   }
 
-  log_info(LD_REND, "Picked intro point: %s", extend_info_describe(info));
-  extend_info_free(info);
+  log_info(LD_REND, "Picked intro point: %s", node_describe(node));
   return ip;
  err:
   service_intro_point_free(ip);
-  extend_info_free(info);
   return NULL;
 }
 

src/feature/hs/hs_service.h

@@ -369,10 +369,7 @@ STATIC hs_service_t *find_service(hs_service_ht *map,
 STATIC void remove_service(hs_service_ht *map, hs_service_t *service);
 STATIC int register_service(hs_service_ht *map, hs_service_t *service);
 /* Service introduction point functions. */
-STATIC hs_service_intro_point_t *service_intro_point_new(
-                            const extend_info_t *ei,
-                            unsigned int is_legacy,
-                            unsigned int supports_ed25519_link_handshake_any);
+STATIC hs_service_intro_point_t *service_intro_point_new(const node_t *node);
 STATIC void service_intro_point_free_(hs_service_intro_point_t *ip);
 #define service_intro_point_free(ip)                            \
   FREE_AND_NULL(hs_service_intro_point_t,             \

src/test/test_hs_cell.c

@@ -39,7 +39,7 @@ test_gen_establish_intro_cell(void *arg)
      attempt to parse it. */
   {
     /* We only need the auth key pair here. */
-    hs_service_intro_point_t *ip = service_intro_point_new(NULL, 0, 0);
+    hs_service_intro_point_t *ip = service_intro_point_new(NULL);
     /* Auth key pair is generated in the constructor so we are all set for
      * using this IP object. */
     ret = hs_cell_build_establish_intro(circ_nonce, ip, buf);
@@ -107,7 +107,7 @@ test_gen_establish_intro_cell_bad(void *arg)
      ed25519_sign_prefixed() function and make it fail. */
   cell = trn_cell_establish_intro_new();
   tt_assert(cell);
-  ip = service_intro_point_new(NULL, 0, 0);
+  ip = service_intro_point_new(NULL);
   cell_len = hs_cell_build_establish_intro(circ_nonce, ip, NULL);
   service_intro_point_free(ip);
   expect_log_msg_containing("Unable to make signature for "

src/test/test_hs_intropoint.c

@@ -50,7 +50,7 @@ new_establish_intro_cell(const char *circ_nonce,
 
   /* Auth key pair is generated in the constructor so we are all set for
    * using this IP object. */
-  ip = service_intro_point_new(NULL, 0, 0);
+  ip = service_intro_point_new(NULL);
   tt_assert(ip);
   cell_len = hs_cell_build_establish_intro(circ_nonce, ip, buf);
   tt_i64_op(cell_len, OP_GT, 0);
@@ -76,7 +76,7 @@ new_establish_intro_encoded_cell(const char *circ_nonce, uint8_t *cell_out)
 
   /* Auth key pair is generated in the constructor so we are all set for
    * using this IP object. */
-  ip = service_intro_point_new(NULL, 0, 0);
+  ip = service_intro_point_new(NULL);
   tt_assert(ip);
   cell_len = hs_cell_build_establish_intro(circ_nonce, ip, cell_out);
   tt_i64_op(cell_len, OP_GT, 0);

src/test/test_hs_service.c

@@ -329,7 +329,7 @@ static hs_service_intro_point_t *
 helper_create_service_ip(void)
 {
   hs_desc_link_specifier_t *ls;
-  hs_service_intro_point_t *ip = service_intro_point_new(NULL, 0, 0);
+  hs_service_intro_point_t *ip = service_intro_point_new(NULL);
   tor_assert(ip);
   /* Add a first unused link specifier. */
   ls = tor_malloc_zero(sizeof(*ls));