Refactor Single Onion code to improve consistency

* Check consistency between the two single onion torrc options
* Use the more relevant option each time we check for single onion mode
* Clarify log messages
* Clarify comments
* Otherwise, no behaviour change

src/or/circuitstats.c

@@ -104,7 +104,7 @@ circuit_build_times_disabled(void)
     int config_disabled = !options->LearnCircuitBuildTimeout;
     int dirauth_disabled = options->AuthoritativeDir;
     int state_disabled = did_last_state_file_write_fail() ? 1 : 0;
-    /* LearnCircuitBuildTimeout and Tor2webMode/OnionServiceSingleHopMode are
+    /* LearnCircuitBuildTimeout and Tor2web/Single Onion Services are
      * incompatible in two ways:
      *
      * - LearnCircuitBuildTimeout results in a low CBT, which

src/or/config.c

@@ -1730,17 +1730,17 @@ options_act(const or_options_t *old_options)
    * poisoning code checks for existing keys, and refuses to modify their
    * directories. */
 
-  /* If we use the insecure OnionServiceSingleHopMode, make sure we poison any
+  /* If we use non-anonymous single onion services, make sure we poison any
      new hidden service directories, so that we never accidentally launch the
      non-anonymous hidden services thinking they are anonymous. */
-  if (running_tor && rend_service_allow_non_anonymous_connection(options)) {
+  if (running_tor && rend_service_non_anonymous_mode_enabled(options)) {
     if (options->RendConfigLines && !num_rend_services()) {
       log_warn(LD_BUG,"Error: hidden services configured, but not parsed.");
       return -1;
     }
     if (rend_service_poison_new_single_onion_dirs(NULL) < 0) {
-      log_warn(LD_GENERAL,"Failed to mark new hidden services as Single "
+      log_warn(LD_GENERAL,"Failed to mark new hidden services as non-anonymous"
-               "Onion.");
+               ".");
       return -1;
     }
   }
@@ -2818,63 +2818,61 @@ warn_about_relative_paths(or_options_t *options)
   }
 }
 
-/* Validate options related to OnionServiceSingleHopMode.
+/* Validate options related to single onion services.
- * Modifies some options that are incompatible with OnionServiceSingleHopMode.
+ * Modifies some options that are incompatible with single onion services.
  * On failure returns -1, and sets *msg to an error string.
  * Returns 0 on success. */
 STATIC int
 options_validate_single_onion(or_options_t *options, char **msg)
 {
-  /* You must set OnionServiceNonAnonymousMode to 1 to use
+  /* The two single onion service options must have matching values. */
-   * OnionServiceSingleHopMode */
   if (options->OnionServiceSingleHopMode &&
-      !rend_service_non_anonymous_mode_enabled(options)) {
+      !options->OnionServiceNonAnonymousMode) {
     REJECT("OnionServiceSingleHopMode does not provide any server anonymity. "
            "It must be used with OnionServiceNonAnonymousMode set to 1.");
   }
-
+  if (options->OnionServiceNonAnonymousMode &&
-  /* If you have OnionServiceNonAnonymousMode set, you must use
-   * OnionServiceSingleHopMode. */
-  if (rend_service_non_anonymous_mode_enabled(options) &&
       !options->OnionServiceSingleHopMode) {
     REJECT("OnionServiceNonAnonymousMode does not provide any server "
            "anonymity. It must be used with OnionServiceSingleHopMode set to "
            "1.");
   }
 
+  /* Now that we've checked that the two options are consistent, we can safely
+   * call the rend_service_* functions that abstract these options. */
+
   /* If you run an anonymous client with an active Single Onion service, the
    * client loses anonymity. */
   const int client_port_set = (options->SocksPort_set ||
                                options->TransPort_set ||
                                options->NATDPort_set ||
                                options->DNSPort_set);
-  if (options->OnionServiceSingleHopMode && client_port_set &&
+  if (rend_service_non_anonymous_mode_enabled(options) && client_port_set &&
       !options->Tor2webMode) {
-    REJECT("OnionServiceSingleHopMode is incompatible with using Tor as an "
+    REJECT("OnionServiceNonAnonymousMode is incompatible with using Tor as an "
            "anonymous client. Please set Socks/Trans/NATD/DNSPort to 0, or "
-           "OnionServiceSingleHopMode to 0, or use the non-anonymous "
+           "OnionServiceNonAnonymousMode to 0, or use the non-anonymous "
            "Tor2webMode.");
   }
 
   /* If you run a hidden service in non-anonymous mode, the hidden service
    * loses anonymity, even if SOCKSPort / Tor2web mode isn't used. */
-  if (!options->OnionServiceSingleHopMode && options->RendConfigLines
+  if (!rend_service_non_anonymous_mode_enabled(options) &&
-      && options->Tor2webMode) {
+      options->RendConfigLines && options->Tor2webMode) {
     REJECT("Non-anonymous (Tor2web) mode is incompatible with using Tor as a "
            "hidden service. Please remove all HiddenServiceDir lines, or use "
            "a version of tor compiled without --enable-tor2web-mode, or use "
-           "the non-anonymous OnionServiceSingleHopMode.");
+           " OnionServiceNonAnonymousMode.");
   }
 
-  if (options->OnionServiceSingleHopMode
+  if (rend_service_allow_non_anonymous_connection(options)
       && options->UseEntryGuards) {
-    /* Single Onion services do not (and should not) use entry guards
+    /* Single Onion services only use entry guards when uploading descriptors,
-     * in any meaningful way.  Further, Single Onions causes the hidden
+     * all other connections are one-hop. Further, Single Onions causes the
-     * service code to do things which break the path bias
+     * hidden service code to do things which break the path bias
      * detector, and it's far easier to turn off entry guards (and
      * thus the path bias detector with it) than to figure out how to
-     * make a piece of code which cannot possibly help Single Onions,
+     * make path bias compatible with single onions.
-     * compatible with OnionServiceSingleHopMode.
      */
     log_notice(LD_CONFIG,
                "OnionServiceSingleHopMode is enabled; disabling "
@@ -2882,12 +2880,12 @@ options_validate_single_onion(or_options_t *options, char **msg)
     options->UseEntryGuards = 0;
   }
 
-  /* Check if existing hidden service keys were created with a different
+  /* Check if existing hidden service keys were created in a different
-   * setting of OnionServiceNonAnonymousMode, and refuse to launch if they
+   * single onion service mode, and refuse to launch if they
    * have. We'll poison new keys in options_act() just before we create them.
    */
   if (rend_service_list_verify_single_onion_poison(NULL, options) < 0) {
-    log_warn(LD_GENERAL, "We are configured with OnionServiceSingleHopMode "
+    log_warn(LD_GENERAL, "We are configured with OnionServiceNonAnonymousMode "
              "%d, but one or more hidden service keys were created in %s "
              "mode. This is not allowed.",
              rend_service_non_anonymous_mode_enabled(options) ? 1 : 0,
@@ -3427,7 +3425,7 @@ options_validate(or_options_t *old_options, or_options_t *options,
 
   if (!(options->UseEntryGuards) &&
       (options->RendConfigLines != NULL) &&
-      !rend_service_non_anonymous_mode_enabled(options)) {
+      !rend_service_allow_non_anonymous_connection(options)) {
     log_warn(LD_CONFIG,
              "UseEntryGuards is disabled, but you have configured one or more "
              "hidden services on this Tor instance.  Your hidden services "
@@ -3450,15 +3448,15 @@ options_validate(or_options_t *old_options, or_options_t *options,
     return -1;
   }
 
-  /* OnionServiceSingleHopMode: one hop between the onion service server and
+  /* Single Onion Services: non-anonymous hidden services */
-   * intro and rendezvous points */
+  if (rend_service_non_anonymous_mode_enabled(options)) {
-  if (options->OnionServiceSingleHopMode) {
     log_warn(LD_CONFIG,
-             "OnionServiceSingleHopMode is set. Every hidden service on this "
+             "OnionServiceNonAnonymousNode is set. Every hidden service on "
-             "tor instance is NON-ANONYMOUS. If OnionServiceSingleHopMode is "
+             "this tor instance is NON-ANONYMOUS. If "
-             "disabled, Tor will refuse to launch hidden services from the "
+             "the OnionServiceNonAnonymousMode option is changed, Tor will "
-             "same directories, to protect against config errors. This "
+             "refuse to launch hidden services from the same directories, to "
-             "setting is for experimental use only.");
+             "protect your anonymity against config errors. This setting is "
+             "for experimental use only.");
   }
 
   if (!options->LearnCircuitBuildTimeout && options->CircuitBuildTimeout &&

src/or/control.c

@@ -4249,7 +4249,7 @@ handle_control_add_onion(control_connection_t *conn,
   int max_streams = 0;
   int max_streams_close_circuit = 0;
   rend_auth_type_t auth_type = REND_NO_AUTH;
-  /* Default to anonymous if no flag is given */
+  /* Default to adding an anonymous hidden service if no flag is given */
   int non_anonymous = 0;
   for (size_t i = 1; i < arg_len; i++) {
     static const char *port_prefix = "Port=";
@@ -4288,10 +4288,9 @@ handle_control_add_onion(control_connection_t *conn,
        *                                exceeded.
        *   * 'BasicAuth' - Client authorization using the 'basic' method.
        *   * 'NonAnonymous' - Add a non-anonymous Single Onion Service. If this
-       *                      flag is present, OnionServiceSingleHopMode and
+       *                      flag is present, tor must be in non-anonymous
-       *                      OnionServiceNonAnonymousMode must both be 1. If
+       *                      hidden service mode. If this flag is absent,
-       *                      this flag is absent, both these options must be
+       *                      tor must be in anonymous hidden service mode.
-       *                      0.
        */
       static const char *discard_flag = "DiscardPK";
       static const char *detach_flag = "Detach";
@@ -4388,15 +4387,17 @@ handle_control_add_onion(control_connection_t *conn,
               smartlist_len(auth_clients) > 16)) {
     connection_printf_to_buf(conn, "512 Too many auth clients\r\n");
     goto out;
-  } else if (non_anonymous != rend_service_allow_non_anonymous_connection(
+  } else if (non_anonymous != rend_service_non_anonymous_mode_enabled(
                                                               get_options())) {
-    /* If we failed, and non-anonymous is set, Tor must be in anonymous mode.
+    /* If we failed, and the non-anonymous flag is set, Tor must be in
+     * anonymous hidden service mode.
      * The error message changes based on the current Tor config:
-     * 512 Tor is in anonymous onion mode
+     * 512 Tor is in anonymous hidden service mode
-     * 512 Tor is in non-anonymous onion mode
+     * 512 Tor is in non-anonymous hidden service mode
      * (I've deliberately written them out in full here to aid searchability.)
      */
-    connection_printf_to_buf(conn, "512 Tor is in %sanonymous onion mode\r\n",
+    connection_printf_to_buf(conn, "512 Tor is in %sanonymous hidden service "
+                             "mode\r\n",
                              non_anonymous ? "" : "non-");
     goto out;
   }

src/or/or.h

@@ -3706,8 +3706,7 @@ typedef struct {
    * rendezvous points. (Onion service descriptors are still posted using
    * 3-hop paths, to avoid onion service directories blocking the service.)
    * This option makes every hidden service instance hosted by
-   * this tor instance a Single Onion Service. One-hop circuits make Single
+   * this tor instance a Single Onion Service.
-   * Onion servers easily locatable, but clients remain location-anonymous.
    * OnionServiceSingleHopMode requires OnionServiceNonAnonymousMode to be set
    * to 1.
    * Use rend_service_allow_non_anonymous_connection() or
@@ -3716,8 +3715,8 @@ typedef struct {
   int OnionServiceSingleHopMode;
   /* Makes hidden service clients and servers non-anonymous on this tor
    * instance. Allows the non-anonymous OnionServiceSingleHopMode. Enables
-   * direct connections in the hidden service protocol.
+   * non-anonymous behaviour in the hidden service protocol.
-   * Use rend_service_non_anonymous_mode() instead of using this option
+   * Use rend_service_non_anonymous_mode_enabled() instead of using this option
    * directly.
    */
   int OnionServiceNonAnonymousMode;

src/or/rendcommon.c

@@ -1097,11 +1097,11 @@ rend_non_anonymous_mode_enabled(const or_options_t *options)
 /* Make sure that tor only builds one-hop circuits when they would not
  * compromise user anonymity.
  *
- * One-hop circuits are permitted in Tor2webMode or OnionServiceSingleHopMode.
+ * One-hop circuits are permitted in Tor2web or Single Onion modes.
  *
- * Tor2webMode and OnionServiceSingleHopMode are also allowed to make
+ * Tor2web or Single Onion modes are also allowed to make multi-hop circuits.
- * multi-hop circuits. For example, single onion HSDir circuits are 3-hop to
+ * For example, single onion HSDir circuits are 3-hop to prevent denial of
- * prevent denial of service.
+ * service.
  */
 void
 assert_circ_anonymity_ok(origin_circuit_t *circ,

src/or/rendservice.c

@@ -1025,10 +1025,10 @@ rend_service_private_key_exists(const rend_service_t *service)
 
 /** Check the single onion service poison state of all existing hidden service
  * directories:
- * - If each service is poisoned, and we are in OnionServiceSingleHopMode,
+ * - If each service is poisoned, and we are in Single Onion Mode,
+ *   return 0,
+ * - If each service is not poisoned, and we are not in Single Onion Mode,
  *   return 0,
- * - If each service is not poisoned, and we are not in
- *   OnionServiceSingleHopMode, return 0,
  * - Otherwise, the poison state is invalid, and a service that was created in
  *   one mode is being used in the other, return -1.
  * Hidden service directories without keys are not checked for consistency.
@@ -1054,7 +1054,7 @@ rend_service_list_verify_single_onion_poison(const smartlist_t *service_list,
   int consistent = 1;
   SMARTLIST_FOREACH_BEGIN(s_list, const rend_service_t *, s) {
     if (service_is_single_onion_poisoned(s) !=
-        rend_service_allow_non_anonymous_connection(options) &&
+        rend_service_non_anonymous_mode_enabled(options) &&
         rend_service_private_key_exists(s)) {
       consistent = 0;
     }
@@ -1063,25 +1063,25 @@ rend_service_list_verify_single_onion_poison(const smartlist_t *service_list,
   return consistent ? 0 : -1;
 }
 
-/*** Helper for rend_service_poison_new_single_onion_dirs(). When in single
+/*** Helper for rend_service_poison_new_single_onion_dirs(). Add a file to
- * onion mode, add a file to this hidden service directory that marks it as a
+ * this hidden service directory that marks it as a single onion service.
- * single onion hidden service. Returns 0 when a directory is successfully
+ * Tor must be in single onion mode before calling this function.
- * poisoned, or if it is already poisoned. Returns -1 on a failure to read
+ * Returns 0 when a directory is successfully poisoned, or if it is already
- * the directory or write the poison file, or if there is an existing private
+ * poisoned. Returns -1 on a failure to read the directory or write the poison
- * key file in the directory. (The service should have been poisoned when the
+ * file, or if there is an existing private key file in the directory. (The
- * key was created.) */
+ * service should have been poisoned when the key was created.) */
 static int
 poison_new_single_onion_hidden_service_dir(const rend_service_t *service)
 {
   /* We must only poison directories if we're in Single Onion mode */
-  tor_assert(rend_service_allow_non_anonymous_connection(get_options()));
+  tor_assert(rend_service_non_anonymous_mode_enabled(get_options()));
 
   int fd;
   int retval = -1;
   char *poison_fname = NULL;
 
   if (!service->directory) {
-    log_info(LD_REND, "Ephemeral HS started in OnionServiceSingleHopMode.");
+    log_info(LD_REND, "Ephemeral HS started in non-anonymous mode.");
     return 0;
   }
 
@@ -1126,7 +1126,7 @@ poison_new_single_onion_hidden_service_dir(const rend_service_t *service)
   return retval;
 }
 
-/** We just got launched in OnionServiceSingleHopMode. That's a non-anoymous
+/** We just got launched in Single Onion Mode. That's a non-anoymous
  * mode for hidden services; hence we should mark all new hidden service
  * directories appropriately so that they are never launched as
  * location-private hidden services again. (New directories don't have private
@@ -1138,7 +1138,7 @@ int
 rend_service_poison_new_single_onion_dirs(const smartlist_t *service_list)
 {
   /* We must only poison directories if we're in Single Onion mode */
-  tor_assert(rend_service_allow_non_anonymous_connection(get_options()));
+  tor_assert(rend_service_non_anonymous_mode_enabled(get_options()));
 
   const smartlist_t *s_list;
   /* If no special service list is provided, then just use the global one. */
@@ -4179,12 +4179,25 @@ rend_service_set_connection_addr_port(edge_connection_t *conn,
     return -2;
 }
 
+/* Are OnionServiceSingleHopMode and OnionServiceNonAnonymousMode consistent?
+ */
+static int
+rend_service_non_anonymous_mode_consistent(const or_options_t *options)
+{
+  /* !! is used to make these options boolean */
+  return (!! options->OnionServiceSingleHopMode ==
+          !! options->OnionServiceNonAnonymousMode);
+}
+
 /* Do the options allow onion services to make direct (non-anonymous)
  * connections to introduction or rendezvous points?
+ * Must only be called after options_validate_single_onion() has successfully
+ * checked onion service option consistency.
  * Returns true if tor is in OnionServiceSingleHopMode. */
 int
 rend_service_allow_non_anonymous_connection(const or_options_t *options)
 {
+  tor_assert(rend_service_non_anonymous_mode_consistent(options));
   return options->OnionServiceSingleHopMode ? 1 : 0;
 }
 
@@ -4192,17 +4205,24 @@ rend_service_allow_non_anonymous_connection(const or_options_t *options)
  * service?
  * Single Onion Services prioritise availability over hiding their
  * startup time, as their IP address is publicly discoverable anyway.
- * Returns true if tor is in OnionServiceSingleHopMode. */
+ * Must only be called after options_validate_single_onion() has successfully
+ * checked onion service option consistency.
+ * Returns true if tor is in non-anonymous hidden service mode. */
 int
 rend_service_reveal_startup_time(const or_options_t *options)
 {
-  return rend_service_allow_non_anonymous_connection(options);
+  tor_assert(rend_service_non_anonymous_mode_consistent(options));
+  return rend_service_non_anonymous_mode_enabled(options);
 }
 
 /* Is non-anonymous mode enabled using the OnionServiceNonAnonymousMode
- * config option? */
+ * config option?
+ * Must only be called after options_validate_single_onion() has successfully
+ * checked onion service option consistency.
+ */
 int
 rend_service_non_anonymous_mode_enabled(const or_options_t *options)
 {
+  tor_assert(rend_service_non_anonymous_mode_consistent(options));
   return options->OnionServiceNonAnonymousMode ? 1 : 0;
 }

src/test/test_options.c

@@ -2813,10 +2813,10 @@ test_options_validate__single_onion(void *ignored)
                                 );
   ret = options_validate(tdata->old_opt, tdata->opt, tdata->def_opt, 0, &msg);
   tt_int_op(ret, OP_EQ, -1);
-  tt_str_op(msg, OP_EQ, "OnionServiceSingleHopMode is incompatible with using "
+  tt_str_op(msg, OP_EQ, "OnionServiceNonAnonymousMode is incompatible with "
-            "Tor as an anonymous client. Please set Socks/Trans/NATD/DNSPort "
+            "using Tor as an anonymous client. Please set "
-            "to 0, or OnionServiceSingleHopMode to 0, or use the "
+            "Socks/Trans/NATD/DNSPort to 0, or OnionServiceNonAnonymousMode "
-            "non-anonymous Tor2webMode.");
+            "to 0, or use the non-anonymous Tor2webMode.");
   tor_free(msg);
   free_options_test_data(tdata);