prop224: Add hs_config.{c|h} with a refactoring

Add the hs_config.{c|h} files contains everything that the HS subsystem needs
to load and configure services. Ultimately, it should also contain client
functions such as client authorization.

This comes with a big refactoring of rend_config_services() which has now
changed to only configure a single service and it is stripped down of the
common directives which are now part of the generic handler.

This is ground work for prop224 of course but only touches version 2 services
and add XXX note for version 3.

Signed-off-by: David Goulet <dgoulet@torproject.org>

src/or/config.c

@@ -91,6 +91,7 @@
 #include "relay.h"
 #include "rendclient.h"
 #include "rendservice.h"
+#include "hs_config.h"
 #include "rephist.h"
 #include "router.h"
 #include "sandbox.h"
@@ -1681,7 +1682,7 @@ options_act(const or_options_t *old_options)
     sweep_bridge_list();
   }
 
-  if (running_tor && rend_config_services(options, 0)<0) {
+  if (running_tor && hs_config_service_all(options, 0)<0) {
     log_warn(LD_BUG,
        "Previously validated hidden services line could not be added!");
     return -1;
@@ -4009,7 +4010,7 @@ options_validate(or_options_t *old_options, or_options_t *options,
     COMPLAIN("V3AuthVotingInterval does not divide evenly into 24 hours.");
   }
 
-  if (rend_config_services(options, 1) < 0)
+  if (hs_config_service_all(options, 1) < 0)
     REJECT("Failed to configure rendezvous options. See logs for details.");
 
   /* Parse client-side authorization for hidden services. */

src/or/hs_common.h

@@ -16,6 +16,9 @@
 #define HS_VERSION_TWO 2
 /* Version 3 of the protocol (prop224). */
 #define HS_VERSION_THREE 3
+/* Earliest and latest version we support. */
+#define HS_VERSION_MIN HS_VERSION_TWO
+#define HS_VERSION_MAX HS_VERSION_THREE
 
 /** Try to maintain this many intro points per service by default. */
 #define NUM_INTRO_POINTS_DEFAULT 3

src/or/hs_config.c

@@ -0,0 +1,292 @@
+/* Copyright (c) 2017, The Tor Project, Inc. */
+/* See LICENSE for licensing information */
+
+/**
+ * \file hs_config.c
+ * \brief Implement hidden service configuration subsystem.
+ *
+ * \details
+ *
+ * This file has basically one main entry point: hs_config_service_all(). It
+ * takes the torrc options and configure hidden service from it. In validate
+ * mode, nothing is added to the global service list or keys are not generated
+ * nor loaded.
+ *
+ * A service is configured in two steps. It is first created using the tor
+ * options and then put in a staging list. It will stay there until
+ * hs_service_load_all_keys() is called. That function is responsible to
+ * load/generate the keys for the service in the staging list and if
+ * successful, transfert the service to the main global service list where
+ * at that point it is ready to be used.
+ *
+ * Configuration handlers are per-version (see config_service_handlers[]) and
+ * there is a main generic one for every option that is common to all version
+ * (config_generic_service).
+ **/
+
+#define HS_CONFIG_PRIVATE
+
+#include "hs_common.h"
+#include "hs_config.h"
+#include "hs_service.h"
+#include "rendservice.h"
+
+/* Configuration handler for a version 3 service. Return 0 on success else a
+ * negative value. */
+static int
+config_service_v3(const config_line_t *line,
+                  const or_options_t *options, int validate_only,
+                  hs_service_t *service)
+{
+  (void) line;
+  (void) service;
+  (void) validate_only;
+  (void) options;
+  /* XXX: Configure a v3 service with specific options. */
+  /* XXX: Add service to v3 list and pruning on reload. */
+  return 0;
+}
+
+/* Configure a service using the given options in line_ and options. This is
+ * called for any service regardless of its version which means that all
+ * directives in this function are generic to any service version. This
+ * function will also check the validity of the service directory path.
+ *
+ * The line_ must be pointing to the directive directly after a
+ * HiddenServiceDir. That way, when hitting the next HiddenServiceDir line or
+ * reaching the end of the list of lines, we know that we have to stop looking
+ * for more options.
+ *
+ * Return 0 on success else -1. */
+static int
+config_generic_service(const config_line_t *line_,
+                       const or_options_t *options,
+                       hs_service_t *service)
+{
+  int ok, dir_seen = 0;
+  const config_line_t *line;
+  hs_service_config_t *config;
+
+  tor_assert(line_);
+  tor_assert(options);
+  tor_assert(service);
+
+  /* Makes thing easier. */
+  config = &service->config;
+  memset(config, 0, sizeof(*config));
+
+  /* The first line starts with HiddenServiceDir so we consider what's next is
+   * the configuration of the service. */
+  for (line = line_; line ; line = line->next) {
+    /* This indicate that we have a new service to configure. */
+    if (!strcasecmp(line->key, "HiddenServiceDir")) {
+      /* This function only configures one service at a time so if we've
+       * already seen one, stop right now. */
+      if (dir_seen) {
+        break;
+      }
+      /* Ok, we've seen one and we are about to configure it. */
+      dir_seen = 1;
+      config->directory_path = tor_strdup(line->value);
+      log_info(LD_CONFIG, "HiddenServiceDir=%s. Configuring...",
+               escaped(config->directory_path));
+      continue;
+    }
+    if (BUG(!dir_seen)) {
+      goto err;
+    }
+    /* Version of the service. */
+    if (!strcasecmp(line->key, "HiddenServiceVersion")) {
+      service->version = (uint32_t) tor_parse_ulong(line->value,
+                                                    10, HS_VERSION_TWO,
+                                                    HS_VERSION_MAX,
+                                                    &ok, NULL);
+      if (!ok) {
+        log_warn(LD_CONFIG,
+                 "HiddenServiceVersion be between %u and %u, not %s",
+                 HS_VERSION_TWO, HS_VERSION_MAX, line->value);
+        goto err;
+      }
+      log_info(LD_CONFIG, "HiddenServiceVersion=%" PRIu32 " for %s",
+               service->version, escaped(config->directory_path));
+      continue;
+    }
+    /* Virtual port. */
+    if (!strcasecmp(line->key, "HiddenServicePort")) {
+      char *err_msg = NULL;
+      /* XXX: Can we rename this? */
+      rend_service_port_config_t *portcfg =
+        rend_service_parse_port_config(line->value, " ", &err_msg);
+      if (!portcfg) {
+        if (err_msg) {
+          log_warn(LD_CONFIG, "%s", err_msg);
+        }
+        tor_free(err_msg);
+        goto err;
+      }
+      tor_assert(!err_msg);
+      smartlist_add(config->ports, portcfg);
+      log_info(LD_CONFIG, "HiddenServicePort=%s for %s",
+               line->value, escaped(config->directory_path));
+      continue;
+    }
+    /* Do we allow unknown ports. */
+    if (!strcasecmp(line->key, "HiddenServiceAllowUnknownPorts")) {
+      config->allow_unknown_ports = (unsigned int) tor_parse_long(line->value,
+                                                                  10, 0, 1,
+                                                                  &ok, NULL);
+      if (!ok) {
+        log_warn(LD_CONFIG,
+                 "HiddenServiceAllowUnknownPorts should be 0 or 1, not %s",
+                 line->value);
+        goto err;
+      }
+      log_info(LD_CONFIG,
+               "HiddenServiceAllowUnknownPorts=%u for %s",
+               config->allow_unknown_ports, escaped(config->directory_path));
+      continue;
+    }
+    /* Directory group readable. */
+    if (!strcasecmp(line->key, "HiddenServiceDirGroupReadable")) {
+      config->dir_group_readable = (unsigned int) tor_parse_long(line->value,
+                                                                 10, 0, 1,
+                                                                 &ok, NULL);
+      if (!ok) {
+        log_warn(LD_CONFIG,
+                 "HiddenServiceDirGroupReadable should be 0 or 1, not %s",
+                 line->value);
+        goto err;
+      }
+      log_info(LD_CONFIG,
+               "HiddenServiceDirGroupReadable=%u for %s",
+               config->dir_group_readable, escaped(config->directory_path));
+      continue;
+    }
+    /* Maximum streams per circuit. */
+    if (!strcasecmp(line->key, "HiddenServiceMaxStreams")) {
+      config->max_streams_per_rdv_circuit = tor_parse_uint64(line->value,
+                                                             10, 0, 65535,
+                                                             &ok, NULL);
+      if (!ok) {
+        log_warn(LD_CONFIG,
+                 "HiddenServiceMaxStreams should be between 0 and %d, not %s",
+                 65535, line->value);
+        goto err;
+      }
+      log_info(LD_CONFIG,
+               "HiddenServiceMaxStreams=%" PRIu64 " for %s",
+               config->max_streams_per_rdv_circuit,
+               escaped(config->directory_path));
+      continue;
+    }
+    /* Maximum amount of streams before we close the circuit. */
+    if (!strcasecmp(line->key, "HiddenServiceMaxStreamsCloseCircuit")) {
+      config->max_streams_close_circuit =
+        (unsigned int) tor_parse_long(line->value, 10, 0, 1, &ok, NULL);
+      if (!ok) {
+        log_warn(LD_CONFIG,
+                 "HiddenServiceMaxStreamsCloseCircuit should be 0 or 1, "
+                 "not %s", line->value);
+        goto err;
+      }
+      log_info(LD_CONFIG,
+               "HiddenServiceMaxStreamsCloseCircuit=%u for %s",
+               config->max_streams_close_circuit,
+               escaped(config->directory_path));
+      continue;
+    }
+  }
+
+  /* Check permission on service directory. */
+  if (hs_check_service_private_dir(options->User, config->directory_path,
+                                   config->dir_group_readable, 0) < 0) {
+    goto err;
+  }
+
+  /* Check if we are configured in non anonymous mode and single hop mode
+   * meaning every service become single onion. */
+  if (rend_service_allow_non_anonymous_connection(options) &&
+      rend_service_non_anonymous_mode_enabled(options)) {
+    config->is_single_onion = 1;
+  }
+
+  /* Success */
+  return 0;
+ err:
+  return -1;
+}
+
+/* Configuration handler indexed by version number. */
+static int
+  (*config_service_handlers[])(const config_line_t *line,
+                               const or_options_t *options,
+                               int validate_only,
+                               hs_service_t *service) =
+{
+  NULL, /* v0 */
+  NULL, /* v1 */
+  rend_config_service, /* v2 */
+  config_service_v3, /* v3 */
+};
+
+/* From a set of <b>options</b>, setup every hidden service found. Return 0 on
+ * success or -1 on failure. If <b>validate_only</b> is set, parse, warn and
+ * return as normal, but don't actually change the configured services. */
+int
+hs_config_service_all(const or_options_t *options, int validate_only)
+{
+  int dir_option_seen = 0;
+  hs_service_t *service = NULL;
+  const config_line_t *line;
+
+  tor_assert(options);
+
+  for (line = options->RendConfigLines; line; line = line->next) {
+    if (!strcasecmp(line->key, "HiddenServiceDir")) {
+      /* We have a new hidden service. */
+      service = hs_service_new(options);
+      /* We'll configure that service as a generic one and then pass it to the
+       * specific handler according to the configured version number. */
+      if (config_generic_service(line, options, service) < 0) {
+        goto err;
+      }
+      tor_assert(service->version <= HS_VERSION_MAX);
+      /* The handler is in charge of specific options for a version. We start
+       * after this service directory line so once we hit another directory
+       * line, the handler knows that it has to stop. */
+      if (config_service_handlers[service->version](line->next, options,
+                                                    validate_only,
+                                                    service) < 0) {
+        goto err;
+      }
+      /* Whatever happens, on success we loose the ownership of the service
+       * object so we nullify the pointer to be safe. */
+      service = NULL;
+      /* Flag that we've seen a directory directive and we'll use that to make
+       * sure that the torrc options ordering are actually valid. */
+      dir_option_seen = 1;
+      continue;
+    }
+    /* The first line must be a directory option else tor is misconfigured. */
+    if (!dir_option_seen) {
+      log_warn(LD_CONFIG, "%s with no preceding HiddenServiceDir directive",
+               line->key);
+      goto err;
+    }
+  }
+
+  if (!validate_only) {
+    /* Trigger service pruning which will make sure the just configured
+     * services end up in the main global list. This is v2 specific. */
+    rend_service_prune_list();
+    /* XXX: Need the v3 one. */
+  }
+
+  /* Success. */
+  return 0;
+ err:
+  hs_service_free(service);
+  /* Tor main should call the free all function. */
+  return -1;
+}
+

src/or/hs_config.h

@@ -0,0 +1,19 @@
+/* Copyright (c) 2016, The Tor Project, Inc. */
+/* See LICENSE for licensing information */
+
+/**
+ * \file hs_config.h
+ * \brief Header file containing configuration ABI/API for the HS subsytem.
+ **/
+
+#ifndef TOR_HS_CONFIG_H
+#define TOR_HS_CONFIG_H
+
+#include "or.h"
+
+/* API */
+
+int hs_config_service_all(const or_options_t *options, int validate_only);
+
+#endif /* TOR_HS_CONFIG_H */
+

src/or/hs_service.c

@@ -19,6 +19,91 @@
 #include "hs/cell_establish_intro.h"
 #include "hs/cell_common.h"
 
+/* Set the default values for a service configuration object <b>c</b>. */
+static void
+set_service_default_config(hs_service_config_t *c,
+                           const or_options_t *options)
+{
+  tor_assert(c);
+  c->ports = smartlist_new();
+  c->directory_path = NULL;
+  c->descriptor_post_period = options->RendPostPeriod;
+  c->max_streams_per_rdv_circuit = 0;
+  c->max_streams_close_circuit = 0;
+  c->num_intro_points = NUM_INTRO_POINTS_DEFAULT;
+  c->allow_unknown_ports = 0;
+  c->is_single_onion = 0;
+  c->dir_group_readable = 0;
+  c->is_ephemeral = 0;
+}
+
+/* Allocate and initilize a service object. The service configuration will
+ * contain the default values. Return the newly allocated object pointer. This
+ * function can't fail. */
+hs_service_t *
+hs_service_new(const or_options_t *options)
+{
+  hs_service_t *service = tor_malloc_zero(sizeof(hs_service_t));
+  /* Set default configuration value. */
+  set_service_default_config(&service->config, options);
+  /* Set the default service version. */
+  service->version = HS_SERVICE_DEFAULT_VERSION;
+  return service;
+}
+
+/* Free the given <b>service</b> object and all its content. This function
+ * also takes care of wiping service keys from memory. It is safe to pass a
+ * NULL pointer. */
+void
+hs_service_free(hs_service_t *service)
+{
+  if (service == NULL) {
+    return;
+  }
+
+  /* Free descriptors. */
+  if (service->desc_current) {
+    hs_descriptor_free(service->desc_current->desc);
+    /* Wipe keys. */
+    memwipe(&service->desc_current->signing_kp, 0,
+            sizeof(service->desc_current->signing_kp));
+    memwipe(&service->desc_current->blinded_kp, 0,
+            sizeof(service->desc_current->blinded_kp));
+    /* XXX: Free intro points. */
+    tor_free(service->desc_current);
+  }
+  if (service->desc_next) {
+    hs_descriptor_free(service->desc_next->desc);
+    /* Wipe keys. */
+    memwipe(&service->desc_next->signing_kp, 0,
+            sizeof(service->desc_next->signing_kp));
+    memwipe(&service->desc_next->blinded_kp, 0,
+            sizeof(service->desc_next->blinded_kp));
+    /* XXX: Free intro points. */
+    tor_free(service->desc_next);
+  }
+
+  /* Free service configuration. */
+  tor_free(service->config.directory_path);
+  if (service->config.ports) {
+    SMARTLIST_FOREACH(service->config.ports, rend_service_port_config_t *, p,
+                      rend_service_port_config_free(p););
+    smartlist_free(service->config.ports);
+  }
+
+  /* Wipe service keys. */
+  memwipe(&service->keys.identity_sk, 0, sizeof(service->keys.identity_sk));
+
+  tor_free(service);
+}
+
+/* Release all global the storage of hidden service subsystem. */
+void
+hs_service_free_all(void)
+{
+  rend_service_free_all();
+}
+
 /* XXX We don't currently use these functions, apart from generating unittest
    data. When we start implementing the service-side support for prop224 we
    should revisit these functions and use them. */

src/or/hs_service.h

@@ -192,6 +192,12 @@ typedef struct hs_service_t {
 
 /* API */
 
+int hs_service_config_all(const or_options_t *options, int validate_only);
+void hs_service_free_all(void);
+
+void hs_service_free(hs_service_t *service);
+hs_service_t *hs_service_new(const or_options_t *options);
+
 /* These functions are only used by unit tests and we need to expose them else
  * hs_service.o ends up with no symbols in libor.a which makes clang throw a
  * warning at compile time. See #21825. */

src/or/include.am

@@ -50,19 +50,20 @@ LIBTOR_A_SOURCES = \
 	src/or/dnsserv.c				\
 	src/or/fp_pair.c				\
 	src/or/geoip.c					\
+	src/or/entrynodes.c				\
+	src/or/ext_orport.c				\
+	src/or/hibernate.c				\
 	src/or/hs_cache.c				\
+	src/or/hs_circuit.c				\
 	src/or/hs_circuitmap.c				\
+	src/or/hs_client.c				\
 	src/or/hs_common.c				\
-	src/or/hs_circuit.c				\
+	src/or/hs_config.c				\
 	src/or/hs_descriptor.c				\
 	src/or/hs_ident.c				\
 	src/or/hs_intropoint.c				\
 	src/or/hs_ntor.c				\
 	src/or/hs_service.c				\
-	src/or/hs_client.c				\
-	src/or/entrynodes.c				\
-	src/or/ext_orport.c				\
-	src/or/hibernate.c				\
 	src/or/keypin.c					\
 	src/or/main.c					\
 	src/or/microdesc.c				\
@@ -183,15 +184,16 @@ ORHEADERS = \
 	src/or/entrynodes.h				\
 	src/or/hibernate.h				\
 	src/or/hs_cache.h				\
-	src/or/hs_common.h				\
 	src/or/hs_circuit.h				\
+	src/or/hs_circuitmap.h				\
+	src/or/hs_client.h				\
+	src/or/hs_common.h				\
+	src/or/hs_config.h				\
 	src/or/hs_descriptor.h				\
 	src/or/hs_ident.h				\
-	src/or/hs_intropoint.h          \
-	src/or/hs_circuitmap.h          \
-	src/or/hs_ntor.h                \
-	src/or/hs_service.h             \
-	src/or/hs_client.h              \
+	src/or/hs_intropoint.h				\
+	src/or/hs_ntor.h				\
+	src/or/hs_service.h				\
 	src/or/keypin.h					\
 	src/or/main.h					\
 	src/or/microdesc.h				\

src/or/main.c

@@ -3216,7 +3216,7 @@ tor_free_all(int postfork)
   networkstatus_free_all();
   addressmap_free_all();
   dirserv_free_all();
-  rend_service_free_all();
+  hs_service_free_all();
   rend_cache_free_all();
   rend_service_authorization_free_all();
   hs_cache_free_all();

src/or/rendservice.c

@@ -236,13 +236,18 @@ rend_service_free(rend_service_t *service)
 void
 rend_service_free_all(void)
 {
-  if (!rend_service_list)
-    return;
-
-  SMARTLIST_FOREACH(rend_service_list, rend_service_t*, ptr,
-                    rend_service_free(ptr));
-  smartlist_free(rend_service_list);
-  rend_service_list = NULL;
+  if (rend_service_list) {
+    SMARTLIST_FOREACH(rend_service_list, rend_service_t*, ptr,
+                      rend_service_free(ptr));
+    smartlist_free(rend_service_list);
+    rend_service_list = NULL;
+  }
+  if (rend_service_staging_list) {
+    SMARTLIST_FOREACH(rend_service_staging_list, rend_service_t*, ptr,
+                      rend_service_free(ptr));
+    smartlist_free(rend_service_staging_list);
+    rend_service_staging_list = NULL;
+  }
 }
 
 /* Validate a <b>service</b>. Use the <b>service_list</b> to make sure there
@@ -335,6 +340,7 @@ rend_add_service(smartlist_t *service_list, rend_service_t *service)
   /* We must have a service list, even if it's a temporary one, so we can
    * check for duplicate services */
   if (BUG(!s_list)) {
+    rend_service_free(service);
     return -1;
   }
 
@@ -496,41 +502,6 @@ rend_service_port_config_free(rend_service_port_config_t *p)
   tor_free(p);
 }
 
-/* Check the directory for <b>service</b>, and add the service to
- * <b>service_list</b>, or to the global list if <b>service_list</b> is NULL.
- * Only add the service to the list if <b>validate_only</b> is false.
- * If <b>validate_only</b> is true, free the service.
- * If <b>service</b> is NULL, ignore it, and return 0.
- * Returns 0 on success, and -1 on failure.
- * Takes ownership of <b>service</b>, either freeing it, or adding it to the
- * global service list.
- */
-STATIC int
-rend_service_check_dir_and_add(smartlist_t *service_list,
-                               const or_options_t *options,
-                               rend_service_t *service,
-                               int validate_only)
-{
-  if (!service) {
-    /* It is ok for a service to be NULL, this means there are no services */
-    return 0;
-  }
-
-  if (rend_service_check_private_dir(options, service, !validate_only)
-      < 0) {
-    rend_service_free(service);
-    return -1;
-  }
-
-  smartlist_t *s_list = rend_get_service_list_mutable(service_list);
-  /* We must have a service list, even if it's a temporary one, so we can
-   * check for duplicate services */
-  if (BUG(!s_list)) {
-    return -1;
-  }
-  return rend_add_service(s_list, service);
-}
-
 /* Helper: Actual implementation of the pruning on reload which we've
  * decoupled in order to make the unit test workeable without ugly hacks.
  * Furthermore, this function does NOT free any memory but will nullify the
@@ -657,19 +628,51 @@ rend_service_prune_list(void)
   }
 }
 
-/** Set up rend_service_list, based on the values of HiddenServiceDir and
- * HiddenServicePort in <b>options</b>.  Return 0 on success and -1 on
- * failure.  (If <b>validate_only</b> is set, parse, warn and return as
- * normal, but don't actually change the configured services.)
- */
+/* Copy all the relevant data that the hs_service object contains over to the
+ * rend_service_t object. The reason to do so is because when configuring a
+ * service, we go through a generic handler that creates an hs_service_t
+ * object which so we have to copy the parsed values to a rend service object
+ * which is version 2 specific. */
+static void
+service_shadow_copy(rend_service_t *service, hs_service_t *hs_service)
+{
+  hs_service_config_t *config;
+
+  tor_assert(service);
+  tor_assert(hs_service);
+
+  config = &hs_service->config;
+  service->directory = tor_strdup(config->directory_path);
+  service->dir_group_readable = config->dir_group_readable;
+  service->allow_unknown_ports = config->allow_unknown_ports;
+  service->max_streams_per_circuit = config->max_streams_per_rdv_circuit;
+  service->max_streams_close_circuit = config->max_streams_close_circuit;
+  service->n_intro_points_wanted = config->num_intro_points;
+  /* Switching ownership of the ports to the rend service object. */
+  smartlist_add_all(service->ports, config->ports);
+  smartlist_free(hs_service->config.ports);
+  hs_service->config.ports = NULL;
+}
+
+/* Parse the hidden service configuration starting at <b>line_</b> using the
+ * already configured generic service in <b>hs_service</b>. This function will
+ * translate the service object to a rend_service_t and add it to the
+ * temporary list if valid. If <b>validate_only</b> is set, parse, warn and
+ * return as normal but don't actually add the service to the list. */
 int
-rend_config_services(const or_options_t *options, int validate_only)
+rend_config_service(const config_line_t *line_,
+                    const or_options_t *options,
+                    int validate_only,
+                    hs_service_t *hs_service)
 {
-  config_line_t *line;
+  (void) validate_only;
+  const config_line_t *line;
   rend_service_t *service = NULL;
-  rend_service_port_config_t *portcfg;
-  int ok = 0;
-  int rv = -1;
+
+  /* line_ can be NULL which would mean that the service configuration only
+   * have one line that is the directory directive. */
+  tor_assert(options);
+  tor_assert(hs_service);
 
   /* Use the staging service list so that we can check then do the pruning
    * process using the main list at the end. */
@@ -677,100 +680,23 @@ rend_config_services(const or_options_t *options, int validate_only)
     rend_service_staging_list = smartlist_new();
   }
 
-  for (line = options->RendConfigLines; line; line = line->next) {
+  /* Initialize service. */
+  service = tor_malloc_zero(sizeof(rend_service_t));
+  service->intro_period_started = time(NULL);
+  service->ports = smartlist_new();
+  /* From the hs_service object which has been used to load the generic
+   * options, we'll copy over the useful data to the rend_service_t object. */
+  service_shadow_copy(service, hs_service);
+
+  for (line = line_; line; line = line->next) {
     if (!strcasecmp(line->key, "HiddenServiceDir")) {
-      if (service) {
-        /* Validate and register the service we just finished parsing this
-         * code registers every service except the last one parsed, which is
-         * validated and registered below the loop. */
-        if (rend_validate_service(rend_service_staging_list, service) < 0) {
-          goto free_and_return;
-        }
-        if (rend_service_check_dir_and_add(rend_service_staging_list, options,
-                                           service, validate_only) < 0) {
-          /* The above frees the service on error so nullify the pointer. */
-          service = NULL;
-          goto free_and_return;
-        }
-      }
-      service = tor_malloc_zero(sizeof(rend_service_t));
-      service->directory = tor_strdup(line->value);
-      service->ports = smartlist_new();
-      service->intro_period_started = time(NULL);
-      service->n_intro_points_wanted = NUM_INTRO_POINTS_DEFAULT;
-      continue;
-    }
-    if (!service) {
-      log_warn(LD_CONFIG, "%s with no preceding HiddenServiceDir directive",
-               line->key);
-      goto free_and_return;
+      /* We just hit the next hidden service, stop right now. */
+      break;
     }
-    if (!strcasecmp(line->key, "HiddenServicePort")) {
-      char *err_msg = NULL;
-      portcfg = rend_service_parse_port_config(line->value, " ", &err_msg);
-      if (!portcfg) {
-        if (err_msg)
-          log_warn(LD_CONFIG, "%s", err_msg);
-        tor_free(err_msg);
-        goto free_and_return;
-      }
-      tor_assert(!err_msg);
-      smartlist_add(service->ports, portcfg);
-    } else if (!strcasecmp(line->key, "HiddenServiceAllowUnknownPorts")) {
-      service->allow_unknown_ports = (int)tor_parse_long(line->value,
-                                                        10, 0, 1, &ok, NULL);
-      if (!ok) {
-        log_warn(LD_CONFIG,
-                 "HiddenServiceAllowUnknownPorts should be 0 or 1, not %s",
-                 line->value);
-        goto free_and_return;
-      }
-      log_info(LD_CONFIG,
-               "HiddenServiceAllowUnknownPorts=%d for %s",
-               (int)service->allow_unknown_ports,
-               rend_service_escaped_dir(service));
-    } else if (!strcasecmp(line->key,
-                           "HiddenServiceDirGroupReadable")) {
-        service->dir_group_readable = (int)tor_parse_long(line->value,
-                                                        10, 0, 1, &ok, NULL);
-        if (!ok) {
-            log_warn(LD_CONFIG,
-                     "HiddenServiceDirGroupReadable should be 0 or 1, not %s",
-                     line->value);
-            goto free_and_return;
-        }
-        log_info(LD_CONFIG,
-                 "HiddenServiceDirGroupReadable=%d for %s",
-                 service->dir_group_readable,
-                 rend_service_escaped_dir(service));
-    } else if (!strcasecmp(line->key, "HiddenServiceMaxStreams")) {
-      service->max_streams_per_circuit = (int)tor_parse_long(line->value,
-                                                    10, 0, 65535, &ok, NULL);
-      if (!ok) {
-        log_warn(LD_CONFIG,
-                 "HiddenServiceMaxStreams should be between 0 and %d, not %s",
-                 65535, line->value);
-        goto free_and_return;
-      }
-      log_info(LD_CONFIG,
-               "HiddenServiceMaxStreams=%d for %s",
-               service->max_streams_per_circuit,
-               rend_service_escaped_dir(service));
-    } else if (!strcasecmp(line->key, "HiddenServiceMaxStreamsCloseCircuit")) {
-      service->max_streams_close_circuit = (int)tor_parse_long(line->value,
-                                                        10, 0, 1, &ok, NULL);
-      if (!ok) {
-        log_warn(LD_CONFIG,
-                 "HiddenServiceMaxStreamsCloseCircuit should be 0 or 1, "
-                 "not %s",
-                 line->value);
-        goto free_and_return;
-      }
-      log_info(LD_CONFIG,
-               "HiddenServiceMaxStreamsCloseCircuit=%d for %s",
-               (int)service->max_streams_close_circuit,
-               rend_service_escaped_dir(service));
-    } else if (!strcasecmp(line->key, "HiddenServiceNumIntroductionPoints")) {
+    /* Number of introduction points. */
+    if (!strcasecmp(line->key, "HiddenServiceNumIntroductionPoints")) {
+      int ok = 0;
+      /* Those are specific defaults for version 2. */
       service->n_intro_points_wanted =
         (unsigned int) tor_parse_long(line->value, 10,
                                       0, NUM_INTRO_POINTS_MAX, &ok, NULL);
@@ -779,12 +705,13 @@ rend_config_services(const or_options_t *options, int validate_only)
                  "HiddenServiceNumIntroductionPoints "
                  "should be between %d and %d, not %s",
                  0, NUM_INTRO_POINTS_MAX, line->value);
-        goto free_and_return;
+        goto err;
       }
       log_info(LD_CONFIG, "HiddenServiceNumIntroductionPoints=%d for %s",
-               service->n_intro_points_wanted,
-               rend_service_escaped_dir(service));
-    } else if (!strcasecmp(line->key, "HiddenServiceAuthorizeClient")) {
+               service->n_intro_points_wanted, escaped(service->directory));
+      continue;
+    }
+    if (!strcasecmp(line->key, "HiddenServiceAuthorizeClient")) {
       /* Parse auth type and comma-separated list of client names and add a
        * rend_authorized_client_t for each client to the service's list
        * of authorized clients. */
@@ -794,7 +721,7 @@ rend_config_services(const or_options_t *options, int validate_only)
       if (service->auth_type != REND_NO_AUTH) {
         log_warn(LD_CONFIG, "Got multiple HiddenServiceAuthorizeClient "
                  "lines for a single service.");
-        goto free_and_return;
+        goto err;
       }
       type_names_split = smartlist_new();
       smartlist_split_string(type_names_split, line->value, " ", 0, 2);
@@ -802,7 +729,8 @@ rend_config_services(const or_options_t *options, int validate_only)
         log_warn(LD_BUG, "HiddenServiceAuthorizeClient has no value. This "
                          "should have been prevented when parsing the "
                          "configuration.");
-        goto free_and_return;
+        smartlist_free(type_names_split);
+        goto err;
       }
       authname = smartlist_get(type_names_split, 0);
       if (!strcasecmp(authname, "basic")) {
@@ -816,7 +744,7 @@ rend_config_services(const or_options_t *options, int validate_only)
                  (char *) smartlist_get(type_names_split, 0));
         SMARTLIST_FOREACH(type_names_split, char *, cp, tor_free(cp));
         smartlist_free(type_names_split);
-        goto free_and_return;
+        goto err;
       }
       service->clients = smartlist_new();
       if (smartlist_len(type_names_split) < 2) {
@@ -853,7 +781,7 @@ rend_config_services(const or_options_t *options, int validate_only)
                    client_name, REND_CLIENTNAME_MAX_LEN);
           SMARTLIST_FOREACH(clients, char *, cp, tor_free(cp));
           smartlist_free(clients);
-          goto free_and_return;
+          goto err;
         }
         client = tor_malloc_zero(sizeof(rend_authorized_client_t));
         client->client_name = tor_strdup(client_name);
@@ -875,56 +803,29 @@ rend_config_services(const or_options_t *options, int validate_only)
                  smartlist_len(service->clients),
                  service->auth_type == REND_BASIC_AUTH ? 512 : 16,
                  service->auth_type == REND_BASIC_AUTH ? "basic" : "stealth");
-        goto free_and_return;
-      }
-    } else {
-      tor_assert(!strcasecmp(line->key, "HiddenServiceVersion"));
-      if (strcmp(line->value, "2")) {
-        log_warn(LD_CONFIG,
-                 "The only supported HiddenServiceVersion is 2.");
-        goto free_and_return;
+        goto err;
       }
+      continue;
     }
   }
-  /* Validate the last service that we just parsed. */
-  if (service &&
-      rend_validate_service(rend_service_staging_list, service) < 0) {
-    goto free_and_return;
-  }
-  /* register the final service after we have finished parsing all services
-   * this code only registers the last service, other services are registered
-   * within the loop. It is ok for this service to be NULL, it is ignored. */
-  if (rend_service_check_dir_and_add(rend_service_staging_list, options,
-                                     service, validate_only) < 0) {
-    /* Service object is freed on error so nullify pointer. */
-    service = NULL;
-    goto free_and_return;
+  /* Validate the service just parsed. */
+  if (rend_validate_service(rend_service_staging_list, service) < 0) {
+    /* Service is in the staging list so don't try to free it. */
+    goto err;
   }
-  /* The service is in the staging list so nullify pointer to avoid double
-   * free of this object in case of error because we lost ownership of it at
-   * this point. */
-  service = NULL;
 
-  /* Free the newly added services if validating */
-  if (validate_only) {
-    rv = 0;
-    goto free_and_return;
+  /* Add it to the temporary list which we will use to prune our current
+   * list if any after configuring all services. */
+  if (rend_add_service(rend_service_staging_list, service) < 0) {
+    /* The object has been freed on error already. */
+    service = NULL;
+    goto err;
   }
 
-  /* This could be a reload of configuration so try to prune the main list
-   * using the staging one. And we know we are not in validate mode here.
-   * After this, the main and staging list will point to the right place and
-   * be in a quiescent usable state. */
-  rend_service_prune_list();
-
   return 0;
- free_and_return:
+ err:
   rend_service_free(service);
-  SMARTLIST_FOREACH(rend_service_staging_list, rend_service_t *, ptr,
-                    rend_service_free(ptr));
-  smartlist_free(rend_service_staging_list);
-  rend_service_staging_list = NULL;
-  return rv;
+  return -1;
 }
 
 /** Add the ephemeral service <b>pk</b>/<b>ports</b> if possible, using
@@ -1548,9 +1449,9 @@ rend_service_load_keys(rend_service_t *s)
   char *fname = NULL;
   char buf[128];
 
-  /* Make sure the directory was created and single onion poisoning was
-   * checked before calling this function */
-  if (BUG(rend_service_check_private_dir(get_options(), s, 0) < 0))
+  /* Create the directory if needed which will also poison it in case of
+   * single onion service. */
+  if (rend_service_check_private_dir(get_options(), s, 1) < 0)
     goto err;
 
   /* Load key */

src/or/rendservice.h

@@ -13,6 +13,7 @@
 #define TOR_RENDSERVICE_H
 
 #include "or.h"
+#include "hs_service.h"
 
 typedef struct rend_intro_cell_s rend_intro_cell_t;
 typedef struct rend_service_port_config_s rend_service_port_config_t;
@@ -119,10 +120,6 @@ typedef struct rend_service_t {
 
 STATIC void rend_service_free(rend_service_t *service);
 STATIC char *rend_service_sos_poison_path(const rend_service_t *service);
-STATIC int rend_service_check_dir_and_add(smartlist_t *service_list,
-                                          const or_options_t *options,
-                                          rend_service_t *service,
-                                          int validate_only);
 STATIC int rend_service_verify_single_onion_poison(
                                                   const rend_service_t *s,
                                                   const or_options_t *options);
@@ -144,7 +141,10 @@ STATIC void rend_service_prune_list_impl_(void);
 #endif /* RENDSERVICE_PRIVATE */
 
 int num_rend_services(void);
-int rend_config_services(const or_options_t *options, int validate_only);
+int rend_config_service(const config_line_t *line_,
+                        const or_options_t *options,
+                        int validate_only,
+                        hs_service_t *hs_service);
 void rend_service_prune_list(void);
 int rend_service_load_all_keys(const smartlist_t *service_list);
 void rend_services_add_filenames_to_lists(smartlist_t *open_lst,

src/test/test_hs.c

@@ -10,6 +10,7 @@
 #define CIRCUITBUILD_PRIVATE
 #define RENDCOMMON_PRIVATE
 #define RENDSERVICE_PRIVATE
+#define HS_SERVICE_PRIVATE
 
 #include "or.h"
 #include "test.h"
@@ -661,17 +662,8 @@ test_single_onion_poisoning(void *arg)
   smartlist_t *services = smartlist_new();
   char *poison_path = NULL;
 
-  /* No services, no service to verify, no problem! */
-  mock_options->HiddenServiceSingleHopMode = 0;
-  mock_options->HiddenServiceNonAnonymousMode = 0;
-  ret = rend_config_services(mock_options, 1);
-  tt_assert(ret == 0);
-
-  /* Either way, no problem. */
   mock_options->HiddenServiceSingleHopMode = 1;
   mock_options->HiddenServiceNonAnonymousMode = 1;
-  ret = rend_config_services(mock_options, 1);
-  tt_assert(ret == 0);
 
   /* Create the data directory, and, if the correct bit in arg is set,
    * create a directory for that service.
@@ -726,8 +718,10 @@ test_single_onion_poisoning(void *arg)
   tt_assert(ret == 0);
 
   /* Add the first service */
-  ret = rend_service_check_dir_and_add(services, mock_options, service_1, 0);
-  tt_assert(ret == 0);
+  ret = hs_check_service_private_dir(mock_options->User, service_1->directory,
+                                     service_1->dir_group_readable, 1);
+  tt_int_op(ret, OP_EQ, 0);
+  smartlist_add(services, service_1);
   /* But don't add the second service yet. */
 
   /* Service directories, but no previous keys, no problem! */
@@ -805,8 +799,10 @@ test_single_onion_poisoning(void *arg)
   tt_assert(ret == 0);
 
   /* Now add the second service: it has no key and no poison file */
-  ret = rend_service_check_dir_and_add(services, mock_options, service_2, 0);
-  tt_assert(ret == 0);
+  ret = hs_check_service_private_dir(mock_options->User, service_2->directory,
+                                     service_2->dir_group_readable, 1);
+  tt_int_op(ret, OP_EQ, 0);
+  smartlist_add(services, service_2);
 
   /* A new service, and an existing poisoned service. Not ok. */
   mock_options->HiddenServiceSingleHopMode = 0;