Decide to advertise begindir support like we decide to advertise DirPort

Decide to advertise begindir support in a similar way to how
we decide to advertise DirPort.

Fix up the associated descriptor-building unit tests.

Resolves #18616, bugfix on 0c8e042c30 in #12538 in 0.2.8.1-alpha.

src/or/config.c

@@ -7005,9 +7005,8 @@ get_first_listener_addrport_string(int listener_type)
 int
 get_first_advertised_port_by_type_af(int listener_type, int address_family)
 {
-  if (!configured_ports)
-    return 0;
-  SMARTLIST_FOREACH_BEGIN(configured_ports, const port_cfg_t *, cfg) {
+  const smartlist_t *conf_ports = get_configured_ports();
+  SMARTLIST_FOREACH_BEGIN(conf_ports, const port_cfg_t *, cfg) {
     if (cfg->type == listener_type &&
         !cfg->server_cfg.no_advertise &&
         (tor_addr_family(&cfg->addr) == address_family ||

src/or/dirserv.c

@@ -1131,8 +1131,11 @@ directory_caches_unknown_auth_certs(const or_options_t *options)
   return dir_server_mode(options) || options->BridgeRelay;
 }
 
-/** Return 1 if we want to keep descriptors, networkstatuses, etc around
- * and we're willing to serve them to others. Else return 0.
+/** Return 1 if we want to keep descriptors, networkstatuses, etc around.
+ * Else return 0.
+ * Check get_options()->DirPort_set and directory_permits_begindir_requests()
+ * to see if we are willing to serve these directory documents to others via
+ * the DirPort and begindir over ORPort, respectively.
  */
 int
 directory_caches_dir_info(const or_options_t *options)

src/or/router.c

@@ -1148,10 +1148,11 @@ router_should_be_directory_server(const or_options_t *options, int dir_port)
                        "seconds long. Raising to 1.");
       interval_length = 1;
     }
-    log_info(LD_GENERAL, "Calculating whether to disable dirport: effective "
+    log_info(LD_GENERAL, "Calculating whether to advertise %s: effective "
                          "bwrate: %u, AccountingMax: "U64_FORMAT", "
-                         "accounting interval length %d", effective_bw,
-                         U64_PRINTF_ARG(options->AccountingMax),
+                         "accounting interval length %d",
+                         dir_port ? "dirport" : "begindir",
+                         effective_bw, U64_PRINTF_ARG(options->AccountingMax),
                          interval_length);
 
     acc_bytes = options->AccountingMax;
@@ -1218,6 +1219,7 @@ decide_to_advertise_dirport(const or_options_t *options, uint16_t dir_port)
     return dir_port;
   if (net_is_disabled())
     return 0;
+  /* redundant - if DirPort is unreachable, we don't publish a descriptor */
   if (!check_whether_dirport_reachable())
     return 0;
   if (!router_get_advertised_dir_port(options, dir_port))
@@ -1229,6 +1231,39 @@ decide_to_advertise_dirport(const or_options_t *options, uint16_t dir_port)
   return router_should_be_directory_server(options, dir_port) ? dir_port : 0;
 }
 
+/** Look at a variety of factors, and return 0 if we don't want to
+ * advertise the fact that we support begindir requests, else return 1.
+ *
+ * Log a helpful message if we change our mind about whether to advertise
+ * support for begindir.
+ */
+static int
+decide_to_advertise_begindir(const or_options_t *options,
+                             int supports_tunnelled_dir_requests)
+{
+  /* Part one: reasons to publish or not publish that aren't
+   * worth mentioning to the user, either because they're obvious
+   * or because they're normal behavior. */
+
+  /* short circuit the rest of the function */
+  if (!supports_tunnelled_dir_requests)
+    return 0;
+  if (authdir_mode(options)) /* always publish */
+    return 1;
+  if (net_is_disabled())
+    return 0;
+  /* redundant - if ORPort is unreachable, we don't publish a descriptor */
+  if (!check_whether_orport_reachable())
+    return 0;
+  if (!router_get_advertised_or_port(options))
+    return 0;
+
+  /* Part two: reasons to publish or not publish that the user
+   * might find surprising. router_should_be_directory_server()
+   * considers config options that make us choose not to publish. */
+  return router_should_be_directory_server(options, 0);
+}
+
 /** Allocate and return a new extend_info_t that can be used to build
  * a circuit to or through the router <b>r</b>. Use the primary
  * address of the router unless <b>for_direct_connect</b> is true, in
@@ -1924,8 +1959,8 @@ router_build_fresh_descriptor(routerinfo_t **r, extrainfo_t **e)
   ri->addr = addr;
   ri->or_port = router_get_advertised_or_port(options);
   ri->dir_port = router_get_advertised_dir_port(options, 0);
-  ri->supports_tunnelled_dir_requests = dir_server_mode(options) &&
-    router_should_be_directory_server(options, ri->dir_port);
+  ri->supports_tunnelled_dir_requests =
+    directory_permits_begindir_requests(options);
   ri->cache_info.published_on = time(NULL);
   ri->onion_pkey = crypto_pk_dup_key(get_onion_key()); /* must invoke from
                                                         * main thread */
@@ -2706,7 +2741,8 @@ router_dump_router_to_string(routerinfo_t *router,
     tor_free(p6);
   }
 
-  if (router->supports_tunnelled_dir_requests) {
+  if (decide_to_advertise_begindir(options,
+                                   router->supports_tunnelled_dir_requests)) {
     smartlist_add(chunks, tor_strdup("tunnelled-dir-server\n"));
   }
 

src/test/test_dir.c

@@ -85,6 +85,15 @@ test_dir_nicknames(void *arg)
   ;
 }
 
+static smartlist_t *mocked_configured_ports = NULL;
+
+/** Returns mocked_configured_ports */
+static const smartlist_t *
+mock_get_configured_ports(void)
+{
+  return mocked_configured_ports;
+}
+
 /** Run unit tests for router descriptor generation logic. */
 static void
 test_dir_formats(void *arg)
@@ -104,6 +113,7 @@ test_dir_formats(void *arg)
   or_options_t *options = get_options_mutable();
   const addr_policy_t *p;
   time_t now = time(NULL);
+  port_cfg_t orport, dirport;
 
   (void)arg;
   pk1 = pk_generate(0);
@@ -185,9 +195,31 @@ test_dir_formats(void *arg)
   /* XXXX025 router_dump_to_string should really take this from ri.*/
   options->ContactInfo = tor_strdup("Magri White "
                                     "<magri@elsewhere.example.com>");
+  /* Skip reachability checks for DirPort and tunnelled-dir-server */
+  options->AssumeReachable = 1;
+
+  /* Fake just enough of an ORPort and DirPort to get by */
+  MOCK(get_configured_ports, mock_get_configured_ports);
+  mocked_configured_ports = smartlist_new();
+
+  memset(&orport, 0, sizeof(orport));
+  orport.type = CONN_TYPE_OR_LISTENER;
+  orport.addr.family = AF_INET;
+  orport.port = 9000;
+  smartlist_add(mocked_configured_ports, &orport);
+
+  memset(&dirport, 0, sizeof(dirport));
+  dirport.type = CONN_TYPE_DIR_LISTENER;
+  dirport.addr.family = AF_INET;
+  dirport.port = 9003;
+  smartlist_add(mocked_configured_ports, &dirport);
 
   buf = router_dump_router_to_string(r1, pk2, NULL, NULL, NULL);
 
+  UNMOCK(get_configured_ports);
+  smartlist_free(mocked_configured_ports);
+  mocked_configured_ports = NULL;
+
   tor_free(options->ContactInfo);
   tt_assert(buf);
 
@@ -308,6 +340,16 @@ test_dir_formats(void *arg)
   strlcat(buf2, "tunnelled-dir-server\n", sizeof(buf2));
   strlcat(buf2, "router-sig-ed25519 ", sizeof(buf2));
 
+  /* Fake just enough of an ORPort to get by */
+  MOCK(get_configured_ports, mock_get_configured_ports);
+  mocked_configured_ports = smartlist_new();
+
+  memset(&orport, 0, sizeof(orport));
+  orport.type = CONN_TYPE_OR_LISTENER;
+  orport.addr.family = AF_INET;
+  orport.port = 9005;
+  smartlist_add(mocked_configured_ports, &orport);
+
   buf = router_dump_router_to_string(r2, pk1, pk2, &r2_onion_keypair, &kp2);
   tt_assert(buf);
   buf[strlen(buf2)] = '\0'; /* Don't compare the sig; it's never the same
@@ -318,6 +360,10 @@ test_dir_formats(void *arg)
 
   buf = router_dump_router_to_string(r2, pk1, NULL, NULL, NULL);
 
+  UNMOCK(get_configured_ports);
+  smartlist_free(mocked_configured_ports);
+  mocked_configured_ports = NULL;
+
   /* Reset for later */
   cp = buf;
   rp2 = router_parse_entry_from_string((const char*)cp,NULL,1,0,NULL,NULL);