[tor-commits] [tor/master] hs: Fix ADD_ONION with client authorization

[asn at torproject.org]

commit cf6e72b7020c47ba20677dc19602663723bd8491
Author: David Goulet <dgoulet at torproject.org>
Date:   Tue May 4 10:37:26 2021 -0400

    hs: Fix ADD_ONION with client authorization
    
    Turns out that passing client authorization keys to ADD_ONION for v3 was
    not working because we were not setting the "is_client_auth_enabled"
    flag to true once the clients were configured. This lead to the
    descriptor being encoded without the clients.
    
    This patch removes that flag and instead adds an inline function that
    can be used to check if a given service has client authorization
    enabled.
    
    This will be much less error prone of needing to keep in sync the client
    list and a flag instead.
    
    Fixes #40378
    
    Signed-off-by: David Goulet <dgoulet at torproject.org>
---
 changes/ticket40378         |  4 ++++
 src/feature/hs/hs_service.c | 18 +++++++++++-------
 src/feature/hs/hs_service.h |  3 ---
 src/test/test_hs_service.c  |  7 -------
 4 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/changes/ticket40378 b/changes/ticket40378
new file mode 100644
index 0000000000..35b2fd7bd4
--- /dev/null
+++ b/changes/ticket40378
@@ -0,0 +1,4 @@
+  o Major bugfixes (onion service, control port):
+    - Make the ADD_ONION command properly configure client authorization. Before
+      this fix, the created onion failed to add the client(s). Fixes bug 40378;
+      bugfix on 0.4.6.1-alpha.
diff --git a/src/feature/hs/hs_service.c b/src/feature/hs/hs_service.c
index a232377221..9b7b590140 100644
--- a/src/feature/hs/hs_service.c
+++ b/src/feature/hs/hs_service.c
@@ -160,6 +160,15 @@ HT_GENERATE2(hs_service_ht, hs_service_t, hs_service_node,
              hs_service_ht_hash, hs_service_ht_eq,
              0.6, tor_reallocarray, tor_free_);
 
+/** Return true iff the given service has client authorization configured that
+ * is the client list is non empty. */
+static inline bool
+is_client_auth_enabled(const hs_service_t *service)
+{
+  return (service->config.clients != NULL &&
+          smartlist_len(service->config.clients) > 0);
+}
+
 /** Query the given service map with a public key and return a service object
  * if found else NULL. It is also possible to set a directory path in the
  * search query. If pk is NULL, then it will be set to zero indicating the
@@ -1302,11 +1311,6 @@ load_client_keys(hs_service_t *service)
 
   } SMARTLIST_FOREACH_END(filename);
 
-  /* If the number of clients is greater than zero, set the flag to be true. */
-  if (smartlist_len(config->clients) > 0) {
-    config->is_client_auth_enabled = 1;
-  }
-
   /* Success. */
   ret = 0;
  end:
@@ -1816,7 +1820,7 @@ build_service_desc_superencrypted(const hs_service_t *service,
 
   /* We do not need to build the desc authorized client if the client
    * authorization is disabled */
-  if (config->is_client_auth_enabled) {
+  if (is_client_auth_enabled(service)) {
     SMARTLIST_FOREACH_BEGIN(config->clients,
                             hs_service_authorized_client_t *, client) {
       hs_desc_authorized_client_t *desc_client;
@@ -3588,7 +3592,7 @@ service_encode_descriptor(const hs_service_t *service,
 
   /* If the client authorization is enabled, send the descriptor cookie to
    * hs_desc_encode_descriptor. Otherwise, send NULL */
-  if (service->config.is_client_auth_enabled) {
+  if (is_client_auth_enabled(service)) {
     descriptor_cookie = desc->descriptor_cookie;
   }
 
diff --git a/src/feature/hs/hs_service.h b/src/feature/hs/hs_service.h
index f5be37b410..c48f470245 100644
--- a/src/feature/hs/hs_service.h
+++ b/src/feature/hs/hs_service.h
@@ -230,9 +230,6 @@ typedef struct hs_service_config_t {
    * HiddenServiceNumIntroductionPoints option. */
   unsigned int num_intro_points;
 
-  /** True iff the client auth is enabled. */
-  unsigned int is_client_auth_enabled : 1;
-
   /** List of hs_service_authorized_client_t's of clients that may access this
    * service. Specified by HiddenServiceAuthorizeClient option. */
   smartlist_t *clients;
diff --git a/src/test/test_hs_service.c b/src/test/test_hs_service.c
index e754eac108..33a3f279c6 100644
--- a/src/test/test_hs_service.c
+++ b/src/test/test_hs_service.c
@@ -341,7 +341,6 @@ helper_create_service_with_clients(int num_clients)
   int i;
   hs_service_t *service = helper_create_service();
   tt_assert(service);
-  service->config.is_client_auth_enabled = 1;
   service->config.clients = smartlist_new();
 
   for (i = 0; i < num_clients; i++) {
@@ -425,9 +424,6 @@ test_load_keys(void *arg)
   tt_int_op(hs_address_is_valid(addr), OP_EQ, 1);
   tt_str_op(addr, OP_EQ, s->onion_address);
 
-  /* Check that the is_client_auth_enabled is not set. */
-  tt_assert(!s->config.is_client_auth_enabled);
-
  done:
   tor_free(hsdir_v3);
   hs_free_all();
@@ -577,9 +573,6 @@ test_load_keys_with_client_auth(void *arg)
   tt_int_op(smartlist_len(service->config.clients), OP_EQ,
             smartlist_len(pubkey_b32_list));
 
-  /* Test that the is_client_auth_enabled flag is set. */
-  tt_assert(service->config.is_client_auth_enabled);
-
   /* Test that the keys in clients are correct. */
   SMARTLIST_FOREACH_BEGIN(pubkey_b32_list, char *, pubkey_b32) {