From 0e9a963b6b87282011fe204e81b5c2530153a935 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 24 Nov 2018 10:30:15 -0500 Subject: Revise nodefamily.c to match proposal 298 Prop298 says that family entries should be formatted with $hexids in uppercase, nicknames in lower case, $hexid~names truncated, and everything sorted lexically. These changes implement that ordering for nodefamily.c. We don't _strictly speaking_ need to nodefamily.c formatting use this for prop298 microdesc generation, but it seems silly to have two separate canonicalization algorithms. --- src/test/test_microdesc.c | 2 +- src/test/test_nodelist.c | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) (limited to 'src/test') diff --git a/src/test/test_microdesc.c b/src/test/test_microdesc.c index 3318408d53..debb11155a 100644 --- a/src/test/test_microdesc.c +++ b/src/test/test_microdesc.c @@ -176,7 +176,7 @@ test_md_cache(void *data) tt_ptr_op(md3->family, OP_NE, NULL); encoded_family = nodefamily_format(md3->family); - tt_str_op(encoded_family, OP_EQ, "nodeX nodeY nodeZ"); + tt_str_op(encoded_family, OP_EQ, "nodex nodey nodez"); /* Now rebuild the cache! */ tt_int_op(microdesc_cache_rebuild(mc, 1), OP_EQ, 0); diff --git a/src/test/test_nodelist.c b/src/test/test_nodelist.c index 0287be3305..afbcc60ac0 100644 --- a/src/test/test_nodelist.c +++ b/src/test/test_nodelist.c @@ -299,7 +299,7 @@ test_nodelist_nodefamily(void *arg) tt_ptr_op(nf1, OP_EQ, nf3); /* Do we get the expected result when we re-encode? */ - tor_asprintf(&enc, "hello $%s", h1); + tor_asprintf(&enc, "$%s hello", h1); enc2 = nodefamily_format(nf1); tt_str_op(enc2, OP_EQ, enc); tor_free(enc2); @@ -399,8 +399,8 @@ test_nodelist_nodefamily_parse_err(void *arg) tt_assert(nf1); enc = nodefamily_format(nf1); tt_str_op(enc, OP_EQ, - "reticulatogranulate " - "$7468696E67732D696E2D7468656D73656C766573"); + "$7468696E67732D696E2D7468656D73656C766573 " + "reticulatogranulate"); tor_free(enc); } @@ -470,11 +470,11 @@ test_nodelist_nodefamily_lookup(void *arg) tt_int_op(smartlist_len(sl), OP_EQ, 3); const node_t *n = smartlist_get(sl, 0); - tt_str_op(n->identity, OP_EQ, "erewhon"); - n = smartlist_get(sl, 1); test_memeq_hex(n->identity, "3333333333333333333333333333333333333333"); - n = smartlist_get(sl, 2); + n = smartlist_get(sl, 1); test_memeq_hex(n->identity, "EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE"); + n = smartlist_get(sl, 2); + tt_str_op(n->identity, OP_EQ, "erewhon"); done: UNMOCK(node_get_by_nickname); @@ -583,9 +583,9 @@ test_nodelist_node_nodefamily(void *arg) node_lookup_declared_family(nodes, &mock_node1); tt_int_op(smartlist_len(nodes), OP_EQ, 2); const node_t *n = smartlist_get(nodes, 0); - tt_str_op(n->identity, OP_EQ, "NodeFour"); - n = smartlist_get(nodes, 1); tt_mem_op(n->identity, OP_EQ, "SecondNodeWe'reTestn", DIGEST_LEN); + n = smartlist_get(nodes, 1); + tt_str_op(n->identity, OP_EQ, "nodefour"); // free, try the other one. SMARTLIST_FOREACH(nodes, node_t *, x, tor_free(x)); @@ -594,9 +594,9 @@ test_nodelist_node_nodefamily(void *arg) node_lookup_declared_family(nodes, &mock_node2); tt_int_op(smartlist_len(nodes), OP_EQ, 2); n = smartlist_get(nodes, 0); + // This gets a truncated hex hex ID since it was looked up by name tt_str_op(n->identity, OP_EQ, "NodeThree"); n = smartlist_get(nodes, 1); - // This gets a truncated hex hex ID since it was looked up by name tt_str_op(n->identity, OP_EQ, "4e6f64654f6e654e6f6"); done: -- cgit v1.2.3-54-g00ecf From d29e3a02d57aef402a1aaf9747ef44393b043d98 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 24 Nov 2018 10:53:38 -0500 Subject: Add a function to canonicalize nodefamilies per prop298 This is the same as the regular canonical nodefamily format, except that unrecognized elements are preserved. --- src/feature/nodelist/nodefamily.c | 45 +++++++++++++++++++++++++++++++++++++-- src/feature/nodelist/nodefamily.h | 5 ++++- src/test/test_nodelist.c | 31 +++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 3 deletions(-) (limited to 'src/test') diff --git a/src/feature/nodelist/nodefamily.c b/src/feature/nodelist/nodefamily.c index 29659ed93d..944ad54755 100644 --- a/src/feature/nodelist/nodefamily.c +++ b/src/feature/nodelist/nodefamily.c @@ -93,12 +93,46 @@ nodefamily_parse(const char *s, const uint8_t *rsa_id_self, { smartlist_t *sl = smartlist_new(); smartlist_split_string(sl, s, NULL, SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); - nodefamily_t *result = nodefamily_from_members(sl, rsa_id_self, flags); + nodefamily_t *result = nodefamily_from_members(sl, rsa_id_self, flags, NULL); SMARTLIST_FOREACH(sl, char *, cp, tor_free(cp)); smartlist_free(sl); return result; } +/** + * Canonicalize the family list s, returning a newly allocated string. + * + * The canonicalization rules are fully specified in dir-spec.txt, but, + * briefly: $hexid entries are put in caps, $hexid[=~]foo entries are + * truncated, nicknames are put into lowercase, unrecognized entries are left + * alone, and everything is sorted. + **/ +char * +nodefamily_canonicalize(const char *s, const uint8_t *rsa_id_self, + unsigned flags) +{ + smartlist_t *sl = smartlist_new(); + smartlist_t *result_members = smartlist_new(); + smartlist_split_string(sl, s, NULL, SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); + nodefamily_t *nf = nodefamily_from_members(sl, rsa_id_self, flags, + result_members); + + char *formatted = nodefamily_format(nf); + smartlist_split_string(result_members, formatted, NULL, + SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0); + smartlist_sort_strings(result_members); + char *combined = smartlist_join_strings(result_members, " ", 0, NULL); + + nodefamily_free(nf); + SMARTLIST_FOREACH(sl, char *, cp, tor_free(cp)); + smartlist_free(sl); + SMARTLIST_FOREACH(result_members, char *, cp, tor_free(cp)); + smartlist_free(result_members); + tor_free(formatted); + + return combined; +} + /** * qsort helper for encoded nodefamily elements. **/ @@ -117,11 +151,15 @@ compare_members(const void *a, const void *b) * family declaration if it is not there already. * * The flags element is interpreted as in nodefamily_parse(). + * + * If unrecognized is provided, fill it copies of any unrecognized + * members. (Note that malformed $hexids are not considered unrecognized.) **/ nodefamily_t * nodefamily_from_members(const smartlist_t *members, const uint8_t *rsa_id_self, - unsigned flags) + unsigned flags, + smartlist_t *unrecognized_out) { const int n_self = rsa_id_self ? 1 : 0; int n_bad_elements = 0; @@ -146,6 +184,9 @@ nodefamily_from_members(const smartlist_t *members, ptr[0] = NODEFAMILY_BY_RSA_ID; memcpy(ptr+1, digest_buf, DIGEST_LEN); } + } else { + if (unrecognized_out) + smartlist_add_strdup(unrecognized_out, cp); } if (bad_element) { diff --git a/src/feature/nodelist/nodefamily.h b/src/feature/nodelist/nodefamily.h index 342f161a07..ea1076876d 100644 --- a/src/feature/nodelist/nodefamily.h +++ b/src/feature/nodelist/nodefamily.h @@ -27,7 +27,8 @@ nodefamily_t *nodefamily_parse(const char *s, unsigned flags); nodefamily_t *nodefamily_from_members(const struct smartlist_t *members, const uint8_t *rsa_id_self, - unsigned flags); + unsigned flags, + smartlist_t *unrecognized_out); void nodefamily_free_(nodefamily_t *family); #define nodefamily_free(family) \ FREE_AND_NULL(nodefamily_t, nodefamily_free_, (family)) @@ -41,6 +42,8 @@ bool nodefamily_contains_node(const nodefamily_t *family, void nodefamily_add_nodes_to_smartlist(const nodefamily_t *family, struct smartlist_t *out); char *nodefamily_format(const nodefamily_t *family); +char *nodefamily_canonicalize(const char *s, const uint8_t *rsa_id_self, + unsigned flags); void nodefamily_free_all(void); diff --git a/src/test/test_nodelist.c b/src/test/test_nodelist.c index afbcc60ac0..ed919f4edf 100644 --- a/src/test/test_nodelist.c +++ b/src/test/test_nodelist.c @@ -610,6 +610,36 @@ test_nodelist_node_nodefamily(void *arg) smartlist_free(nodes); } +static void +test_nodelist_nodefamily_canonicalize(void *arg) +{ + (void)arg; + char *c = NULL; + + c = nodefamily_canonicalize("", NULL, 0); + tt_str_op(c, OP_EQ, ""); + tor_free(c); + + uint8_t own_id[20]; + memset(own_id, 0, sizeof(own_id)); + c = nodefamily_canonicalize( + "alice BOB caroL %potrzebie !!!@#@# " + "$bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb=fred " + "ffffffffffffffffffffffffffffffffffffffff " + "$cccccccccccccccccccccccccccccccccccccccc ", own_id, 0); + tt_str_op(c, OP_EQ, + "!!!@#@# " + "$0000000000000000000000000000000000000000 " + "$BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB " + "$CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC " + "$FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF " + "%potrzebie " + "alice bob carol"); + + done: + tor_free(c); +} + #define NODE(name, flags) \ { #name, test_nodelist_##name, (flags), NULL, NULL } @@ -623,5 +653,6 @@ struct testcase_t nodelist_tests[] = { NODE(nodefamily_lookup, TT_FORK), NODE(nickname_matches, 0), NODE(node_nodefamily, TT_FORK), + NODE(nodefamily_canonicalize, 0), END_OF_TESTCASES }; -- cgit v1.2.3-54-g00ecf From 0a0c612b79cb5230cc70922634a37f73a1c87c10 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 24 Nov 2018 11:51:41 -0500 Subject: Add a consensus method in which md families get canonicalized. Implements prop298. Closes ticket 28266. --- changes/ticket28266 | 5 +++++ src/feature/dirauth/dirvote.c | 18 +++++++++++++++--- src/feature/dirauth/dirvote.h | 8 +++++++- src/test/test_microdesc.c | 33 +++++++++++++++++++++++++++++++++ 4 files changed, 60 insertions(+), 4 deletions(-) create mode 100644 changes/ticket28266 (limited to 'src/test') diff --git a/changes/ticket28266 b/changes/ticket28266 new file mode 100644 index 0000000000..d90dc74f76 --- /dev/null +++ b/changes/ticket28266 @@ -0,0 +1,5 @@ + o Minor features (directory authority): + - Directory authorities support a new consensus algorithm, + 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. diff --git a/src/feature/dirauth/dirvote.c b/src/feature/dirauth/dirvote.c index 066a9e6e8a..6c4e12cd51 100644 --- a/src/feature/dirauth/dirvote.c +++ b/src/feature/dirauth/dirvote.c @@ -28,6 +28,7 @@ #include "feature/nodelist/fmt_routerstatus.h" #include "feature/nodelist/microdesc.h" #include "feature/nodelist/networkstatus.h" +#include "feature/nodelist/nodefamily.h" #include "feature/nodelist/nodelist.h" #include "feature/nodelist/routerlist.h" #include "feature/relay/router.h" @@ -3799,8 +3800,16 @@ dirvote_create_microdescriptor(const routerinfo_t *ri, int consensus_method) smartlist_add_asprintf(chunks, "a %s\n", fmt_addrport(&ri->ipv6_addr, ri->ipv6_orport)); - if (family) - smartlist_add_asprintf(chunks, "family %s\n", family); + if (family) { + if (consensus_method < MIN_METHOD_FOR_CANONICAL_FAMILIES_IN_MICRODESCS) { + smartlist_add_asprintf(chunks, "family %s\n", family); + } else { + const uint8_t *id = (const uint8_t *)ri->cache_info.identity_digest; + char *canonical_family = nodefamily_canonicalize(family, id, 0); + smartlist_add_asprintf(chunks, "family %s\n", canonical_family); + tor_free(canonical_family); + } + } if (summary && strcmp(summary, "reject 1-65535")) smartlist_add_asprintf(chunks, "p %s\n", summary); @@ -3898,7 +3907,10 @@ static const struct consensus_method_range_t { int high; } microdesc_consensus_methods[] = { {MIN_SUPPORTED_CONSENSUS_METHOD, MIN_METHOD_FOR_NO_A_LINES_IN_MICRODESC - 1}, - {MIN_METHOD_FOR_NO_A_LINES_IN_MICRODESC, MAX_SUPPORTED_CONSENSUS_METHOD}, + {MIN_METHOD_FOR_NO_A_LINES_IN_MICRODESC, + MIN_METHOD_FOR_CANONICAL_FAMILIES_IN_MICRODESCS - 1}, + {MIN_METHOD_FOR_CANONICAL_FAMILIES_IN_MICRODESCS, + MAX_SUPPORTED_CONSENSUS_METHOD}, {-1, -1} }; diff --git a/src/feature/dirauth/dirvote.h b/src/feature/dirauth/dirvote.h index a21e9f3455..6afb6047ff 100644 --- a/src/feature/dirauth/dirvote.h +++ b/src/feature/dirauth/dirvote.h @@ -57,7 +57,7 @@ #define MIN_SUPPORTED_CONSENSUS_METHOD 25 /** The highest consensus method that we currently support. */ -#define MAX_SUPPORTED_CONSENSUS_METHOD 28 +#define MAX_SUPPORTED_CONSENSUS_METHOD 29 /** Lowest consensus method where authorities vote on required/recommended * protocols. */ @@ -79,6 +79,12 @@ * addresses. See #23828 and #20916. */ #define MIN_METHOD_FOR_NO_A_LINES_IN_MICRODESC 28 +/** + * Lowest consensus method where microdescriptor lines are put in canonical + * form for improved compressibility and ease of storage. See proposal 298. + **/ +#define MIN_METHOD_FOR_CANONICAL_FAMILIES_IN_MICRODESCS 29 + /** Default bandwidth to clip unmeasured bandwidths to using method >= * MIN_METHOD_TO_CLIP_UNMEASURED_BW. (This is not a consensus method; do not * get confused with the above macros.) */ diff --git a/src/test/test_microdesc.c b/src/test/test_microdesc.c index debb11155a..fd79aee6be 100644 --- a/src/test/test_microdesc.c +++ b/src/test/test_microdesc.c @@ -421,6 +421,28 @@ static const char test_md2_21[] = "ntor-onion-key hbxdRnfVUJJY7+KcT4E3Rs7/zuClbN3hJrjSBiEGMgI=\n" "id ed25519 wqfLzgfCtRfYNg88LsL1QpzxS0itapJ1aj6TbnByx/Q\n"; +static const char test_md2_withfamily_28[] = + "onion-key\n" + "-----BEGIN RSA PUBLIC KEY-----\n" + "MIGJAoGBAL2R8EfubUcahxha4u02P4VAR0llQIMwFAmrHPjzcK7apcQgDOf2ovOA\n" + "+YQnJFxlpBmCoCZC6ssCi+9G0mqo650lFuTMP5I90BdtjotfzESfTykHLiChyvhd\n" + "l0dlqclb2SU/GKem/fLRXH16aNi72CdSUu/1slKs/70ILi34QixRAgMBAAE=\n" + "-----END RSA PUBLIC KEY-----\n" + "ntor-onion-key hbxdRnfVUJJY7+KcT4E3Rs7/zuClbN3hJrjSBiEGMgI=\n" + "family OtherNode !Strange\n" + "id ed25519 wqfLzgfCtRfYNg88LsL1QpzxS0itapJ1aj6TbnByx/Q\n"; + +static const char test_md2_withfamily_29[] = + "onion-key\n" + "-----BEGIN RSA PUBLIC KEY-----\n" + "MIGJAoGBAL2R8EfubUcahxha4u02P4VAR0llQIMwFAmrHPjzcK7apcQgDOf2ovOA\n" + "+YQnJFxlpBmCoCZC6ssCi+9G0mqo650lFuTMP5I90BdtjotfzESfTykHLiChyvhd\n" + "l0dlqclb2SU/GKem/fLRXH16aNi72CdSUu/1slKs/70ILi34QixRAgMBAAE=\n" + "-----END RSA PUBLIC KEY-----\n" + "ntor-onion-key hbxdRnfVUJJY7+KcT4E3Rs7/zuClbN3hJrjSBiEGMgI=\n" + "family !Strange $B7E27F104213C36F13E7E9829182845E495997A0 othernode\n" + "id ed25519 wqfLzgfCtRfYNg88LsL1QpzxS0itapJ1aj6TbnByx/Q\n"; + static void test_md_generate(void *arg) { @@ -451,6 +473,17 @@ test_md_generate(void *arg) tt_assert(ed25519_pubkey_eq(md->ed25519_identity_pkey, &ri->cache_info.signing_key_cert->signing_key)); + // Try family encoding. + microdesc_free(md); + ri->declared_family = smartlist_new(); + smartlist_add_strdup(ri->declared_family, "OtherNode !Strange"); + md = dirvote_create_microdescriptor(ri, 28); + tt_str_op(md->body, OP_EQ, test_md2_withfamily_28); + + microdesc_free(md); + md = dirvote_create_microdescriptor(ri, 29); + tt_str_op(md->body, OP_EQ, test_md2_withfamily_29); + done: microdesc_free(md); routerinfo_free(ri); -- cgit v1.2.3-54-g00ecf From 05dee063c8d74437c792bdee2432293f97b6307c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sat, 24 Nov 2018 16:35:58 -0500 Subject: 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 | 5 ++ src/feature/relay/router.c | 135 +++++++++++++++++++++++++++++---------- src/feature/relay/router.h | 4 ++ src/test/test_router.c | 155 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 266 insertions(+), 33 deletions(-) (limited to 'src/test') diff --git a/changes/ticket28266 b/changes/ticket28266 index d90dc74f76..e0bc171080 100644 --- a/changes/ticket28266 +++ b/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. diff --git a/src/feature/relay/router.c b/src/feature/relay/router.c index 3f153e3a2d..d57fcc3a44 100644 --- a/src/feature/relay/router.c +++ b/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 digest */ +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 * options. 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. */ diff --git a/src/feature/relay/router.h b/src/feature/relay/router.h index 4575172afb..872eed1f46 100644 --- a/src/feature/relay/router.h +++ b/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) */ diff --git a/src/test/test_router.c b/src/test/test_router.c index 921ec42904..b92e96ff3e 100644 --- a/src/test/test_router.c +++ b/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 }; -- cgit v1.2.3-54-g00ecf