r14623@tombo: nickm | 2007-11-01 22:25:18 -0400

 More tweaks from karsten, with some cleanup and commentary.


svn:r12319

8 files changed, 80 insertions, 55 deletions

diff --git a/doc/tor.1.in b/doc/tor.1.in
index 39683b9b5..6fd6763d3 100644
--- a/doc/tor.1.in
+++ b/doc/tor.1.in
@@ -1107,6 +1107,11 @@ if you're using a Tor controller that handles hidserv publishing for you.
 (Default: 1)
 .LP
 .TP
+\fBHiddenServiceVersion \fR\fIversion\fR,\fIversion\fR,\fI...\fP
+A list of rendezvous service descriptor versions to publish for the hidden
+service. Possible version numbers are 0 and 2. (Default: 0, 2)
+.LP
+.TP
 \fBRendPostPeriod \fR\fIN\fR \fBseconds\fR|\fBminutes\fR|\fBhours\fR|\fBdays\fR|\fBweeks\fP
 Every time the specified period elapses, Tor uploads any rendezvous
 service descriptors to the directory servers.  This information is also
diff --git a/src/config/torrc.complete.in b/src/config/torrc.complete.in
index 7057eaa6a..310458a5c 100644
--- a/src/config/torrc.complete.in
+++ b/src/config/torrc.complete.in
@@ -521,6 +521,10 @@
 ## hidden service. In normal use there is no reason to set this.
 #HiddenServiceExcludeNodes nickname,nickname,...
 
+## Publish the given rendezvous service descriptor versions for the
+## hidden service.
+#HiddenServiceVersion 0,2
+
 ## Every time the specified period elapses, Tor uploads any ren-
 ## dezvous service descriptors to the directory servers.  This
 ## information is also uploaded whenever it changes. 
diff --git a/src/or/config.c b/src/or/config.c
index 7dd4042a9..795d1804d 100644
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -192,7 +192,6 @@ static config_var_t _option_vars[] = {
   VAR("HiddenServiceNodes",  LINELIST_S, RendConfigLines,    NULL),
   VAR("HiddenServiceOptions",LINELIST_V, RendConfigLines,    NULL),
   VAR("HiddenServicePort",   LINELIST_S, RendConfigLines,    NULL),
-  /*DOCDOC in tor manpage*/
   VAR("HiddenServiceVersion",LINELIST_S, RendConfigLines,    NULL),
   V(HSAuthoritativeDir,          BOOL,     "0"),
   V(HSAuthorityRecordStats,      BOOL,     "0"),
diff --git a/src/or/main.c b/src/or/main.c
index ea09bf524..7a7fc5321 100644
--- a/src/or/main.c
+++ b/src/or/main.c
@@ -973,6 +973,7 @@ run_scheduled_events(time_t now)
      * and the rend cache. */
     rep_history_clean(now - options->RephistTrackTime);
     rend_cache_clean();
+    rend_cache_clean_v2_descs_as_dir();
     /* XXX020 we only clean this stuff if DirPort is set?! -RD */
   }
 
diff --git a/src/or/or.h b/src/or/or.h
index afc190639..2a8cd6441 100644
--- a/src/or/or.h
+++ b/src/or/or.h
@@ -1785,6 +1785,11 @@ typedef struct origin_circuit_t {
    * rendezvous service ID, but different descriptor versions.
    * XXXX020 I believe this is a bitmap, but the doc doesn't say so. If so,
    *  why?  A circuit can't be using two different rendezvous decriptors. -NM
+   * Yes, it is a bitmap. The reason is that it might turn out that a version
+   * 3 _can_ use the same introduction point as version 2; only version 0
+   * is incompatible. Would it be clearer to switch to a single version number
+   * for now and switch back to a bitmap, when the above becomes true? -KL
+   * Yes.  "YAGNI." -NM
    */
   uint8_t rend_desc_version;
 
@@ -3485,14 +3490,8 @@ typedef struct rend_cache_entry_t {
 } rend_cache_entry_t;
 
 void rend_cache_init(void);
-/*XXXX020 clean *and* clean_up *and* clean_v2_dir? Rename some. */
-/*XXXX020 Call clean_up and clean_v2_dir from somewhere; nothing calls them
- * now. */
-/* Those functions were called from the (removed) replication functionality.
- * We need to call them from somewhere periodically; main()? -KL */
 void rend_cache_clean(void);
-void rend_cache_clean_up(void);
-void rend_cache_clean_v2_dir(void);
+void rend_cache_clean_v2_descs_as_dir(void);
 void rend_cache_free_all(void);
 int rend_valid_service_id(const char *query);
 int rend_cache_lookup_desc(const char *query, int version, const char **desc,
diff --git a/src/or/rendcommon.c b/src/or/rendcommon.c
index b0d4b2dd6..196d6d488 100644
--- a/src/or/rendcommon.c
+++ b/src/or/rendcommon.c
@@ -22,6 +22,8 @@ rend_cmp_service_ids(const char *one, const char *two)
 /** Helper: Release the storage held by the intro key in <b>_ent</b>.
  */
 /*XXXX020 there's also one of these in rendservice.c */
+/* Right. But the only alternative to that (which I know) would be to
+ * write it to or.h. Should I do that? -KL */
 static void
 intro_key_free(void *_ent)
 {
@@ -436,7 +438,6 @@ rend_encode_v2_descriptors(smartlist_t *desc_strs_out,
     /* Add signature. */
     strlcpy(desc_str + written, "signature\n", desc_len - written);
     written += strlen(desc_str + written);
-    desc_str[written] = '\0'; /* XXXX020 strlcpy always nul-terminates. */
     if (crypto_digest(desc_digest, desc_str, written) < 0) {
       log_warn(LD_BUG, "could not create digest.");
       tor_free(desc_str);
@@ -674,25 +675,26 @@ rend_cache_clean(void)
   }
 }
 
-/** Remove all old entries on v2 hidden service directories. */
+/** Remove all old v2 descriptors and those for which this hidden service
+ * directory is not responsible for any more. */
 void
-rend_cache_clean_v2_dir(void)
+rend_cache_clean_v2_descs_as_dir(void)
 {
   digestmap_iter_t *iter;
-  const char *key;
-  void *val;
-  rend_cache_entry_t *ent;
-  time_t cutoff;
-  cutoff = time(NULL) - REND_CACHE_MAX_AGE - REND_CACHE_MAX_SKEW;
+  smartlist_t *hs_dirs = hid_serv_create_routing_table();
+  time_t cutoff = time(NULL) - REND_CACHE_MAX_AGE - REND_CACHE_MAX_SKEW;
   for (iter = digestmap_iter_init(rend_cache_v2_dir);
        !digestmap_iter_done(iter); ) {
+    const char *key;
+    void *val;
+    rend_cache_entry_t *ent;
     digestmap_iter_get(iter, &key, &val);
     ent = val;
-    if (ent->parsed->timestamp < cutoff) {
+    if (ent->parsed->timestamp < cutoff ||
+        !hid_serv_responsible_for_desc_id(key, hs_dirs)) {
       char key_base32[REND_DESC_ID_V2_LEN_BASE32 + 1];
       base32_encode(key_base32, sizeof(key_base32), key, DIGEST_LEN);
-      log_info(LD_REND, "Removing descriptor with ID '%s' from cache, "
-                        "because it is too old!",
+      log_info(LD_REND, "Removing descriptor with ID '%s' from cache",
                key_base32);
       iter = digestmap_iter_next_rmv(rend_cache_v2_dir, iter);
       _rend_cache_entry_free(ent);
@@ -700,6 +702,7 @@ rend_cache_clean_v2_dir(void)
       iter = digestmap_iter_next(rend_cache_v2_dir, iter);
     }
   }
+  smartlist_free(hs_dirs);
 }
 
 /** Determines whether <b>a</b> is in the interval of <b>b</b> (excluded) and
@@ -733,35 +736,6 @@ rend_id_is_in_interval(const char *a, const char *b, const char *c)
   return 1;
 }
 
-/** Clean up all values for which this node as hidden service directory is
- * not responsible */
-void
-rend_cache_clean_up(void)
-{
-  digestmap_iter_t *iter;
-  smartlist_t *hs_dirs = hid_serv_create_routing_table();
-  for (iter = digestmap_iter_init(rend_cache_v2_dir);
-       !digestmap_iter_done(iter); ) {
-    const char *key;
-    void *val;
-    rend_cache_entry_t *ent;
-    digestmap_iter_get(iter, &key, &val);
-    ent = val;
-    if (!hid_serv_responsible_for_desc_id(key, hs_dirs)) {
-      char key_base32[REND_DESC_ID_V2_LEN_BASE32 + 1];
-      base32_encode(key_base32, sizeof(key_base32),
-                    key, DIGEST_LEN);
-      log_info(LD_REND, "Removing descriptor with ID '%s' from cache, "
-                        "because we are not reponsible for it!", key_base32);
-      iter = digestmap_iter_next_rmv(rend_cache_v2_dir, iter);
-      _rend_cache_entry_free(ent);
-    } else {
-      iter = digestmap_iter_next(rend_cache_v2_dir, iter);
-    }
-  }
-  smartlist_free(hs_dirs);
-}
-
 /** Return true iff <b>query</b> is a syntactically valid service ID (as
  * generated by rend_get_service_id).  */
 int
@@ -1067,6 +1041,19 @@ rend_cache_store_v2_desc_as_client(const char *desc,
 {
   /*XXXX this seems to have a bit of duplicate code with
    * rend_cache_store_v2_desc_as_dir().  Fix that. */
+  /* Though having similar elements, both functions were separated on
+   * purpose:
+   * - dirs don't care about encoded/encrypted introduction points, clients
+   *   do.
+   * - dirs store descriptors in a separate cache by descriptor ID, whereas
+   *   clients store them by service ID; both caches are different data
+   *   structures and have different access methods.
+   * - dirs store a descriptor only if they are responsible for its ID,
+   *   clients do so in every way (because they have requested it before).
+   * - dirs can process multiple concatenated descriptors which is required
+   *   for replication, whereas clients only accept a single descriptor.
+   * Thus, combining both methods would result in a lot of if statements
+   * which probably would not improve, but worsen code readability. -KL */
   rend_service_descriptor_t *parsed = NULL;
   char desc_id[DIGEST_LEN];
   char *intro_content = NULL;
diff --git a/src/or/rendservice.c b/src/or/rendservice.c
index d1b1932f0..7665b2793 100644
--- a/src/or/rendservice.c
+++ b/src/or/rendservice.c
@@ -62,6 +62,11 @@ typedef struct rend_service_t {
   time_t next_upload_time;
   /* XXXX020 A service never actually has both descriptor versions; perhaps
    * this should be an int rather than in intmax. */
+  /* A service can never publish v0 and v2 descriptors, but maybe it can
+   * publish v2 and v3 descriptors in the future. As with origin_circuit_t:
+   * Would it be clearer to switch to a single version number for now and
+   * switch back to a bitmap, when the above becomes true? -KL */
+  /* Yes. s/when/if/.  "YAGNI" -NM. */
   int descriptor_versions; /**< bitmask of rendezvous descriptor versions
                             * that will be published. "0" means "default." */
 } rend_service_t;
@@ -138,7 +143,8 @@ add_service(rend_service_t *service)
   if (!service->intro_exclude_nodes)
     service->intro_exclude_nodes = tor_strdup("");
   if (service->descriptor_versions == 0)
-    service->descriptor_versions = 1; /* Default is v0 only. */
+    service->descriptor_versions = 1 + (1<<2); /**< Default is v0 and v2 in
+                                                * parallel. */
   service->intro_keys = strmap_new();
 
   /* If the service is configured to publish unversioned (v0) and versioned
@@ -1210,7 +1216,7 @@ rend_services_introduce(void)
         log_info(LD_REND,"Giving up on %s as intro point for %s.",
                  intro, service->service_id);
         tor_free(intro);
-        /* XXXX020 We could also remove the intro key here... */
+        /* XXXXX020 we could also remove the intro key here. */
         smartlist_del(service->intro_nodes,j--);
         changed = 1;
         service->desc_is_dirty = now;
diff --git a/src/or/routerparse.c b/src/or/routerparse.c
index 9073cd79d..2cf5a1e7f 100644
--- a/src/or/routerparse.c
+++ b/src/or/routerparse.c
@@ -3257,9 +3257,11 @@ rend_parse_v2_service_descriptor(rend_service_descriptor_t **parsed_out,
   tor_assert(tok);
   tor_assert(tok->n_args == 1);
   result->version = atoi(tok->args[0]);
-  if (result->version < 2) { /*XXXX020 what if > 2? */
-                             /* Good question: should higher versions
-                              * be rejected by directories? -KL */
+  if (result->version != 2) {
+    /* If it's <2, it shouldn't be under this format.  If the number
+     * is greater than 2, we bumped it because we broke backward
+     * compatibility.  See how version numbers in our other formats
+     * work. */
     log_warn(LD_REND, "Wrong descriptor version: %d", result->version);
     goto err;
   }
@@ -3300,6 +3302,15 @@ rend_parse_v2_service_descriptor(rend_service_descriptor_t **parsed_out,
                          SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 0);
   for (i = 0; i < smartlist_len(versions); i++) {
     /* XXXX020 validate the numbers here. */
+    /* As above, validating these numbers on a hidden service directory
+     * might require an extension to new valid numbers at some time. But
+     * this would require making a distinction of hidden service direcoties
+     * which accept the old valid numbers and those which accept the new
+     * valid numbers. -KL */
+    /* As above, increased version numbers are for
+     * non-backward-compatible changes.  This code doesn't know how to
+     * parse a v3 descriptor, because a v3 descriptor is by definitition not 
+     * compatible with this code.  */
     version = atoi(smartlist_get(versions, i));
     result->protocols |= 1 << version;
   }
@@ -3308,7 +3319,11 @@ rend_parse_v2_service_descriptor(rend_service_descriptor_t **parsed_out,
   /* Parse encrypted introduction points. Don't verify. */
   tok = find_first_by_keyword(tokens, R_INTRODUCTION_POINTS);
   tor_assert(tok);
-  /* XXXX020 make sure it's "BEGIN MESSAGE", not "BEGIN SOMETHINGELSE" */
+  if (strcmp(tok->object_type, "MESSAGE")) {
+    log_warn(LD_DIR, "Bad object type: introduction points should be of "
+             "type MESSAGE");
+    goto err;
+  }
   *intro_points_encrypted_out = tok->object_body;
   *intro_points_encrypted_size_out = tok->object_size;
   tok->object_body = NULL; /* Prevent free. */
@@ -3446,8 +3461,14 @@ rend_decrypt_introduction_points(rend_service_descriptor_t *parsed,
     info->addr = ntohl(ip.s_addr);
     /* Parse onion port. */
     tok = find_first_by_keyword(tokens, R_IPO_ONION_PORT);
-    /* XXXX020 validate range. */
     info->port = (uint16_t) atoi(tok->args[0]);
+    /* XXXX020 this next check fails with ports like 65537. */
+    if (!info->port) {
+      log_warn(LD_REND, "Introduction point onion port is out of range: %d",
+               info->port);
+      tor_free(info);
+      goto err;
+    }
     /* Parse onion key. */
     tok = find_first_by_keyword(tokens, R_IPO_ONION_KEY);
     info->onion_key = tok->key;
@@ -3461,6 +3482,9 @@ rend_decrypt_introduction_points(rend_service_descriptor_t *parsed,
   }
   /* Write extend infos to descriptor. */
   /* XXXX020 what if intro_points (&tc) are already set? */
+  /* This function is not intended to be invoced multiple times for
+   * the same descriptor. Should this be asserted? -KL */
+  /* Yes. -NM */
   parsed->n_intro_points = smartlist_len(intropoints);
   parsed->intro_point_extend_info =
     tor_malloc_zero(sizeof(extend_info_t *) * parsed->n_intro_points);