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 16c6788fbc metrics: Add a `reason` label to the HS error metrics. new 3fa08dc9a7 Merge branch 'tor-gitlab/mr/697' 16c6788fbc is described below

commit 16c6788fbc30cf0a2611dd021d1797931a53a86d Author: Gabriela Moldovan gabi@torproject.org AuthorDate: Wed Feb 15 14:52:35 2023 +0000

metrics: Add a `reason` label to the HS error metrics.

This adds a `reason` label to the `hs_intro_rejected_intro_req_count` and `hs_rdv_error_count` metrics introduced in #40755.

Metric look up and intialization is now more a bit more involved. This may be fine for now, but it will become unwieldy if/when we add more labels (and as such will need to be refactored).

Also, in the future, we may want to introduce finer grained `reason` labels. For example, the `invalid_introduce2` label actually covers multiple types of errors that can happen during the processing of an INTRODUCE2 cell (such as cell parse errors, replays, decryption errors).

Signed-off-by: Gabriela Moldovan gabi@torproject.org --- changes/ticket40758 | 3 + src/core/or/circuituse.c | 6 +- src/feature/hs/hs_circuit.c | 38 ++++++---- src/feature/hs/hs_metrics.c | 132 ++++++++++++++++++++++++++-------- src/feature/hs/hs_metrics.h | 40 ++++++----- src/feature/hs/hs_metrics_entry.c | 32 +++++++++ src/feature/hs/hs_metrics_entry.h | 34 ++++++++- src/feature/hs/hs_service.c | 7 +- src/lib/metrics/metrics_store_entry.c | 20 ++++++ src/lib/metrics/metrics_store_entry.h | 3 + src/test/test_hs_metrics.c | 19 +++-- src/test/test_hs_service.c | 92 ++++++++++++++++++++++-- 12 files changed, 345 insertions(+), 81 deletions(-)

diff --git a/changes/ticket40758 b/changes/ticket40758 new file mode 100644 index 0000000000..e6981d0e2b --- /dev/null +++ b/changes/ticket40758 @@ -0,0 +1,3 @@ + o Minor features (metrics): + - Add a `reason` label to the HS error metrics. + Closes ticket 40758. diff --git a/src/core/or/circuituse.c b/src/core/or/circuituse.c index 421d3662af..77c5deaafb 100644 --- a/src/core/or/circuituse.c +++ b/src/core/or/circuituse.c @@ -1753,7 +1753,8 @@ circuit_build_failed(origin_circuit_t *circ)

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

@@ -1866,7 +1867,8 @@ circuit_build_failed(origin_circuit_t *circ) 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_metrics_failed_rdv(&circ->hs_ident->identity_pk, + HS_METRICS_ERR_RDV_RP_CONN_FAILURE); 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 bf8a2f3382..006ba964fe 100644 --- a/src/feature/hs/hs_circuit.c +++ b/src/feature/hs/hs_circuit.c @@ -494,7 +494,10 @@ 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); + + hs_metrics_failed_rdv(&circ->hs_ident->identity_pk, + HS_METRICS_ERR_RDV_RETRY); + goto done; }

@@ -832,7 +835,6 @@ 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); @@ -864,33 +866,34 @@ hs_circ_service_rp_has_opened(const hs_service_t *service, payload_len = HS_LEGACY_RENDEZVOUS_CELL_SIZE; }

- 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) { + if (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", TO_CIRCUIT(circ)->n_circ_id, safe_str_client(service->onion_address)); + + hs_metrics_failed_rdv(&service->keys.identity_pk, + HS_METRICS_ERR_RDV_RENDEZVOUS1); goto done; }

/* Setup end-to-end rendezvous circuit between the client and us. */ - if ((reason = hs_circuit_setup_e2e_rend_circ(circ, + if (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"); + + hs_metrics_failed_rdv(&service->keys.identity_pk, HS_METRICS_ERR_RDV_E2E); goto done; }

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

@@ -1004,12 +1007,15 @@ hs_circ_handle_introduce2(const hs_service_t *service, data.replay_cache = ip->replay_cache; data.cc_enabled = 0;

- if (get_subcredential_for_handling_intro2_cell(service, - &data, subcredential)) { + if (get_subcredential_for_handling_intro2_cell(service, &data, + subcredential)) { + hs_metrics_reject_intro_req(service, + HS_METRICS_ERR_INTRO_REQ_SUBCREDENTIAL); goto done; }

if (hs_cell_parse_introduce2(&data, circ, service) < 0) { + hs_metrics_reject_intro_req(service, HS_METRICS_ERR_INTRO_REQ_INTRODUCE2); goto done; }

@@ -1027,6 +1033,8 @@ hs_circ_handle_introduce2(const hs_service_t *service, log_info(LD_REND, "We received an INTRODUCE2 cell with same REND_COOKIE " "field %ld seconds ago. Dropping cell.", (long int) elapsed); + hs_metrics_reject_intro_req(service, + HS_METRICS_ERR_INTRO_REQ_INTRODUCE2_REPLAY); goto done; }

diff --git a/src/feature/hs/hs_metrics.c b/src/feature/hs/hs_metrics.c index 854cd294c4..4c6d957cf8 100644 --- a/src/feature/hs/hs_metrics.c +++ b/src/feature/hs/hs_metrics.c @@ -13,6 +13,7 @@ #include "lib/malloc/malloc.h" #include "lib/container/smartlist.h" #include "lib/metrics/metrics_store.h" +#include "lib/log/util_bug.h"

#include "feature/hs/hs_metrics.h" #include "feature/hs/hs_metrics_entry.h" @@ -29,51 +30,118 @@ port_to_str(const uint16_t port) return buf; }

-/** Initialize a metrics store for the given service. +/** Add a new metric to the metrics store of the service. * - * Essentially, this goes over the base_metrics array and adds them all to the - * store set with their label(s) if any. */ + * <b>metric</b> is the index of the metric in the <b>base_metrics</b> array. + */ static void -init_store(hs_service_t *service) +add_metric_with_labels(hs_service_t *service, hs_metrics_key_t metric, + bool port_as_label, uint16_t port) { metrics_store_t *store; + const char **error_reasons = NULL; + size_t num_error_reasons = 0;

tor_assert(service);

+ if (BUG(metric >= base_metrics_size)) + return; + store = service->metrics.store;

+ /* Check whether the current metric is an error metric, because error metrics + * require an additional `reason` label. */ + switch (metric) { + case HS_METRICS_NUM_REJECTED_INTRO_REQ: + error_reasons = hs_metrics_intro_req_error_reasons; + num_error_reasons = hs_metrics_intro_req_error_reasons_size; + break; + case HS_METRICS_NUM_FAILED_RDV: + error_reasons = hs_metrics_rend_error_reasons; + num_error_reasons = hs_metrics_rend_error_reasons_size; + break; + /* Fall through for all other metrics, as they don't need a + * reason label. */ + case HS_METRICS_NUM_INTRODUCTIONS: FALLTHROUGH; + case HS_METRICS_APP_WRITE_BYTES: FALLTHROUGH; + case HS_METRICS_APP_READ_BYTES: FALLTHROUGH; + case HS_METRICS_NUM_ESTABLISHED_RDV: FALLTHROUGH; + case HS_METRICS_NUM_RDV: FALLTHROUGH; + case HS_METRICS_NUM_ESTABLISHED_INTRO: FALLTHROUGH; + default: + break; + } + + /* We don't need a reason label for this metric */ + if (!num_error_reasons) { + metrics_store_entry_t *entry = metrics_store_add( + store, base_metrics[metric].type, base_metrics[metric].name, + base_metrics[metric].help); + + metrics_store_entry_add_label(entry, + metrics_format_label("onion", service->onion_address)); + + if (port_as_label) { + metrics_store_entry_add_label(entry, + metrics_format_label("port", port_to_str(port))); + } + + return; + } + + tor_assert(error_reasons); + + /* Add entries with reason as label. We need one metric line per + * reason. */ + for (size_t i = 0; i < num_error_reasons; ++i) { + metrics_store_entry_t *entry = + metrics_store_add(store, base_metrics[metric].type, + base_metrics[metric].name, + base_metrics[metric].help); + /* Add labels to the entry. */ + metrics_store_entry_add_label(entry, + metrics_format_label("onion", service->onion_address)); + metrics_store_entry_add_label(entry, + metrics_format_label("reason", error_reasons[i])); + + if (port_as_label) { + metrics_store_entry_add_label(entry, + metrics_format_label("port", port_to_str(port))); + } + } +} + +/** Initialize a metrics store for the given service. + * + * Essentially, this goes over the base_metrics array and adds them all to the + * store set with their label(s) if any. */ +static void +init_store(hs_service_t *service) +{ + tor_assert(service); + for (size_t i = 0; i < base_metrics_size; ++i) { /* Add entries with port as label. We need one metric line per port. */ if (base_metrics[i].port_as_label && service->config.ports) { SMARTLIST_FOREACH_BEGIN(service->config.ports, const hs_port_config_t *, p) { - metrics_store_entry_t *entry = - metrics_store_add(store, base_metrics[i].type, base_metrics[i].name, - base_metrics[i].help); - - /* Add labels to the entry. */ - metrics_store_entry_add_label(entry, - metrics_format_label("onion", service->onion_address)); - metrics_store_entry_add_label(entry, - metrics_format_label("port", port_to_str(p->virtual_port))); + add_metric_with_labels(service, base_metrics[i].key, true, + p->virtual_port); } SMARTLIST_FOREACH_END(p); } else { - metrics_store_entry_t *entry = - metrics_store_add(store, base_metrics[i].type, base_metrics[i].name, - base_metrics[i].help); - metrics_store_entry_add_label(entry, - metrics_format_label("onion", service->onion_address)); + add_metric_with_labels(service, base_metrics[i].key, false, 0); } } }

/** Update the metrics key entry in the store in the given service. The port, - * if non 0, is used to find the correct metrics entry. The value n is the - * value used to update the entry. */ + * if non 0, and the reason label, if non NULL, are used to find the correct + * metrics entry. The value n is the value used to update the entry. */ void hs_metrics_update_by_service(const hs_metrics_key_t key, const hs_service_t *service, - uint16_t port, int64_t n) + uint16_t port, const char *reason, + int64_t n) { tor_assert(service);

@@ -89,25 +157,29 @@ hs_metrics_update_by_service(const hs_metrics_key_t key, * XXX: This is not the most optimal due to the string format. Maybe at some * point turn this into a kvline and a map in a metric entry? */ SMARTLIST_FOREACH_BEGIN(entries, metrics_store_entry_t *, entry) { - if (port == 0 || - metrics_store_entry_has_label(entry, - metrics_format_label("port", port_to_str(port)))) { - metrics_store_entry_update(entry, n); - break; + if ((port == 0 || + metrics_store_entry_has_label( + entry, metrics_format_label("port", port_to_str(port)))) && + ((!reason || metrics_store_entry_has_label( + entry, metrics_format_label("reason", reason))))) { + metrics_store_entry_update(entry, n); + break; } } SMARTLIST_FOREACH_END(entry); }

/** Update the metrics key entry in the store of a service identified by the - * given identity public key. The port, if non 0, is used to find the correct - * metrics entry. The value n is the value used to update the entry. + * given identity public key. The port, if non 0, and the reason label, if non + * NULL, are used to find the correct metrics entry. The value n is the value + * used to update the entry. * * This is used by callsite that have access to the key but not the service * object so an extra lookup is done to find the service. */ 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) + const uint16_t port, const char *reason, + int64_t n) { hs_service_t *service;

@@ -121,7 +193,7 @@ hs_metrics_update_by_ident(const hs_metrics_key_t key, * service and thus the only way to know is to lookup the service. */ return; } - hs_metrics_update_by_service(key, service, port, n); + hs_metrics_update_by_service(key, service, port, reason, n); }

/** Return a list of all the onion service metrics stores. This is the diff --git a/src/feature/hs/hs_metrics.h b/src/feature/hs/hs_metrics.h index db02fefcea..a32f76e3bc 100644 --- a/src/feature/hs/hs_metrics.h +++ b/src/feature/hs/hs_metrics.h @@ -26,53 +26,59 @@ const smartlist_t *hs_metrics_get_stores(void); /* Metrics Update. */ 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); + const uint16_t port, const char *reason, + int64_t n); void hs_metrics_update_by_service(const hs_metrics_key_t key, const hs_service_t *service, - uint16_t port, int64_t n); + uint16_t port, const char *reason, + 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) + hs_metrics_update_by_service(HS_METRICS_NUM_INTRODUCTIONS, (s), 0, NULL, 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) +#define hs_metrics_reject_intro_req(s, reason) \ + hs_metrics_update_by_service(HS_METRICS_NUM_REJECTED_INTRO_REQ, (s), 0, \ + (reason), 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)) +#define hs_metrics_app_write_bytes(i, port, n) \ + hs_metrics_update_by_ident(HS_METRICS_APP_WRITE_BYTES, (i), (port), NULL, \ + (n))

/** Number of bytes read from the application to the service. */ #define hs_metrics_app_read_bytes(i, port, n) \ - hs_metrics_update_by_ident(HS_METRICS_APP_READ_BYTES, (i), (port), (n)) + hs_metrics_update_by_ident(HS_METRICS_APP_READ_BYTES, (i), (port), NULL, (n))

/** Newly established rendezvous. This is called as soon as the circuit purpose * is REND_JOINED which is when the RENDEZVOUS2 cell is sent. */ #define hs_metrics_new_established_rdv(s) \ - hs_metrics_update_by_service(HS_METRICS_NUM_ESTABLISHED_RDV, (s), 0, 1) + hs_metrics_update_by_service(HS_METRICS_NUM_ESTABLISHED_RDV, (s), 0, NULL, 1)

/** New rendezvous circuit failure. */ -#define hs_metrics_failed_rdv(i) \ - hs_metrics_update_by_ident(HS_METRICS_NUM_FAILED_RDV, (i), 0, 1) +#define hs_metrics_failed_rdv(i, reason) \ + hs_metrics_update_by_ident(HS_METRICS_NUM_FAILED_RDV, (i), 0, (reason), 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) \ - hs_metrics_update_by_ident(HS_METRICS_NUM_ESTABLISHED_RDV, (i), 0, -1) + hs_metrics_update_by_ident(HS_METRICS_NUM_ESTABLISHED_RDV, (i), 0, NULL, -1)

/** New rendezvous circuit being launched. */ #define hs_metrics_new_rdv(i) \ - hs_metrics_update_by_ident(HS_METRICS_NUM_RDV, (i), 0, 1) + hs_metrics_update_by_ident(HS_METRICS_NUM_RDV, (i), 0, NULL, 1)

/** New introduction circuit has been established. This is called when the * INTRO_ESTABLISHED has been received by the service. */ -#define hs_metrics_new_established_intro(s) \ - hs_metrics_update_by_service(HS_METRICS_NUM_ESTABLISHED_INTRO, (s), 0, 1) +#define hs_metrics_new_established_intro(s) \ + hs_metrics_update_by_service(HS_METRICS_NUM_ESTABLISHED_INTRO, (s), 0, \ + NULL, 1)

/** Established introduction circuit closes. This is called when * INTRO_ESTABLISHED circuit is marked for close. */ -#define hs_metrics_close_established_intro(i) \ - hs_metrics_update_by_ident(HS_METRICS_NUM_ESTABLISHED_INTRO, (i), 0, -1) +#define hs_metrics_close_established_intro(i) \ + hs_metrics_update_by_ident(HS_METRICS_NUM_ESTABLISHED_INTRO, (i), 0, NULL, \ + -1)

#endif /* !defined(TOR_FEATURE_HS_HS_METRICS_H) */ diff --git a/src/feature/hs/hs_metrics_entry.c b/src/feature/hs/hs_metrics_entry.c index 2d4d3ce4f6..9181d770e0 100644 --- a/src/feature/hs/hs_metrics_entry.c +++ b/src/feature/hs/hs_metrics_entry.c @@ -8,9 +8,13 @@

#define HS_METRICS_ENTRY_PRIVATE

+#include <stddef.h> + #include "orconfig.h"

#include "lib/cc/compat_compiler.h" +#include "lib/log/log.h" +#include "lib/log/util_bug.h"

#include "feature/hs/hs_metrics_entry.h"

@@ -75,3 +79,31 @@ const hs_metrics_entry_t base_metrics[] =

/** Size of base_metrics array that is number of entries. */ const size_t base_metrics_size = ARRAY_LENGTH(base_metrics); + +/** Possible values for the reason label of the + * hs_intro_rejected_intro_req_count metric. */ +const char *hs_metrics_intro_req_error_reasons[] = +{ + HS_METRICS_ERR_INTRO_REQ_BAD_AUTH_KEY, + HS_METRICS_ERR_INTRO_REQ_INTRODUCE2, + HS_METRICS_ERR_INTRO_REQ_SUBCREDENTIAL, + HS_METRICS_ERR_INTRO_REQ_INTRODUCE2_REPLAY, +}; + +/** The number of entries in the hs_metrics_intro_req_error_reasons array. */ +const size_t hs_metrics_intro_req_error_reasons_size = + ARRAY_LENGTH(hs_metrics_intro_req_error_reasons); + +/** Possible values for the reason label of the hs_rdv_error_count metric. */ +const char *hs_metrics_rend_error_reasons[] = +{ + HS_METRICS_ERR_RDV_RP_CONN_FAILURE, + HS_METRICS_ERR_RDV_PATH, + HS_METRICS_ERR_RDV_RENDEZVOUS1, + HS_METRICS_ERR_RDV_E2E, + HS_METRICS_ERR_RDV_RETRY, +}; + +/** The number of entries in the hs_metrics_rend_error_reasons array. */ +const size_t hs_metrics_rend_error_reasons_size = + ARRAY_LENGTH(hs_metrics_rend_error_reasons); diff --git a/src/feature/hs/hs_metrics_entry.h b/src/feature/hs/hs_metrics_entry.h index 2f732aa614..07693972c0 100644 --- a/src/feature/hs/hs_metrics_entry.h +++ b/src/feature/hs/hs_metrics_entry.h @@ -13,6 +13,33 @@

#include "lib/metrics/metrics_common.h"

+/* Possible values for the reason label of the + * hs_intro_rejected_intro_req_count metric. */ +/** The hidden service received an unknown introduction auth key. */ +#define HS_METRICS_ERR_INTRO_REQ_BAD_AUTH_KEY "bad_auth_key" +/** The hidden service received a malformed INTRODUCE2 cell. */ +#define HS_METRICS_ERR_INTRO_REQ_INTRODUCE2 "invalid_introduce2" +/** The hidden service does not have the necessary subcredential. */ +#define HS_METRICS_ERR_INTRO_REQ_SUBCREDENTIAL "subcredential" +/** The hidden service received an INTRODUCE2 replay. */ +#define HS_METRICS_ERR_INTRO_REQ_INTRODUCE2_REPLAY "replay" + +/* Possible values for the reason label of the hs_rdv_error_count metric. */ +/** The hidden service failed to connect to the rendezvous point. */ +#define HS_METRICS_ERR_RDV_RP_CONN_FAILURE "rp_conn_failure" +/** The hidden service failed to build a circuit to the rendezvous point due + * to an invalid selected path. */ +#define HS_METRICS_ERR_RDV_PATH "invalid_path" +/** The hidden service failed to send the RENDEZVOUS1 cell on rendezvous + * circuit. */ +#define HS_METRICS_ERR_RDV_RENDEZVOUS1 "rendezvous1" +/** The hidden service failed to set up an end-to-end rendezvous circuit to + * the client. */ +#define HS_METRICS_ERR_RDV_E2E "e2e_circ" +/** The hidden service reattempted to connect to the rendezvous point by + * launching a new circuit to it, but failed */ +#define HS_METRICS_ERR_RDV_RETRY "retry" + /** Metrics key which are used as an index in the main base metrics array. */ typedef enum { /** Number of introduction requests. */ @@ -50,6 +77,11 @@ typedef struct hs_metrics_entry_t { extern const hs_metrics_entry_t base_metrics[]; extern const size_t base_metrics_size;

-#endif /* defined(HS_METRICS_ENTRY_PRIVATE) */ +extern const char *hs_metrics_intro_req_error_reasons[]; +extern const size_t hs_metrics_intro_req_error_reasons_size;

+extern const char *hs_metrics_rend_error_reasons[]; +extern const size_t hs_metrics_rend_error_reasons_size; + +#endif /* defined(HS_METRICS_ENTRY_PRIVATE) */ #endif /* !defined(TOR_FEATURE_HS_METRICS_ENTRY_H) */ diff --git a/src/feature/hs/hs_service.c b/src/feature/hs/hs_service.c index 9c792b71c7..b5a69b8d59 100644 --- a/src/feature/hs/hs_service.c +++ b/src/feature/hs/hs_service.c @@ -42,6 +42,7 @@ #include "feature/hs/hs_ident.h" #include "feature/hs/hs_intropoint.h" #include "feature/hs/hs_metrics.h" +#include "feature/hs/hs_metrics_entry.h" #include "feature/hs/hs_service.h" #include "feature/hs/hs_stats.h" #include "feature/hs/hs_ob.h" @@ -3508,6 +3509,9 @@ service_handle_introduce2(origin_circuit_t *circ, const uint8_t *payload, "an INTRODUCE2 cell on circuit %u for service %s", TO_CIRCUIT(circ)->n_circ_id, safe_str_client(service->onion_address)); + + hs_metrics_reject_intro_req(service, + HS_METRICS_ERR_INTRO_REQ_BAD_AUTH_KEY); goto err; } /* If we have an IP object, we MUST have a descriptor object. */ @@ -3524,9 +3528,6 @@ 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/lib/metrics/metrics_store_entry.c b/src/lib/metrics/metrics_store_entry.c index 482ec8d7d9..971d9379bd 100644 --- a/src/lib/metrics/metrics_store_entry.c +++ b/src/lib/metrics/metrics_store_entry.c @@ -127,3 +127,23 @@ metrics_store_entry_has_label(const metrics_store_entry_t *entry,

return smartlist_contains_string(entry->labels, label); } + +/** Return the first entry that has the given label, or NULL if none + * of the entries have the label. */ +metrics_store_entry_t * +metrics_store_find_entry_with_label(const smartlist_t *entries, + const char *label) +{ + tor_assert(entries); + tor_assert(label); + + SMARTLIST_FOREACH_BEGIN(entries, metrics_store_entry_t *, entry) { + tor_assert(entry); + + if (smartlist_contains_string(entry->labels, label)) { + return entry; + } + } SMARTLIST_FOREACH_END(entry); + + return NULL; +} diff --git a/src/lib/metrics/metrics_store_entry.h b/src/lib/metrics/metrics_store_entry.h index e4dc7a8b9a..0e09e099fe 100644 --- a/src/lib/metrics/metrics_store_entry.h +++ b/src/lib/metrics/metrics_store_entry.h @@ -12,6 +12,7 @@ #include "lib/cc/torint.h"

#include "lib/metrics/metrics_common.h" +#include "lib/container/smartlist.h"

#ifdef METRICS_STORE_ENTRY_PRIVATE

@@ -57,6 +58,8 @@ void metrics_store_entry_free_(metrics_store_entry_t *entry); int64_t metrics_store_entry_get_value(const metrics_store_entry_t *entry); bool metrics_store_entry_has_label(const metrics_store_entry_t *entry, const char *label); +metrics_store_entry_t *metrics_store_find_entry_with_label( + const smartlist_t *entries, const char *label);

/* Modifiers. */ void metrics_store_entry_add_label(metrics_store_entry_t *entry, diff --git a/src/test/test_hs_metrics.c b/src/test/test_hs_metrics.c index 03f6aedbb4..b7c0ab53da 100644 --- a/src/test/test_hs_metrics.c +++ b/src/test/test_hs_metrics.c @@ -40,7 +40,7 @@ test_metrics(void *arg)

/* Update entry by identifier. */ hs_metrics_update_by_ident(HS_METRICS_NUM_INTRODUCTIONS, - &service->keys.identity_pk, 0, 42); + &service->keys.identity_pk, 0, NULL, 42);

/* Confirm the entry value. */ const smartlist_t *entries = metrics_store_get_all(service->metrics.store, @@ -53,24 +53,29 @@ test_metrics(void *arg)

/* Update entry by service now. */ hs_metrics_update_by_service(HS_METRICS_NUM_INTRODUCTIONS, - service, 0, 42); + service, 0, NULL, 42); tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 84);

+ const char *reason = HS_METRICS_ERR_INTRO_REQ_BAD_AUTH_KEY; + /* 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); + &service->keys.identity_pk, 0, reason, 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_int_op(smartlist_len(entries), OP_EQ, + hs_metrics_intro_req_error_reasons_size); + + entry = metrics_store_find_entry_with_label( + entries, "reason="bad_auth_key""); 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); + hs_metrics_update_by_service(HS_METRICS_NUM_REJECTED_INTRO_REQ, service, 0, + reason, 10); tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 122);

done: diff --git a/src/test/test_hs_service.c b/src/test/test_hs_service.c index 7e675620bd..03a4800f25 100644 --- a/src/test/test_hs_service.c +++ b/src/test/test_hs_service.c @@ -1229,14 +1229,34 @@ 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); + /* There are `hs_metrics_intro_req_size` entries (one for each + * possible `reason` label value). */ + tt_int_op(smartlist_len(entries), OP_EQ, + hs_metrics_intro_req_error_reasons_size); + + /* Make sure the tor_hs_intro_rejected_intro_req_count metric was + * only incremented for reason HS_METRICS_ERR_INTRO_REQ_BAD_AUTH_KEY. */ + for (size_t i = 0; i < hs_metrics_intro_req_error_reasons_size; ++i) { + const char *reason = hs_metrics_intro_req_error_reasons[i]; + + if (!strcmp(reason, HS_METRICS_ERR_INTRO_REQ_BAD_AUTH_KEY)) { + continue; + } + + entry = metrics_store_find_entry_with_label( + entries, + metrics_format_label("reason", reason)); + tt_assert(entry); + tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 0); + } + + entry = metrics_store_find_entry_with_label( + entries, + metrics_format_label("reason", HS_METRICS_ERR_INTRO_REQ_BAD_AUTH_KEY)); tt_assert(entry); tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 1);

@@ -1257,8 +1277,31 @@ test_bad_introduce2(void *arg) 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); + * a second time, this time, with reason="invalid_introduce2_cell". */ + entry = metrics_store_find_entry_with_label( + entries, + metrics_format_label("reason", HS_METRICS_ERR_INTRO_REQ_INTRODUCE2)); + tt_assert(entry); + tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 1); + + /* The metric entries with other reason labels are unaffected */ + entry = metrics_store_find_entry_with_label( + entries, + metrics_format_label("reason", HS_METRICS_ERR_INTRO_REQ_SUBCREDENTIAL)); + tt_assert(entry); + tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 0); + + entry = metrics_store_find_entry_with_label( + entries, metrics_format_label( + "reason", HS_METRICS_ERR_INTRO_REQ_INTRODUCE2_REPLAY)); + tt_assert(entry); + tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 0); + + entry = metrics_store_find_entry_with_label( + entries, + metrics_format_label("reason", HS_METRICS_ERR_INTRO_REQ_BAD_AUTH_KEY)); + tt_assert(entry); + tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 1);

done: or_state_free(dummy_state); @@ -2293,6 +2336,8 @@ test_intro2_handling(void *arg) /* Disable onionbalance */ x_service.config.ob_master_pubkeys = NULL; x_service.state.replay_cache_rend_cookie = replaycache_new(0,0); + /* Initialize the metrics store */ + hs_metrics_service_init(&x_service);

/* Create subcredential for x: */ ed25519_keypair_t x_identity_keypair; @@ -2456,6 +2501,20 @@ test_intro2_handling(void *arg) (uint8_t*)relay_payload, relay_payload_len); tt_int_op(retval, OP_EQ, 0);

+ /* We haven't encountered any errors yet, so all the introduction request + * error metrics should be 0 */ + const smartlist_t *entries = metrics_store_get_all( + x_service.metrics.store, "tor_hs_intro_rejected_intro_req_count"); + const metrics_store_entry_t *entry = NULL; + + for (size_t i = 0; i < hs_metrics_intro_req_error_reasons_size; ++i) { + entry = metrics_store_find_entry_with_label( + entries, + metrics_format_label("reason", hs_metrics_intro_req_error_reasons[i])); + tt_assert(entry); + tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 0); + } + /* ************************************************************ */

/* Act IV: @@ -2473,6 +2532,11 @@ test_intro2_handling(void *arg) tt_int_op(retval, OP_EQ, -1); expect_log_msg_containing("with the same ENCRYPTED section"); teardown_capture_of_logs(); + entry = metrics_store_find_entry_with_label( + entries, + metrics_format_label("reason", HS_METRICS_ERR_INTRO_REQ_INTRODUCE2)); + tt_assert(entry); + tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 1);

/* Now cleanup the intro point replay cache but not the service replay cache and see that this one triggers this time. */ @@ -2487,6 +2551,12 @@ test_intro2_handling(void *arg) expect_log_msg_containing("with same REND_COOKIE"); teardown_capture_of_logs();

+ entry = metrics_store_find_entry_with_label( + entries, metrics_format_label( + "reason", HS_METRICS_ERR_INTRO_REQ_INTRODUCE2_REPLAY)); + tt_assert(entry); + tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 1); + /* Now just to make sure cleanup both replay caches and make sure that the cell gets through */ replaycache_free(x_ip->replay_cache); @@ -2499,6 +2569,9 @@ test_intro2_handling(void *arg) (uint8_t*)relay_payload, relay_payload_len); tt_int_op(retval, OP_EQ, 0);

+ /* This time, the error metric was *not* incremented */ + tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 1); + /* As a final thing, create an INTRODUCE1 cell from Alice to X using Y's * subcred (should fail since Y is just another instance and not the frontend * service!) */ @@ -2515,7 +2588,13 @@ test_intro2_handling(void *arg) intro_circ, x_ip, &y_subcred, (uint8_t*)relay_payload, relay_payload_len); + tt_int_op(retval, OP_EQ, -1); + entry = metrics_store_find_entry_with_label( + entries, + metrics_format_label("reason", HS_METRICS_ERR_INTRO_REQ_INTRODUCE2)); + tt_assert(entry); + tt_int_op(metrics_store_entry_get_value(entry), OP_EQ, 2);

done: /* Start cleaning up X */ @@ -2524,6 +2603,7 @@ test_intro2_handling(void *arg) tor_free(x_service.state.ob_subcreds); service_descriptor_free(x_service.desc_current); service_descriptor_free(x_service.desc_next); + hs_metrics_service_free(&x_service); service_intro_point_free(x_ip);

/* Clean up Alice */