Some test and logic corrections

src/feature/control/control_cmd.c

@@ -1831,8 +1831,9 @@ handle_control_add_onion(control_connection_t *conn,
       }
     } else if (!strcasecmp(arg->key, "ClientAuthV3")) {
       hs_service_authorized_client_t *client_v3 =
-                                       parse_authorized_client_key(arg->value);
+                                parse_authorized_client_key(arg->value, false);
       if (!client_v3) {
+        control_write_endreply(conn, 512, "Cannot decode v3 client auth key");
         goto out;
       }
 
@@ -1925,7 +1926,6 @@ handle_control_add_onion(control_connection_t *conn,
                                    auth_clients, auth_clients_v3, &service_id);
   port_cfgs = NULL; /* port_cfgs is now owned by the rendservice code. */
   auth_clients = NULL; /* so is auth_clients */
-  auth_clients_v3 = NULL; /* so is auth_clients_v3 */
   switch (ret) {
   case RSAE_OKAY:
   {

src/feature/hs/hs_service.c

@@ -1119,17 +1119,19 @@ client_filename_is_valid(const char *filename)
  *
  * Return the key on success, return NULL, otherwise. */
 hs_service_authorized_client_t *
-parse_authorized_client_key(const char *key_str)
+parse_authorized_client_key(const char *key_str, bool log)
 {
   hs_service_authorized_client_t *client = NULL;
 
-  /* We expect a specific length of the base32 encoded key so make sure we
+  /* We expect a specific length of the base64 encoded key so make sure we
    * have that so we don't successfully decode a value with a different length
    * and end up in trouble when copying the decoded key into a fixed length
    * buffer. */
   if (strlen(key_str) != BASE32_NOPAD_LEN(CURVE25519_PUBKEY_LEN)) {
+    if (log) {
       log_warn(LD_REND, "Client authorization encoded base32 public key "
                         "length is invalid: %s", key_str);
+    }
     goto err;
   }
 
@@ -1138,8 +1140,10 @@ parse_authorized_client_key(const char *key_str)
                     sizeof(client->client_pk.public_key),
                     key_str, strlen(key_str)) !=
       sizeof(client->client_pk.public_key)) {
-    log_warn(LD_REND, "Client authorization public key cannot be decoded: %s",
-             key_str);
+    if (log) {
+      log_warn(LD_REND, "Client authorization public key cannot be decoded: "
+               "%s", key_str);
+    }
     goto err;
   }
 
@@ -1198,7 +1202,7 @@ parse_authorized_client(const char *client_key_str)
     goto err;
   }
 
-  if ((client = parse_authorized_client_key(pubkey_b32)) == NULL) {
+  if ((client = parse_authorized_client_key(pubkey_b32, true)) == NULL) {
     goto err;
   }
 
@@ -3753,14 +3757,14 @@ hs_service_add_ephemeral(ed25519_secret_key_t *sk, smartlist_t *ports,
     goto err;
   }
 
+  if (auth_clients_v3) {
     if (service->config.clients == NULL) {
       service->config.clients = smartlist_new();
     }
     SMARTLIST_FOREACH(auth_clients_v3, hs_service_authorized_client_t *, c, {
-    if (c != NULL) {
       smartlist_add(service->config.clients, c);
-    }
     });
+  }
 
   /* Build the onion address for logging purposes but also the control port
    * uses it for the HS_DESC event. */

src/feature/hs/hs_service.h

@@ -390,7 +390,7 @@ void hs_service_dump_stats(int severity);
 void hs_service_circuit_cleanup_on_close(const circuit_t *circ);
 
 hs_service_authorized_client_t *
-parse_authorized_client_key(const char *key_str);
+parse_authorized_client_key(const char *key_str, bool log);
 
 void
 service_authorized_client_free_(hs_service_authorized_client_t *client);

src/test/test_hs_control.c

@@ -743,70 +743,46 @@ test_hs_control_add_onion_with_bad_pubkey(void *arg)
 static void
 test_hs_add_onion_helper_add_service(void *arg)
 {
-  int hs_version_good, hs_version_bad;
-  add_onion_secret_key_t sk_good, sk_bad;
-  ed25519_public_key_t pk_good, pk_bad;
-  char *key_new_blob_good = NULL, *key_new_blob_bad = NULL;
-  const char *key_new_alg_good = NULL, *key_new_alg_bad = NULL;
-  hs_service_authorized_client_t *client_good, *client_bad;
-  smartlist_t *list_v2, *list_good, *list_bad;
-  hs_service_ht *global_map;
-  rend_service_port_config_t *portcfg;
-  smartlist_t *portcfgs;
-  char *address_out_good, *address_out_bad;
+  control_connection_t conn;
+  char *args = NULL, *cp1 = NULL;
+  size_t sz;
 
   (void) arg;
 
   hs_init();
-  global_map = get_hs_service_map();
 
-  portcfg = rend_service_parse_port_config("8080", ",", NULL);
-  portcfgs = smartlist_new();
-  smartlist_add(portcfgs, portcfg);
+  memset(&conn, 0, sizeof(control_connection_t));
+  TO_CONN(&conn)->outbuf = buf_new();
+  conn.current_cmd = tor_strdup("ADD_ONION");
+  args = tor_strdup("ED25519-V3:KLMQ4CLKwlDCHuMPn8j3od33cU5LhnrLNoZh7CWChl3VkY"
+    "pNAkeP5dGW8xeKR9HxQBWQ/w7Kr12lA/U8Pd/oxw== "
+    "ClientAuthV3=dz4q5xqlb4ldnbs72iarrml4ephk3du4i7o2cgiva5lwr6wkquja "
+    "Flags=V3Auth Port=9735,127.0.0.1");
+  handle_control_command(&conn, (uint32_t) strlen(args), args);
+  cp1 = buf_get_contents(TO_CONN(&conn)->outbuf, &sz);
+  tt_str_op(cp1, OP_EQ,
+   "250-ServiceID=n35etu3yjxrqjpntmfziom5sjwspoydchmelc4xleoy4jk2u4lziz2yd\r\n"
+   "250-ClientAuthV3=dz4q5xqlb4ldnbs72iarrml4ephk3du4i7o2cgiva5lwr6wkquja\r\n"
+   "250 OK\r\n");
+  tor_free(args);
+  tor_free(cp1);
 
-  memset(&sk_good, 0, sizeof(sk_good));
-  memset(&sk_bad, 0, sizeof(sk_bad));
-
-  add_onion_helper_keyarg("NEW:ED25519-V3", 0, &key_new_alg_good,
-                         &key_new_blob_good, &sk_good, &hs_version_good, NULL);
-  add_onion_helper_keyarg("NEW:ED25519-V3", 0, &key_new_alg_bad,
-                         &key_new_blob_bad, &sk_bad, &hs_version_bad, NULL);
-
-  ed25519_public_key_generate(&pk_good, sk_good.v3);
-  ed25519_public_key_generate(&pk_bad, sk_bad.v3);
-
-  client_good = parse_authorized_client_key(
-                       "N2NU7BSRL6YODZCYPN4CREB54TYLKGIE2KYOQWLFYC23ZJVCE5DQ");
-  client_bad = parse_authorized_client_key("dummy");
-
-  list_v2 = smartlist_new();
-  list_good = smartlist_new();
-  smartlist_add(list_good, client_good);
-  list_bad = smartlist_new();
-  smartlist_add(list_bad, client_bad);
-
-  add_onion_helper_add_service(HS_VERSION_THREE, &sk_good, portcfgs, 1, 1,
-                          REND_V3_AUTH, list_v2, list_good, &address_out_good);
-  add_onion_helper_add_service(HS_VERSION_THREE, &sk_bad, portcfgs, 1, 1,
-                          REND_V3_AUTH, list_v2, list_bad, &address_out_bad);
-
-  hs_service_t *srv_good = find_service(global_map, &pk_good);
-  hs_service_t *srv_bad = find_service(global_map, &pk_bad);
-
-  tt_int_op(smartlist_len(srv_good->config.clients), OP_EQ, 1);
-  tt_int_op(smartlist_len(srv_bad->config.clients), OP_EQ, 0);
+  args = tor_strdup("ED25519-V3:iIU8EBi71qE7G6UTsROU1kWN0JMrRP/YukC0Xk5WLGyil3"
+    "gm4u3wEBXr+/TaCpXS+65Pcdqz+PG+4+oWHLN05A== "
+    "ClientAuthV3=dummy Flags=V3Auth Port=9735,127.0.0.1");
+  handle_control_command(&conn, (uint32_t) strlen(args), args);
+  cp1 = buf_get_contents(TO_CONN(&conn)->outbuf, &sz);
+  tt_str_op(cp1, OP_EQ, "512 Cannot decode v3 client auth key\r\n");
 
  done:
-  tor_free(key_new_blob_good);
-  tor_free(key_new_blob_bad);
-  tor_free(address_out_good);
-  tor_free(address_out_bad);
-
-  service_authorized_client_free(client_good);
-
-  smartlist_free(list_v2);
-  smartlist_free(list_good);
-  smartlist_free(list_bad);
+  tor_free(args);
+  tor_free(cp1);
+  tor_free(conn.current_cmd);
+  buf_free(TO_CONN(&conn)->outbuf);
+  SMARTLIST_FOREACH(conn.ephemeral_onion_services, char *,
+                    service, tor_free(service));
+  smartlist_free(conn.ephemeral_onion_services);
+  hs_client_free_all();
 }
 
 struct testcase_t hs_control_tests[] = {