Emit router families in canonical form

This patch has routers use the same canonicalization logic as
authorities when encoding their family lists.  Additionally, they
now warn if any router in their list is given by nickname, since
that's error-prone.

This patch also adds some long-overdue tests for family formatting.

changes/ticket28266

@@ -3,3 +3,8 @@
       under which microdescriptor entries are encoded in a canonical
       form. This improves their compressibility in transit and on the client.
       Closes ticket 28266; implements proposal 298.
+
+  o Minor features (relay):
+    - When listing relay families, list them in canonical form including the
+      relay's own identity, and try to give a more useful set of warnings.
+      Part of ticket 28266 and proposal 298.

src/feature/relay/router.c

@@ -30,6 +30,7 @@
 #include "feature/nodelist/dirlist.h"
 #include "feature/nodelist/networkstatus.h"
 #include "feature/nodelist/nickname.h"
+#include "feature/nodelist/nodefamily.h"
 #include "feature/nodelist/nodelist.h"
 #include "feature/nodelist/routerlist.h"
 #include "feature/nodelist/torcert.h"
@@ -340,6 +341,16 @@ set_server_identity_key(crypto_pk_t *k)
   }
 }
 
+#ifdef TOR_UNIT_TESTS
+/** Testing only -- set the server's RSA identity digest to
+ * be <b>digest</b> */
+void
+set_server_identity_key_digest_testing(const uint8_t *digest)
+{
+  memcpy(server_identitykey_digest, digest, DIGEST_LEN);
+}
+#endif
+
 /** Make sure that we have set up our identity keys to match or not match as
  * appropriate, and die with an assertion if we have not. */
 static void
@@ -1691,10 +1702,6 @@ router_get_descriptor_gen_reason(void)
   return desc_gen_reason;
 }
 
-/** A list of nicknames that we've warned about including in our family
- * declaration verbatim rather than as digests. */
-static smartlist_t *warned_nonexistent_family = NULL;
-
 static int router_guess_address_from_dir_headers(uint32_t *guess);
 
 /** Make a current best guess at our address, either because
@@ -1807,13 +1814,17 @@ router_check_descriptor_address_consistency(uint32_t ipv4h_desc_addr)
                                                    CONN_TYPE_DIR_LISTENER);
 }
 
+/** A list of nicknames that we've warned about including in our family,
+ * for one reason or another. */
+static smartlist_t *warned_family = NULL;
+
 /**
  * Return a new smartlist containing the family members configured in
  * <b>options</b>.  Warn about invalid or missing entries.  Return NULL
  * if this relay should not declare a family.
  **/
-static smartlist_t *
-get_declared_family(const or_options_t *options)
+STATIC smartlist_t *
+get_my_declared_family(const or_options_t *options)
 {
   if (!options->MyFamily)
     return NULL;
@@ -1821,55 +1832,112 @@ get_declared_family(const or_options_t *options)
   if (options->BridgeRelay)
     return NULL;
 
-  if (!warned_nonexistent_family)
-    warned_nonexistent_family = smartlist_new();
+  if (!warned_family)
+    warned_family = smartlist_new();
 
   smartlist_t *declared_family = smartlist_new();
   config_line_t *family;
+
+  /* First we try to get the whole family in the form of hexdigests. */
   for (family = options->MyFamily; family; family = family->next) {
     char *name = family->value;
     const node_t *member;
-    if (!strcasecmp(name, options->Nickname))
-      continue; /* Don't list ourself, that's redundant */
+    if (options->Nickname && !strcasecmp(name, options->Nickname))
+      continue; /* Don't list ourself by nickname, that's redundant */
     else
       member = node_get_by_nickname(name, 0);
+
     if (!member) {
+      /* This node doesn't seem to exist, so warn about it if it is not
+       * a hexdigest. */
       int is_legal = is_legal_nickname_or_hexdigest(name);
-      if (!smartlist_contains_string(warned_nonexistent_family, name) &&
+      if (!smartlist_contains_string(warned_family, name) &&
           !is_legal_hexdigest(name)) {
         if (is_legal)
           log_warn(LD_CONFIG,
-                   "I have no descriptor for the router named \"%s\" in my "
-                   "declared family; I'll use the nickname as is, but "
-                   "this may confuse clients.", name);
+                   "There is a router named %s in my declared family, but "
+                   "I have no descriptor for it. I'll use the nickname "
+                   "as is, but this may confuse clients. Please list it "
+                   "by identity digest instead.", escaped(name));
         else
-          log_warn(LD_CONFIG, "There is a router named \"%s\" in my "
-                   "declared family, but that isn't a legal nickname. "
+          log_warn(LD_CONFIG, "There is a router named %s in my declared "
+                   "family, but that isn't a legal digest or nickname. "
                    "Skipping it.", escaped(name));
-        smartlist_add_strdup(warned_nonexistent_family, name);
+        smartlist_add_strdup(warned_family, name);
       }
       if (is_legal) {
         smartlist_add_strdup(declared_family, name);
       }
-    } else if (router_digest_is_me(member->identity)) {
-      /* Don't list ourself in our own family; that's redundant */
-      /* XXX shouldn't be possible */
     } else {
+      /* List the node by digest. */
       char *fp = tor_malloc(HEX_DIGEST_LEN+2);
       fp[0] = '$';
       base16_encode(fp+1,HEX_DIGEST_LEN+1,
                     member->identity, DIGEST_LEN);
       smartlist_add(declared_family, fp);
-      if (smartlist_contains_string(warned_nonexistent_family, name))
-        smartlist_string_remove(warned_nonexistent_family, name);
+
+      if (! is_legal_hexdigest(name) &&
+          !smartlist_contains_string(warned_family, name)) {
+        /* Warn if this node was not specified by hexdigest. */
+        log_warn(LD_CONFIG, "There is a router named %s in my declared "
+                 "family, but it wasn't listed by digest. Please consider "
+                 "saying %s instead, if that's what you meant.",
+                 escaped(name), fp);
+        smartlist_add_strdup(warned_family, name);
+      }
     }
   }
 
-  /* remove duplicates from the list */
-  smartlist_sort_strings(declared_family);
-  smartlist_uniq_strings(declared_family);
+  /* Now declared_family should have the closest we can come to the
+   * identities that the user wanted.
+   *
+   * Unlike older versions of Tor, we _do_ include our own identity: this
+   * helps microdescriptor compression, and helps in-memory compression
+   * on clients. */
+  nodefamily_t *nf = nodefamily_from_members(declared_family,
+                                     router_get_my_id_digest(),
+                                     NF_WARN_MALFORMED,
+                                     NULL);
+  SMARTLIST_FOREACH(declared_family, char *, s, tor_free(s));
+  smartlist_free(declared_family);
+  if (!nf) {
+    return NULL;
+  }
+
+  char *s = nodefamily_format(nf);
+  nodefamily_free(nf);
+
+  smartlist_t *result = smartlist_new();
+  smartlist_split_string(result, s, NULL,
+                         SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0);
+  tor_free(s);
+
+  if (smartlist_len(result) == 1) {
+    /* This is a one-element list containing only ourself; instead return
+     * nothing */
+    const char *singleton = smartlist_get(result, 0);
+    bool is_me = false;
+    if (singleton[0] == '$') {
+      char d[DIGEST_LEN];
+      int n = base16_decode(d, sizeof(d), singleton+1, strlen(singleton+1));
+      if (n == DIGEST_LEN &&
+          fast_memeq(d, router_get_my_id_digest(), DIGEST_LEN)) {
+        is_me = true;
+      }
+    }
+    if (!is_me) {
+      // LCOV_EXCL_START
+      log_warn(LD_BUG, "Found a singleton family list with an element "
+               "that wasn't us! Element was %s", escaped(singleton));
+      // LCOV_EXCL_STOP
+    } else {
+      SMARTLIST_FOREACH(result, char *, cp, tor_free(cp));
+      smartlist_free(result);
+      return NULL;
+    }
+  }
 
-  return declared_family;
+  return result;
 }
 
 /** Build a fresh routerinfo, signed server descriptor, and extra-info document
@@ -1986,7 +2054,7 @@ router_build_fresh_descriptor(routerinfo_t **r, extrainfo_t **e)
     tor_free(p_tmp);
   }
 
-  ri->declared_family = get_declared_family(options);
+  ri->declared_family = get_my_declared_family(options);
 
   /* Now generate the extrainfo. */
   ei = tor_malloc_zero(sizeof(extrainfo_t));
@@ -3077,9 +3145,9 @@ extrainfo_dump_to_string(char **s_out, extrainfo_t *extrainfo,
 void
 router_reset_warnings(void)
 {
-  if (warned_nonexistent_family) {
-    SMARTLIST_FOREACH(warned_nonexistent_family, char *, cp, tor_free(cp));
-    smartlist_clear(warned_nonexistent_family);
+  if (warned_family) {
+    SMARTLIST_FOREACH(warned_family, char *, cp, tor_free(cp));
+    smartlist_clear(warned_family);
   }
 }
 
@@ -3103,11 +3171,12 @@ router_free_all(void)
   memwipe(&curve25519_onion_key, 0, sizeof(curve25519_onion_key));
   memwipe(&last_curve25519_onion_key, 0, sizeof(last_curve25519_onion_key));
 
-  if (warned_nonexistent_family) {
-    SMARTLIST_FOREACH(warned_nonexistent_family, char *, cp, tor_free(cp));
-    smartlist_free(warned_nonexistent_family);
+  if (warned_family) {
+    SMARTLIST_FOREACH(warned_family, char *, cp, tor_free(cp));
+    smartlist_free(warned_family);
   }
 }
+
 /* From the given RSA key object, convert it to ASN-1 encoded format and set
  * the newly allocated object in onion_pkey_out. The length of the key is set
  * in onion_pkey_len_out. */

src/feature/relay/router.h

@@ -117,6 +117,10 @@ void router_free_all(void);
 /* Used only by router.c and test.c */
 STATIC void get_platform_str(char *platform, size_t len);
 STATIC int router_write_fingerprint(int hashed);
+STATIC smartlist_t *get_my_declared_family(const or_options_t *options);
+#ifdef TOR_UNIT_TESTS
+void set_server_identity_key_digest_testing(const uint8_t *digest);
+#endif
 #endif
 
 #endif /* !defined(TOR_ROUTER_H) */

src/test/test_router.c

@@ -7,16 +7,21 @@
  * \brief Unittests for code in router.c
  **/
 
+#define CONFIG_PRIVATE
+#define ROUTER_PRIVATE
 #include "core/or/or.h"
 #include "app/config/config.h"
 #include "core/mainloop/mainloop.h"
 #include "feature/hibernate/hibernate.h"
+#include "feature/nodelist/node_st.h"
+#include "feature/nodelist/nodelist.h"
 #include "feature/nodelist/routerinfo_st.h"
 #include "feature/nodelist/routerlist.h"
 #include "feature/relay/router.h"
 #include "feature/stats/rephist.h"
 #include "lib/crypt_ops/crypto_curve25519.h"
 #include "lib/crypt_ops/crypto_ed25519.h"
+#include "lib/encoding/confline.h"
 
 /* Test suite stuff */
 #include "test/test.h"
@@ -231,11 +236,161 @@ test_router_check_descriptor_bandwidth_changed(void *arg)
   UNMOCK(we_are_hibernating);
 }
 
+static node_t fake_node;
+static const node_t *
+mock_node_get_by_nickname(const char *name, unsigned flags)
+{
+  (void)flags;
+  if (!strcasecmp(name, "crumpet"))
+    return &fake_node;
+  else
+    return NULL;
+}
+
+static void
+test_router_get_my_family(void *arg)
+{
+  (void)arg;
+  or_options_t *options = options_new();
+  smartlist_t *sl = NULL;
+  char *join = NULL;
+  // Overwrite the result of router_get_my_identity_digest().  This
+  // happens to be okay, but only for testing.
+  set_server_identity_key_digest_testing(
+                                   (const uint8_t*)"holeinthebottomofthe");
+
+  setup_capture_of_logs(LOG_WARN);
+
+  // No family listed -- so there's no list.
+  sl = get_my_declared_family(options);
+  tt_ptr_op(sl, OP_EQ, NULL);
+  expect_no_log_entry();
+
+#define CLEAR() do {                                    \
+    if (sl) {                                           \
+      SMARTLIST_FOREACH(sl, char *, cp, tor_free(cp));  \
+      smartlist_free(sl);                               \
+    }                                                   \
+    tor_free(join);                                     \
+    mock_clean_saved_logs();                            \
+  } while (0)
+
+  // Add a single nice friendly hex member.  This should be enough
+  // to have our own ID added.
+  tt_ptr_op(options->MyFamily, OP_EQ, NULL);
+  config_line_append(&options->MyFamily, "MyFamily",
+                     "$AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA");
+
+  sl = get_my_declared_family(options);
+  tt_ptr_op(sl, OP_NE, NULL);
+  tt_int_op(smartlist_len(sl), OP_EQ, 2);
+  join = smartlist_join_strings(sl, " ", 0, NULL);
+  tt_str_op(join, OP_EQ,
+            "$686F6C65696E746865626F74746F6D6F66746865 "
+            "$AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA");
+  expect_no_log_entry();
+  CLEAR();
+
+  // Add a hex member with a ~.  The ~ part should get removed.
+  config_line_append(&options->MyFamily, "MyFamily",
+                     "$0123456789abcdef0123456789abcdef01234567~Muffin");
+  sl = get_my_declared_family(options);
+  tt_ptr_op(sl, OP_NE, NULL);
+  tt_int_op(smartlist_len(sl), OP_EQ, 3);
+  join = smartlist_join_strings(sl, " ", 0, NULL);
+  tt_str_op(join, OP_EQ,
+            "$0123456789ABCDEF0123456789ABCDEF01234567 "
+            "$686F6C65696E746865626F74746F6D6F66746865 "
+            "$AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA");
+  expect_no_log_entry();
+  CLEAR();
+
+  // Nickname lookup will fail, so a nickname will appear verbatim.
+  config_line_append(&options->MyFamily, "MyFamily",
+                     "BAGEL");
+  sl = get_my_declared_family(options);
+  tt_ptr_op(sl, OP_NE, NULL);
+  tt_int_op(smartlist_len(sl), OP_EQ, 4);
+  join = smartlist_join_strings(sl, " ", 0, NULL);
+  tt_str_op(join, OP_EQ,
+            "$0123456789ABCDEF0123456789ABCDEF01234567 "
+            "$686F6C65696E746865626F74746F6D6F66746865 "
+            "$AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA "
+            "bagel");
+  expect_single_log_msg_containing(
+           "There is a router named \"BAGEL\" in my declared family, but "
+           "I have no descriptor for it.");
+  CLEAR();
+
+  // A bogus digest should fail entirely.
+  config_line_append(&options->MyFamily, "MyFamily",
+                     "$painauchocolat");
+  sl = get_my_declared_family(options);
+  tt_ptr_op(sl, OP_NE, NULL);
+  tt_int_op(smartlist_len(sl), OP_EQ, 4);
+  join = smartlist_join_strings(sl, " ", 0, NULL);
+  tt_str_op(join, OP_EQ,
+            "$0123456789ABCDEF0123456789ABCDEF01234567 "
+            "$686F6C65696E746865626F74746F6D6F66746865 "
+            "$AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA "
+            "bagel");
+  // "BAGEL" is still there, but it won't make a warning, because we already
+  // warned about it.
+  expect_single_log_msg_containing(
+           "There is a router named \"$painauchocolat\" in my declared "
+           "family, but that isn't a legal digest or nickname. Skipping it.");
+  CLEAR();
+
+  // Let's introduce a node we can look up by nickname
+  memset(&fake_node, 0, sizeof(fake_node));
+  memcpy(fake_node.identity, "whydoyouasknonononon", DIGEST_LEN);
+  MOCK(node_get_by_nickname, mock_node_get_by_nickname);
+
+  config_line_append(&options->MyFamily, "MyFamily",
+                     "CRUmpeT");
+  sl = get_my_declared_family(options);
+  tt_ptr_op(sl, OP_NE, NULL);
+  tt_int_op(smartlist_len(sl), OP_EQ, 5);
+  join = smartlist_join_strings(sl, " ", 0, NULL);
+  tt_str_op(join, OP_EQ,
+            "$0123456789ABCDEF0123456789ABCDEF01234567 "
+            "$686F6C65696E746865626F74746F6D6F66746865 "
+            "$776879646F796F7561736B6E6F6E6F6E6F6E6F6E "
+            "$AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA "
+            "bagel");
+  // "BAGEL" is still there, but it won't make a warning, because we already
+  // warned about it.  Some with "$painauchocolat".
+  expect_single_log_msg_containing(
+           "There is a router named \"CRUmpeT\" in my declared "
+           "family, but it wasn't listed by digest. Please consider saying "
+           "$776879646F796F7561736B6E6F6E6F6E6F6E6F6E instead, if that's "
+           "what you meant.");
+  CLEAR();
+  UNMOCK(node_get_by_nickname);
+
+  // Try a singleton list containing only us: It should give us NULL.
+  config_free_lines(options->MyFamily);
+  config_line_append(&options->MyFamily, "MyFamily",
+                     "$686F6C65696E746865626F74746F6D6F66746865");
+  sl = get_my_declared_family(options);
+  tt_ptr_op(sl, OP_EQ, NULL);
+  expect_no_log_entry();
+
+ done:
+  or_options_free(options);
+  teardown_capture_of_logs();
+  CLEAR();
+  UNMOCK(node_get_by_nickname);
+
+#undef CLEAR
+}
+
 #define ROUTER_TEST(name, flags)                          \
   { #name, test_router_ ## name, flags, NULL, NULL }
 
 struct testcase_t router_tests[] = {
   ROUTER_TEST(check_descriptor_bandwidth_changed, TT_FORK),
   ROUTER_TEST(dump_router_to_string_no_bridge_distribution_method, TT_FORK),
+  ROUTER_TEST(get_my_family, TT_FORK),
   END_OF_TESTCASES
 };