This is an automated email from the git hooks/post-receive script.

dgoulet pushed a commit to branch main in repository tor.

The following commit(s) were added to refs/heads/main by this push: new db4c4d656a metrics: Add metrics for rendezvous and introduction request failures. db4c4d656a is described below

commit db4c4d656a8d3cc58bd6179c8dedd3f3b5f68dd6 Author: Gabriela Moldovan gabi@torproject.org AuthorDate: Fri Feb 10 12:20:23 2023 +0000

metrics: Add metrics for rendezvous and introduction request failures.

This introduces a couple of new service side metrics: * `hs_intro_rejected_intro_req_count`, which counts the number of introduction requests rejected by the hidden service * `hs_rdv_error_count`, which counts the number of rendezvous errors as seen by the hidden service (this number includes the number of circuit establishment failures, failed retries, end-to-end circuit setup failures)

Closes #40755. This partially addresses #40717.

Signed-off-by: Gabriela Moldovan gabi@torproject.org --- changes/ticket40755 | 3 +++ src/core/or/circuituse.c | 7 ++++++- src/feature/hs/hs_circuit.c | 19 +++++++++++++------ src/feature/hs/hs_metrics.c | 4 ++-- src/feature/hs/hs_metrics.h | 12 ++++++++++-- src/feature/hs/hs_metrics_entry.c | 16 ++++++++++++++-- src/feature/hs/hs_metrics_entry.h | 6 +++++- src/feature/hs/hs_service.c | 6 +++++- src/test/test_hs_metrics.c | 17 +++++++++++++++++ src/test/test_hs_service.c | 17 +++++++++++++++++ 10 files changed, 92 insertions(+), 15 deletions(-)

diff --git a/changes/ticket40755 b/changes/ticket40755 new file mode 100644 index 0000000000..a40bfd9239 --- /dev/null +++ b/changes/ticket40755 @@ -0,0 +1,3 @@ + o Minor features (metrics): + - Add service side metrics for REND and introduction request failures. + Closes ticket 40755. diff --git a/src/core/or/circuituse.c b/src/core/or/circuituse.c index dbeea10821..421d3662af 100644 --- a/src/core/or/circuituse.c +++ b/src/core/or/circuituse.c @@ -51,6 +51,7 @@ #include "feature/hs/hs_client.h" #include "feature/hs/hs_common.h" #include "feature/hs/hs_ident.h" +#include "feature/hs/hs_metrics.h" #include "feature/hs/hs_stats.h" #include "feature/nodelist/describe.h" #include "feature/nodelist/networkstatus.h" @@ -1751,8 +1752,10 @@ circuit_build_failed(origin_circuit_t *circ) circuit_purpose_to_string(TO_CIRCUIT(circ)->purpose));

/* If the path failed on an RP, retry it. */ - if (TO_CIRCUIT(circ)->purpose == CIRCUIT_PURPOSE_S_CONNECT_REND) + if (TO_CIRCUIT(circ)->purpose == CIRCUIT_PURPOSE_S_CONNECT_REND) { + hs_metrics_failed_rdv(&circ->hs_ident->identity_pk); hs_circ_retry_service_rendezvous_point(circ); + }

/* In all other cases, just bail. The rest is just failure accounting * that we don't want to do */ @@ -1862,6 +1865,8 @@ circuit_build_failed(origin_circuit_t *circ) "(%s hop failed).", escaped(build_state_get_exit_nickname(circ->build_state)), failed_at_last_hop?"last":"non-last"); + + hs_metrics_failed_rdv(&circ->hs_ident->identity_pk); hs_circ_retry_service_rendezvous_point(circ); break; /* default: diff --git a/src/feature/hs/hs_circuit.c b/src/feature/hs/hs_circuit.c index 53855d40a9..bf8a2f3382 100644 --- a/src/feature/hs/hs_circuit.c +++ b/src/feature/hs/hs_circuit.c @@ -494,6 +494,7 @@ retry_service_rendezvous_point(const origin_circuit_t *circ) if (new_circ == NULL) { log_warn(LD_REND, "Failed to launch rendezvous circuit to %s", safe_str_client(extend_info_describe(bstate->chosen_exit))); + hs_metrics_failed_rdv(&circ->hs_ident->identity_pk); goto done; }

@@ -831,6 +832,7 @@ hs_circ_service_rp_has_opened(const hs_service_t *service, { size_t payload_len; uint8_t payload[RELAY_PAYLOAD_SIZE] = {0}; + int reason = 0;

tor_assert(service); tor_assert(circ); @@ -862,10 +864,11 @@ hs_circ_service_rp_has_opened(const hs_service_t *service, payload_len = HS_LEGACY_RENDEZVOUS_CELL_SIZE; }

- if (relay_send_command_from_edge(CONTROL_CELL_ID, TO_CIRCUIT(circ), - RELAY_COMMAND_RENDEZVOUS1, - (const char *) payload, payload_len, - circ->cpath->prev) < 0) { + if ((reason = relay_send_command_from_edge(CONTROL_CELL_ID, TO_CIRCUIT(circ), + RELAY_COMMAND_RENDEZVOUS1, + (const char *) payload, + payload_len, + circ->cpath->prev)) < 0) { /* On error, circuit is closed. */ log_warn(LD_REND, "Unable to send RENDEZVOUS1 cell on circuit %u " "for service %s", @@ -875,15 +878,19 @@ hs_circ_service_rp_has_opened(const hs_service_t *service, }

/* Setup end-to-end rendezvous circuit between the client and us. */ - if (hs_circuit_setup_e2e_rend_circ(circ, + if ((reason = hs_circuit_setup_e2e_rend_circ(circ, circ->hs_ident->rendezvous_ntor_key_seed, sizeof(circ->hs_ident->rendezvous_ntor_key_seed), - 1) < 0) { + 1)) < 0) { log_warn(LD_GENERAL, "Failed to setup circ"); goto done; }

done: + if (reason < 0) { + hs_metrics_failed_rdv(&service->keys.identity_pk); + } + memwipe(payload, 0, sizeof(payload)); }

diff --git a/src/feature/hs/hs_metrics.c b/src/feature/hs/hs_metrics.c index e80d98c2dd..854cd294c4 100644 --- a/src/feature/hs/hs_metrics.c +++ b/src/feature/hs/hs_metrics.c @@ -72,8 +72,8 @@ init_store(hs_service_t *service) * value used to update the entry. */ void hs_metrics_update_by_service(const hs_metrics_key_t key, - hs_service_t *service, const uint16_t port, - int64_t n) + const hs_service_t *service, + uint16_t port, int64_t n) { tor_assert(service);

diff --git a/src/feature/hs/hs_metrics.h b/src/feature/hs/hs_metrics.h index 2e0fa5048d..db02fefcea 100644 --- a/src/feature/hs/hs_metrics.h +++ b/src/feature/hs/hs_metrics.h @@ -28,13 +28,17 @@ void hs_metrics_update_by_ident(const hs_metrics_key_t key, const ed25519_public_key_t *ident_pk, const uint16_t port, int64_t n); void hs_metrics_update_by_service(const hs_metrics_key_t key, - hs_service_t *service, const uint16_t port, - int64_t n); + const hs_service_t *service, + uint16_t port, int64_t n);

/** New introducion request received. */ #define hs_metrics_new_introduction(s) \ hs_metrics_update_by_service(HS_METRICS_NUM_INTRODUCTIONS, (s), 0, 1)

+/** Introducion request rejected. */ +#define hs_metrics_reject_intro_req(s) \ + hs_metrics_update_by_service(HS_METRICS_NUM_REJECTED_INTRO_REQ, (s), 0, 1) + /** Number of bytes written to the application from the service. */ #define hs_metrics_app_write_bytes(i, port, n) \ hs_metrics_update_by_ident(HS_METRICS_APP_WRITE_BYTES, (i), (port), (n)) @@ -48,6 +52,10 @@ void hs_metrics_update_by_service(const hs_metrics_key_t key, #define hs_metrics_new_established_rdv(s) \ hs_metrics_update_by_service(HS_METRICS_NUM_ESTABLISHED_RDV, (s), 0, 1)

+/** New rendezvous circuit failure. */ +#define hs_metrics_failed_rdv(i) \ + hs_metrics_update_by_ident(HS_METRICS_NUM_FAILED_RDV, (i), 0, 1) + /** Established rendezvous closed. This is called when the circuit in * REND_JOINED state is marked for close. */ #define hs_metrics_close_established_rdv(i) \ diff --git a/src/feature/hs/hs_metrics_entry.c b/src/feature/hs/hs_metrics_entry.c index 46d2d88aca..2d4d3ce4f6 100644 --- a/src/feature/hs/hs_metrics_entry.c +++ b/src/feature/hs/hs_metrics_entry.c @@ -45,13 +45,19 @@ const hs_metrics_entry_t base_metrics[] = .key = HS_METRICS_NUM_ESTABLISHED_RDV, .type = METRICS_TYPE_GAUGE, .name = METRICS_NAME(hs_rdv_established_count), - .help = "Total number of established rendezvous circuit", + .help = "Total number of established rendezvous circuits", }, { .key = HS_METRICS_NUM_RDV, .type = METRICS_TYPE_COUNTER, .name = METRICS_NAME(hs_rdv_num_total), - .help = "Total number of rendezvous circuit created", + .help = "Total number of rendezvous circuits created", + }, + { + .key = HS_METRICS_NUM_FAILED_RDV, + .type = METRICS_TYPE_COUNTER, + .name = METRICS_NAME(hs_rdv_error_count), + .help = "Total number of rendezvous circuit errors", }, { .key = HS_METRICS_NUM_ESTABLISHED_INTRO, @@ -59,6 +65,12 @@ const hs_metrics_entry_t base_metrics[] = .name = METRICS_NAME(hs_intro_established_count), .help = "Total number of established introduction circuit", }, + { + .key = HS_METRICS_NUM_REJECTED_INTRO_REQ, + .type = METRICS_TYPE_COUNTER, + .name = METRICS_NAME(hs_intro_rejected_intro_req_count), + .help = "Total number of rejected introduction circuits", + }, };

/** Size of base_metrics array that is number of entries. */ diff --git a/src/feature/hs/hs_metrics_entry.h b/src/feature/hs/hs_metrics_entry.h index b9786ac6f7..2f732aa614 100644 --- a/src/feature/hs/hs_metrics_entry.h +++ b/src/feature/hs/hs_metrics_entry.h @@ -25,8 +25,12 @@ typedef enum { HS_METRICS_NUM_ESTABLISHED_RDV = 3, /** Number of rendezsvous circuits created. */ HS_METRICS_NUM_RDV = 4, + /** Number of failed rendezsvous. */ + HS_METRICS_NUM_FAILED_RDV = 5, /** Number of established introducton points. */ - HS_METRICS_NUM_ESTABLISHED_INTRO = 5, + HS_METRICS_NUM_ESTABLISHED_INTRO = 6, + /** Number of rejected introducton requests. */ + HS_METRICS_NUM_REJECTED_INTRO_REQ = 7, } hs_metrics_key_t;

/** The metadata of an HS metrics. */ diff --git a/src/feature/hs/hs_service.c b/src/feature/hs/hs_service.c index 479ac4670f..9c792b71c7 100644 --- a/src/feature/hs/hs_service.c +++ b/src/feature/hs/hs_service.c @@ -144,7 +144,7 @@ hs_service_ht_hash(const hs_service_t *service) sizeof(service->keys.identity_pk.pubkey)); }

-/** This is _the_ global hash map of hidden services which indexed the service +/** This is _the_ global hash map of hidden services which indexes the services * contained in it by master public identity key which is roughly the onion * address of the service. */ static struct hs_service_ht *hs_service_map; @@ -3524,6 +3524,10 @@ service_handle_introduce2(origin_circuit_t *circ, const uint8_t *payload,

return 0; err: + if (service) { + hs_metrics_reject_intro_req(service); + } + return -1; }

diff --git a/src/test/test_hs_metrics.c b/src/test/test_hs_metrics.c index 8625933df7..03f6aedbb4 100644 --- a/src/test/test_hs_metrics.c +++ b/src/test/test_hs_metrics.c @@ -56,6 +56,23 @@ test_metrics(void *arg) service, 0, 42); tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 84);

+ /* Update tor_hs_intro_rejected_intro_req_count */ + hs_metrics_update_by_ident(HS_METRICS_NUM_REJECTED_INTRO_REQ, + &service->keys.identity_pk, 0, 112); + + entries = metrics_store_get_all(service->metrics.store, + "tor_hs_intro_rejected_intro_req_count"); + tt_assert(entries); + tt_int_op(smartlist_len(entries), OP_EQ, 1); + entry = smartlist_get(entries, 0); + tt_assert(entry); + tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 112); + + /* Update tor_hs_intro_rejected_intro_req_count entry by service now. */ + hs_metrics_update_by_service(HS_METRICS_NUM_REJECTED_INTRO_REQ, + service, 0, 10); + tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 122); + done: hs_free_all(); } diff --git a/src/test/test_hs_service.c b/src/test/test_hs_service.c index 482ee1a014..7e675620bd 100644 --- a/src/test/test_hs_service.c +++ b/src/test/test_hs_service.c @@ -1183,6 +1183,8 @@ test_bad_introduce2(void *arg) origin_circuit_t *circ = NULL; hs_service_t *service = NULL; hs_service_intro_point_t *ip = NULL; + const smartlist_t *entries = NULL; + const metrics_store_entry_t *entry = NULL;

(void) arg;

@@ -1227,6 +1229,17 @@ test_bad_introduce2(void *arg) "an INTRODUCE2 cell on circuit"); teardown_capture_of_logs();

+ /* Make sure the tor_hs_intro_rejected_intro_req_count metric was + * incremented */ + entries = metrics_store_get_all(service->metrics.store, + "tor_hs_intro_rejected_intro_req_count"); + + tt_assert(entries); + tt_int_op(smartlist_len(entries), OP_EQ, 1); + entry = smartlist_get(entries, 0); + tt_assert(entry); + tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 1); + /* Set an IP object now for this circuit. */ { ip = helper_create_service_ip(); @@ -1243,6 +1256,10 @@ test_bad_introduce2(void *arg) tt_int_op(ret, OP_EQ, -1); tt_u64_op(ip->introduce2_count, OP_EQ, 0);

+ /* Make sure the tor_hs_intro_rejected_intro_req_count metric was incremented + * a second time */ + tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 2); + done: or_state_free(dummy_state); dummy_state = NULL;