Log more info for duplicate ed25519 IDs

Occasionally, key pinning doesn't catch a relay that shares an ed25519
ID with another relay.  Log the identity fingerprints and the shared
ed25519 ID when this happens, instead of making a BUG() warning.

Fixes bug 27800; bugfix on 0.3.2.1-alpha.

3 files changed, 65 insertions, 12 deletions

diff --git a/changes/bug27800 b/changes/bug27800
new file mode 100644
index 0000000000..63d5dbc681
--- /dev/null
+++ b/changes/bug27800
@@ -0,0 +1,4 @@
+  o Minor bugfixes (directory authority):
+    - Log additional info when we get a relay that shares an ed25519
+      ID with a different relay, instead making a BUG() warning.
+      Fixes bug 27800; bugfix on 0.3.2.1-alpha.
diff --git a/src/or/nodelist.c b/src/or/nodelist.c
index 212606d2f7..82e0926289 100644
--- a/src/or/nodelist.c
+++ b/src/or/nodelist.c
@@ -43,6 +43,7 @@
 #include "or.h"
 #include "address.h"
 #include "address_set.h"
+#include "backtrace.h"
 #include "bridges.h"
 #include "config.h"
 #include "control.h"
@@ -63,6 +64,7 @@
 #include "routerparse.h"
 #include "routerset.h"
 #include "torcert.h"
+#include "util_format.h"
 
 #include <string.h>
 
@@ -260,6 +262,20 @@ node_remove_from_ed25519_map(node_t *node)
   return rv;
 }
 
+/** Helper function to log details of duplicated ed2559_ids */
+static void
+node_log_dup_ed_id(const node_t *old, const node_t *node, const char *ed_id)
+{
+  char *s;
+  char *olddesc = tor_strdup(node_describe(old));
+
+  tor_asprintf(&s, "Reused ed25519_id %s: old %s new %s", ed_id,
+               olddesc, node_describe(node));
+  log_backtrace(LOG_NOTICE, LD_DIR, s);
+  tor_free(olddesc);
+  tor_free(s);
+}
+
 /** If <b>node</b> has an ed25519 id, and it is not already in the ed25519 id
  * map, set its ed25519_id field, and add it to the ed25519 map.
  */
@@ -281,11 +297,24 @@ node_add_to_ed25519_map(node_t *node)
   node_t *old;
   memcpy(&node->ed25519_id, key, sizeof(node->ed25519_id));
   old = HT_FIND(nodelist_ed_map, &the_nodelist->nodes_by_ed_id, node);
-  if (BUG(old)) {
-    /* XXXX order matters here, and this may mean that authorities aren't
-     * pinning. */
-    if (old != node)
+  if (old) {
+    char ed_id[BASE32_BUFSIZE(sizeof(key->pubkey))];
+
+    base32_encode(ed_id, sizeof(ed_id), (const char *)key->pubkey,
+                  sizeof(key->pubkey));
+    if (BUG(old == node)) {
+      /* Actual bug: all callers of this function call
+       * node_remove_from_ed25519_map first. */
+      log_err(LD_BUG,
+              "Unexpectedly found deleted node with ed25519_id %s", ed_id);
+    } else {
+      /* Distinct nodes sharing a ed25519 id, possibly due to relay
+       * misconfiguration.  The key pinning might not catch this,
+       * possibly due to downloading a missing descriptor during
+       * consensus voting. */
+      node_log_dup_ed_id(old, node, ed_id);
       memset(&node->ed25519_id, 0, sizeof(node->ed25519_id));
+    }
     return 0;
   }
 
diff --git a/src/test/test_nodelist.c b/src/test/test_nodelist.c
index a873003d72..094e934712 100644
--- a/src/test/test_nodelist.c
+++ b/src/test/test_nodelist.c
@@ -11,6 +11,7 @@
 #include "nodelist.h"
 #include "torcert.h"
 #include "test.h"
+#include "log_test_helpers.h"
 
 /** Test the case when node_get_by_id() returns NULL,
  * node_get_verbose_nickname_by_id should return the base 16 encoding
@@ -118,9 +119,10 @@ mock_networkstatus_get_latest_consensus_by_flavor(consensus_flavor_t f)
 static void
 test_nodelist_ed_id(void *arg)
 {
-  routerstatus_t *rs[4];
-  microdesc_t *md[4];
-  routerinfo_t *ri[4];
+#define N_NODES 5
+  routerstatus_t *rs[N_NODES];
+  microdesc_t *md[N_NODES];
+  routerinfo_t *ri[N_NODES];
   networkstatus_t *ns;
   int i;
   (void)arg;
@@ -137,7 +139,7 @@ test_nodelist_ed_id(void *arg)
   /* Make a bunch of dummy objects that we can play around with.  Only set the
      necessary fields */
 
-  for (i = 0; i < 4; ++i) {
+  for (i = 0; i < N_NODES; ++i) {
     rs[i] = tor_malloc_zero(sizeof(*rs[i]));
     md[i] = tor_malloc_zero(sizeof(*md[i]));
     ri[i] = tor_malloc_zero(sizeof(*ri[i]));
@@ -154,7 +156,7 @@ test_nodelist_ed_id(void *arg)
     memcpy(&ri[i]->cache_info.signing_key_cert->signing_key,
            md[i]->ed25519_identity_pkey, sizeof(ed25519_public_key_t));
 
-    if (i != 3)
+    if (i < 3)
       smartlist_add(ns->routerstatus_list, rs[i]);
   }
 
@@ -184,13 +186,30 @@ test_nodelist_ed_id(void *arg)
 
   /* Register the 4th by ri only -- we never put it into the networkstatus,
    * so it has to be independent */
-  n = nodelist_set_routerinfo(ri[3], &ri_old);
-  tt_ptr_op(n, OP_EQ, node_get_by_ed25519_id(md[3]->ed25519_identity_pkey));
+  node_t *n3 = nodelist_set_routerinfo(ri[3], &ri_old);
+  tt_ptr_op(n3, OP_EQ, node_get_by_ed25519_id(md[3]->ed25519_identity_pkey));
   tt_ptr_op(ri_old, OP_EQ, NULL);
   tt_int_op(4, OP_EQ, smartlist_len(nodelist_get_list()));
 
+  /* Register the 5th by ri only, and rewrite its ed25519 pubkey to be
+   * the same as the 4th, to test the duplicate ed25519 key logging in
+   * nodelist.c */
+  memcpy(md[4]->ed25519_identity_pkey, md[3]->ed25519_identity_pkey,
+         sizeof(ed25519_public_key_t));
+  memcpy(&ri[4]->cache_info.signing_key_cert->signing_key,
+         md[3]->ed25519_identity_pkey, sizeof(ed25519_public_key_t));
+
+  setup_capture_of_logs(LOG_NOTICE);
+  node_t *n4 = nodelist_set_routerinfo(ri[4], &ri_old);
+  tt_ptr_op(ri_old, OP_EQ, NULL);
+  tt_int_op(5, OP_EQ, smartlist_len(nodelist_get_list()));
+  tt_ptr_op(n4, OP_NE, node_get_by_ed25519_id(md[3]->ed25519_identity_pkey));
+  tt_ptr_op(n3, OP_EQ, node_get_by_ed25519_id(md[3]->ed25519_identity_pkey));
+  expect_log_msg_containing("Reused ed25519_id");
+
  done:
-  for (i = 0; i < 4; ++i) {
+  teardown_capture_of_logs();
+  for (i = 0; i < N_NODES; ++i) {
     tor_free(rs[i]);
     tor_free(md[i]->ed25519_identity_pkey);
     tor_free(md[i]);
@@ -201,6 +220,7 @@ test_nodelist_ed_id(void *arg)
   networkstatus_vote_free(ns);
   UNMOCK(networkstatus_get_latest_consensus);
   UNMOCK(networkstatus_get_latest_consensus_by_flavor);
+#undef N_NODES
 }
 
 #define NODE(name, flags) \