Improve documentation all around the codebase.

src/or/hs_cell.h

@@ -20,22 +20,27 @@ typedef enum {
 /* This data structure contains data that we need to parse an INTRODUCE2 cell
  * which is used by the INTRODUCE2 cell parsing function. On a successful
  * parsing, the onion_pk and rendezvous_cookie will be populated with the
- * computed key material from the cell data. */
+ * computed key material from the cell data. This structure is only used during
+ * INTRO2 parsing and discarded after that. */
 typedef struct hs_cell_introduce2_data_t {
-  /*** Immutable Section. ***/
+  /*** Immutable Section: Set on structure init. ***/
 
-  /* Introduction point authentication public key. */
+  /* Introduction point authentication public key. Pointer owned by the
+     introduction point object through which we received the INTRO2 cell. */
   const ed25519_public_key_t *auth_pk;
-  /* Introduction point encryption keypair for the ntor handshake. */
+  /* Introduction point encryption keypair for the ntor handshake. Pointer
+     owned by the introduction point object through which we received the
+     INTRO2 cell*/
   const curve25519_keypair_t *enc_kp;
-  /* Subcredentials of the service. */
+  /* Subcredentials of the service. Pointer owned by the descriptor that owns
+     the introduction point through which we received the INTRO2 cell. */
   const uint8_t *subcredential;
   /* Payload of the received encoded cell. */
   const uint8_t *payload;
   /* Size of the payload of the received encoded cell. */
   size_t payload_len;
 
-  /*** Muttable Section. ***/
+  /*** Mutable Section: Set upon parsing INTRODUCE2 cell. ***/
 
   /* Onion public key computed using the INTRODUCE2 encrypted section. */
   curve25519_public_key_t onion_pk;

src/or/hs_circuit.c

@@ -233,7 +233,7 @@ count_opened_desc_intro_point_circuits(const hs_service_t *service,
   return count;
 }
 
-/* From a given service, rendezvous cookie and handshake infor, create a
+/* From a given service, rendezvous cookie and handshake info, create a
  * rendezvous point circuit identifier. This can't fail. */
 static hs_ident_circuit_t *
 create_rp_circuit_identifier(const hs_service_t *service,
@@ -351,7 +351,7 @@ send_establish_intro(const hs_service_t *service,
  *  if direct_conn, IPv6 is prefered if we have one available.
  *  if firewall does not allow the chosen address, error.
  *
- * Return NULL if we can fulfill the conditions. */
+ * Return NULL if we can't fulfill the conditions. */
 static extend_info_t *
 get_rp_extend_info(const smartlist_t *link_specifiers,
                    const curve25519_public_key_t *onion_key, int direct_conn)
@@ -778,6 +778,8 @@ hs_circ_service_intro_has_opened(hs_service_t *service,
   tor_assert(desc);
   tor_assert(circ);
 
+  /* Cound opened circuits that have sent ESTABLISH_INTRO cells or are already
+   * established introduction circuits */
   num_intro_circ = count_opened_desc_intro_point_circuits(service, desc);
   num_needed_circ = service->config.num_intro_points;
   if (num_intro_circ > num_needed_circ) {
@@ -879,8 +881,9 @@ hs_circ_service_rp_has_opened(const hs_service_t *service,
   memwipe(payload, 0, sizeof(payload));
 }
 
-/* Handle an INTRO_ESTABLISHED cell payload of length payload_len arriving on
- * the given introduction circuit circ. The service is only used for logging
+/* Circ has been expecting an INTRO_ESTABLISHED cell that just arrived. Handle
+ * the INTRO_ESTABLISHED cell payload of length payload_len arriving on the
+ * given introduction circuit circ. The service is only used for logging
  * purposes. Return 0 on success else a negative value. */
 int
 hs_circ_handle_intro_established(const hs_service_t *service,
@@ -919,9 +922,10 @@ hs_circ_handle_intro_established(const hs_service_t *service,
   return ret;
 }
 
-/* Handle an INTRODUCE2 unparsed payload of payload_len for the given circuit
- * and service. This cell is associated with the intro point object ip and the
- * subcredential. Return 0 on success else a negative value. */
+/* We just received an INTRODUCE2 cell on the established introduction circuit
+ * circ.  Handle the INTRODUCE2 payload of size payload_len for the given
+ * circuit and service. This cell is associated with the intro point object ip
+ * and the subcredential. Return 0 on success else a negative value. */
 int
 hs_circ_handle_introduce2(const hs_service_t *service,
                           const origin_circuit_t *circ,

src/or/hs_common.c

@@ -669,7 +669,8 @@ hs_parse_address_impl(const char *address, ed25519_public_key_t *key_out,
 }
 
 /* Using the given identity public key and a blinded public key, compute the
- * subcredential and put it in subcred_out. This can't fail. */
+ * subcredential and put it in subcred_out (must be of size DIGEST256_LEN).
+ * This can't fail. */
 void
 hs_get_subcredential(const ed25519_public_key_t *identity_pk,
                      const ed25519_public_key_t *blinded_pk,
@@ -707,9 +708,9 @@ hs_get_subcredential(const ed25519_public_key_t *identity_pk,
   memwipe(credential, 0, sizeof(credential));
 }
 
-/* From the given list of hidden service ports, find the matching one from the
- * given edge connection conn and set the connection address from the service
- * port object. Return 0 on success or -1 if none. */
+/* From the given list of hidden service ports, find the ones that much the
+ * given edge connection conn, pick one at random and use it to set the
+ * connection address. Return 0 on success or -1 if none. */
 int
 hs_set_conn_addr_port(const smartlist_t *ports, edge_connection_t *conn)
 {

src/or/hs_ident.h

@@ -53,7 +53,9 @@ typedef struct hs_ident_circuit_t {
   hs_ident_circuit_type_t circuit_type;
 
   /* (All circuit) Introduction point authentication key. It's also needed on
-   * the rendezvous circuit for the ntor handshake. */
+   * the rendezvous circuit for the ntor handshake. It's used as the unique key
+   * of the introduction point so it should not be shared between multiple
+   * intro points. */
   ed25519_public_key_t intro_auth_pk;
 
   /* (Only client rendezvous circuit) Introduction point encryption public

src/or/hs_service.c

@@ -206,8 +206,9 @@ service_clear_config(hs_service_config_t *config)
   memset(config, 0, sizeof(*config));
 }
 
-/* Return the number of minimum INTRODUCE2 cell defined by a consensus
- * parameter or the default value. */
+/* Return the lower bound of maximum INTRODUCE2 cells per circuit before we
+ * rotate intro point (defined by a consensus parameter or the default
+ * value). */
 static int32_t
 get_intro_point_min_introduce2(void)
 {
@@ -218,8 +219,9 @@ get_intro_point_min_introduce2(void)
                                  0, INT32_MAX);
 }
 
-/* Return the number of maximum INTRODUCE2 cell defined by a consensus
- * parameter or the default value. */
+/* Return the upper bound of maximum INTRODUCE2 cells per circuit before we
+ * rotate intro point (defined by a consensus parameter or the default
+ * value). */
 static int32_t
 get_intro_point_max_introduce2(void)
 {
@@ -230,8 +232,8 @@ get_intro_point_max_introduce2(void)
                                  0, INT32_MAX);
 }
 
-/* Return the minimum lifetime of an introduction point defined by a consensus
- * parameter or the default value. */
+/* Return the minimum lifetime in seconds of an introduction point defined by a
+ * consensus parameter or the default value. */
 static int32_t
 get_intro_point_min_lifetime(void)
 {
@@ -247,8 +249,8 @@ get_intro_point_min_lifetime(void)
                                  0, INT32_MAX);
 }
 
-/* Return the maximum lifetime of an introduction point defined by a consensus
- * parameter or the default value. */
+/* Return the maximum lifetime in seconds of an introduction point defined by a
+ * consensus parameter or the default value. */
 static int32_t
 get_intro_point_max_lifetime(void)
 {
@@ -466,7 +468,16 @@ service_intro_point_find(const hs_service_t *service,
   tor_assert(service);
   tor_assert(auth_key);
 
-  /* Trying all descriptors. */
+  /* Trying all descriptors to find the right intro point.
+   *
+   * Even if we use the same node as intro point in both descriptors, the node
+   * will have a different intro auth key for each descriptor since we generate
+   * a new one everytime we pick an intro point.
+   *
+   * After #22893 gets implemented, intro points will be moved to be
+   * per-service instead of per-descriptor so this function will need to
+   * change.
+   */
   FOR_EACH_DESCRIPTOR_BEGIN(service, desc) {
     if ((ip = digest256map_get(desc->intro_points.map,
                                auth_key->pubkey)) != NULL) {
@@ -1647,6 +1658,10 @@ rotate_service_descriptors(hs_service_t *service)
 STATIC void
 rotate_all_descriptors(time_t now)
 {
+  /* XXX We rotate all our service descriptors at once. In the future it might
+   *     be wise, to rotate service descriptors independently to hide that all
+   *     those descriptors are on the same tor instance */
+
   FOR_EACH_SERVICE_BEGIN(service) {
     /* We are _not_ in the overlap period so skip rotation. */
     if (!hs_overlap_mode_is_active(NULL, now)) {
@@ -2379,7 +2394,7 @@ service_rendezvous_circ_has_opened(origin_circuit_t *circ)
 
   tor_assert(circ);
   tor_assert(circ->cpath);
-  /* Getting here means this is a v3 intro circuit. */
+  /* Getting here means this is a v3 rendezvous circuit. */
   tor_assert(circ->hs_ident);
   tor_assert(TO_CIRCUIT(circ)->purpose == CIRCUIT_PURPOSE_S_CONNECT_REND);
 
@@ -2411,8 +2426,9 @@ service_rendezvous_circ_has_opened(origin_circuit_t *circ)
   return;
 }
 
-/* Handle an INTRO_ESTABLISHED cell arriving on the given introduction
- * circuit. Return 0 on success else a negative value. */
+/* We've been expecting an INTRO_ESTABLISHED cell on this circuit and it just
+ * arrived. Handle the INTRO_ESTABLISHED cell arriving on the given
+ * introduction circuit. Return 0 on success else a negative value. */
 static int
 service_handle_intro_established(origin_circuit_t *circ,
                                  const uint8_t *payload,
@@ -2446,7 +2462,8 @@ service_handle_intro_established(origin_circuit_t *circ,
   }
 
   /* Try to parse the payload into a cell making sure we do actually have a
-   * valid cell. On success, the ip object is updated. */
+   * valid cell. On success, the ip object and circuit purpose is updated to
+   * reflect the fact that the introduction circuit is established. */
   if (hs_circ_handle_intro_established(service, ip, circ, payload,
                                        payload_len) < 0) {
     goto err;
@@ -2467,8 +2484,8 @@ service_handle_intro_established(origin_circuit_t *circ,
   return -1;
 }
 
-/* Handle an INTRODUCE2 cell arriving on the given introduction circuit.
- * Return 0 on success else a negative value. */
+/* We just received an INTRODUCE2 cell on the established introduction circuit
+ * circ. Handle the cell and return 0 on success else a negative value. */
 static int
 service_handle_introduce2(origin_circuit_t *circ, const uint8_t *payload,
                           size_t payload_len)

src/or/hs_service.h

@@ -123,11 +123,11 @@ typedef struct hs_service_descriptor_t {
    * couldn't pick any nodes. */
   unsigned int missing_intro_points : 1;
 
-  /* List of hidden service directories node_t object to which we couldn't
-   * upload this descriptor because we didn't have its router descriptor at
-   * the time. If this list is non-empty, only the relays in this list are
-   * re-tried to upload this descriptor when our directory information have
-   * been updated. */
+  /* List of identity digests for hidden service directories to which we
+   * couldn't upload this descriptor because we didn't have its router
+   * descriptor at the time. If this list is non-empty, only the relays in this
+   * list are re-tried to upload this descriptor when our directory information
+   * have been updated. */
   smartlist_t *hsdir_missing_info;
 } hs_service_descriptor_t;
 

src/or/or.h

@@ -421,9 +421,9 @@ typedef enum {
 #define DIR_PURPOSE_FETCH_RENDDESC_V2 18
 /** A connection to a directory server: download a microdescriptor. */
 #define DIR_PURPOSE_FETCH_MICRODESC 19
-/** A connetion to a hidden service directory: upload a descriptor. */
+/** A connection to a hidden service directory: upload a v3 descriptor. */
 #define DIR_PURPOSE_UPLOAD_HSDESC 20
-/** A connetion to a hidden service directory: fetch a descriptor. */
+/** A connection to a hidden service directory: fetch a v3 descriptor. */
 #define DIR_PURPOSE_FETCH_HSDESC 21
 #define DIR_PURPOSE_MAX_ 21
 

src/test/test_hs_common.c

@@ -16,6 +16,7 @@
 #include "hs_common.h"
 #include "config.h"
 
+/** Test the validation of HS v3 addresses */
 static void
 test_validate_address(void *arg)
 {
@@ -69,6 +70,7 @@ test_validate_address(void *arg)
   ;
 }
 
+/** Test building HS v3 onion addresses */
 static void
 test_build_address(void *arg)
 {
@@ -133,6 +135,7 @@ test_time_period(void *arg)
   ;
 }
 
+/** Test that we can correctly find the start time of the next time period */
 static void
 test_start_time_of_next_time_period(void *arg)
 {

src/test/test_hs_service.c

@@ -397,6 +397,7 @@ test_access_service(void *arg)
   hs_free_all();
 }
 
+/** Test that we can create intro point objects, index them and find them */
 static void
 test_service_intro_point(void *arg)
 {
@@ -609,6 +610,7 @@ test_helper_functions(void *arg)
   UNMOCK(node_get_by_id);
 }
 
+/** Test that we do the right operations when an intro circuit opens */
 static void
 test_intro_circuit_opened(void *arg)
 {
@@ -666,6 +668,8 @@ test_intro_circuit_opened(void *arg)
   UNMOCK(relay_send_command_from_edge_);
 }
 
+/** Test the operations we do on a circuit after we learn that we successfuly
+ *  established an intro point on it */
 static void
 test_intro_established(void *arg)
 {
@@ -735,6 +739,8 @@ test_intro_established(void *arg)
   UNMOCK(circuit_mark_for_close_);
 }
 
+/** Check the operations we do on a rendezvous circuit after we learn it's
+ *  open */
 static void
 test_rdv_circuit_opened(void *arg)
 {
@@ -776,6 +782,7 @@ test_rdv_circuit_opened(void *arg)
   UNMOCK(relay_send_command_from_edge_);
 }
 
+/** Test sending and receiving introduce2 cells */
 static void
 test_introduce2(void *arg)
 {
@@ -852,6 +859,8 @@ test_introduce2(void *arg)
   UNMOCK(circuit_mark_for_close_);
 }
 
+/** Test basic hidden service housekeeping operations (maintaining intro
+ *  points, etc) */
 static void
 test_service_event(void *arg)
 {
@@ -933,6 +942,7 @@ test_service_event(void *arg)
   UNMOCK(circuit_mark_for_close_);
 }
 
+/** Test that we rotate descriptors correctly in overlap period */
 static void
 test_rotate_descriptors(void *arg)
 {
@@ -1013,6 +1023,8 @@ test_rotate_descriptors(void *arg)
   UNMOCK(networkstatus_get_live_consensus);
 }
 
+/** Test building descriptors: picking intro points, setting up their link
+ *  specifiers, etc. */
 static void
 test_build_update_descriptors(void *arg)
 {