hs-v3: Cancel active descriptor directory connections before uploading

It is possible that two descriptor upload requests are launched in a very
short time frame which can lead to the second request finishing before the
first one and where that first one will make the HSDir send back a 400
malformed descriptor leading to a warning.

To avoid such, cancel all active directory connections for the specific
descriptor we are about to upload.

Note that this race is still possible on the HSDir side which triggers a log
info to be printed out but that is fine.

Fixes #23457

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

4 files changed, 72 insertions, 2 deletions

diff --git a/src/or/hs_client.c b/src/or/hs_client.c
index f85ebc8473..2f8953ba78 100644
--- a/src/or/hs_client.c
+++ b/src/or/hs_client.c
@@ -156,7 +156,8 @@ directory_launch_v3_desc_fetch(const ed25519_public_key_t *onion_identity_pk,
   }
 
   /* Copy onion pk to a dir_ident so that we attach it to the dir conn */
-  ed25519_pubkey_copy(&hs_conn_dir_ident.identity_pk, onion_identity_pk);
+  hs_ident_dir_conn_init(onion_identity_pk, &blinded_pubkey,
+                         &hs_conn_dir_ident);
 
   /* Setup directory request */
   directory_request_t *req =
diff --git a/src/or/hs_ident.c b/src/or/hs_ident.c
index df39285158..b0e4e36a9b 100644
--- a/src/or/hs_ident.c
+++ b/src/or/hs_ident.c
@@ -65,6 +65,22 @@ hs_ident_dir_conn_free(hs_ident_dir_conn_t *ident)
   tor_free(ident);
 }
 
+/* Initialized the allocated ident object with identity_pk and blinded_pk.
+ * None of them can be NULL since a valid directory connection identifier must
+ * have all fields set. */
+void
+hs_ident_dir_conn_init(const ed25519_public_key_t *identity_pk,
+                       const ed25519_public_key_t *blinded_pk,
+                       hs_ident_dir_conn_t *ident)
+{
+  tor_assert(identity_pk);
+  tor_assert(blinded_pk);
+  tor_assert(ident);
+
+  ed25519_pubkey_copy(&ident->identity_pk, identity_pk);
+  ed25519_pubkey_copy(&ident->blinded_pk, blinded_pk);
+}
+
 /* Return a newly allocated edge connection identifier. The given public key
  * identity_pk is copied into the identifier. */
 hs_ident_edge_conn_t *
diff --git a/src/or/hs_ident.h b/src/or/hs_ident.h
index cfcde781d1..101c1cfffd 100644
--- a/src/or/hs_ident.h
+++ b/src/or/hs_ident.h
@@ -96,6 +96,11 @@ typedef struct hs_ident_dir_conn_t {
    * in the onion address. */
   ed25519_public_key_t identity_pk;
 
+  /* The blinded public key used to uniquely identify the descriptor that this
+   * directory connection identifier is for. Only used by the service-side code
+   * to fine control descriptor uploads. */
+  ed25519_public_key_t blinded_pk;
+
   /* XXX: Client authorization. */
 } hs_ident_dir_conn_t;
 
@@ -120,6 +125,9 @@ hs_ident_circuit_t *hs_ident_circuit_dup(const hs_ident_circuit_t *src);
 /* Directory connection identifier API. */
 hs_ident_dir_conn_t *hs_ident_dir_conn_dup(const hs_ident_dir_conn_t *src);
 void hs_ident_dir_conn_free(hs_ident_dir_conn_t *ident);
+void hs_ident_dir_conn_init(const ed25519_public_key_t *identity_pk,
+                            const ed25519_public_key_t *blinded_pk,
+                            hs_ident_dir_conn_t *ident);
 
 /* Edge connection identifier API. */
 hs_ident_edge_conn_t *hs_ident_edge_conn_new(
diff --git a/src/or/hs_service.c b/src/or/hs_service.c
index 5759aa8127..d32a120bc8 100644
--- a/src/or/hs_service.c
+++ b/src/or/hs_service.c
@@ -14,6 +14,7 @@
 #include "circuitlist.h"
 #include "circuituse.h"
 #include "config.h"
+#include "connection.h"
 #include "directory.h"
 #include "main.h"
 #include "networkstatus.h"
@@ -641,6 +642,41 @@ count_desc_circuit_established(const hs_service_descriptor_t *desc)
   return count;
 }
 
+/* For a given service and descriptor of that service, close all active
+ * directory connections. */
+static void
+close_directory_connections(const hs_service_t *service,
+                            const hs_service_descriptor_t *desc)
+{
+  unsigned int count = 0;
+  smartlist_t *dir_conns;
+
+  tor_assert(service);
+  tor_assert(desc);
+
+  /* Close pending HS desc upload connections for the blinded key of 'desc'. */
+  dir_conns = connection_list_by_type_purpose(CONN_TYPE_DIR,
+                                              DIR_PURPOSE_UPLOAD_HSDESC);
+  SMARTLIST_FOREACH_BEGIN(dir_conns, connection_t *, conn) {
+    dir_connection_t *dir_conn = TO_DIR_CONN(conn);
+    if (ed25519_pubkey_eq(&dir_conn->hs_ident->identity_pk,
+                          &service->keys.identity_pk) &&
+        ed25519_pubkey_eq(&dir_conn->hs_ident->blinded_pk,
+                          &desc->blinded_kp.pubkey)) {
+      connection_mark_for_close(conn);
+      count++;
+      continue;
+    }
+  } SMARTLIST_FOREACH_END(conn);
+
+  log_info(LD_REND, "Closed %u active service directory connections for "
+                    "descriptor %s of service %s",
+           count, safe_str_client(ed25519_fmt(&desc->blinded_kp.pubkey)),
+           safe_str_client(service->onion_address));
+  /* We don't have ownership of the objects in this list. */
+  smartlist_free(dir_conns);
+}
+
 /* Close all rendezvous circuits for the given service. */
 static void
 close_service_rp_circuits(hs_service_t *service)
@@ -2137,7 +2173,9 @@ upload_descriptor_to_hsdir(const hs_service_t *service,
   }
 
   /* Setup the connection identifier. */
-  ed25519_pubkey_copy(&ident.identity_pk, &service->keys.identity_pk);
+  hs_ident_dir_conn_init(&service->keys.identity_pk, &desc->blinded_kp.pubkey,
+                         &ident);
+
   /* This is our resource when uploading which is used to construct the URL
    * with the version number: "/tor/hs/<version>/publish". */
   tor_snprintf(version_str, sizeof(version_str), "%u",
@@ -2387,6 +2425,13 @@ upload_descriptor_to_all(const hs_service_t *service,
   tor_assert(service);
   tor_assert(desc);
 
+  /* We'll first cancel any directory request that are ongoing for this
+   * descriptor. It is possible that we can trigger multiple uploads in a
+   * short time frame which can lead to a race where the second upload arrives
+   * before the first one leading to a 400 malformed descriptor response from
+   * the directory. Closing all pending requests avoids that. */
+  close_directory_connections(service, desc);
+
   /* Get our list of responsible HSDir. */
   responsible_dirs = smartlist_new();
   /* The parameter 0 means that we aren't a client so tell the function to use