Merge branch 'tor-github/pr/438' into maint-0.3.5

changes/ticket27838

@@ -0,0 +1,4 @@
+  o Minor bugfixes (hidden service v3):
+    - Build the service descriptor signing key certificate before uploading so
+      we always have a fresh one leaving no chances for it to expire service
+      side. Fixes bug 27838; bugfix on 0.3.2.1-alpha.

src/feature/hs/hs_service.c

@@ -1696,6 +1696,32 @@ build_desc_intro_points(const hs_service_t *service,
   } DIGEST256MAP_FOREACH_END;
 }
 
+/* Build the descriptor signing key certificate. */
+static void
+build_desc_signing_key_cert(hs_service_descriptor_t *desc, time_t now)
+{
+  hs_desc_plaintext_data_t *plaintext;
+
+  tor_assert(desc);
+  tor_assert(desc->desc);
+
+  /* Ease our life a bit. */
+  plaintext = &desc->desc->plaintext_data;
+
+  /* Get rid of what we have right now. */
+  tor_cert_free(plaintext->signing_key_cert);
+
+  /* Fresh certificate for the signing key. */
+  plaintext->signing_key_cert =
+    tor_cert_create(&desc->blinded_kp, CERT_TYPE_SIGNING_HS_DESC,
+                    &desc->signing_kp.pubkey, now, HS_DESC_CERT_LIFETIME,
+                    CERT_FLAG_INCLUDE_SIGNING_KEY);
+  /* If the cert creation fails, the descriptor encoding will fail and thus
+   * ultimately won't be uploaded. We'll get a stack trace to help us learn
+   * where the call came from and the tor_cert_create() will log the error. */
+  tor_assert_nonfatal(plaintext->signing_key_cert);
+}
+
 /* Populate the descriptor encrypted section from the given service object.
  * This will generate a valid list of introduction points that can be used
  * after for circuit creation. Return 0 on success else -1 on error. */
@@ -1811,17 +1837,15 @@ build_service_desc_superencrypted(const hs_service_t *service,
 
 /* Populate the descriptor plaintext section from the given service object.
  * The caller must make sure that the keys in the descriptors are valid that
- * is are non-zero. Return 0 on success else -1 on error. */
+ * is are non-zero. This can't fail. */
-static int
+static void
 build_service_desc_plaintext(const hs_service_t *service,
-                             hs_service_descriptor_t *desc, time_t now)
+                             hs_service_descriptor_t *desc)
 {
-  int ret = -1;
   hs_desc_plaintext_data_t *plaintext;
 
   tor_assert(service);
   tor_assert(desc);
-  /* XXX: Use a "assert_desc_ok()" ? */
   tor_assert(!tor_mem_is_zero((char *) &desc->blinded_kp,
                               sizeof(desc->blinded_kp)));
   tor_assert(!tor_mem_is_zero((char *) &desc->signing_kp,
@@ -1835,24 +1859,13 @@ build_service_desc_plaintext(const hs_service_t *service,
 
   plaintext->version = service->config.version;
   plaintext->lifetime_sec = HS_DESC_DEFAULT_LIFETIME;
-  plaintext->signing_key_cert =
-    tor_cert_create(&desc->blinded_kp, CERT_TYPE_SIGNING_HS_DESC,
-                    &desc->signing_kp.pubkey, now, HS_DESC_CERT_LIFETIME,
-                    CERT_FLAG_INCLUDE_SIGNING_KEY);
-  if (plaintext->signing_key_cert == NULL) {
-    log_warn(LD_REND, "Unable to create descriptor signing certificate for "
-                      "service %s",
-             safe_str_client(service->onion_address));
-    goto end;
-  }
   /* Copy public key material to go in the descriptor. */
   ed25519_pubkey_copy(&plaintext->signing_pubkey, &desc->signing_kp.pubkey);
   ed25519_pubkey_copy(&plaintext->blinded_pubkey, &desc->blinded_kp.pubkey);
-  /* Success. */
-  ret = 0;
 
- end:
+  /* Create the signing key certificate. This will be updated before each
-  return ret;
+   * upload but we create it here so we don't complexify our unit tests. */
+  build_desc_signing_key_cert(desc, approx_time());
 }
 
 /** Compute the descriptor's OPE cipher for encrypting revision counters. */
@@ -1942,8 +1955,7 @@ build_service_desc_keys(const hs_service_t *service,
  *
  * This can error if we are unable to create keys or certificate. */
 static void
-build_service_descriptor(hs_service_t *service, time_t now,
+build_service_descriptor(hs_service_t *service, uint64_t time_period_num,
-                         uint64_t time_period_num,
                          hs_service_descriptor_t **desc_out)
 {
   char *encoded_desc;
@@ -1962,9 +1974,8 @@ build_service_descriptor(hs_service_t *service, time_t now,
     goto err;
   }
   /* Setup plaintext descriptor content. */
-  if (build_service_desc_plaintext(service, desc, now) < 0) {
+  build_service_desc_plaintext(service, desc);
-    goto err;
+
-  }
   /* Setup superencrypted descriptor content. */
   if (build_service_desc_superencrypted(service, desc) < 0) {
     goto err;
@@ -2037,10 +2048,8 @@ build_descriptors_for_new_service(hs_service_t *service, time_t now)
   }
 
   /* Build descriptors. */
-  build_service_descriptor(service, now, current_desc_tp,
+  build_service_descriptor(service, current_desc_tp, &service->desc_current);
-                           &service->desc_current);
+  build_service_descriptor(service, next_desc_tp, &service->desc_next);
-  build_service_descriptor(service, now, next_desc_tp,
-                           &service->desc_next);
   log_info(LD_REND, "Hidden service %s has just started. Both descriptors "
                     "built. Now scheduled for upload.",
            safe_str_client(service->onion_address));
@@ -2070,7 +2079,7 @@ build_all_descriptors(time_t now)
     }
 
     if (service->desc_next == NULL) {
-      build_service_descriptor(service, now, hs_get_next_time_period_num(0),
+      build_service_descriptor(service, hs_get_next_time_period_num(0),
                                &service->desc_next);
       log_info(LD_REND, "Hidden service %s next descriptor successfully "
                         "built. Now scheduled for upload.",
@@ -2282,12 +2291,9 @@ service_desc_schedule_upload(hs_service_descriptor_t *desc,
   }
 }
 
-/* Update the given descriptor from the given service. The possible update
+/* Pick missing intro points for this descriptor if needed. */
- * actions includes:
- *    - Picking missing intro points if needed.
- */
 static void
-update_service_descriptor(hs_service_t *service,
+update_service_descriptor_intro_points(hs_service_t *service,
                           hs_service_descriptor_t *desc, time_t now)
 {
   unsigned int num_intro_points;
@@ -2326,15 +2332,17 @@ update_service_descriptor(hs_service_t *service,
   }
 }
 
-/* Update descriptors for each service if needed. */
+/* Update descriptor intro points for each service if needed. We do this as
+ * part of the periodic event because we need to establish intro point circuits
+ * before we publish descriptors. */
 STATIC void
-update_all_descriptors(time_t now)
+update_all_descriptors_intro_points(time_t now)
 {
   FOR_EACH_SERVICE_BEGIN(service) {
     /* We'll try to update each descriptor that is if certain conditions apply
      * in order for the descriptor to be updated. */
     FOR_EACH_DESCRIPTOR_BEGIN(service, desc) {
-      update_service_descriptor(service, desc, now);
+      update_service_descriptor_intro_points(service, desc, now);
     } FOR_EACH_DESCRIPTOR_END;
   } FOR_EACH_SERVICE_END;
 }
@@ -2619,10 +2627,10 @@ run_build_descriptor_event(time_t now)
    * been rotated or we just started up. */
   build_all_descriptors(now);
 
-  /* Finally, we'll check if we should update the descriptors. Missing
+  /* Finally, we'll check if we should update the descriptors' intro
-   * introduction points will be picked in this function which is useful for
+   * points. Missing introduction points will be picked in this function which
-   * newly built descriptors. */
+   * is useful for newly built descriptors. */
-  update_all_descriptors(now);
+  update_all_descriptors_intro_points(now);
 }
 
 /* For the given service, launch any intro point circuits that could be
@@ -3083,6 +3091,37 @@ should_service_upload_descriptor(const hs_service_t *service,
   return 0;
 }
 
+/* Refresh the given service descriptor meaning this will update every mutable
+ * field that needs to be updated before we upload.
+ *
+ * This should ONLY be called before uploading a descriptor. It assumes that
+ * the descriptor has been built (desc->desc) and that all intro point
+ * circuits have been established.  */
+static void
+refresh_service_descriptor(const hs_service_t *service,
+                           hs_service_descriptor_t *desc, time_t now)
+{
+  /* There are few fields that we consider "mutable" in the descriptor meaning
+   * we need to update them regurlarly over the lifetime fo the descriptor.
+   * The rest are set once and should not be modified.
+   *
+   *  - Signing key certificate.
+   *  - Revision counter.
+   *  - Introduction points which includes many thing. See
+   *    hs_desc_intro_point_t. and the setup_desc_intro_point() function.
+   */
+
+  /* Create the signing key certificate. */
+  build_desc_signing_key_cert(desc, now);
+
+  /* Build the intro points descriptor section. The refresh step is just
+   * before we upload so all circuits have been properly established. */
+  build_desc_intro_points(service, desc, now);
+
+  /* Set the desc revision counter right before uploading */
+  set_descriptor_revision_counter(desc, now, service->desc_current == desc);
+}
+
 /* Scheduled event run from the main loop. Try to upload the descriptor for
  * each service. */
 STATIC void
@@ -3118,15 +3157,12 @@ run_upload_descriptor_event(time_t now)
                service->config.num_intro_points,
                (desc->missing_intro_points) ? " (couldn't pick more)" : "");
 
-      /* At this point, we have to upload the descriptor so start by building
+      /* We are about to upload so we need to do one last step which is to
-       * the intro points descriptor section which we are now sure to be
+       * update the service's descriptor mutable fields in order to upload a
-       * accurate because all circuits have been established. */
+       * coherent descriptor. */
-      build_desc_intro_points(service, desc, now);
+      refresh_service_descriptor(service, desc, now);
-
-      /* Set the desc revision counter right before uploading */
-      set_descriptor_revision_counter(desc, approx_time(),
-                                      service->desc_current == desc);
 
+      /* Proceed with the upload, the descriptor is ready to be encoded. */
       upload_descriptor_to_all(service, desc);
     } FOR_EACH_DESCRIPTOR_END;
   } FOR_EACH_SERVICE_END;

src/feature/hs/hs_service.h

@@ -99,49 +99,65 @@ typedef struct hs_service_intropoints_t {
   digestmap_t *failed_id;
 } hs_service_intropoints_t;
 
-/* Representation of a service descriptor. */
+/* Representation of a service descriptor.
-typedef struct hs_service_descriptor_t {
+ *
-  /* Decoded descriptor. This object is used for encoding when the service
+ * Some elements of the descriptor are mutable whereas others are immutable:
-   * publishes the descriptor. */
-  hs_descriptor_t *desc;
 
-  /* Client authorization ephemeral keypair. */
+ * Immutable elements are initialized once when the descriptor is built (when
+ * service descriptors gets rotated). This means that these elements are
+ * initialized once and then they don't change for the lifetime of the
+ * descriptor. See build_service_descriptor().
+ *
+ * Mutable elements are initialized when we build the descriptor but they are
+ * also altered during the lifetime of the descriptor. They could be
+ * _refreshed_ everytime we upload the descriptor (which happens multiple times
+ * over the lifetime of the descriptor), or through periodic events. We do this
+ * for elements like the descriptor revision counter and various
+ * certificates. See refresh_service_descriptor() and
+ * update_service_descriptor_intro_points().
+ */
+typedef struct hs_service_descriptor_t {
+  /* Immutable: Client authorization ephemeral keypair. */
   curve25519_keypair_t auth_ephemeral_kp;
 
-  /* Descriptor cookie used to encrypt the descriptor, when the client
+  /* Immutable: Descriptor cookie used to encrypt the descriptor, when the
-   * authorization is enabled */
+   * client authorization is enabled */
   uint8_t descriptor_cookie[HS_DESC_DESCRIPTOR_COOKIE_LEN];
 
-  /* Descriptor signing keypair. */
+  /* Immutable: Descriptor signing keypair. */
   ed25519_keypair_t signing_kp;
 
-  /* Blinded keypair derived from the master identity public key. */
+  /* Immutable: Blinded keypair derived from the master identity public key. */
   ed25519_keypair_t blinded_kp;
 
-  /* When is the next time when we should upload the descriptor. */
+  /* Immutable: The time period number this descriptor has been created for. */
-  time_t next_upload_time;
-
-  /* Introduction points assign to this descriptor which contains
-   * hs_service_intropoints_t object indexed by authentication key (the RSA
-   * key if the node is legacy). */
-  hs_service_intropoints_t intro_points;
-
-  /* The time period number this descriptor has been created for. */
   uint64_t time_period_num;
 
-  /* True iff we have missing intro points for this descriptor because we
+  /** Immutable: The OPE cipher for encrypting revision counters for this
-   * couldn't pick any nodes. */
+   *  descriptor.  Tied to the descriptor blinded key. */
+  struct crypto_ope_t *ope_cipher;
+
+  /* Mutable: Decoded descriptor. This object is used for encoding when the
+   * service publishes the descriptor. */
+  hs_descriptor_t *desc;
+
+  /* Mutable: When is the next time when we should upload the descriptor. */
+  time_t next_upload_time;
+
+  /* Mutable: Introduction points assign to this descriptor which contains
+   * hs_service_intropoints_t object indexed by authentication key (the RSA key
+   * if the node is legacy). */
+  hs_service_intropoints_t intro_points;
+
+  /* Mutable: True iff we have missing intro points for this descriptor because
+   * we couldn't pick any nodes. */
   unsigned int missing_intro_points : 1;
 
-  /** List of the responsible HSDirs (their b64ed identity digest) last time we
+  /** Mutable: List of the responsible HSDirs (their b64ed identity digest)
-   *  uploaded this descriptor. If the set of responsible HSDirs is different
+   *  last time we uploaded this descriptor. If the set of responsible HSDirs
-   *  from this list, this means we received new dirinfo and we need to
+   *  is different from this list, this means we received new dirinfo and we
-   *  reupload our descriptor. */
+   *  need to reupload our descriptor. */
   smartlist_t *previous_hsdirs;
-
-  /** The OPE cipher for encrypting revision counters for this descriptor.
-   *  Tied to the descriptor blinded key. */
-  struct crypto_ope_t *ope_cipher;
 } hs_service_descriptor_t;
 
 /* Service key material. */
@@ -390,7 +406,7 @@ STATIC int intro_point_should_expire(const hs_service_intro_point_t *ip,
 STATIC void run_housekeeping_event(time_t now);
 STATIC void rotate_all_descriptors(time_t now);
 STATIC void build_all_descriptors(time_t now);
-STATIC void update_all_descriptors(time_t now);
+STATIC void update_all_descriptors_intro_points(time_t now);
 STATIC void run_upload_descriptor_event(time_t now);
 
 STATIC void service_descriptor_free_(hs_service_descriptor_t *desc);

src/test/test_hs_service.c

@@ -1457,7 +1457,7 @@ test_build_update_descriptors(void *arg)
   /* Time to test the update of those descriptors. At first, we have no node
    * in the routerlist so this will find NO suitable node for the IPs. */
   setup_full_capture_of_logs(LOG_INFO);
-  update_all_descriptors(now);
+  update_all_descriptors_intro_points(now);
   expect_log_msg_containing("Unable to find a suitable node to be an "
                             "introduction point for service");
   teardown_capture_of_logs();
@@ -1508,7 +1508,7 @@ test_build_update_descriptors(void *arg)
 
   /* We expect to pick only one intro point from the node above. */
   setup_full_capture_of_logs(LOG_INFO);
-  update_all_descriptors(now);
+  update_all_descriptors_intro_points(now);
   tor_free(node->ri->onion_curve25519_pkey); /* Avoid memleak. */
   tor_free(node->ri->cache_info.signing_key_cert);
   tor_free(node->ri->onion_pkey);