commit 5ca9b830eacf46acde56469e5f19f977e5f02668 Author: George Kadianakis <desnacked@riseup.net> Date: Thu Aug 3 16:02:51 2017 +0300 Improve documentation all around the codebase. --- src/or/hs_cell.h | 17 +++++++++++------ src/or/hs_circuit.c | 18 +++++++++++------- src/or/hs_common.c | 9 +++++---- src/or/hs_ident.h | 4 +++- src/or/hs_service.c | 47 +++++++++++++++++++++++++++++++--------------- src/or/hs_service.h | 10 +++++----- src/or/or.h | 4 ++-- src/test/test_hs_common.c | 3 +++ src/test/test_hs_service.c | 12 ++++++++++++ 9 files changed, 84 insertions(+), 40 deletions(-) diff --git a/src/or/hs_cell.h b/src/or/hs_cell.h index b7c3d1127..f32f7a421 100644 --- a/src/or/hs_cell.h +++ b/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; diff --git a/src/or/hs_circuit.c b/src/or/hs_circuit.c index 3d67f24cb..2a753f659 100644 --- a/src/or/hs_circuit.c +++ b/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, diff --git a/src/or/hs_common.c b/src/or/hs_common.c index d68c446fd..570c1132c 100644 --- a/src/or/hs_common.c +++ b/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) { diff --git a/src/or/hs_ident.h b/src/or/hs_ident.h index a3ebd07da..e259fde54 100644 --- a/src/or/hs_ident.h +++ b/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 diff --git a/src/or/hs_service.c b/src/or/hs_service.c index a6f548d31..b46fd6329 100644 --- a/src/or/hs_service.c +++ b/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) diff --git a/src/or/hs_service.h b/src/or/hs_service.h index cf2e1fa6f..1ab0c1a4f 100644 --- a/src/or/hs_service.h +++ b/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; diff --git a/src/or/or.h b/src/or/or.h index 14dd4e0d6..1e498aecf 100644 --- a/src/or/or.h +++ b/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 diff --git a/src/test/test_hs_common.c b/src/test/test_hs_common.c index a5c499112..60577a2a5 100644 --- a/src/test/test_hs_common.c +++ b/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) { diff --git a/src/test/test_hs_service.c b/src/test/test_hs_service.c index 4ee2cdac8..10eeaa432 100644 --- a/src/test/test_hs_service.c +++ b/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) {