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 0c8e042c30946faa in #12538 in 0.2.8.1-alpha.

3 files changed, 49 insertions, 11 deletions

diff --git a/src/or/config.c b/src/or/config.c
index 5d938d101a..f07ca649f8 100644
--- a/src/or/config.c
+++ b/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 ||
diff --git a/src/or/dirserv.c b/src/or/dirserv.c
index a045f3ac55..5f848bd575 100644
--- a/src/or/dirserv.c
+++ b/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)
diff --git a/src/or/router.c b/src/or/router.c
index 68bcf1326e..545753c82e 100644
--- a/src/or/router.c
+++ b/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"));
   }