From 4005cca5518d3ce8b1556cf10abcd7db146ae18e Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 18 Aug 2026 17:18:36 +0100 Subject: [PATCH 1/7] docs(issues): define IP override staged activation --- ...78-configuration-overhaul-final-cleanup.md | 28 ++-- .../ISSUE.md | 150 ++++++++++++++---- 2 files changed, 131 insertions(+), 47 deletions(-) diff --git a/docs/issues/open/1980-1978-configuration-overhaul-final-cleanup.md b/docs/issues/open/1980-1978-configuration-overhaul-final-cleanup.md index cdde8359b..43d30c0e0 100644 --- a/docs/issues/open/1980-1978-configuration-overhaul-final-cleanup.md +++ b/docs/issues/open/1980-1978-configuration-overhaul-final-cleanup.md @@ -7,7 +7,7 @@ github-issue: 1980 spec-path: docs/issues/open/1980-1978-configuration-overhaul-final-cleanup.md branch: "config-final-cleanup" related-pr: null -last-updated-utc: 2026-07-23 17:02 +last-updated-utc: 2026-08-18 00:00 semantic-links: skill-links: - create-issue @@ -151,18 +151,19 @@ Files that import `torrust_tracker_configuration::logging` (the module, not the ## Implementation Plan -| ID | Status | Task | Notes | -| --- | ------ | ------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| T1 | TODO | Migrate all consumer imports to explicit `v3_0_0` paths | ~30 files; see Consumer Migration Map above | -| T2 | TODO | Remove global type aliases from `lib.rs` | `pub type Configuration = ...` etc. | -| T3 | TODO | Remove crate-root `logging.rs` | Already copied into `v2_0_0/` and `v3_0_0/` | -| T4 | TODO | Remove `pub mod logging;` from `lib.rs` | Or redirect to versioned module if needed | -| T5 | TODO | Enable #1453's v3 ban-cleanup interval | Replace its temporary 24-hour default-constant bootstrap value after consumer migration | -| T6 | TODO | Remove hardcoded `ConnectionIdValidationPolicy` in test environment | `packages/udp-server/src/testing/environment.rs` hardcodes `Strict` because v2 config lacks the field; after v3 migration the field is available natively in `UdpTracker` | -| T7 | TODO | Apply any additional cleanup discovered during EPIC | Document in progress log | -| T8 | TODO | Run #889 deferred manual verification scenarios (M1–M5) | After consumer migration, run tracker with v3 config and verify all four trace styles + `trace_filter` filtering | -| T9 | TODO | Run `linter all` and full test suite | | -| T10 | TODO | Finalize migration guide | `docs/issues/open/1978-configuration-overhaul-epic/configuration-v2-to-v3-migration.md` — this is the final cleanup, so the guide should be complete at this point | +| ID | Status | Task | Notes | +| --- | ------ | ------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| T1 | TODO | Migrate all consumer imports to explicit `v3_0_0` paths | ~30 files; see Consumer Migration Map above | +| T2 | TODO | Remove global type aliases from `lib.rs` | `pub type Configuration = ...` etc. | +| T3 | TODO | Remove crate-root `logging.rs` | Already copied into `v2_0_0/` and `v3_0_0/` | +| T4 | TODO | Remove `pub mod logging;` from `lib.rs` | Or redirect to versioned module if needed | +| T5 | TODO | Enable #1453's v3 ban-cleanup interval | Replace its temporary 24-hour default-constant bootstrap value after consumer migration | +| T6 | TODO | Remove hardcoded `ConnectionIdValidationPolicy` in test environment | `packages/udp-server/src/testing/environment.rs` hardcodes `Strict` because v2 config lacks the field; after v3 migration the field is available natively in `UdpTracker` | +| T7 | TODO | Apply any additional cleanup discovered during EPIC | Document in progress log | +| T8 | TODO | Run #889 deferred manual verification scenarios (M1–M5) | After consumer migration, run tracker with v3 config and verify all four trace styles + `trace_filter` filtering | +| T9 | TODO | Run `linter all` and full test suite | | +| T10 | TODO | Finalize migration guide | `docs/issues/open/1978-configuration-overhaul-epic/configuration-v2-to-v3-migration.md` — this is the final cleanup, so the guide should be complete at this point | +| T11 | TODO | Run #1987 enabled-mode local manual verification | After consumer migration activates v3.0.0 at runtime, enable `use_ip_from_query_string` for a local HTTP tracker and execute #1987's enabled-mode local scenarios. Append reproducible commands and evidence to `docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/manual-verification.md`. | ## Progress Tracking @@ -189,6 +190,7 @@ Files that import `torrust_tracker_configuration::logging` (the module, not the - 2026-07-28 00:00 UTC - agent - Added T8: run #889 deferred manual verification scenarios (M1–M5) after consumer migration. These scenarios require the tracker to use v3 config, which is not possible until this cleanup migrates global callers. +- 2026-08-18 00:00 UTC - Copilot/User - Added T11: run #1987 enabled-mode local manual verification after this issue activates v3.0.0 configuration at runtime. ## Acceptance Criteria diff --git a/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md index 73fa9d2af..695151832 100644 --- a/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md +++ b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md @@ -9,8 +9,9 @@ branch: "1987-add-config-option-to-use-ip-from-announce-query-string" related-pr: null depends-on: - docs/issues/open/1985-rename-peer-addr-to-ip-in-http-announce-request/ISSUE.md + - docs/issues/open/1980-1978-configuration-overhaul-final-cleanup.md blocks: null -last-updated-utc: 2026-07-15 00:00 +last-updated-utc: 2026-08-18 00:00 semantic-links: skill-links: - create-issue @@ -54,6 +55,31 @@ This feature requires adding a new per-HTTP-tracker configuration field. The con This issue depends on the `ip` GET parameter rename (from `peer_addr` to `ip`) being completed first. The rename issue must be resolved before this feature is implemented. +The runtime activation of configuration schema v3.0.0 depends on #1980. Until that migration is complete, this issue uses a temporary internal disabled policy for production wiring. The v3 configuration field and all address-selection behavior are implemented and unit tested now, but the running tracker cannot enable the setting from configuration until #1980 is complete. + +### `ip` parameter validation and selection + +The tracker distinguishes **absent** and **empty** `ip` parameters: + +- **Absent**: the query string does not contain an `ip` parameter. +- **Empty**: the query string contains `ip=` with an empty value. + +Both absent and empty parameters are accepted and use the normal connection-derived address (or the address derived through reverse-proxy handling). This deliberately supports clients that automatically emit all known query parameter names while omitting values that are not relevant. + +For a non-empty `ip` parameter, the tracker accepts only IPv4 or IPv6 literals. DNS names are not supported. The following contract applies: + +| `ip` parameter | `use_ip_from_query_string = false` | `use_ip_from_query_string = true` | +| ----------------------- | --------------------------------------------- | ------------------------------------------- | +| Absent | Accept; use the connection/reverse-proxy IP | Accept; use the connection/reverse-proxy IP | +| Empty (`ip=`) | Accept; treat as absent | Accept; treat as absent | +| Valid IPv4/IPv6 literal | Reject; client-supplied peer IPs are disabled | Accept; use the supplied IP | +| DNS name | Reject; DNS names are unsupported | Reject; DNS names are unsupported | +| Invalid non-empty value | Reject; an IPv4 or IPv6 literal is required | Reject; an IPv4 or IPv6 literal is required | + +This makes the setting control whether a non-empty client-supplied peer IP override is accepted. A client must receive a protocol failure rather than a successful announce that silently registers a different peer address. + +Malformed query-string encoding remains a normal request-parsing failure. The tracker should provide the most specific failure reason it can reliably determine. + ### Interaction with `on_reverse_proxy` When both `use_ip_from_query_string` and `on_reverse_proxy` are enabled, the query string `ip` takes precedence over the `X-Forwarded-For` header. This is because the operator explicitly opted into trusting the query string value. When `use_ip_from_query_string` is disabled (default), the existing `on_reverse_proxy` logic applies unchanged. The two settings are not mutually exclusive; the query string IP wins when both are active and a valid IP is provided. @@ -67,9 +93,14 @@ Enabling this feature allows a remote client to claim any IP address in its anno ### In Scope - Add a new optional boolean configuration field to the per-HTTP-tracker configuration (name TBD during schema design, e.g. `use_ip_from_query_string`), disabled by default. -- When the option is enabled, and the `ip` GET parameter contains a valid IP address, use that IP as the peer's address instead of the connection IP. +- Accept an absent or empty `ip` GET parameter in both configuration modes, using the normal connection-derived address. +- Reject a non-empty `ip` parameter that is invalid, is a DNS name, or is supplied while the option is disabled, with a precise protocol failure reason. +- When the option is enabled and the `ip` GET parameter contains a valid IP address, use that IP as the peer's address instead of the connection IP. +- Record rejected `ip` parameters in an operator-visible counter and debug log without treating them as application errors. - Document the security implications of enabling this option in the configuration schema and in the module documentation. -- Add contract tests covering both the enabled and disabled behaviour. +- Preserve the `ip` parameter's raw request state at the HTTP protocol boundary so absent, empty, valid literal, DNS-name, and invalid non-empty values remain distinguishable. +- Add exhaustive tests for every raw-parameter validation and address-selection case. Prefer focused unit tests; add contract/integration tests only where HTTP boundary behavior cannot be validated by unit tests. +- Until schema v3.0.0 is active at runtime, wire the production announce service to an explicit internal disabled policy. Do not add an environment-variable override or a temporary v2 configuration setting. ### Out of Scope @@ -82,17 +113,22 @@ Enabling this feature allows a remote client to claim any IP address in its anno Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. -| ID | Status | Task | Notes / Expected Output | -| --- | ------ | --------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| T1 | TODO | Design the configuration field name and schema placement | Align with #1978 schema v3.0.0 design; propose name (e.g. `use_ip_from_query_string`) | -| T2 | TODO | Add the field to the per-HTTP-tracker configuration struct | Target the v3.0.0 schema under `packages/configuration/` as part of the #1978 overhaul | -| T3 | TODO | Thread the config value through to the announce service | `packages/http-core/src/services/announce.rs` `peer_from_request` | -| T4 | TODO | Implement the conditional IP selection in `peer_from_request` | Use `announce_request.ip` if `use_ip_from_query_string` is `true` and the field is `Some`; otherwise use the connection IP. When both `use_ip_from_query_string` and `on_reverse_proxy` are enabled, the query string IP takes precedence. Requires prerequisite issue (rename `peer_addr` → `ip`) to be merged first. | -| T5 | TODO | Add contract tests for enabled and disabled behaviour | New tests in `packages/axum-http-server/tests/` | -| T6 | TODO | Update configuration documentation | `packages/configuration/` docs and `share/default/` config file | -| T7 | TODO | Run `cargo test --workspace` — no regressions | All tests pass | -| T8 | TODO | Run `linter all` | Must exit `0` | -| T9 | TODO | Update migration guide if this subissue affects the config public API | `docs/issues/open/1978-configuration-overhaul-epic/configuration-v2-to-v3-migration.md` | +| ID | Status | Task | Notes / Expected Output | +| --- | ------- | --------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| T1 | TODO | Design the configuration field name and schema placement | Align with #1978 schema v3.0.0 design; propose name (e.g. `use_ip_from_query_string`) | +| T2 | TODO | Add the field to the per-HTTP-tracker configuration struct | Target the v3.0.0 schema under `packages/configuration/` as part of the #1978 overhaul | +| T3 | TODO | Preserve the raw `ip` parameter state in the HTTP protocol | Replace the lossy `Option` parsing boundary, or add a companion representation, so absent, empty, valid literal, DNS-name, and invalid non-empty values can receive their required distinct treatment. | +| T4 | TODO | Inject the address-selection policy into the announce service | `packages/http-core/src/services/announce.rs` `peer_from_request`. Until #1980 activates v3 at runtime, production constructs the service with an explicit disabled policy; unit tests inject both policy values. | +| T5 | TODO | Validate and select the peer IP | Accept absent and empty `ip`; reject non-empty invalid/DNS `ip`, and a valid non-empty `ip` when the policy is disabled; use a valid `ip` only when enabled. When both this option and `on_reverse_proxy` are enabled, the query-string IP takes precedence. | +| T6 | TODO | Add rejected-parameter observability | Increment a bounded-reason counter and emit debug logging for rejection reasons; do not log raw client-supplied IP values or application errors. | +| T7 | TODO | Add exhaustive tests for validation and selection | Prefer focused unit tests for raw parameter state, absent, empty, valid, invalid, DNS, enabled, disabled, and reverse-proxy precedence behavior. Add only the minimum contract/integration coverage needed to verify HTTP failure responses and configuration wiring. | +| T8 | TODO | Update configuration documentation | `packages/configuration/` docs and `share/default/` config file | +| T9 | TODO | Run `cargo test --workspace` — no regressions | All tests pass | +| T10 | TODO | Run `linter all` | Must exit `0` | +| T11 | TODO | Update migration guide if this subissue affects the config public API | `docs/issues/open/1978-configuration-overhaul-epic/configuration-v2-to-v3-migration.md` | +| T12 | TODO | Capture baseline behavior locally | Before implementation, run a local HTTP tracker and local tracker client against the baseline request matrix. Record actual behavior in `manual-verification.md`; in particular, establish that non-empty `ip` values are currently silently ignored. | +| T13 | TODO | Manually verify disabled behavior locally | After implementation but before v3 runtime activation, rerun the baseline request matrix. Verify absent/empty behavior is unchanged and document the intentional change: non-empty `ip` values are rejected with precise failure reasons. Append evidence to `manual-verification.md`. | +| T14 | BLOCKED | Manually verify enabled behavior locally with active v3 configuration | After #1980 activates schema v3.0.0 at runtime, enable `use_ip_from_query_string` in a local per-HTTP-tracker config and run the enabled-mode scenarios with the local tracker and tracker client. Append reproducible evidence to `manual-verification.md`. | ## Progress Tracking @@ -114,18 +150,25 @@ Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. ### Progress Log - 2026-07-15 00:00 UTC - Copilot/User - Spec drafted as a sub-issue of #1978; feature deferred to the configuration overhaul epic. +- 2026-08-18 00:00 UTC - Copilot/User - Clarified the strict `ip` parameter contract: absent and empty values are accepted as no override; non-empty invalid/DNS values and valid overrides supplied while disabled are rejected. Added observability requirements for rejected parameters. +- 2026-08-18 00:00 UTC - Copilot/User - Required post-implementation manual verification against a local tracker using the local tracker client, with reproducible evidence retained in this issue directory. +- 2026-08-18 00:00 UTC - Copilot/User - Chose staged delivery while v2 remains the active runtime schema: production wiring remains explicitly disabled; unit tests cover both policies; enabled-mode local manual verification is deferred until #1980 activates v3.0.0 configuration. +- 2026-08-18 00:00 UTC - Copilot/User - Required a three-phase local manual verification record: baseline behavior before implementation, disabled-policy behavior after implementation, and enabled-v3 behavior after #1980. The baseline documents the intentional change from silently ignoring non-empty `ip` values to rejecting them when overrides are disabled. ## Acceptance Criteria -- [ ] AC1: When `use_ip_from_query_string` is `false` (default), the tracker always uses the connection IP regardless of the `ip` GET parameter. +- [ ] AC1: When `use_ip_from_query_string` is `false` (default), an absent or empty `ip` GET parameter uses the connection IP; a non-empty `ip` value is rejected with a precise failure reason. - [ ] AC2: When `use_ip_from_query_string` is `true` and a valid IP is provided in the `ip` GET parameter, the tracker uses that IP as the peer's address. -- [ ] AC3: When `use_ip_from_query_string` is `true` but the `ip` GET parameter is absent or contains a non-IP value, the tracker falls back to the connection IP. +- [ ] AC3: When `use_ip_from_query_string` is `true`, an absent or empty `ip` GET parameter uses the connection IP; a non-empty invalid IP or DNS name is rejected with a precise failure reason. - [ ] AC4: The default configuration file (`share/default/`) has `use_ip_from_query_string` set to `false` (or omitted, defaulting to `false`). - [ ] AC5: The configuration schema documentation clearly states the security implications of enabling this option. -- [ ] AC6: Contract tests cover both enabled and disabled cases. +- [ ] AC6: Focused unit tests cover every `ip` parameter validation and address-selection case; minimum contract/integration tests verify HTTP failure responses and configuration wiring where unit tests cannot. +- [ ] AC6a: A counter records rejected non-empty `ip` parameters using bounded reason labels, and debug logs record the rejection without raw client IP values or application error logs. - [ ] AC7: `linter all` exits with code `0`. - [ ] AC8: Relevant tests pass with no regressions. -- [ ] Manual verification scenarios are executed and documented (status + evidence). +- [ ] AC9: Baseline manual verification runs a local tracker and local tracker client before implementation; reproducible commands, output, expected/actual results, and environment details are recorded in `manual-verification.md` in this issue directory. +- [ ] AC10: Before v3.0.0 runtime activation, manual verification reruns the baseline matrix and documents the intentional disabled-policy change: absent/empty values remain accepted while non-empty values are rejected with precise failure reasons. +- [ ] AC11: After #1980 activates v3.0.0 configuration at runtime, manual verification runs a local tracker and local tracker client with `use_ip_from_query_string` enabled; the resulting evidence is appended to `manual-verification.md`. - [ ] Acceptance criteria are re-reviewed after implementation and reflect actual behaviour. - [ ] Documentation is updated when behaviour/workflow changes. @@ -137,33 +180,72 @@ Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. - `cargo test --workspace` - Pre-push checks (when applicable) +### Required Automated Test Matrix + +The implementation must add automated coverage for every row in the parameter contract. Prefer unit tests at the validation and peer-address selection boundaries. Use contract/integration tests only for behavior that requires the HTTP transport boundary. + +| ID | `ip` value | Setting | Expected outcome | Preferred test level | +| --- | ------------------------------------------ | -------- | ---------------------------------------------------------------- | -------------------------------------- | +| A1 | Raw state: absent | Disabled | Accept; use connection/reverse-proxy address | Protocol unit + service unit | +| A2 | Raw state: empty (`ip=`) | Disabled | Accept; treat as absent | Protocol unit + service unit | +| A3 | Valid IPv4 literal | Disabled | Reject with a disabled-override failure reason | Unit + HTTP contract response | +| A4 | Valid IPv6 literal | Disabled | Reject with a disabled-override failure reason | Unit + HTTP contract response | +| A5 | Raw state: DNS name | Disabled | Reject with a DNS-not-supported failure reason | Protocol unit + HTTP contract response | +| A6 | Raw state: invalid non-empty value | Disabled | Reject with an invalid-IP failure reason | Protocol unit + HTTP contract response | +| A7 | Raw state: absent | Enabled | Accept; use connection/reverse-proxy address | Protocol unit + service unit | +| A8 | Raw state: empty (`ip=`) | Enabled | Accept; treat as absent | Protocol unit + service unit | +| A9 | Valid IPv4 literal | Enabled | Accept; use supplied address | Unit | +| A10 | Valid IPv6 literal | Enabled | Accept; use supplied address | Unit | +| A11 | Raw state: DNS name | Enabled | Reject with a DNS-not-supported failure reason | Protocol unit + HTTP contract response | +| A12 | Raw state: invalid non-empty value | Enabled | Reject with an invalid-IP failure reason | Protocol unit + HTTP contract response | +| A13 | Valid IPv4/IPv6 literal with reverse proxy | Enabled | Accept; supplied address takes precedence over `X-Forwarded-For` | Unit + minimum integration coverage | +| A14 | Rejected value | Either | Increment the bounded-reason counter and emit a safe debug entry | Unit | + ### Manual Verification Scenarios Status values: `TODO`, `IN_PROGRESS`, `DONE`, `FAILED`, `BLOCKED`. -| ID | Scenario | Command/Steps | Expected Result | Status | Evidence | -| --- | --------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------ | -------------------------------------------------------- | ------ | -------- | -| M1 | Default config: `ip` GET param is ignored | Start tracker with default config; announce with `ip=1.2.3.4` from a different source IP; check the peer list | Peer is registered with the connection IP, not `1.2.3.4` | TODO | | -| M2 | Opt-in config: `ip` GET param is used | Enable `use_ip_from_query_string`; announce with `ip=1.2.3.4`; check the peer list | Peer is registered with `1.2.3.4` | TODO | | -| M3 | Opt-in config: no `ip` param — fallback | Enable `use_ip_from_query_string`; announce without `ip` param | Peer is registered with the connection IP | TODO | | -| M4 | Opt-in + reverse proxy: `ip` param takes precedence | Enable both `use_ip_from_query_string` and `on_reverse_proxy`; announce with `ip=1.2.3.4` and `X-Forwarded-For: 5.6.7.8` | Peer is registered with `1.2.3.4` (query string wins) | TODO | | +Run the same applicable request matrix against a local tracker in three phases: before implementation, after implementation with the disabled policy, and after #1980 activates v3 configuration with the setting enabled. Use the local `tracker_client` for typed valid-IP announces. Use a raw local HTTP client (for example, `curl`) for `ip=`, DNS-name, invalid-IP, and `X-Forwarded-For` requests, which the typed tracker client cannot construct. Follow `.github/skills/dev/environment-setup/run-tracker-locally/SKILL.md` and `.github/skills/usage/use-tracker-client/SKILL.md`. Do not rely on a public tracker for this verification. Record every execution in `manual-verification.md` in this directory, including: + +- date/time, commit SHA, OS, Rust toolchain, and effective local tracker configuration; +- exact tracker and client commands, with sensitive values redacted; +- relevant client output and metric/debug-log evidence; +- expected and actual results for every executed scenario. + +| ID | Scenario | Command/Steps | Expected Result | Status | Evidence | +| --- | --------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------ | ------------------------------------------------------------------------- | ------ | -------- | +| M1 | Default config: valid non-empty `ip` is rejected | Start tracker with default config; announce with `ip=1.2.3.4` | Announce fails, explaining that client-supplied peer IPs are disabled | TODO | | +| M2 | Opt-in config: `ip` GET param is used | Enable `use_ip_from_query_string`; announce with `ip=1.2.3.4`; check the peer list | Peer is registered with `1.2.3.4` | TODO | | +| M3 | Opt-in config: absent or empty `ip` — fallback | Enable `use_ip_from_query_string`; announce without `ip` and with `ip=` | Peer is registered with the connection IP in both cases | TODO | | +| M4 | Opt-in + reverse proxy: `ip` param takes precedence | Enable both `use_ip_from_query_string` and `on_reverse_proxy`; announce with `ip=1.2.3.4` and `X-Forwarded-For: 5.6.7.8` | Peer is registered with `1.2.3.4` (query string wins) | TODO | | +| M5 | Non-empty invalid or DNS `ip` is rejected | Announce with enabled and disabled configurations using `ip=invalid` and `ip=example.com` | Announce fails with the specific validation reason | TODO | | +| M6 | Rejected parameters are observable | Submit rejected non-empty `ip` parameters and inspect metrics and debug logs | Counter increments by bounded reason; debug log has reason without raw IP | TODO | | + +**Baseline expectation:** Before implementation, use M1–M5 as an address-selection request matrix. Valid, DNS-name, and invalid non-empty `ip` values are expected to be silently ignored and the announce is expected to succeed using the connection-derived address. Empty and absent values are expected to succeed. M6 is post-implementation only because its metric and debug log do not yet exist. + +**Post-implementation disabled-policy expectation:** M1, the disabled-mode portion of M5, and M6 apply. M2–M4 and the enabled-mode portion of M5 remain blocked until #1980 activates schema v3.0.0 configuration at runtime. Execute and document them under T14 once the setting can be enabled in the local tracker configuration. ### Acceptance Verification -| AC ID | Status (`TODO`/`DONE`) | Evidence | -| ----- | ---------------------- | -------- | -| AC1 | TODO | | -| AC2 | TODO | | -| AC3 | TODO | | -| AC4 | TODO | | -| AC5 | TODO | | -| AC6 | TODO | | -| AC7 | TODO | | -| AC8 | TODO | | +| AC ID | Status (`TODO`/`DONE`) | Evidence | +| ----- | ---------------------- | ----------------------------------------------------------- | +| AC1 | TODO | | +| AC2 | TODO | | +| AC3 | TODO | | +| AC4 | TODO | | +| AC5 | TODO | | +| AC6 | TODO | | +| AC6a | TODO | | +| AC7 | TODO | | +| AC8 | TODO | | +| AC9 | TODO | | +| AC10 | TODO | | +| AC11 | BLOCKED | Requires #1980 to activate v3.0.0 configuration at runtime. | ## Risks and Trade-offs - **IP spoofing**: When enabled, a client can register any IP address in the peer list. This is inherent to the feature and must be clearly documented. The opt-in default mitigates the risk for deployments that do not need this. +- **Compatibility versus ambiguity**: This feature intentionally rejects non-empty `ip` overrides while disabled, rather than silently ignoring them. This makes configuration support transparent to clients, but is a documented HTTP announce compatibility change for 4.0.0. - **Interaction with reverse proxy mode**: Resolved — when both `use_ip_from_query_string` and `on_reverse_proxy` are enabled, the query string `ip` takes precedence. See "Interaction with `on_reverse_proxy`" above for rationale. - **IPv4/IPv6**: The `ip` parameter accepts both IPv4 and IPv6 addresses (via `IpAddr::from_str`). If the tracker is bound to an IPv6-only socket and a client sends an IPv4 `ip`, the address is accepted as-is — the tracker does not validate address family compatibility with the listener binding. From 3820989e6418093e43f6f70899d6dc2db7efcf0f Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 19 Aug 2026 10:17:16 +0100 Subject: [PATCH 2/7] feat(http): validate announce query IP overrides --- Cargo.lock | 126 +-------- .../configuration-v2-to-v3-migration.md | 9 +- .../ISSUE.md | 168 ++++++----- .../manual-verification.md | 173 ++++++++++++ packages/axum-http-server/Cargo.toml | 1 - .../src/v1/extractors/announce_request.rs | 4 +- .../src/v1/handlers/announce.rs | 8 +- .../receiving_an_announce_request.rs | 134 +++++---- .../configuration/src/v3_0_0/http_tracker.rs | 32 +++ packages/http-core/benches/helpers/util.rs | 4 +- packages/http-core/src/container.rs | 7 +- packages/http-core/src/event.rs | 24 ++ packages/http-core/src/services/announce.rs | 266 +++++++++++++++++- .../http-core/src/statistics/event/handler.rs | 46 ++- packages/http-core/src/statistics/metrics.rs | 15 +- packages/http-core/src/statistics/mod.rs | 6 + .../http-protocol/src/v1/requests/announce.rs | 194 +++++++++---- packages/test-helpers/src/http.rs | 4 +- tests/scaffold.rs | 2 +- 19 files changed, 893 insertions(+), 330 deletions(-) create mode 100644 docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/manual-verification.md diff --git a/Cargo.lock b/Cargo.lock index 92edf5ac9..af55d72e9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1040,38 +1040,14 @@ dependencies = [ "cmov", ] -[[package]] -name = "darling" -version = "0.20.11" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fc7f46116c46ff9ab3eb1597a45688b6715c6e628b5c133e288e709a29bcb4ee" -dependencies = [ - "darling_core 0.20.11", - "darling_macro 0.20.11", -] - [[package]] name = "darling" version = "0.23.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "25ae13da2f202d56bd7f91c25fba009e7717a1e4a1cc98a76d844b65ae912e9d" dependencies = [ - "darling_core 0.23.0", - "darling_macro 0.23.0", -] - -[[package]] -name = "darling_core" -version = "0.20.11" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0d00b9596d185e565c2207a0b01f8bd1a135483d02d9b7b0a54b11da8d53412e" -dependencies = [ - "fnv", - "ident_case", - "proc-macro2", - "quote", - "strsim", - "syn 2.0.119", + "darling_core", + "darling_macro", ] [[package]] @@ -1087,24 +1063,13 @@ dependencies = [ "syn 2.0.119", ] -[[package]] -name = "darling_macro" -version = "0.20.11" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fc34b93ccb385b40dc71c6fceac4b2ad23662c7eeb248cf10d529b7e055b6ead" -dependencies = [ - "darling_core 0.20.11", - "quote", - "syn 2.0.119", -] - [[package]] name = "darling_macro" version = "0.23.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ac3984ec7bd6cfa798e62b4a642426a5be0e68f9401cfc2a01e3fa9ea2fcdb8d" dependencies = [ - "darling_core 0.23.0", + "darling_core", "quote", "syn 2.0.119", ] @@ -1174,37 +1139,6 @@ dependencies = [ "serde_core", ] -[[package]] -name = "derive_builder" -version = "0.20.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "507dfb09ea8b7fa618fcf76e953f4f5e192547945816d5358edffe39f6f94947" -dependencies = [ - "derive_builder_macro", -] - -[[package]] -name = "derive_builder_core" -version = "0.20.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2d5bcf7b024d6835cfb3d473887cd966994907effbe9227e8c8219824d06c4e8" -dependencies = [ - "darling 0.20.11", - "proc-macro2", - "quote", - "syn 2.0.119", -] - -[[package]] -name = "derive_builder_macro" -version = "0.20.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ab63b0e2bf4d5928aff72e83a7dace85d7bba5fe12dcc3c5a572d78caffd3f3c" -dependencies = [ - "derive_builder_core", - "syn 2.0.119", -] - [[package]] name = "derive_more" version = "1.0.0" @@ -1695,17 +1629,6 @@ dependencies = [ "wasm-bindgen", ] -[[package]] -name = "getset" -version = "0.1.7" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6cf442baaabe4213ce7d1239afc26c039180b6456da2cededa316ae2c8a77a77" -dependencies = [ - "proc-macro2", - "quote", - "syn 2.0.119", -] - [[package]] name = "glob" version = "0.3.4" @@ -2399,17 +2322,6 @@ version = "0.8.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "47d9d19d1d6efa0109d2f65ff4c85cddd50bd572e5a00127ab10987290bcefae" -[[package]] -name = "local-ip-address" -version = "0.6.13" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "aa08fb2b1ec3ea84575e94b489d06d4ce0cbf052d12acd515838f50e3c3d63e3" -dependencies = [ - "libc", - "neli", - "windows-sys 0.61.2", -] - [[package]] name = "lock_api" version = "0.4.14" @@ -2542,35 +2454,6 @@ dependencies = [ "tempfile", ] -[[package]] -name = "neli" -version = "0.7.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "22f9786d56d972959e1408b6a93be6af13b9c1392036c5c1fafa08a1b0c6ee87" -dependencies = [ - "bitflags 2.13.1", - "byteorder", - "derive_builder", - "getset", - "libc", - "log", - "neli-proc-macros", - "parking_lot", -] - -[[package]] -name = "neli-proc-macros" -version = "0.2.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "05d8d08c6e98f20a62417478ebf7be8e1425ec9acecc6f63e22da633f6b71609" -dependencies = [ - "either", - "proc-macro2", - "quote", - "serde", - "syn 2.0.119", -] - [[package]] name = "nonempty" version = "0.7.0" @@ -3920,7 +3803,7 @@ version = "3.22.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8705578779c2b6bd90d84d66eb2e206b708b1a4d7b9f17641b293545bf1c7e46" dependencies = [ - "darling 0.23.0", + "darling", "proc-macro2", "quote", "syn 2.0.119", @@ -4961,7 +4844,6 @@ dependencies = [ "derive_more 2.1.1", "futures", "hyper", - "local-ip-address", "rand 0.9.5", "reqwest", "serde", diff --git a/docs/issues/open/1978-configuration-overhaul-epic/configuration-v2-to-v3-migration.md b/docs/issues/open/1978-configuration-overhaul-epic/configuration-v2-to-v3-migration.md index 0898ff1ee..1e6cb1a59 100644 --- a/docs/issues/open/1978-configuration-overhaul-epic/configuration-v2-to-v3-migration.md +++ b/docs/issues/open/1978-configuration-overhaul-epic/configuration-v2-to-v3-migration.md @@ -215,9 +215,12 @@ ip_bans_reset_interval_in_secs = 86400 **Subissue**: #1987 — Use peer IP from the HTTP announce `ip` parameter -> **TODO**: This section will be filled after #1987 is implemented. -> A new per-HTTP-tracker opt-in field will control whether the tracker -> trusts the client-provided `ip` parameter in announce requests. +> **Staged delivery**: #1987 currently supplies the protocol policy and keeps +> production explicitly disabled until #1980 activates v3 configuration at +> runtime. This section will document the final v3 TOML field and its default +> when that consumer migration is complete. The setting will remain opt-in: +> enabling it trusts a client-provided announce `ip` and can therefore allow +> peers to spoof addresses. ## Final cleanup diff --git a/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md index 695151832..1b6d07027 100644 --- a/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md +++ b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md @@ -57,6 +57,18 @@ This issue depends on the `ip` GET parameter rename (from `peer_addr` to `ip`) b The runtime activation of configuration schema v3.0.0 depends on #1980. Until that migration is complete, this issue uses a temporary internal disabled policy for production wiring. The v3 configuration field and all address-selection behavior are implemented and unit tested now, but the running tracker cannot enable the setting from configuration until #1980 is complete. +### HTTP protocol API compatibility + +`torrust-tracker-http-protocol` publicly exposes `Announce`. To preserve the +raw distinction required by this issue, its public `ip` field changes from +`Option` to `PeerIp`. Consumers constructing `Announce` directly must +use `PeerIp::Absent`, `PeerIp::Empty`, `PeerIp::Literal`, `PeerIp::DnsName`, or +`PeerIp::Invalid` as appropriate; client code should prefer +`AnnounceBuilder::with_ip`. `PeerIp::from_raw` performs strict percent-decoding +and returns a parsing error for malformed encoding. This breaking protocol API +change is released with the next major version; it is not a configuration-v2 +to-v3 migration concern. + ### `ip` parameter validation and selection The tracker distinguishes **absent** and **empty** `ip` parameters: @@ -88,6 +100,14 @@ When both `use_ip_from_query_string` and `on_reverse_proxy` are enabled, the que Enabling this feature allows a remote client to claim any IP address in its announce request. The tracker would accept that address and include it in the peer list. This is a potential source of IP spoofing in the peer list. The feature must therefore be **opt-in**, disabled by default, and clearly documented as a trust-based setting — suitable only for private/controlled deployments, or as a workaround for peers behind symmetric NAT that cannot be reached via their connection IP. +### Rejection observability scope + +This issue defines observability only for the new peer-IP rejection event and metric. Its debug log records a bounded reason code, not the raw `ip` parameter value. The rejection metric likewise uses bounded labels only. + +Existing HTTP request logging, including its request-URI behavior, is outside this issue's scope. This issue does not establish a tracker-wide policy for redacting query parameters, client addresses, peer IDs, or other client-controlled request data. A cross-cutting request-log privacy and diagnostic policy requires a separate issue and, if adopted, an ADR. + +Do not add raw invalid values to the new rejection log merely because they are not valid IP literals: arbitrary invalid values can still contain personal, sensitive, or unsafe client-controlled data. If future operations work needs more diagnostic detail, use bounded classifications (for example, `numeric_dot` or `non_ip_text`) rather than raw values. Logging a sanitized, truncated raw representation at an explicitly enabled trace diagnostic level is a separate policy decision and is out of scope. + ## Scope ### In Scope @@ -96,7 +116,7 @@ Enabling this feature allows a remote client to claim any IP address in its anno - Accept an absent or empty `ip` GET parameter in both configuration modes, using the normal connection-derived address. - Reject a non-empty `ip` parameter that is invalid, is a DNS name, or is supplied while the option is disabled, with a precise protocol failure reason. - When the option is enabled and the `ip` GET parameter contains a valid IP address, use that IP as the peer's address instead of the connection IP. -- Record rejected `ip` parameters in an operator-visible counter and debug log without treating them as application errors. +- Record rejected `ip` parameters in an operator-visible bounded-reason counter and rejection-specific debug log without treating them as application errors or logging raw `ip` parameter values. - Document the security implications of enabling this option in the configuration schema and in the module documentation. - Preserve the `ip` parameter's raw request state at the HTTP protocol boundary so absent, empty, valid literal, DNS-name, and invalid non-empty values remain distinguishable. - Add exhaustive tests for every raw-parameter validation and address-selection case. Prefer focused unit tests; add contract/integration tests only where HTTP boundary behavior cannot be validated by unit tests. @@ -113,22 +133,22 @@ Enabling this feature allows a remote client to claim any IP address in its anno Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. -| ID | Status | Task | Notes / Expected Output | -| --- | ------- | --------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| T1 | TODO | Design the configuration field name and schema placement | Align with #1978 schema v3.0.0 design; propose name (e.g. `use_ip_from_query_string`) | -| T2 | TODO | Add the field to the per-HTTP-tracker configuration struct | Target the v3.0.0 schema under `packages/configuration/` as part of the #1978 overhaul | -| T3 | TODO | Preserve the raw `ip` parameter state in the HTTP protocol | Replace the lossy `Option` parsing boundary, or add a companion representation, so absent, empty, valid literal, DNS-name, and invalid non-empty values can receive their required distinct treatment. | -| T4 | TODO | Inject the address-selection policy into the announce service | `packages/http-core/src/services/announce.rs` `peer_from_request`. Until #1980 activates v3 at runtime, production constructs the service with an explicit disabled policy; unit tests inject both policy values. | -| T5 | TODO | Validate and select the peer IP | Accept absent and empty `ip`; reject non-empty invalid/DNS `ip`, and a valid non-empty `ip` when the policy is disabled; use a valid `ip` only when enabled. When both this option and `on_reverse_proxy` are enabled, the query-string IP takes precedence. | -| T6 | TODO | Add rejected-parameter observability | Increment a bounded-reason counter and emit debug logging for rejection reasons; do not log raw client-supplied IP values or application errors. | -| T7 | TODO | Add exhaustive tests for validation and selection | Prefer focused unit tests for raw parameter state, absent, empty, valid, invalid, DNS, enabled, disabled, and reverse-proxy precedence behavior. Add only the minimum contract/integration coverage needed to verify HTTP failure responses and configuration wiring. | -| T8 | TODO | Update configuration documentation | `packages/configuration/` docs and `share/default/` config file | -| T9 | TODO | Run `cargo test --workspace` — no regressions | All tests pass | -| T10 | TODO | Run `linter all` | Must exit `0` | -| T11 | TODO | Update migration guide if this subissue affects the config public API | `docs/issues/open/1978-configuration-overhaul-epic/configuration-v2-to-v3-migration.md` | -| T12 | TODO | Capture baseline behavior locally | Before implementation, run a local HTTP tracker and local tracker client against the baseline request matrix. Record actual behavior in `manual-verification.md`; in particular, establish that non-empty `ip` values are currently silently ignored. | -| T13 | TODO | Manually verify disabled behavior locally | After implementation but before v3 runtime activation, rerun the baseline request matrix. Verify absent/empty behavior is unchanged and document the intentional change: non-empty `ip` values are rejected with precise failure reasons. Append evidence to `manual-verification.md`. | -| T14 | BLOCKED | Manually verify enabled behavior locally with active v3 configuration | After #1980 activates schema v3.0.0 at runtime, enable `use_ip_from_query_string` in a local per-HTTP-tracker config and run the enabled-mode scenarios with the local tracker and tracker client. Append reproducible evidence to `manual-verification.md`. | +| ID | Status | Task | Notes / Expected Output | +| --- | ------- | --------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| T1 | DONE | Design the configuration field name and schema placement | Implemented `use_ip_from_query_string` in the v3 per-HTTP-tracker schema. | +| T2 | DONE | Add the field to the per-HTTP-tracker configuration struct | Added `HttpTracker::use_ip_from_query_string`, defaulting to `false`, with security documentation. | +| T3 | DONE | Preserve the raw `ip` parameter state in the HTTP protocol | Replaced lossy `Option` parsing with `PeerIp`, preserving absent, empty, literal, DNS-name, and invalid states. | +| T4 | DONE | Inject the address-selection policy into the announce service | Production constructs an explicit disabled policy pending #1980; unit tests inject both policy values. | +| T5 | DONE | Validate and select the peer IP | Implemented strict failures, enabled literal selection, and query-IP precedence over the connection/reverse-proxy address. | +| T6 | DONE | Add rejected-parameter observability | Added a bounded-reason counter and rejection-specific debug event logging without raw `ip` parameter values. Existing request-URI logging is outside this issue's scope. | +| T7 | DONE | Add exhaustive tests for validation and selection | Added protocol/service unit tests and HTTP contract coverage for raw states and failure responses. | +| T8 | DONE | Update configuration documentation | Documented the v3 field and staged activation. The active v2 default config is intentionally unchanged pending #1980. | +| T9 | DONE | Run `cargo test --workspace` — no regressions | Full workspace test suite passed on 2026-08-19 after updating the scaffold fixture to omit the now-disallowed non-empty `ip` override. | +| T10 | DONE | Run `linter all` | Passed through the pre-commit gate on 2026-08-18. | +| T11 | DONE | Update migration guide if this subissue affects the config public API | Updated `docs/issues/open/1978-configuration-overhaul-epic/configuration-v2-to-v3-migration.md`. | +| T12 | DONE | Capture baseline behavior locally | Recorded in `manual-verification.md`. | +| T13 | DONE | Manually verify disabled behavior locally | Recorded successful fallback, strict failures, client response, metric, and safe debug evidence in `manual-verification.md`. | +| T14 | BLOCKED | Manually verify enabled behavior locally with active v3 configuration | After #1980 activates schema v3.0.0 at runtime, enable `use_ip_from_query_string` in a local per-HTTP-tracker config and run the enabled-mode scenarios with the local tracker and tracker client. Append reproducible evidence to `manual-verification.md`. | ## Progress Tracking @@ -139,12 +159,12 @@ Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. - [ ] GitHub issue created and issue number added to this spec - [ ] Prerequisites completed (rename `peer_addr` → `ip` issue resolved) - [ ] (Optional, recommended for complex issues) Spec-only PR merged into `develop` before implementation -- [ ] Implementation completed -- [ ] Automatic verification completed (`linter all`, relevant tests, and any pre-push checks) -- [ ] Manual verification scenarios executed and recorded (status + evidence) -- [ ] Acceptance criteria reviewed after implementation and updated with evidence +- [x] Implementation completed +- [x] Automatic verification completed (`linter all`, relevant tests, and any pre-push checks) +- [x] Manual verification scenarios executed and recorded (status + evidence); enabled-v3 scenarios remain blocked on #1980 +- [x] Acceptance criteria reviewed after implementation and updated with evidence - [ ] Reviewer validated acceptance criteria and updated checkboxes -- [ ] Committer verified spec progress is up to date before commit +- [x] Committer verified spec progress is up to date before commit - [ ] Issue closed and spec moved from `docs/issues/open/` to `docs/issues/closed/` ### Progress Log @@ -154,23 +174,25 @@ Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. - 2026-08-18 00:00 UTC - Copilot/User - Required post-implementation manual verification against a local tracker using the local tracker client, with reproducible evidence retained in this issue directory. - 2026-08-18 00:00 UTC - Copilot/User - Chose staged delivery while v2 remains the active runtime schema: production wiring remains explicitly disabled; unit tests cover both policies; enabled-mode local manual verification is deferred until #1980 activates v3.0.0 configuration. - 2026-08-18 00:00 UTC - Copilot/User - Required a three-phase local manual verification record: baseline behavior before implementation, disabled-policy behavior after implementation, and enabled-v3 behavior after #1980. The baseline documents the intentional change from silently ignoring non-empty `ip` values to rejecting them when overrides are disabled. +- 2026-08-19 00:00 UTC - Copilot/User - Implemented the staged disabled-policy behavior, v3 schema field, strict raw `ip` parsing, bounded observability, automated coverage, and baseline/disabled local verification. Enabled-v3 manual verification remains blocked on #1980. +- 2026-08-19 00:00 UTC - Copilot/User - Clarified observability scope: the new rejection event and metric use bounded reason data only; existing HTTP request-URI logging is out of scope. A tracker-wide logging privacy/diagnostic policy requires a separate issue and ADR decision. ## Acceptance Criteria -- [ ] AC1: When `use_ip_from_query_string` is `false` (default), an absent or empty `ip` GET parameter uses the connection IP; a non-empty `ip` value is rejected with a precise failure reason. -- [ ] AC2: When `use_ip_from_query_string` is `true` and a valid IP is provided in the `ip` GET parameter, the tracker uses that IP as the peer's address. -- [ ] AC3: When `use_ip_from_query_string` is `true`, an absent or empty `ip` GET parameter uses the connection IP; a non-empty invalid IP or DNS name is rejected with a precise failure reason. -- [ ] AC4: The default configuration file (`share/default/`) has `use_ip_from_query_string` set to `false` (or omitted, defaulting to `false`). -- [ ] AC5: The configuration schema documentation clearly states the security implications of enabling this option. -- [ ] AC6: Focused unit tests cover every `ip` parameter validation and address-selection case; minimum contract/integration tests verify HTTP failure responses and configuration wiring where unit tests cannot. -- [ ] AC6a: A counter records rejected non-empty `ip` parameters using bounded reason labels, and debug logs record the rejection without raw client IP values or application error logs. -- [ ] AC7: `linter all` exits with code `0`. -- [ ] AC8: Relevant tests pass with no regressions. -- [ ] AC9: Baseline manual verification runs a local tracker and local tracker client before implementation; reproducible commands, output, expected/actual results, and environment details are recorded in `manual-verification.md` in this issue directory. -- [ ] AC10: Before v3.0.0 runtime activation, manual verification reruns the baseline matrix and documents the intentional disabled-policy change: absent/empty values remain accepted while non-empty values are rejected with precise failure reasons. +- [x] AC1: When `use_ip_from_query_string` is `false` (default), an absent or empty `ip` GET parameter uses the connection IP; a non-empty `ip` value is rejected with a precise failure reason. Evidence: `manual-verification.md` Phase 2. +- [x] AC2: When `use_ip_from_query_string` is `true` and a valid IP is provided in the `ip` GET parameter, the tracker uses that IP as the peer's address. Evidence: focused service unit tests; local runtime activation awaits #1980. +- [x] AC3: When `use_ip_from_query_string` is `true`, an absent or empty `ip` GET parameter uses the connection IP; a non-empty invalid IP or DNS name is rejected with a precise failure reason. Evidence: focused service/protocol tests; local runtime activation awaits #1980. +- [x] AC4: The default configuration file (`share/default/`) has `use_ip_from_query_string` set to `false` (or omitted, defaulting to `false`). Evidence: v3 schema field defaults to `false`; active v2 default file intentionally remains unchanged pending #1980. +- [x] AC5: The configuration schema documentation clearly states the security implications of enabling this option. +- [x] AC6: Focused unit tests cover every `ip` parameter validation and address-selection case; minimum contract/integration tests verify HTTP failure responses and configuration wiring where unit tests cannot. +- [x] AC6a: A counter records rejected non-empty `ip` parameters using bounded reason labels, and the rejection-specific debug log records only a bounded reason without raw `ip` parameter values or application error logs. Existing request-URI logging is outside scope. Evidence: `manual-verification.md` Phase 2. +- [x] AC7: `linter all` exits with code `0`. Evidence: pre-commit gate passed on 2026-08-18. +- [x] AC8: Relevant tests pass with no regressions. Evidence: `cargo +1.88.0 test --workspace` passed on 2026-08-19. +- [x] AC9: Baseline manual verification runs a local tracker and local tracker client before implementation; reproducible commands, output, expected/actual results, and environment details are recorded in `manual-verification.md` in this issue directory. +- [x] AC10: Before v3.0.0 runtime activation, manual verification reruns the baseline matrix and documents the intentional disabled-policy change: absent/empty values remain accepted while non-empty values are rejected with precise failure reasons. - [ ] AC11: After #1980 activates v3.0.0 configuration at runtime, manual verification runs a local tracker and local tracker client with `use_ip_from_query_string` enabled; the resulting evidence is appended to `manual-verification.md`. -- [ ] Acceptance criteria are re-reviewed after implementation and reflect actual behaviour. -- [ ] Documentation is updated when behaviour/workflow changes. +- [x] Acceptance criteria are re-reviewed after implementation and reflect actual behaviour. +- [x] Documentation is updated when behaviour/workflow changes. ## Verification Plan @@ -184,22 +206,22 @@ Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. The implementation must add automated coverage for every row in the parameter contract. Prefer unit tests at the validation and peer-address selection boundaries. Use contract/integration tests only for behavior that requires the HTTP transport boundary. -| ID | `ip` value | Setting | Expected outcome | Preferred test level | -| --- | ------------------------------------------ | -------- | ---------------------------------------------------------------- | -------------------------------------- | -| A1 | Raw state: absent | Disabled | Accept; use connection/reverse-proxy address | Protocol unit + service unit | -| A2 | Raw state: empty (`ip=`) | Disabled | Accept; treat as absent | Protocol unit + service unit | -| A3 | Valid IPv4 literal | Disabled | Reject with a disabled-override failure reason | Unit + HTTP contract response | -| A4 | Valid IPv6 literal | Disabled | Reject with a disabled-override failure reason | Unit + HTTP contract response | -| A5 | Raw state: DNS name | Disabled | Reject with a DNS-not-supported failure reason | Protocol unit + HTTP contract response | -| A6 | Raw state: invalid non-empty value | Disabled | Reject with an invalid-IP failure reason | Protocol unit + HTTP contract response | -| A7 | Raw state: absent | Enabled | Accept; use connection/reverse-proxy address | Protocol unit + service unit | -| A8 | Raw state: empty (`ip=`) | Enabled | Accept; treat as absent | Protocol unit + service unit | -| A9 | Valid IPv4 literal | Enabled | Accept; use supplied address | Unit | -| A10 | Valid IPv6 literal | Enabled | Accept; use supplied address | Unit | -| A11 | Raw state: DNS name | Enabled | Reject with a DNS-not-supported failure reason | Protocol unit + HTTP contract response | -| A12 | Raw state: invalid non-empty value | Enabled | Reject with an invalid-IP failure reason | Protocol unit + HTTP contract response | -| A13 | Valid IPv4/IPv6 literal with reverse proxy | Enabled | Accept; supplied address takes precedence over `X-Forwarded-For` | Unit + minimum integration coverage | -| A14 | Rejected value | Either | Increment the bounded-reason counter and emit a safe debug entry | Unit | +| ID | `ip` value | Setting | Expected outcome | Preferred test level | +| --- | ------------------------------------------ | -------- | ----------------------------------------------------------------------------------------------------- | -------------------------------------- | +| A1 | Raw state: absent | Disabled | Accept; use connection/reverse-proxy address | Protocol unit + service unit | +| A2 | Raw state: empty (`ip=`) | Disabled | Accept; treat as absent | Protocol unit + service unit | +| A3 | Valid IPv4 literal | Disabled | Reject with a disabled-override failure reason | Unit + HTTP contract response | +| A4 | Valid IPv6 literal | Disabled | Reject with a disabled-override failure reason | Unit + HTTP contract response | +| A5 | Raw state: DNS name | Disabled | Reject with a DNS-not-supported failure reason | Protocol unit + HTTP contract response | +| A6 | Raw state: invalid non-empty value | Disabled | Reject with an invalid-IP failure reason | Protocol unit + HTTP contract response | +| A7 | Raw state: absent | Enabled | Accept; use connection/reverse-proxy address | Protocol unit + service unit | +| A8 | Raw state: empty (`ip=`) | Enabled | Accept; treat as absent | Protocol unit + service unit | +| A9 | Valid IPv4 literal | Enabled | Accept; use supplied address | Unit | +| A10 | Valid IPv6 literal | Enabled | Accept; use supplied address | Unit | +| A11 | Raw state: DNS name | Enabled | Reject with a DNS-not-supported failure reason | Protocol unit + HTTP contract response | +| A12 | Raw state: invalid non-empty value | Enabled | Reject with an invalid-IP failure reason | Protocol unit + HTTP contract response | +| A13 | Valid IPv4/IPv6 literal with reverse proxy | Enabled | Accept; supplied address takes precedence over `X-Forwarded-For` | Unit + minimum integration coverage | +| A14 | Rejected value | Either | Increment the bounded-reason counter and emit a rejection-specific debug entry with no raw `ip` value | Unit | ### Manual Verification Scenarios @@ -212,14 +234,14 @@ Run the same applicable request matrix against a local tracker in three phases: - relevant client output and metric/debug-log evidence; - expected and actual results for every executed scenario. -| ID | Scenario | Command/Steps | Expected Result | Status | Evidence | -| --- | --------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------ | ------------------------------------------------------------------------- | ------ | -------- | -| M1 | Default config: valid non-empty `ip` is rejected | Start tracker with default config; announce with `ip=1.2.3.4` | Announce fails, explaining that client-supplied peer IPs are disabled | TODO | | -| M2 | Opt-in config: `ip` GET param is used | Enable `use_ip_from_query_string`; announce with `ip=1.2.3.4`; check the peer list | Peer is registered with `1.2.3.4` | TODO | | -| M3 | Opt-in config: absent or empty `ip` — fallback | Enable `use_ip_from_query_string`; announce without `ip` and with `ip=` | Peer is registered with the connection IP in both cases | TODO | | -| M4 | Opt-in + reverse proxy: `ip` param takes precedence | Enable both `use_ip_from_query_string` and `on_reverse_proxy`; announce with `ip=1.2.3.4` and `X-Forwarded-For: 5.6.7.8` | Peer is registered with `1.2.3.4` (query string wins) | TODO | | -| M5 | Non-empty invalid or DNS `ip` is rejected | Announce with enabled and disabled configurations using `ip=invalid` and `ip=example.com` | Announce fails with the specific validation reason | TODO | | -| M6 | Rejected parameters are observable | Submit rejected non-empty `ip` parameters and inspect metrics and debug logs | Counter increments by bounded reason; debug log has reason without raw IP | TODO | | +| ID | Scenario | Command/Steps | Expected Result | Status | Evidence | +| --- | --------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------ | ---------------------------------------------------------------------------------------- | ----------- | --------------------------------------------------------------------------------------------------- | +| M1 | Default config: valid non-empty `ip` is rejected | Start tracker with default config; announce with `ip=1.2.3.4` | Announce fails, explaining that client-supplied peer IPs are disabled | DONE | `manual-verification.md` Phase 2 | +| M2 | Opt-in config: `ip` GET param is used | Enable `use_ip_from_query_string`; announce with `ip=1.2.3.4`; check the peer list | Peer is registered with `1.2.3.4` | TODO | | +| M3 | Opt-in config: absent or empty `ip` — fallback | Enable `use_ip_from_query_string`; announce without `ip` and with `ip=` | Peer is registered with the connection IP in both cases | TODO | | +| M4 | Opt-in + reverse proxy: `ip` param takes precedence | Enable both `use_ip_from_query_string` and `on_reverse_proxy`; announce with `ip=1.2.3.4` and `X-Forwarded-For: 5.6.7.8` | Peer is registered with `1.2.3.4` (query string wins) | TODO | | +| M5 | Non-empty invalid or DNS `ip` is rejected | Announce with enabled and disabled configurations using `ip=invalid_ip` and `ip=example.com` | Announce fails with the specific validation reason | IN_PROGRESS | Disabled-mode evidence complete in `manual-verification.md`; enabled-mode verification awaits #1980 | +| M6 | Rejected parameters are observable | Submit rejected non-empty `ip` parameters and inspect the rejection metric and rejection-specific debug log | Counter increments by bounded reason; rejection-specific debug log has no raw `ip` value | DONE | `manual-verification.md` Phase 2 | **Baseline expectation:** Before implementation, use M1–M5 as an address-selection request matrix. Valid, DNS-name, and invalid non-empty `ip` values are expected to be silently ignored and the announce is expected to succeed using the connection-derived address. Empty and absent values are expected to succeed. M6 is post-implementation only because its metric and debug log do not yet exist. @@ -227,20 +249,20 @@ Run the same applicable request matrix against a local tracker in three phases: ### Acceptance Verification -| AC ID | Status (`TODO`/`DONE`) | Evidence | -| ----- | ---------------------- | ----------------------------------------------------------- | -| AC1 | TODO | | -| AC2 | TODO | | -| AC3 | TODO | | -| AC4 | TODO | | -| AC5 | TODO | | -| AC6 | TODO | | -| AC6a | TODO | | -| AC7 | TODO | | -| AC8 | TODO | | -| AC9 | TODO | | -| AC10 | TODO | | -| AC11 | BLOCKED | Requires #1980 to activate v3.0.0 configuration at runtime. | +| AC ID | Status (`TODO`/`DONE`) | Evidence | +| ----- | ---------------------- | ------------------------------------------------------------------------------------- | +| AC1 | DONE | `manual-verification.md` Phase 2 | +| AC2 | DONE | Focused enabled-policy service tests; runtime verification deferred to #1980 | +| AC3 | DONE | Focused enabled-policy service/protocol tests; runtime verification deferred to #1980 | +| AC4 | DONE | v3 schema default is `false`; active v2 default config unchanged pending #1980 | +| AC5 | DONE | v3 `HttpTracker` field documentation | +| AC6 | DONE | Focused protocol, service, and Axum HTTP contract tests | +| AC6a | DONE | `manual-verification.md` Phase 2 | +| AC7 | DONE | Pre-commit gate passed 2026-08-18 | +| AC8 | DONE | `cargo +1.88.0 test --workspace` passed 2026-08-19 | +| AC9 | DONE | `manual-verification.md` Phase 1 | +| AC10 | DONE | `manual-verification.md` Phase 2 | +| AC11 | BLOCKED | Requires #1980 to activate v3.0.0 configuration at runtime. | ## Risks and Trade-offs diff --git a/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/manual-verification.md b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/manual-verification.md new file mode 100644 index 000000000..2c517dbc8 --- /dev/null +++ b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/manual-verification.md @@ -0,0 +1,173 @@ +# Manual Verification — Issue #1987 + +This evidence file records three comparable local verification phases: + +1. Baseline behavior before implementation. +2. Behavior after implementation while the internal policy remains disabled. +3. Behavior after #1980 activates configuration schema v3.0.0 and the setting is enabled. + +## Phase 1 — Baseline Before Implementation + +**Status:** DONE + +### Environment + +| Item | Value | +| --------------------- | ----------------------------------------------------------------------- | +| Date/time (UTC) | 2026-08-18; exact time not captured | +| Commit | `4005cca5518d3ce8b1556cf10abcd7db146ae18e` | +| OS | Linux | +| Rust toolchain | Rust `1.88.0` (`rustc 1.88.0`, `cargo 1.88.0`) | +| Tracker configuration | `share/default/config/tracker.development.sqlite3.toml` (schema v2.0.0) | +| Local HTTP tracker | `http://127.0.0.1:7070` | + +### Request Matrix + +| Case | Request form | Expected baseline behavior | Actual result | +| ------------- | ----------------- | ----------------------------------------------------- | ----------------------------------- | +| Absent | No `ip` parameter | Announce succeeds using connection-derived address | HTTP 200; bencoded success response | +| Empty | `ip=` | Announce succeeds using connection-derived address | HTTP 200; bencoded success response | +| Valid IPv4 | `ip=1.2.3.4` | Announce succeeds; supplied value is silently ignored | HTTP 200; bencoded success response | +| Valid IPv6 | `ip=2001:db8::1` | Announce succeeds; supplied value is silently ignored | HTTP 200; bencoded success response | +| DNS name | `ip=example.com` | Announce succeeds; supplied value is silently ignored | HTTP 200; bencoded success response | +| Invalid value | `ip=invalid_ip` | Announce succeeds; supplied value is silently ignored | HTTP 200; bencoded success response | + +### Commands and Output + +The local tracker was started with: + +```sh +cargo +1.88.0 run --bin torrust-tracker +``` + +The raw HTTP matrix used a single valid announce query with each `ip` suffix below: + +```text +(absent) +&ip= +&ip=1.2.3.4 +&ip=2001%3Adb8%3A%3A1 +&ip=example.com +&ip=invalid_ip +``` + +All six requests returned HTTP 200 and the same bencoded announce success response: + +```text +d8:completei0e10:incompletei1e8:intervali120e12:min intervali120e5:peers0:6:peers6e +``` + +This confirms the pre-implementation behavior: the tracker does not distinguish absent, empty, valid, DNS-name, and invalid `ip` values at the HTTP response boundary; every supplied value is silently ignored. + +The local typed tracker client also confirmed that a valid supplied address is ignored: + +```sh +cargo +1.88.0 run -p torrust-tracker-client --bin tracker_client -- \ + http announce http://127.0.0.1:7070 \ + 9c38422213e30bff212b30c360d26f9a02136422 \ + --ip 1.2.3.4 +``` + +It returned a successful JSON announce response whose peer list contains the connection address, not `1.2.3.4`: + +```json +{ + "complete": 1, + "incomplete": 1, + "interval": 120, + "min interval": 120, + "peers": [ + { + "ip": "127.0.0.1", + "peer id": [ + 45, 77, 86, 48, 48, 48, 49, 45, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, + 48, 49 + ], + "port": 6881 + } + ] +} +``` + +## Phase 2 — Post-Implementation Disabled Policy + +**Status:** DONE + +### Environment + +| Item | Value | +| --------------------- | ----------------------------------------------------------------------- | +| Date/time (UTC) | 2026-08-18 to 2026-08-19; exact time not captured | +| Commit | Uncommitted #1987 implementation after `4005cca` | +| OS | Linux | +| Rust toolchain | Rust `1.88.0` | +| Tracker configuration | `share/default/config/tracker.development.sqlite3.toml` (schema v2.0.0) | +| Local HTTP tracker | `http://127.0.0.1:7070` | + +### Address-Selection Request Matrix + +The same raw HTTP announce matrix from Phase 1 was run after rebuilding the tracker. Every response used HTTP 200, as required by the BitTorrent HTTP tracker failure-response convention; failed announces carry a bencoded `failure reason`. + +| Case | Request form | Actual result | +| ----------------------------- | ---------------------- | ---------------------------------------------------------------------------------------------------------------------------- | +| Absent | No `ip` parameter | HTTP 200; bencoded announce success response | +| Empty | `ip=` | HTTP 200; bencoded announce success response | +| Valid IPv4 | `ip=1.2.3.4` | HTTP 200; `failure reason`: `Client-supplied peer IPs are disabled` | +| Valid encoded IPv6 | `ip=2001%3Adb8%3A%3A1` | HTTP 200; `failure reason`: `Client-supplied peer IPs are disabled` | +| DNS name | `ip=example.com` | HTTP 200; `failure reason`: `DNS names are not supported for the announce ip parameter` | +| Single-label DNS name | `ip=localhost` | HTTP 200; `failure reason`: `DNS names are not supported for the announce ip parameter` | +| Invalid value | `ip=invalid_ip` | HTTP 200; `failure reason`: `The announce ip parameter must be an IPv4 or IPv6 literal` | +| Invalid numeric IP-like value | `ip=999.999.999.999` | HTTP 200; `failure reason`: `The announce ip parameter must be an IPv4 or IPv6 literal` | +| Malformed encoding | `ip=%ZZ` | HTTP 200; `failure reason`: `Bad request. Cannot parse query params for announce request: malformed percent encoding for ip` | + +This verifies the intentional baseline change: absent and empty values remain successful, while every non-empty override is explicitly rejected until schema v3.0.0 can activate the opt-in policy. + +### Observability Evidence + +The tracker was restarted with a temporary local debug logging override, then received `ip=1.2.3.4` while the policy remained disabled. + +```sh +TORRUST_TRACKER_CONFIG_OVERRIDE_LOGGING__THRESHOLD=debug \ + cargo +1.88.0 run --bin torrust-tracker +``` + +The rejection-specific debug output used only the bounded reason code and did not contain the raw submitted value (`1.2.3.4`): + +```text +DEBUG torrust_tracker_http_core::statistics::event::handler: Recorded rejected HTTP announce peer IP parameter reason="override_disabled" +``` + +The authenticated local metrics endpoint reported one rejection with the bounded `reason="override_disabled"` label and no raw submitted-IP label: + +```text +# HELP http_tracker_core_announce_peer_ip_rejections_total Total rejected HTTP announce peer IP parameters +# TYPE http_tracker_core_announce_peer_ip_rejections_total counter +http_tracker_core_announce_peer_ip_rejections_total{client_address_ip_family="inet",client_address_ip_type="plain",reason="override_disabled",server_binding_address_ip_family="inet",server_binding_address_ip_type="plain",server_binding_ip="0.0.0.0",server_binding_port="7070",server_binding_protocol="http"}1 +``` + +Existing HTTP request middleware logs the full request URI at `info`, including query values. That established behavior is outside #1987's rejection-observability scope and is not evidence that raw values are globally absent from tracker logs. + +### Local Tracker-Client Result + +The local typed client was run against the rebuilt tracker: + +```sh +cargo +1.88.0 run -p torrust-tracker-client --bin tracker_client -- \ + http announce http://127.0.0.1:7070 \ + 9c38422213e30bff212b30c360d26f9a02136422 \ + --ip 1.2.3.4 +``` + +The client displayed the expected tracker failure reason: + +```json +{ "failure reason": "Client-supplied peer IPs are disabled" } +``` + +It then returned its existing generic client-side error, `unrecognized announce response from tracker`. The tracker response itself is correct and matches the raw HTTP evidence above; this client-side classification behavior is not changed by #1987. + +## Phase 3 — Active v3 Enabled Policy + +**Status:** BLOCKED + +Blocked until issue #1980 activates schema v3.0.0 configuration at runtime. This phase will enable `use_ip_from_query_string` for a local HTTP tracker and verify valid overrides, fallback behavior, validation failures, and reverse-proxy precedence. diff --git a/packages/axum-http-server/Cargo.toml b/packages/axum-http-server/Cargo.toml index cc5295325..a04f433a2 100644 --- a/packages/axum-http-server/Cargo.toml +++ b/packages/axum-http-server/Cargo.toml @@ -41,7 +41,6 @@ tracing = "0" socket2 = "0.6.4" [dev-dependencies] -local-ip-address = "0" rand = "0.9" serde_bencode = "0" serde_bytes = "0" diff --git a/packages/axum-http-server/src/v1/extractors/announce_request.rs b/packages/axum-http-server/src/v1/extractors/announce_request.rs index b6072d29c..3a4266297 100644 --- a/packages/axum-http-server/src/v1/extractors/announce_request.rs +++ b/packages/axum-http-server/src/v1/extractors/announce_request.rs @@ -88,7 +88,7 @@ mod tests { use std::str::FromStr; use torrust_info_hash::InfoHash; - use torrust_tracker_http_protocol::v1::requests::announce::{Announce, Compact, Event, NumberOfBytes}; + use torrust_tracker_http_protocol::v1::requests::announce::{Announce, Compact, Event, NumberOfBytes, PeerIp}; use torrust_tracker_http_protocol::v1::responses::error::Error; use torrust_tracker_primitives::PeerId; @@ -113,7 +113,7 @@ mod tests { info_hash: InfoHash::from_str("3b245504cf5f11bbdbe1201cea6a6bf45aee1bc0").unwrap(), // DevSkim: ignore DS173237 peer_id: PeerId(*b"-qB00000000000000001"), port: 17548, - ip: Some(IpAddr::V4(Ipv4Addr::new(2, 137, 87, 41))), + ip: PeerIp::Literal(IpAddr::V4(Ipv4Addr::new(2, 137, 87, 41))), downloaded: Some(NumberOfBytes::new(0)), uploaded: Some(NumberOfBytes::new(0)), left: Some(NumberOfBytes::new(0)), diff --git a/packages/axum-http-server/src/v1/handlers/announce.rs b/packages/axum-http-server/src/v1/handlers/announce.rs index 73e4868bd..1b02df036 100644 --- a/packages/axum-http-server/src/v1/handlers/announce.rs +++ b/packages/axum-http-server/src/v1/handlers/announce.rs @@ -27,7 +27,7 @@ pub async fn handle_without_key( ExtractRequest(announce_request): ExtractRequest, ExtractClientIpSources(client_ip_sources): ExtractClientIpSources, ) -> Response { - tracing::debug!("http announce request: {:#?}", announce_request); + tracing::debug!("Received HTTP announce request"); handle(&state.0, &announce_request, &client_ip_sources, &state.1, None).await } @@ -41,7 +41,7 @@ pub async fn handle_with_key( ExtractClientIpSources(client_ip_sources): ExtractClientIpSources, ExtractKey(key): ExtractKey, ) -> Response { - tracing::debug!("http announce request: {:#?}", announce_request); + tracing::debug!("Received HTTP announce request"); handle(&state.0, &announce_request, &client_ip_sources, &state.1, Some(key)).await } @@ -143,7 +143,7 @@ mod tests { use torrust_tracker_http_core::services::announce::AnnounceService; use torrust_tracker_http_core::statistics::event::listener::run_event_listener; use torrust_tracker_http_core::statistics::repository::Repository; - use torrust_tracker_http_protocol::v1::requests::announce::Announce; + use torrust_tracker_http_protocol::v1::requests::announce::{Announce, PeerIp}; use torrust_tracker_http_protocol::v1::responses; use torrust_tracker_http_protocol::v1::services::peer_ip_resolver::ClientIpSources; use torrust_tracker_primitives::PeerId; @@ -220,7 +220,7 @@ mod tests { info_hash: sample_info_hash(), peer_id: PeerId(*b"-qB00000000000000001"), port: 17548, - ip: None, + ip: PeerIp::Absent, downloaded: None, uploaded: None, left: None, diff --git a/packages/axum-http-server/tests/server/v1/contract/for_all_config_modes/receiving_an_announce_request.rs b/packages/axum-http-server/tests/server/v1/contract/for_all_config_modes/receiving_an_announce_request.rs index bbd6c68c6..9b8d5c426 100644 --- a/packages/axum-http-server/tests/server/v1/contract/for_all_config_modes/receiving_an_announce_request.rs +++ b/packages/axum-http-server/tests/server/v1/contract/for_all_config_modes/receiving_an_announce_request.rs @@ -14,7 +14,6 @@ use std::str::FromStr; use std::sync::Arc; use std::time::Duration; -use local_ip_address::local_ip; use reqwest::{Response, StatusCode}; use tokio::net::TcpListener; use torrust_info_hash::InfoHash; @@ -96,6 +95,29 @@ async fn should_fail_when_the_url_query_component_is_empty() { env.stop().await; } +#[tokio::test] +async fn it_should_return_a_failure_response_for_a_non_empty_peer_ip_when_overrides_are_disabled() { + // Arrange + logging::setup(); + let cfg = configuration::ephemeral_public(); + let core_config = Arc::new(cfg.core.clone()); + let http_tracker_config = Arc::new(cfg.http_trackers.unwrap()[0].clone()); + let env = Started::new(&core_config, &http_tracker_config).await; + let announce = AnnounceBuilder::default().with_ip("192.0.2.1".parse().unwrap()).query(); + + // Act + let response = Client::new(env.base_url(), Duration::from_secs(5)) + .unwrap() + .announce(&announce) + .await + .unwrap(); + + // Assert + let response_body = String::from_utf8(response.bytes().await.unwrap().to_vec()).unwrap(); + assert!(response_body.contains("Client-supplied peer IPs are disabled")); + env.stop().await; +} + #[tokio::test] async fn should_fail_when_url_query_parameters_are_invalid() { logging::setup(); @@ -206,14 +228,9 @@ async fn should_fail_when_the_info_hash_param_is_invalid() { } #[tokio::test] -async fn should_not_fail_when_the_ip_param_is_invalid() { +async fn should_reject_an_invalid_peer_ip_parameter() { logging::setup(); - // AnnounceQuery does not even contain the `ip` param when it is invalid - // The peer IP is obtained in two ways: - // 1. If tracker is NOT running `on_reverse_proxy` from the remote client IP. - // 2. If tracker is running `on_reverse_proxy` from `X-Forwarded-For` request HTTP header. - let cfg = configuration::ephemeral(); let core_config = Arc::new(cfg.core.clone()); let http_tracker_config = Arc::new(cfg.http_trackers.unwrap()[0].clone()); @@ -224,7 +241,7 @@ async fn should_not_fail_when_the_ip_param_is_invalid() { percent_encode_byte_array(&AnnounceBuilder::default().query().info_hash.bytes()), percent_encode_byte_array(&AnnounceBuilder::default().query().peer_id.0), AnnounceBuilder::default().query().port, - "INVALID-IP-ADDRESS", + "invalid_ip", ); let response = Client::new(env.base_url(), Duration::from_secs(5)) @@ -233,7 +250,47 @@ async fn should_not_fail_when_the_ip_param_is_invalid() { .await .unwrap(); - assert_is_announce_response(response).await; + let response_body = String::from_utf8(response.bytes().await.unwrap().to_vec()).unwrap(); + assert!(response_body.contains("The announce ip parameter must be an IPv4 or IPv6 literal")); + + env.stop().await; +} + +#[tokio::test] +async fn it_should_return_distinct_failure_reasons_for_non_literal_peer_ip_parameters() { + // Arrange + logging::setup(); + let cfg = configuration::ephemeral(); + let core_config = Arc::new(cfg.core.clone()); + let http_tracker_config = Arc::new(cfg.http_trackers.unwrap()[0].clone()); + let env = Started::new(&core_config, &http_tracker_config).await; + let required_parameters = format!( + "info_hash={}&peer_id={}&port={}", + percent_encode_byte_array(&AnnounceBuilder::default().query().info_hash.bytes()), + percent_encode_byte_array(&AnnounceBuilder::default().query().peer_id.0), + AnnounceBuilder::default().query().port, + ); + + // Act / Assert + for (ip, expected_failure_reason) in [ + ("localhost", "DNS names are not supported for the announce ip parameter"), + ("tracker", "DNS names are not supported for the announce ip parameter"), + ("example.com", "DNS names are not supported for the announce ip parameter"), + ("999.999.999.999", "The announce ip parameter must be an IPv4 or IPv6 literal"), + ( + "%ZZ", + "Bad request. Cannot parse query params for announce request: malformed percent encoding for ip", + ), + ] { + let response = Client::new(env.base_url(), Duration::from_secs(5)) + .unwrap() + .get(&format!("announce?{required_parameters}&ip={ip}")) + .await + .unwrap(); + + let response_body = String::from_utf8(response.bytes().await.unwrap().to_vec()).unwrap(); + assert!(response_body.contains(expected_failure_reason), "ip={ip}"); + } env.stop().await; } @@ -675,7 +732,8 @@ async fn should_return_the_list_of_previously_announced_peers_including_peers_us } #[tokio::test] -async fn should_consider_two_peers_to_be_the_same_when_they_have_the_same_socket_address_even_if_the_peer_id_is_different() { +async fn should_consider_two_peers_to_be_the_same_when_they_have_the_same_connection_socket_address_even_if_the_peer_id_is_different() + { logging::setup(); let cfg = configuration::ephemeral_public(); @@ -689,19 +747,16 @@ async fn should_consider_two_peers_to_be_the_same_when_they_have_the_same_socket let announce_query_1 = AnnounceBuilder::default() .with_info_hash(&info_hash) .with_peer_id(&PeerId(peer.peer_id.0)) - .with_ip(peer.peer_addr.ip()) .with_port(peer.peer_addr.port()) .query(); let announce_query_2 = AnnounceBuilder::default() .with_info_hash(&info_hash) .with_peer_id(&PeerId(*b"-qB00000000000000002")) // Different peer ID - .with_ip(peer.peer_addr.ip()) .with_port(peer.peer_addr.port()) .query(); - // Same peer socket address - assert_eq!(announce_query_1.ip, announce_query_2.ip); + // Same connection peer socket address. assert_eq!(announce_query_1.port, announce_query_2.port); // Different peer ID @@ -887,33 +942,28 @@ async fn should_increase_the_number_of_tcp6_announce_requests_handled_in_statist } #[tokio::test] -async fn should_not_increase_the_number_of_tcp6_announce_requests_handled_if_the_client_is_not_using_an_ipv6_ip() { +async fn should_reject_a_valid_ipv6_peer_ip_when_overrides_are_disabled() { logging::setup(); - // The tracker ignores the peer address in the request param. It uses the client remote ip address. - let cfg = configuration::ephemeral_public(); let core_config = Arc::new(cfg.core.clone()); let http_tracker_config = Arc::new(cfg.http_trackers.unwrap()[0].clone()); let env = Started::new(&core_config, &http_tracker_config).await; - Client::new(env.base_url(), Duration::from_secs(5)) + let response = Client::new(env.base_url(), Duration::from_secs(5)) .unwrap() .announce(&AnnounceBuilder::default().with_ip(IpAddr::V6(Ipv6Addr::LOCALHOST)).query()) .await .unwrap(); - let stats = env.container.http_tracker_core_container.stats_repository.get_stats().await; - - assert_eq!(stats.tcp6_announces_handled(), 0); - - drop(stats); + let response_body = String::from_utf8(response.bytes().await.unwrap().to_vec()).unwrap(); + assert!(response_body.contains("Client-supplied peer IPs are disabled")); env.stop().await; } #[tokio::test] -async fn should_assign_to_the_peer_ip_the_remote_client_ip_instead_of_the_peer_address_in_the_request_param() { +async fn should_reject_a_valid_ipv4_peer_ip_when_overrides_are_disabled() { logging::setup(); let cfg = configuration::ephemeral_public(); @@ -922,36 +972,24 @@ async fn should_assign_to_the_peer_ip_the_remote_client_ip_instead_of_the_peer_a let env = Started::new(&core_config, &http_tracker_config).await; let info_hash = InfoHash::from_str("9c38422213e30bff212b30c360d26f9a02136422").unwrap(); // DevSkim: ignore DS173237 - let client_ip = local_ip().unwrap(); - let announce_query = AnnounceBuilder::default() .with_info_hash(&info_hash) .with_ip(IpAddr::from_str("2.2.2.2").unwrap()) .query(); { - let client = Client::bind(env.base_url(), Duration::from_secs(5), client_ip).unwrap(); - let status = client.announce(&announce_query).await.unwrap().status(); + let client = Client::new(env.base_url(), Duration::from_secs(5)).unwrap(); + let response = client.announce(&announce_query).await.unwrap(); + let response_body = String::from_utf8(response.bytes().await.unwrap().to_vec()).unwrap(); - assert_eq!(status, StatusCode::OK); + assert!(response_body.contains("Client-supplied peer IPs are disabled")); } - let peers = env - .container - .tracker_core_container - .in_memory_torrent_repository - .get_torrent_peers(&info_hash, usize::MAX) - .await; - let peer_addr = peers[0].peer_addr; - - assert_eq!(peer_addr.ip(), client_ip); - assert_ne!(peer_addr.ip(), IpAddr::from_str("2.2.2.2").unwrap()); - env.stop().await; } #[tokio::test] -async fn when_the_client_ip_is_a_loopback_ipv4_it_should_assign_to_the_peer_ip_the_external_ip_in_the_tracker_configuration() { +async fn when_the_client_ip_is_a_loopback_ipv4_it_should_assign_to_the_peer_ip_the_external_ip_without_an_ip_parameter() { logging::setup(); /* We assume that both the client and tracker share the same public IP. @@ -968,10 +1006,7 @@ async fn when_the_client_ip_is_a_loopback_ipv4_it_should_assign_to_the_peer_ip_t let loopback_ip = IpAddr::from_str("127.0.0.1").unwrap(); let client_ip = loopback_ip; - let announce_query = AnnounceBuilder::default() - .with_info_hash(&info_hash) - .with_ip(IpAddr::from_str("2.2.2.2").unwrap()) - .query(); + let announce_query = AnnounceBuilder::default().with_info_hash(&info_hash).query(); { let client = Client::bind(env.base_url(), Duration::from_secs(5), client_ip).unwrap(); @@ -997,13 +1032,12 @@ async fn when_the_client_ip_is_a_loopback_ipv4_it_should_assign_to_the_peer_ip_t .unwrap() .into(); assert_eq!(peer_addr.ip(), ext_ip); - assert_ne!(peer_addr.ip(), IpAddr::from_str("2.2.2.2").unwrap()); env.stop().await; } #[tokio::test] -async fn when_the_client_ip_is_a_loopback_ipv6_it_should_assign_to_the_peer_ip_the_external_ip_in_the_tracker_configuration() { +async fn when_the_client_ip_is_a_loopback_ipv6_it_should_assign_to_the_peer_ip_the_external_ip_without_an_ip_parameter() { logging::setup(); /* We assume that both the client and tracker share the same public IP. @@ -1021,10 +1055,7 @@ async fn when_the_client_ip_is_a_loopback_ipv6_it_should_assign_to_the_peer_ip_t let loopback_ip = IpAddr::from_str("127.0.0.1").unwrap(); let client_ip = loopback_ip; - let announce_query = AnnounceBuilder::default() - .with_info_hash(&info_hash) - .with_ip(IpAddr::from_str("2.2.2.2").unwrap()) - .query(); + let announce_query = AnnounceBuilder::default().with_info_hash(&info_hash).query(); { let client = Client::bind(env.base_url(), Duration::from_secs(5), client_ip).unwrap(); @@ -1050,7 +1081,6 @@ async fn when_the_client_ip_is_a_loopback_ipv6_it_should_assign_to_the_peer_ip_t .unwrap() .into(); assert_eq!(peer_addr.ip(), ext_ip); - assert_ne!(peer_addr.ip(), IpAddr::from_str("2.2.2.2").unwrap()); env.stop().await; } diff --git a/packages/configuration/src/v3_0_0/http_tracker.rs b/packages/configuration/src/v3_0_0/http_tracker.rs index bb2e9066e..c19efa821 100644 --- a/packages/configuration/src/v3_0_0/http_tracker.rs +++ b/packages/configuration/src/v3_0_0/http_tracker.rs @@ -31,6 +31,12 @@ pub struct HttpTracker { #[serde(default = "HttpTracker::default_tracker_usage_statistics")] pub tracker_usage_statistics: bool, + /// Whether to trust a non-empty BEP 3 `ip` query parameter as the peer + /// address. Defaults to `false` because enabling it allows clients to + /// spoof peer addresses; use only in a controlled, trusted deployment. + #[serde(default = "HttpTracker::default_use_ip_from_query_string")] + pub use_ip_from_query_string: bool, + /// The public-facing URL of this HTTP tracker instance, e.g. /// `"https://tracker.example.com/announce"`. Used for metrics labels, /// logging, and API discovery. Must use the `http://` or `https://` scheme. @@ -49,6 +55,7 @@ impl Default for HttpTracker { bind_address: Self::default_bind_address(), tls_config: Self::default_tls_config(), tracker_usage_statistics: Self::default_tracker_usage_statistics(), + use_ip_from_query_string: Self::default_use_ip_from_query_string(), public_url: Self::default_public_url(), network: Self::default_network(), } @@ -68,6 +75,10 @@ impl HttpTracker { false } + fn default_use_ip_from_query_string() -> bool { + false + } + fn default_public_url() -> Option { None } @@ -110,6 +121,27 @@ mod tests { assert!(configuration.public_url.is_none()); } + #[test] + fn it_should_default_use_ip_from_query_string_to_false() { + // Act + let configuration = HttpTracker::default(); + + // Assert + assert!(!configuration.use_ip_from_query_string); + } + + #[test] + fn it_should_deserialize_use_ip_from_query_string() { + // Arrange + let toml = "use_ip_from_query_string = true"; + + // Act + let configuration: HttpTracker = toml::from_str(toml).expect("configuration should deserialize"); + + // Assert + assert!(configuration.use_ip_from_query_string); + } + #[test] fn it_should_accept_public_url_when_scheme_is_https() { // Arrange diff --git a/packages/http-core/benches/helpers/util.rs b/packages/http-core/benches/helpers/util.rs index 7faf6f86e..3ba64c2e9 100644 --- a/packages/http-core/benches/helpers/util.rs +++ b/packages/http-core/benches/helpers/util.rs @@ -22,7 +22,7 @@ use torrust_tracker_http_core::event::sender::Broadcaster; use torrust_tracker_http_core::statistics::event::listener::run_event_listener; use torrust_tracker_http_core::statistics::repository::Repository; use torrust_tracker_http_protocol::v1::requests::announce::{ - Announce, Event as ProtocolAnnounceEvent, NumberOfBytes as ProtocolNumberOfBytes, + Announce, Event as ProtocolAnnounceEvent, NumberOfBytes as ProtocolNumberOfBytes, PeerIp, }; use torrust_tracker_http_protocol::v1::services::peer_ip_resolver::ClientIpSources; use torrust_tracker_primitives::peer::Peer; @@ -107,7 +107,7 @@ pub fn sample_announce_request_for_peer(peer: Peer) -> (Announce, ClientIpSource info_hash: sample_info_hash(), peer_id: peer.peer_id, port: peer.peer_addr.port(), - ip: None, + ip: PeerIp::Absent, uploaded: Some(ProtocolNumberOfBytes::new(peer.uploaded.0)), downloaded: Some(ProtocolNumberOfBytes::new(peer.downloaded.0)), left: Some(ProtocolNumberOfBytes::new(peer.left.0)), diff --git a/packages/http-core/src/container.rs b/packages/http-core/src/container.rs index ea0150ce1..62e2eafcd 100644 --- a/packages/http-core/src/container.rs +++ b/packages/http-core/src/container.rs @@ -6,7 +6,7 @@ use torrust_tracker_swarm_coordination_registry::container::SwarmCoordinationReg use crate::event::bus::EventBus; use crate::event::sender::Broadcaster; -use crate::services::announce::AnnounceService; +use crate::services::announce::{AnnounceService, PeerIpSelectionPolicy}; use crate::services::scrape::ScrapeService; use crate::statistics::repository::Repository; use crate::{event, services, statistics}; @@ -86,12 +86,15 @@ impl HttpTrackerCoreServices { let http_stats_event_sender = http_stats_event_bus.sender(); - let http_announce_service = Arc::new(AnnounceService::new( + let http_announce_service = Arc::new(AnnounceService::new_with_peer_ip_selection_policy( tracker_core_container.core_config.clone(), tracker_core_container.announce_handler.clone(), tracker_core_container.authentication_service.clone(), tracker_core_container.whitelist_authorization.clone(), http_stats_event_sender.clone(), + // Configuration v3 is not runtime-active until #1980. Keep the + // production policy explicitly disabled rather than adding v2 wiring. + PeerIpSelectionPolicy::disabled(), )); let http_scrape_service = Arc::new(ScrapeService::new( diff --git a/packages/http-core/src/event.rs b/packages/http-core/src/event.rs index b7c6b9655..7df267cc1 100644 --- a/packages/http-core/src/event.rs +++ b/packages/http-core/src/event.rs @@ -29,6 +29,30 @@ pub enum Event { TcpScrape { connection: ConnectionContext, }, + /// A non-empty announce `ip` parameter was rejected by address-selection policy. + TcpAnnouncePeerIpRejected { + connection: ConnectionContext, + reason: PeerIpRejectionReason, + }, +} + +/// Bounded reasons for rejecting an announce `ip` parameter. +#[derive(Debug, PartialEq, Eq, Clone, Copy)] +pub enum PeerIpRejectionReason { + OverrideDisabled, + DnsNameUnsupported, + InvalidIpAddress, +} + +impl PeerIpRejectionReason { + #[must_use] + pub const fn as_str(self) -> &'static str { + match self { + Self::OverrideDisabled => "override_disabled", + Self::DnsNameUnsupported => "dns_name_unsupported", + Self::InvalidIpAddress => "invalid_ip_address", + } + } } #[derive(Debug, PartialEq, Eq, Clone)] diff --git a/packages/http-core/src/services/announce.rs b/packages/http-core/src/services/announce.rs index 608943534..af4ddb471 100644 --- a/packages/http-core/src/services/announce.rs +++ b/packages/http-core/src/services/announce.rs @@ -19,7 +19,7 @@ use torrust_tracker_core::authentication::{self, Key}; use torrust_tracker_core::error::{AnnounceError, TrackerCoreError, WhitelistError}; use torrust_tracker_core::whitelist; use torrust_tracker_http_protocol::v1::requests::announce::{ - Announce, Event as ProtocolAnnounceEvent, NumberOfBytes as ProtocolNumberOfBytes, + Announce, Event as ProtocolAnnounceEvent, NumberOfBytes as ProtocolNumberOfBytes, PeerIp, }; use torrust_tracker_http_protocol::v1::responses::error::Error as HttpProtocolErrorResponse; use torrust_tracker_http_protocol::v1::services::peer_ip_resolver::{ @@ -29,7 +29,7 @@ use torrust_tracker_primitives::peer::PeerAnnouncement; use torrust_tracker_primitives::{AnnounceData, AnnounceEvent, NumberOfBytes}; use crate::event; -use crate::event::Event; +use crate::event::{Event, PeerIpRejectionReason}; use crate::services::error_mapping::protocol_error_from_tracker_core_error; /// The HTTP tracker `announce` service. @@ -44,6 +44,30 @@ pub struct AnnounceService { authentication_service: Arc, whitelist_authorization: Arc, opt_http_stats_event_sender: event::sender::Sender, + peer_ip_selection_policy: PeerIpSelectionPolicy, +} + +/// Controls whether an HTTP announce may override its peer IP with BEP 3's +/// non-empty `ip` parameter. Enabling this trusts client-supplied addresses. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub struct PeerIpSelectionPolicy { + use_ip_from_query_string: bool, +} + +impl PeerIpSelectionPolicy { + #[must_use] + pub const fn disabled() -> Self { + Self { + use_ip_from_query_string: false, + } + } + + #[must_use] + pub const fn enabled() -> Self { + Self { + use_ip_from_query_string: true, + } + } } impl AnnounceService { @@ -54,6 +78,25 @@ impl AnnounceService { authentication_service: Arc, whitelist_authorization: Arc, opt_http_stats_event_sender: event::sender::Sender, + ) -> Self { + Self::new_with_peer_ip_selection_policy( + core_config, + announce_handler, + authentication_service, + whitelist_authorization, + opt_http_stats_event_sender, + PeerIpSelectionPolicy::disabled(), + ) + } + + #[must_use] + pub fn new_with_peer_ip_selection_policy( + core_config: Arc, + announce_handler: Arc, + authentication_service: Arc, + whitelist_authorization: Arc, + opt_http_stats_event_sender: event::sender::Sender, + peer_ip_selection_policy: PeerIpSelectionPolicy, ) -> Self { Self { core_config, @@ -61,6 +104,7 @@ impl AnnounceService { authentication_service, whitelist_authorization, opt_http_stats_event_sender, + peer_ip_selection_policy, } } @@ -85,18 +129,22 @@ impl AnnounceService { let remote_client_addr = resolve_remote_client_addr(&self.core_config.net.on_reverse_proxy.into(), client_ip_sources)?; - let mut peer = Self::peer_from_request(announce_request, &remote_client_addr.ip()); + let peer_ip = match self.select_peer_ip(announce_request, remote_client_addr.ip()) { + Ok(peer_ip) => peer_ip, + Err(reason) => { + self.send_peer_ip_rejection_event(remote_client_addr, server_service_binding.clone(), reason) + .await; + return Err(reason.into()); + } + }; + + let mut peer = Self::peer_from_request(announce_request, &peer_ip); let peers_wanted = Self::peers_wanted(announce_request); let announce_data = self .announce_handler - .handle_announcement( - &announce_request.info_hash, - &mut peer, - &remote_client_addr.ip(), - &peers_wanted, - ) + .handle_announcement(&announce_request.info_hash, &mut peer, &peer_ip, &peers_wanted) .await?; self.send_event( @@ -138,6 +186,30 @@ impl AnnounceService { } } + fn select_peer_ip( + &self, + announce_request: &Announce, + connection_peer_ip: std::net::IpAddr, + ) -> Result { + Self::select_peer_ip_with_policy(self.peer_ip_selection_policy, announce_request, connection_peer_ip) + } + + fn select_peer_ip_with_policy( + peer_ip_selection_policy: PeerIpSelectionPolicy, + announce_request: &Announce, + connection_peer_ip: std::net::IpAddr, + ) -> Result { + match &announce_request.ip { + PeerIp::Absent | PeerIp::Empty => Ok(connection_peer_ip), + PeerIp::Literal(_) if !peer_ip_selection_policy.use_ip_from_query_string => { + Err(PeerIpRejectionReason::OverrideDisabled) + } + PeerIp::Literal(ip) => Ok(*ip), + PeerIp::DnsName => Err(PeerIpRejectionReason::DnsNameUnsupported), + PeerIp::Invalid => Err(PeerIpRejectionReason::InvalidIpAddress), + } + } + async fn authenticate(&self, maybe_key: Option) -> Result<(), authentication::key::Error> { if self.core_config.private { let key = maybe_key.ok_or(authentication::key::Error::MissingAuthKey { @@ -181,6 +253,24 @@ impl AnnounceService { http_stats_event_sender.send(event).await; } } + + async fn send_peer_ip_rejection_event( + &self, + remote_client_addr: RemoteClientAddr, + server_service_binding: ServiceBinding, + reason: PeerIpRejectionReason, + ) { + tracing::debug!(reason = reason.as_str(), "Rejected HTTP announce peer IP parameter"); + + if let Some(http_stats_event_sender) = self.opt_http_stats_event_sender.as_deref() { + http_stats_event_sender + .send(Event::TcpAnnouncePeerIpRejected { + connection: event::ConnectionContext::new(remote_client_addr, server_service_binding), + reason, + }) + .await; + } + } } /// Errors related to announce requests. @@ -191,6 +281,25 @@ pub enum HttpAnnounceError { #[error("Tracker core error: {source}")] TrackerCoreError { source: TrackerCoreError }, + + #[error("Client-supplied peer IPs are disabled")] + PeerIpOverrideDisabled, + + #[error("DNS names are not supported for the announce ip parameter")] + PeerIpDnsNameUnsupported, + + #[error("The announce ip parameter must be an IPv4 or IPv6 literal")] + PeerIpInvalid, +} + +impl From for HttpAnnounceError { + fn from(reason: PeerIpRejectionReason) -> Self { + match reason { + PeerIpRejectionReason::OverrideDisabled => Self::PeerIpOverrideDisabled, + PeerIpRejectionReason::DnsNameUnsupported => Self::PeerIpDnsNameUnsupported, + PeerIpRejectionReason::InvalidIpAddress => Self::PeerIpInvalid, + } + } } impl From for HttpAnnounceError { @@ -238,6 +347,11 @@ impl From for HttpProtocolErrorResponse { match error { HttpAnnounceError::PeerIpResolutionError { source } => source.into(), HttpAnnounceError::TrackerCoreError { source } => protocol_error_from_tracker_core_error(source), + HttpAnnounceError::PeerIpOverrideDisabled + | HttpAnnounceError::PeerIpDnsNameUnsupported + | HttpAnnounceError::PeerIpInvalid => Self { + failure_reason: error.to_string(), + }, } } } @@ -257,7 +371,7 @@ mod tests { use torrust_tracker_core::torrent::repository::in_memory::InMemoryTorrentRepository; use torrust_tracker_core::whitelist::authorization::WhitelistAuthorization; use torrust_tracker_core::whitelist::repository::in_memory::InMemoryWhitelist; - use torrust_tracker_http_protocol::v1::requests::announce::Announce; + use torrust_tracker_http_protocol::v1::requests::announce::{Announce, PeerIp}; use torrust_tracker_http_protocol::v1::services::peer_ip_resolver::ClientIpSources; use torrust_tracker_primitives::peer::Peer; use torrust_tracker_test_helpers::configuration; @@ -328,7 +442,7 @@ mod tests { info_hash: sample_info_hash(), peer_id: peer.peer_id, port: peer.peer_addr.port(), - ip: None, + ip: PeerIp::Absent, uploaded: Some(torrust_tracker_http_protocol::v1::requests::announce::NumberOfBytes::new( peer.uploaded.0, )), @@ -392,20 +506,78 @@ mod tests { use mockall::predicate::{self}; use torrust_net_primitives::service_binding::{Protocol, ServiceBinding}; use torrust_tracker_configuration::Configuration; - use torrust_tracker_http_protocol::v1::services::peer_ip_resolver::{RemoteClientAddr, ResolvedIp}; + use torrust_tracker_http_protocol::v1::requests::announce::{Announce, PeerIp}; + use torrust_tracker_http_protocol::v1::services::peer_ip_resolver::{ClientIpSources, RemoteClientAddr, ResolvedIp}; use torrust_tracker_primitives::swarm_metadata::SwarmMetadata; use torrust_tracker_primitives::{AnnounceData, peer}; use torrust_tracker_test_helpers::configuration; use crate::event::test::announce_events_match; - use crate::event::{ConnectionContext, Event}; - use crate::services::announce::AnnounceService; + use crate::event::{ConnectionContext, Event, PeerIpRejectionReason}; use crate::services::announce::tests::{ MockHttpStatsEventSender, initialize_core_tracker_services, initialize_core_tracker_services_with_config, sample_announce_request_for_peer, }; + use crate::services::announce::{AnnounceService, PeerIpSelectionPolicy}; use crate::tests::{sample_info_hash, sample_peer, sample_peer_using_ipv4, sample_peer_using_ipv6}; + #[test] + fn it_should_select_the_connection_address_for_absent_or_empty_peer_ip() { + // Arrange + let connection_ip = IpAddr::V4(Ipv4Addr::LOCALHOST); + let policy = PeerIpSelectionPolicy::enabled(); + + // Act / Assert + for ip in [PeerIp::Absent, PeerIp::Empty] { + let request = sample_announce_request_for_peer(sample_peer()).0; + let request = Announce { ip, ..request }; + + assert_eq!( + AnnounceService::select_peer_ip_with_policy(policy, &request, connection_ip), + Ok(connection_ip) + ); + } + } + + #[test] + fn it_should_reject_or_select_non_empty_peer_ip_according_to_policy() { + // Arrange + let connection_ip = IpAddr::V4(Ipv4Addr::LOCALHOST); + let cases = [ + ( + PeerIp::Literal("192.0.2.1".parse().unwrap()), + Ok("192.0.2.1".parse().unwrap()), + ), + ( + PeerIp::Literal("2001:db8::1".parse().unwrap()), + Ok("2001:db8::1".parse().unwrap()), + ), + (PeerIp::DnsName, Err(PeerIpRejectionReason::DnsNameUnsupported)), + (PeerIp::Invalid, Err(PeerIpRejectionReason::InvalidIpAddress)), + ]; + + // Act / Assert + for (ip, enabled_result) in cases { + let request = Announce { + ip, + ..sample_announce_request_for_peer(sample_peer()).0 + }; + assert_eq!( + AnnounceService::select_peer_ip_with_policy(PeerIpSelectionPolicy::enabled(), &request, connection_ip), + enabled_result + ); + assert_eq!( + AnnounceService::select_peer_ip_with_policy(PeerIpSelectionPolicy::disabled(), &request, connection_ip), + match request.ip { + PeerIp::Literal(_) => Err(PeerIpRejectionReason::OverrideDisabled), + PeerIp::DnsName => Err(PeerIpRejectionReason::DnsNameUnsupported), + PeerIp::Invalid => Err(PeerIpRejectionReason::InvalidIpAddress), + PeerIp::Absent | PeerIp::Empty => Ok(connection_ip), + } + ); + } + } + #[tokio::test] async fn it_should_return_the_announce_data() { let (core_tracker_services, core_http_tracker_services) = initialize_core_tracker_services().await; @@ -443,6 +615,72 @@ mod tests { assert_eq!(announce_data, expected_announce_data); } + #[tokio::test] + async fn it_should_prefer_a_query_string_peer_ip_over_the_x_forwarded_for_ip_when_overrides_are_enabled() { + // Arrange + let (core_tracker_services, mut core_http_tracker_services) = + initialize_core_tracker_services_with_config(&configuration::ephemeral_with_reverse_proxy()).await; + let server_service_binding = + ServiceBinding::new(Protocol::HTTP, SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 7070)).unwrap(); + let query_string_peer_ip = "198.51.100.42".parse().unwrap(); + let x_forwarded_for_ip = "203.0.113.195".parse().unwrap(); + let peer = sample_peer(); + let peer_port = peer.peer_addr.port(); + let (announce_request, _) = sample_announce_request_for_peer(peer); + let announce_request = Announce { + ip: PeerIp::Literal(query_string_peer_ip), + ..announce_request + }; + let client_ip_sources = ClientIpSources { + right_most_x_forwarded_for: Some(x_forwarded_for_ip), + connection_info_socket_address: Some(SocketAddr::new("192.0.2.10".parse().unwrap(), 8080)), + }; + + let mut http_stats_event_sender_mock = MockHttpStatsEventSender::new(); + http_stats_event_sender_mock + .expect_send() + .with(predicate::function(move |event| { + let mut announcement = peer; + announcement.peer_addr = SocketAddr::new(query_string_peer_ip, peer_port); + + let expected_event = Event::TcpAnnounce { + connection: ConnectionContext::new( + RemoteClientAddr::new(ResolvedIp::FromXForwardedFor(x_forwarded_for_ip), Some(8080)), + server_service_binding.clone(), + ), + info_hash: sample_info_hash(), + announcement, + }; + + announce_events_match(event, &expected_event) + })) + .times(1) + .returning(|_| Box::pin(future::ready(Some(Ok(1))))); + core_http_tracker_services.http_stats_event_sender = Some(Arc::new(http_stats_event_sender_mock)); + + let announce_service = AnnounceService::new_with_peer_ip_selection_policy( + core_tracker_services.core_config, + core_tracker_services.announce_handler, + core_tracker_services.authentication_service, + core_tracker_services.whitelist_authorization, + core_http_tracker_services.http_stats_event_sender, + PeerIpSelectionPolicy::enabled(), + ); + + // Act + let result = announce_service + .handle_announce( + &announce_request, + &client_ip_sources, + &ServiceBinding::new(Protocol::HTTP, SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 7070)).unwrap(), + None, + ) + .await; + + // Assert + assert!(result.is_ok()); + } + #[tokio::test] async fn it_should_send_the_tcp_4_announce_event_when_the_peer_uses_ipv4() { let server_socket_addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 7070); diff --git a/packages/http-core/src/statistics/event/handler.rs b/packages/http-core/src/statistics/event/handler.rs index 96dfc3cc4..f25564411 100644 --- a/packages/http-core/src/statistics/event/handler.rs +++ b/packages/http-core/src/statistics/event/handler.rs @@ -5,8 +5,8 @@ use torrust_metrics::label::{LabelSet, LabelValue}; use torrust_metrics::{label_name, metric_name}; use crate::event::Event; -use crate::statistics::HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL; use crate::statistics::repository::Repository; +use crate::statistics::{HTTP_TRACKER_CORE_ANNOUNCE_PEER_IP_REJECTIONS_TOTAL, HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL}; pub async fn handle_event(event: Event, stats_repository: &Arc, now: DurationSinceUnixEpoch) { match event { @@ -44,6 +44,22 @@ pub async fn handle_event(event: Event, stats_repository: &Arc, now: Err(err) => tracing::error!("Failed to increase the counter: {}", err), } } + Event::TcpAnnouncePeerIpRejected { connection, reason } => { + let mut label_set = LabelSet::from(connection); + label_set.upsert(label_name!("reason"), LabelValue::new(reason.as_str())); + + match stats_repository + .increase_counter( + &metric_name!(HTTP_TRACKER_CORE_ANNOUNCE_PEER_IP_REJECTIONS_TOTAL), + &label_set, + now, + ) + .await + { + Ok(()) => tracing::debug!(reason = reason.as_str(), "Recorded rejected HTTP announce peer IP parameter"), + Err(err) => tracing::error!("Failed to increase the counter: {}", err), + } + } } tracing::debug!("stats: {:?}", stats_repository.get_stats().await); @@ -59,7 +75,7 @@ mod tests { use torrust_tracker_http_protocol::v1::services::peer_ip_resolver::{RemoteClientAddr, ResolvedIp}; use crate::CurrentClock; - use crate::event::{ConnectionContext, Event}; + use crate::event::{ConnectionContext, Event, PeerIpRejectionReason}; use crate::statistics::event::handler::handle_event; use crate::statistics::repository::Repository; use crate::tests::{sample_info_hash, sample_peer_using_ipv4, sample_peer_using_ipv6}; @@ -163,4 +179,30 @@ mod tests { assert_eq!(stats.tcp6_scrapes_handled(), 1); } + + #[tokio::test] + async fn it_should_increase_the_peer_ip_rejection_counter_for_the_rejection_reason() { + // Arrange + let stats_repository = Arc::new(Repository::new()); + let reason = PeerIpRejectionReason::DnsNameUnsupported; + + // Act + handle_event( + Event::TcpAnnouncePeerIpRejected { + connection: ConnectionContext::new( + RemoteClientAddr::new(ResolvedIp::FromSocketAddr(IpAddr::V4(Ipv4Addr::LOCALHOST)), Some(8080)), + ServiceBinding::new(Protocol::HTTP, SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 7070)).unwrap(), + ), + reason, + }, + &stats_repository, + CurrentClock::now(), + ) + .await; + + // Assert + let stats = stats_repository.get_stats().await; + assert_eq!(stats.announce_peer_ip_rejections_total(reason.as_str()), 1); + assert_eq!(stats.announce_peer_ip_rejections_total("invalid_ip_address"), 0); + } } diff --git a/packages/http-core/src/statistics/metrics.rs b/packages/http-core/src/statistics/metrics.rs index acb67d4bf..03e206b09 100644 --- a/packages/http-core/src/statistics/metrics.rs +++ b/packages/http-core/src/statistics/metrics.rs @@ -6,7 +6,7 @@ use torrust_metrics::metric_collection::aggregate::sum::Sum; use torrust_metrics::metric_collection::{Error, MetricCollection}; use torrust_metrics::metric_name; -use crate::statistics::HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL; +use crate::statistics::{HTTP_TRACKER_CORE_ANNOUNCE_PEER_IP_REJECTIONS_TOTAL, HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL}; /// Metrics collected by the tracker. #[derive(Debug, Clone, PartialEq, Default, Serialize)] @@ -94,4 +94,17 @@ impl Metrics { ) .unwrap_or_default() as u64 } + + /// Total rejected HTTP announce `ip` parameters for a bounded rejection reason. + #[must_use] + #[allow(clippy::cast_sign_loss)] + #[allow(clippy::cast_possible_truncation)] + pub fn announce_peer_ip_rejections_total(&self, reason: &str) -> u64 { + self.metric_collection + .sum( + &metric_name!(HTTP_TRACKER_CORE_ANNOUNCE_PEER_IP_REJECTIONS_TOTAL), + &[("reason", reason)].into(), + ) + .unwrap_or_default() as u64 + } } diff --git a/packages/http-core/src/statistics/mod.rs b/packages/http-core/src/statistics/mod.rs index 741d8489a..d8fedbc48 100644 --- a/packages/http-core/src/statistics/mod.rs +++ b/packages/http-core/src/statistics/mod.rs @@ -8,6 +8,7 @@ use torrust_metrics::metric_name; use torrust_metrics::unit::Unit; pub const HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL: &str = "http_tracker_core_requests_received_total"; +pub const HTTP_TRACKER_CORE_ANNOUNCE_PEER_IP_REJECTIONS_TOTAL: &str = "http_tracker_core_announce_peer_ip_rejections_total"; #[must_use] pub fn describe_metrics() -> Metrics { @@ -18,6 +19,11 @@ pub fn describe_metrics() -> Metrics { Some(Unit::Count), Some(MetricDescription::new("Total number of HTTP requests received")), ); + metrics.metric_collection.describe_counter( + &metric_name!(HTTP_TRACKER_CORE_ANNOUNCE_PEER_IP_REJECTIONS_TOTAL), + Some(Unit::Count), + Some(MetricDescription::new("Total rejected HTTP announce peer IP parameters")), + ); metrics } diff --git a/packages/http-protocol/src/v1/requests/announce.rs b/packages/http-protocol/src/v1/requests/announce.rs index 4f91e1ca1..5e4ea60f8 100644 --- a/packages/http-protocol/src/v1/requests/announce.rs +++ b/packages/http-protocol/src/v1/requests/announce.rs @@ -46,43 +46,90 @@ impl NumberOfBytes { } } +/// Raw state of the optional BEP 3 `ip` parameter. +/// +/// This preserves the distinction between an absent parameter, `ip=`, an IP +/// literal, a DNS name, and another non-empty invalid value for service-level +/// policy enforcement. +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum PeerIp { + /// The request did not include an `ip` parameter. + Absent, + /// The request included `ip=`. + Empty, + /// The request included an IPv4 or IPv6 literal. + Literal(IpAddr), + /// The request included a DNS name. DNS resolution is deliberately unsupported. + DnsName, + /// The request included a non-empty value that is neither an IP literal nor a DNS name. + Invalid, +} + +impl PeerIp { + /// Classifies a raw query value after strict percent-decoding. + /// + /// This is public because [`Announce::ip`] is public. Consumers that + /// construct requests manually must use this method so malformed encoding + /// is not silently treated as an invalid address. + /// + /// # Errors + /// + /// Returns an error when `value` contains malformed percent encoding or + /// bytes that are not valid UTF-8. + pub fn from_raw(value: Option) -> Result { + match value { + None => Ok(Self::Absent), + Some(value) if value.is_empty() => Ok(Self::Empty), + Some(value) => { + let value = percent_decode_ip_parameter(&value)?; + + Ok(match IpAddr::from_str(&value) { + Ok(ip) => Self::Literal(ip), + Err(_) if is_dns_name(&value) => Self::DnsName, + Err(_) => Self::Invalid, + }) + } + } + } +} + +fn is_dns_name(value: &str) -> bool { + value.bytes().any(|byte| byte.is_ascii_alphabetic()) + && value.split('.').all(|label| { + !label.is_empty() + && !label.starts_with('-') + && !label.ends_with('-') + && label.bytes().all(|byte| byte.is_ascii_alphanumeric() || byte == b'-') + }) +} + +fn percent_decode_ip_parameter(value: &str) -> Result { + let bytes = value.as_bytes(); + let mut index = 0; + + while index < bytes.len() { + if bytes[index] == b'%' { + if index + 2 >= bytes.len() || !bytes[index + 1].is_ascii_hexdigit() || !bytes[index + 2].is_ascii_hexdigit() { + return Err(ParseAnnounceQueryError::MalformedIpPercentEncoding); + } + index += 3; + } else { + index += 1; + } + } + + percent_encoding::percent_decode_str(value) + .decode_utf8() + .map(std::borrow::Cow::into_owned) + .map_err(|_| ParseAnnounceQueryError::MalformedIpPercentEncoding) +} + /// The `Announce` request. Fields use protocol-local types after parsing the /// query params of the request; boundary layers map them to domain types. /// -/// This type is used for both server-side parsing and client-side construction: -/// -/// - **Server-side**: Parsed from incoming HTTP query strings via `TryFrom`. -/// - **Client-side**: Built via `AnnounceBuilder` and serialized to a URL query -/// string via `Display`. -/// -/// ```rust -/// use torrust_tracker_http_protocol::v1::requests::announce::{Announce, Compact, Event}; -/// use torrust_info_hash::InfoHash; -/// use torrust_peer_id::PeerId; -/// use torrust_tracker_http_protocol::v1::requests::announce::NumberOfBytes; -/// -/// let request = Announce { -/// // Mandatory params -/// info_hash: "3b245504cf5f11bbdbe1201cea6a6bf45aee1bc0".parse::().unwrap(), -/// peer_id: PeerId(*b"-RC3000-000000000001"), -/// port: 17548, -/// // Optional params -/// ip: None, -/// downloaded: Some(NumberOfBytes::new(1)), -/// uploaded: Some(NumberOfBytes::new(1)), -/// left: Some(NumberOfBytes::new(1)), -/// event: Some(Event::Started), -/// compact: Some(Compact::NotAccepted), -/// numwant: Some(50) -/// }; -/// ``` -/// -/// > **NOTICE**: The [BEP 03. The `BitTorrent` Protocol Specification](https://www.bittorrent.org/beps/bep_0003.html) -/// > specifies that only the peer `IP` and `event` are optional. However, the -/// > tracker defines default values for some of the mandatory params. -/// -/// > **NOTICE**: The struct contains `ip` as per BEP 3. The tracker -/// > implementation may choose to use it or derive the IP from the connection. +/// This type is used both for server-side parsing and client-side construction. +/// The `ip` field preserves its raw semantic state so service policy can make a +/// client-visible decision without silently ignoring non-empty values. #[derive(Clone, Debug, PartialEq)] pub struct Announce { // Mandatory params @@ -96,8 +143,8 @@ pub struct Announce { pub port: u16, // Optional params - /// The peer IP address (BEP 3 `ip` parameter). - pub ip: Option, + /// The raw-state-preserving peer IP parameter (BEP 3 `ip`). + pub ip: PeerIp, /// The number of bytes downloaded by the peer. pub downloaded: Option, @@ -163,6 +210,9 @@ pub enum ParseAnnounceQueryError { param_value: String, source: LocatedError<'static, PeerIdConversionError>, }, + /// The `ip` parameter contains malformed percent encoding or invalid UTF-8. + #[error("malformed percent encoding for ip")] + MalformedIpPercentEncoding, } /// The event that the peer is reporting: `started`, `completed` or `stopped`. @@ -291,7 +341,7 @@ impl TryFrom for Announce { event: extract_event(&query)?, compact: extract_compact(&query)?, numwant: extract_numwant(&query)?, - ip: extract_ip(&query), + ip: extract_ip(&query)?, }) } } @@ -304,7 +354,7 @@ impl fmt::Display for Announce { params.push(("peer_id", percent_encode_byte_array(&self.peer_id.0))); params.push(("port", self.port.to_string())); - if let Some(ip) = &self.ip { + if let PeerIp::Literal(ip) = &self.ip { params.push((IP, ip.to_string())); } if let Some(downloaded) = self.downloaded { @@ -376,7 +426,7 @@ impl AnnounceBuilder { info_hash: InfoHash::from_str("9c38422213e30bff212b30c360d26f9a02136422").unwrap(), // DevSkim: ignore DS173237 peer_id: PeerId(*b"-qB00000000000000001"), port: 17548, - ip: Some(IpAddr::V4(std::net::Ipv4Addr::new(192, 168, 1, 88))), + ip: PeerIp::Absent, downloaded: None, uploaded: None, left: None, @@ -409,7 +459,7 @@ impl AnnounceBuilder { #[must_use] pub fn with_ip(mut self, ip: IpAddr) -> Self { - self.announce.ip = Some(ip); + self.announce.ip = PeerIp::Literal(ip); self } @@ -563,11 +613,8 @@ fn extract_number_of_bytes_from_param(param_name: &str, query: &Query) -> Result } } -fn extract_ip(query: &Query) -> Option { - match query.get_param(IP) { - Some(raw_param) => IpAddr::from_str(&raw_param).ok(), - None => None, - } +fn extract_ip(query: &Query) -> Result { + PeerIp::from_raw(query.get_param(IP)) } fn extract_event(query: &Query) -> Result, ParseAnnounceQueryError> { @@ -608,8 +655,8 @@ mod tests { use crate::v1::query::Query; use crate::v1::requests::announce::{ - Announce, COMPACT, Compact, DOWNLOADED, EVENT, Event, INFO_HASH, LEFT, NUMWANT, NumberOfBytes, PEER_ID, PORT, - UPLOADED, + Announce, COMPACT, Compact, DOWNLOADED, EVENT, Event, INFO_HASH, IP, LEFT, NUMWANT, NumberOfBytes, PEER_ID, PORT, + PeerIp, UPLOADED, }; #[test] @@ -631,7 +678,7 @@ mod tests { info_hash: "3b245504cf5f11bbdbe1201cea6a6bf45aee1bc0".parse::().unwrap(), // DevSkim: ignore DS173237 peer_id: PeerId(*b"-RC3000-000000000001"), port: 17548, - ip: None, + ip: PeerIp::Absent, downloaded: None, uploaded: None, left: None, @@ -667,7 +714,7 @@ mod tests { info_hash: "3b245504cf5f11bbdbe1201cea6a6bf45aee1bc0".parse::().unwrap(), // DevSkim: ignore DS173237 peer_id: PeerId(*b"-RC3000-000000000001"), port: 17548, - ip: None, + ip: PeerIp::Absent, downloaded: Some(NumberOfBytes::new(1)), uploaded: Some(NumberOfBytes::new(2)), left: Some(NumberOfBytes::new(3)), @@ -678,6 +725,55 @@ mod tests { ); } + #[test] + fn it_should_preserve_all_peer_ip_parameter_states() { + // Arrange + let mandatory_params = vec![ + (INFO_HASH, "%3B%24U%04%CF%5F%11%BB%DB%E1%20%1C%EAjk%F4Z%EE%1B%C0"), + (PEER_ID, "-RC3000-000000000001"), + (PORT, "17548"), + ]; + + // Act / Assert + for (ip, expected) in [ + (None, PeerIp::Absent), + (Some(""), PeerIp::Empty), + (Some("192.0.2.1"), PeerIp::Literal("192.0.2.1".parse().unwrap())), + (Some("2001%3Adb8%3A%3A1"), PeerIp::Literal("2001:db8::1".parse().unwrap())), + (Some("localhost"), PeerIp::DnsName), + (Some("tracker"), PeerIp::DnsName), + (Some("example.com"), PeerIp::DnsName), + (Some("999.999.999.999"), PeerIp::Invalid), + (Some("invalid_ip"), PeerIp::Invalid), + ] { + let mut params = mandatory_params.clone(); + if let Some(ip) = ip { + params.push((IP, ip)); + } + + let announce = Announce::try_from(Query::from(params)).unwrap(); + + assert_eq!(announce.ip, expected); + } + } + + #[test] + fn it_should_reject_malformed_percent_encoding_in_the_peer_ip_parameter() { + // Arrange + let raw_query = format!( + "{INFO_HASH}=%3B%24U%04%CF%5F%11%BB%DB%E1%20%1C%EAjk%F4Z%EE%1B%C0&{PEER_ID}=-RC3000-000000000001&{PORT}=17548&{IP}=%ZZ" + ); + + // Act + let error = Announce::try_from(raw_query.parse::().unwrap()).unwrap_err(); + + // Assert + assert!(matches!( + error, + crate::v1::requests::announce::ParseAnnounceQueryError::MalformedIpPercentEncoding + )); + } + mod when_it_is_instantiated_from_the_url_query_params { use crate::v1::query::Query; diff --git a/packages/test-helpers/src/http.rs b/packages/test-helpers/src/http.rs index 72553a002..ee15c6163 100644 --- a/packages/test-helpers/src/http.rs +++ b/packages/test-helpers/src/http.rs @@ -3,7 +3,7 @@ use std::time::Duration; use torrust_tracker_client::http::client::Client; -use torrust_tracker_http_protocol::v1::requests::announce::{Announce, Event}; +use torrust_tracker_http_protocol::v1::requests::announce::{Announce, Event, PeerIp}; use url::Url; /// Sends an HTTP announce to the given tracker URL. @@ -18,7 +18,7 @@ pub async fn http_announce(tracker_url: &Url, info_hash: &[u8; 20], peer_id: &[u info_hash: torrust_info_hash::InfoHash(*info_hash), peer_id: torrust_peer_id::PeerId(*peer_id), port, - ip: None, + ip: PeerIp::Absent, downloaded: None, uploaded: None, left: None, diff --git a/tests/scaffold.rs b/tests/scaffold.rs index 2693b5ea2..58100928e 100644 --- a/tests/scaffold.rs +++ b/tests/scaffold.rs @@ -116,7 +116,7 @@ async fn the_stats_api_endpoint_should_aggregate_announces_across_multiple_track let client = reqwest::Client::new(); for url in &tracker_urls { let announce_url = url - .join("/announce?info_hash=%9c8b%22%13%e3%0b%ff%21%2b0%c3%60%d2o%9a%02%13d%22&peer_id=-qB00000000000000001&port=17548&ip=127.0.0.1&event=started&compact=0") + .join("/announce?info_hash=%9c8b%22%13%e3%0b%ff%21%2b0%c3%60%d2o%9a%02%13d%22&peer_id=-qB00000000000000001&port=17548&event=started&compact=0") .expect("announce URL should be valid"); let resp = client.get(announce_url.as_str()).send().await.unwrap(); let status = resp.status(); From b2e6c800d13599540c6984ea1b554aa70bfad6b8 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 19 Aug 2026 10:52:31 +0100 Subject: [PATCH 3/7] docs(readme): remove IP query override roadmap item --- README.md | 1 - 1 file changed, 1 deletion(-) diff --git a/README.md b/README.md index a3ee7674d..1d5687140 100644 --- a/README.md +++ b/README.md @@ -41,7 +41,6 @@ Visit the [Torrust Demo repository][torrust-demo] to get started with your own t Core: -- [ ] New option `want_ip_from_query_string`. See . - [ ] Peer and torrents specific statistics. See . Persistence: From 6928864fd19a648a04a823ebc845056d1a004ebf Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 19 Aug 2026 11:14:54 +0100 Subject: [PATCH 4/7] fix(http): preserve empty announce IP parameters --- .../manual-verification.md | 22 +++---- .../receiving_an_announce_request.rs | 2 +- .../http-protocol/src/v1/requests/announce.rs | 63 ++++++++++++------- 3 files changed, 53 insertions(+), 34 deletions(-) diff --git a/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/manual-verification.md b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/manual-verification.md index 2c517dbc8..a4e3052bb 100644 --- a/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/manual-verification.md +++ b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/manual-verification.md @@ -108,17 +108,17 @@ It returned a successful JSON announce response whose peer list contains the con The same raw HTTP announce matrix from Phase 1 was run after rebuilding the tracker. Every response used HTTP 200, as required by the BitTorrent HTTP tracker failure-response convention; failed announces carry a bencoded `failure reason`. -| Case | Request form | Actual result | -| ----------------------------- | ---------------------- | ---------------------------------------------------------------------------------------------------------------------------- | -| Absent | No `ip` parameter | HTTP 200; bencoded announce success response | -| Empty | `ip=` | HTTP 200; bencoded announce success response | -| Valid IPv4 | `ip=1.2.3.4` | HTTP 200; `failure reason`: `Client-supplied peer IPs are disabled` | -| Valid encoded IPv6 | `ip=2001%3Adb8%3A%3A1` | HTTP 200; `failure reason`: `Client-supplied peer IPs are disabled` | -| DNS name | `ip=example.com` | HTTP 200; `failure reason`: `DNS names are not supported for the announce ip parameter` | -| Single-label DNS name | `ip=localhost` | HTTP 200; `failure reason`: `DNS names are not supported for the announce ip parameter` | -| Invalid value | `ip=invalid_ip` | HTTP 200; `failure reason`: `The announce ip parameter must be an IPv4 or IPv6 literal` | -| Invalid numeric IP-like value | `ip=999.999.999.999` | HTTP 200; `failure reason`: `The announce ip parameter must be an IPv4 or IPv6 literal` | -| Malformed encoding | `ip=%ZZ` | HTTP 200; `failure reason`: `Bad request. Cannot parse query params for announce request: malformed percent encoding for ip` | +| Case | Request form | Actual result | +| ----------------------------- | ---------------------- | --------------------------------------------------------------------------------------------------------------------------------------------- | +| Absent | No `ip` parameter | HTTP 200; bencoded announce success response | +| Empty | `ip=` | HTTP 200; bencoded announce success response | +| Valid IPv4 | `ip=1.2.3.4` | HTTP 200; `failure reason`: `Client-supplied peer IPs are disabled` | +| Valid encoded IPv6 | `ip=2001%3Adb8%3A%3A1` | HTTP 200; `failure reason`: `Client-supplied peer IPs are disabled` | +| DNS name | `ip=example.com` | HTTP 200; `failure reason`: `DNS names are not supported for the announce ip parameter` | +| Single-label DNS name | `ip=localhost` | HTTP 200; `failure reason`: `DNS names are not supported for the announce ip parameter` | +| Invalid value | `ip=invalid_ip` | HTTP 200; `failure reason`: `The announce ip parameter must be an IPv4 or IPv6 literal` | +| Invalid numeric IP-like value | `ip=999.999.999.999` | HTTP 200; `failure reason`: `The announce ip parameter must be an IPv4 or IPv6 literal` | +| Malformed encoding | `ip=%ZZ` | HTTP 200; `failure reason`: `Bad request. Cannot parse query params for announce request: malformed percent encoding or invalid UTF-8 for ip` | This verifies the intentional baseline change: absent and empty values remain successful, while every non-empty override is explicitly rejected until schema v3.0.0 can activate the opt-in policy. diff --git a/packages/axum-http-server/tests/server/v1/contract/for_all_config_modes/receiving_an_announce_request.rs b/packages/axum-http-server/tests/server/v1/contract/for_all_config_modes/receiving_an_announce_request.rs index 9b8d5c426..82c4e1628 100644 --- a/packages/axum-http-server/tests/server/v1/contract/for_all_config_modes/receiving_an_announce_request.rs +++ b/packages/axum-http-server/tests/server/v1/contract/for_all_config_modes/receiving_an_announce_request.rs @@ -279,7 +279,7 @@ async fn it_should_return_distinct_failure_reasons_for_non_literal_peer_ip_param ("999.999.999.999", "The announce ip parameter must be an IPv4 or IPv6 literal"), ( "%ZZ", - "Bad request. Cannot parse query params for announce request: malformed percent encoding for ip", + "Bad request. Cannot parse query params for announce request: malformed percent encoding or invalid UTF-8 for ip", ), ] { let response = Client::new(env.base_url(), Duration::from_secs(5)) diff --git a/packages/http-protocol/src/v1/requests/announce.rs b/packages/http-protocol/src/v1/requests/announce.rs index 5e4ea60f8..7b9034a43 100644 --- a/packages/http-protocol/src/v1/requests/announce.rs +++ b/packages/http-protocol/src/v1/requests/announce.rs @@ -110,7 +110,7 @@ fn percent_decode_ip_parameter(value: &str) -> Result= bytes.len() || !bytes[index + 1].is_ascii_hexdigit() || !bytes[index + 2].is_ascii_hexdigit() { - return Err(ParseAnnounceQueryError::MalformedIpPercentEncoding); + return Err(ParseAnnounceQueryError::MalformedIpEncoding); } index += 3; } else { @@ -121,7 +121,7 @@ fn percent_decode_ip_parameter(value: &str) -> Result, }, /// The `ip` parameter contains malformed percent encoding or invalid UTF-8. - #[error("malformed percent encoding for ip")] - MalformedIpPercentEncoding, + #[error("malformed percent encoding or invalid UTF-8 for ip")] + MalformedIpEncoding, } /// The event that the peer is reporting: `started`, `completed` or `stopped`. @@ -354,8 +354,10 @@ impl fmt::Display for Announce { params.push(("peer_id", percent_encode_byte_array(&self.peer_id.0))); params.push(("port", self.port.to_string())); - if let PeerIp::Literal(ip) = &self.ip { - params.push((IP, ip.to_string())); + match &self.ip { + PeerIp::Absent | PeerIp::DnsName | PeerIp::Invalid => {} + PeerIp::Empty => params.push((IP, String::new())), + PeerIp::Literal(ip) => params.push((IP, ip.to_string())), } if let Some(downloaded) = self.downloaded { params.push(("downloaded", downloaded.0.to_string())); @@ -655,8 +657,8 @@ mod tests { use crate::v1::query::Query; use crate::v1::requests::announce::{ - Announce, COMPACT, Compact, DOWNLOADED, EVENT, Event, INFO_HASH, IP, LEFT, NUMWANT, NumberOfBytes, PEER_ID, PORT, - PeerIp, UPLOADED, + Announce, AnnounceBuilder, COMPACT, Compact, DOWNLOADED, EVENT, Event, INFO_HASH, IP, LEFT, NUMWANT, NumberOfBytes, + PEER_ID, PORT, PeerIp, UPLOADED, }; #[test] @@ -689,6 +691,20 @@ mod tests { ); } + #[test] + fn should_serialize_an_empty_peer_ip_parameter() { + // Arrange + let mut announce = AnnounceBuilder::default().query(); + announce.ip = PeerIp::Empty; + + // Act + let query = announce.to_string(); + + // Assert + assert!(query.contains("ip=")); + assert_eq!(Announce::try_from(query.parse::().unwrap()).unwrap().ip, PeerIp::Empty); + } + #[test] fn should_be_instantiated_from_the_url_query_params() { let raw_query = Query::from(vec![ @@ -758,20 +774,23 @@ mod tests { } #[test] - fn it_should_reject_malformed_percent_encoding_in_the_peer_ip_parameter() { - // Arrange - let raw_query = format!( - "{INFO_HASH}=%3B%24U%04%CF%5F%11%BB%DB%E1%20%1C%EAjk%F4Z%EE%1B%C0&{PEER_ID}=-RC3000-000000000001&{PORT}=17548&{IP}=%ZZ" - ); - - // Act - let error = Announce::try_from(raw_query.parse::().unwrap()).unwrap_err(); - - // Assert - assert!(matches!( - error, - crate::v1::requests::announce::ParseAnnounceQueryError::MalformedIpPercentEncoding - )); + fn it_should_reject_malformed_encoding_or_invalid_utf8_in_the_peer_ip_parameter() { + for peer_ip in ["%ZZ", "%FF"] { + // Arrange + let raw_query = format!( + "{INFO_HASH}=%3B%24U%04%CF%5F%11%BB%DB%E1%20%1C%EAjk%F4Z%EE%1B%C0&{PEER_ID}=-RC3000-000000000001&{PORT}=17548&{IP}={peer_ip}" + ); + + // Act + let error = Announce::try_from(raw_query.parse::().unwrap()).unwrap_err(); + + // Assert + assert!(matches!( + error, + crate::v1::requests::announce::ParseAnnounceQueryError::MalformedIpEncoding + )); + assert_eq!(error.to_string(), "malformed percent encoding or invalid UTF-8 for ip"); + } } mod when_it_is_instantiated_from_the_url_query_params { From 3ed8eb650a2f9cfa4ed283eb9f48e6121391cdd5 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 19 Aug 2026 13:24:28 +0100 Subject: [PATCH 5/7] test(http): clarify announce IP policy scenarios --- .../ISSUE.md | 64 +++-- .../receiving_an_announce_request.rs | 261 +++++++++--------- packages/http-core/src/services/announce.rs | 63 +++++ 3 files changed, 234 insertions(+), 154 deletions(-) diff --git a/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md index 1b6d07027..8158991ac 100644 --- a/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md +++ b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md @@ -92,9 +92,16 @@ This makes the setting control whether a non-empty client-supplied peer IP overr Malformed query-string encoding remains a normal request-parsing failure. The tracker should provide the most specific failure reason it can reliably determine. -### Interaction with `on_reverse_proxy` +### Peer address precedence -When both `use_ip_from_query_string` and `on_reverse_proxy` are enabled, the query string `ip` takes precedence over the `X-Forwarded-For` header. This is because the operator explicitly opted into trusting the query string value. When `use_ip_from_query_string` is disabled (default), the existing `on_reverse_proxy` logic applies unchanged. The two settings are not mutually exclusive; the query string IP wins when both are active and a valid IP is provided. +The tracker resolves a normal peer address before applying the query-string override. The precedence for a valid non-empty `ip` parameter when `use_ip_from_query_string` is enabled is: + +1. The query-string `ip` literal. +2. The configured `external_ip` when the observed connection is loopback. +3. The rightmost `X-Forwarded-For` address when `on_reverse_proxy` is enabled. +4. The direct connection address. + +Thus, the query-string `ip` takes precedence over both `external_ip` and `X-Forwarded-For`. It is an explicit client override that an operator chose to trust by enabling the setting. A client that does not know its reachable address must omit `ip` or send `ip=`; that preserves the normal `external_ip`, reverse-proxy, or connection-address resolution. ### Security consideration @@ -139,7 +146,7 @@ Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. | T2 | DONE | Add the field to the per-HTTP-tracker configuration struct | Added `HttpTracker::use_ip_from_query_string`, defaulting to `false`, with security documentation. | | T3 | DONE | Preserve the raw `ip` parameter state in the HTTP protocol | Replaced lossy `Option` parsing with `PeerIp`, preserving absent, empty, literal, DNS-name, and invalid states. | | T4 | DONE | Inject the address-selection policy into the announce service | Production constructs an explicit disabled policy pending #1980; unit tests inject both policy values. | -| T5 | DONE | Validate and select the peer IP | Implemented strict failures, enabled literal selection, and query-IP precedence over the connection/reverse-proxy address. | +| T5 | DONE | Validate and select the peer IP | Implemented strict failures and enabled literal selection. A valid enabled query IP overrides `external_ip`, reverse-proxy, and connection-derived addresses; absent/empty values preserve normal resolution. | | T6 | DONE | Add rejected-parameter observability | Added a bounded-reason counter and rejection-specific debug event logging without raw `ip` parameter values. Existing request-URI logging is outside this issue's scope. | | T7 | DONE | Add exhaustive tests for validation and selection | Added protocol/service unit tests and HTTP contract coverage for raw states and failure responses. | | T8 | DONE | Update configuration documentation | Documented the v3 field and staged activation. The active v2 default config is intentionally unchanged pending #1980. | @@ -176,6 +183,7 @@ Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. - 2026-08-18 00:00 UTC - Copilot/User - Required a three-phase local manual verification record: baseline behavior before implementation, disabled-policy behavior after implementation, and enabled-v3 behavior after #1980. The baseline documents the intentional change from silently ignoring non-empty `ip` values to rejecting them when overrides are disabled. - 2026-08-19 00:00 UTC - Copilot/User - Implemented the staged disabled-policy behavior, v3 schema field, strict raw `ip` parsing, bounded observability, automated coverage, and baseline/disabled local verification. Enabled-v3 manual verification remains blocked on #1980. - 2026-08-19 00:00 UTC - Copilot/User - Clarified observability scope: the new rejection event and metric use bounded reason data only; existing HTTP request-URI logging is out of scope. A tracker-wide logging privacy/diagnostic policy requires a separate issue and ADR decision. +- 2026-08-19 00:00 UTC - Copilot/User - Clarified future enabled-policy precedence: a valid query `ip` overrides loopback `external_ip`, `X-Forwarded-For`, and the direct connection address; absent or empty `ip` preserves normal address resolution. Grouped disabled-policy HTTP contract tests and reserved the enabled-policy group for #1980 runtime activation. ## Acceptance Criteria @@ -206,22 +214,22 @@ Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. The implementation must add automated coverage for every row in the parameter contract. Prefer unit tests at the validation and peer-address selection boundaries. Use contract/integration tests only for behavior that requires the HTTP transport boundary. -| ID | `ip` value | Setting | Expected outcome | Preferred test level | -| --- | ------------------------------------------ | -------- | ----------------------------------------------------------------------------------------------------- | -------------------------------------- | -| A1 | Raw state: absent | Disabled | Accept; use connection/reverse-proxy address | Protocol unit + service unit | -| A2 | Raw state: empty (`ip=`) | Disabled | Accept; treat as absent | Protocol unit + service unit | -| A3 | Valid IPv4 literal | Disabled | Reject with a disabled-override failure reason | Unit + HTTP contract response | -| A4 | Valid IPv6 literal | Disabled | Reject with a disabled-override failure reason | Unit + HTTP contract response | -| A5 | Raw state: DNS name | Disabled | Reject with a DNS-not-supported failure reason | Protocol unit + HTTP contract response | -| A6 | Raw state: invalid non-empty value | Disabled | Reject with an invalid-IP failure reason | Protocol unit + HTTP contract response | -| A7 | Raw state: absent | Enabled | Accept; use connection/reverse-proxy address | Protocol unit + service unit | -| A8 | Raw state: empty (`ip=`) | Enabled | Accept; treat as absent | Protocol unit + service unit | -| A9 | Valid IPv4 literal | Enabled | Accept; use supplied address | Unit | -| A10 | Valid IPv6 literal | Enabled | Accept; use supplied address | Unit | -| A11 | Raw state: DNS name | Enabled | Reject with a DNS-not-supported failure reason | Protocol unit + HTTP contract response | -| A12 | Raw state: invalid non-empty value | Enabled | Reject with an invalid-IP failure reason | Protocol unit + HTTP contract response | -| A13 | Valid IPv4/IPv6 literal with reverse proxy | Enabled | Accept; supplied address takes precedence over `X-Forwarded-For` | Unit + minimum integration coverage | -| A14 | Rejected value | Either | Increment the bounded-reason counter and emit a rejection-specific debug entry with no raw `ip` value | Unit | +| ID | `ip` value | Setting | Expected outcome | Preferred test level | +| --- | -------------------------------------------------------------------- | -------- | ----------------------------------------------------------------------------------------------------- | -------------------------------------- | +| A1 | Raw state: absent | Disabled | Accept; use connection/reverse-proxy address | Protocol unit + service unit | +| A2 | Raw state: empty (`ip=`) | Disabled | Accept; treat as absent | Protocol unit + service unit | +| A3 | Valid IPv4 literal | Disabled | Reject with a disabled-override failure reason | Unit + HTTP contract response | +| A4 | Valid IPv6 literal | Disabled | Reject with a disabled-override failure reason | Unit + HTTP contract response | +| A5 | Raw state: DNS name | Disabled | Reject with a DNS-not-supported failure reason | Protocol unit + HTTP contract response | +| A6 | Raw state: invalid non-empty value | Disabled | Reject with an invalid-IP failure reason | Protocol unit + HTTP contract response | +| A7 | Raw state: absent | Enabled | Accept; use connection/reverse-proxy address | Protocol unit + service unit | +| A8 | Raw state: empty (`ip=`) | Enabled | Accept; treat as absent | Protocol unit + service unit | +| A9 | Valid IPv4 literal | Enabled | Accept; use supplied address | Unit | +| A10 | Valid IPv6 literal | Enabled | Accept; use supplied address | Unit | +| A11 | Raw state: DNS name | Enabled | Reject with a DNS-not-supported failure reason | Protocol unit + HTTP contract response | +| A12 | Raw state: invalid non-empty value | Enabled | Reject with an invalid-IP failure reason | Protocol unit + HTTP contract response | +| A13 | Valid IPv4/IPv6 literal with reverse proxy or loopback `external_ip` | Enabled | Accept; supplied address takes precedence over `X-Forwarded-For` and `external_ip` | Unit + minimum integration coverage | +| A14 | Rejected value | Either | Increment the bounded-reason counter and emit a rejection-specific debug entry with no raw `ip` value | Unit | ### Manual Verification Scenarios @@ -234,14 +242,14 @@ Run the same applicable request matrix against a local tracker in three phases: - relevant client output and metric/debug-log evidence; - expected and actual results for every executed scenario. -| ID | Scenario | Command/Steps | Expected Result | Status | Evidence | -| --- | --------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------ | ---------------------------------------------------------------------------------------- | ----------- | --------------------------------------------------------------------------------------------------- | -| M1 | Default config: valid non-empty `ip` is rejected | Start tracker with default config; announce with `ip=1.2.3.4` | Announce fails, explaining that client-supplied peer IPs are disabled | DONE | `manual-verification.md` Phase 2 | -| M2 | Opt-in config: `ip` GET param is used | Enable `use_ip_from_query_string`; announce with `ip=1.2.3.4`; check the peer list | Peer is registered with `1.2.3.4` | TODO | | -| M3 | Opt-in config: absent or empty `ip` — fallback | Enable `use_ip_from_query_string`; announce without `ip` and with `ip=` | Peer is registered with the connection IP in both cases | TODO | | -| M4 | Opt-in + reverse proxy: `ip` param takes precedence | Enable both `use_ip_from_query_string` and `on_reverse_proxy`; announce with `ip=1.2.3.4` and `X-Forwarded-For: 5.6.7.8` | Peer is registered with `1.2.3.4` (query string wins) | TODO | | -| M5 | Non-empty invalid or DNS `ip` is rejected | Announce with enabled and disabled configurations using `ip=invalid_ip` and `ip=example.com` | Announce fails with the specific validation reason | IN_PROGRESS | Disabled-mode evidence complete in `manual-verification.md`; enabled-mode verification awaits #1980 | -| M6 | Rejected parameters are observable | Submit rejected non-empty `ip` parameters and inspect the rejection metric and rejection-specific debug log | Counter increments by bounded reason; rejection-specific debug log has no raw `ip` value | DONE | `manual-verification.md` Phase 2 | +| ID | Scenario | Command/Steps | Expected Result | Status | Evidence | +| --- | ---------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------- | ----------- | --------------------------------------------------------------------------------------------------- | +| M1 | Default config: valid non-empty `ip` is rejected | Start tracker with default config; announce with `ip=1.2.3.4` | Announce fails, explaining that client-supplied peer IPs are disabled | DONE | `manual-verification.md` Phase 2 | +| M2 | Opt-in config: `ip` GET param is used | Enable `use_ip_from_query_string`; announce with `ip=1.2.3.4`; check the peer list | Peer is registered with `1.2.3.4` | TODO | | +| M3 | Opt-in config: absent or empty `ip` — fallback | Enable `use_ip_from_query_string`; announce without `ip` and with `ip=` | Peer is registered with the connection IP in both cases | TODO | | +| M4 | Opt-in + resolved-address fallbacks: `ip` takes precedence | Enable `use_ip_from_query_string` with either `on_reverse_proxy` or loopback `external_ip`; announce with `ip=1.2.3.4` | Peer is registered with `1.2.3.4` (query string wins over `X-Forwarded-For` and `external_ip`) | TODO | | +| M5 | Non-empty invalid or DNS `ip` is rejected | Announce with enabled and disabled configurations using `ip=invalid_ip` and `ip=example.com` | Announce fails with the specific validation reason | IN_PROGRESS | Disabled-mode evidence complete in `manual-verification.md`; enabled-mode verification awaits #1980 | +| M6 | Rejected parameters are observable | Submit rejected non-empty `ip` parameters and inspect the rejection metric and rejection-specific debug log | Counter increments by bounded reason; rejection-specific debug log has no raw `ip` value | DONE | `manual-verification.md` Phase 2 | **Baseline expectation:** Before implementation, use M1–M5 as an address-selection request matrix. Valid, DNS-name, and invalid non-empty `ip` values are expected to be silently ignored and the announce is expected to succeed using the connection-derived address. Empty and absent values are expected to succeed. M6 is post-implementation only because its metric and debug log do not yet exist. @@ -268,7 +276,7 @@ Run the same applicable request matrix against a local tracker in three phases: - **IP spoofing**: When enabled, a client can register any IP address in the peer list. This is inherent to the feature and must be clearly documented. The opt-in default mitigates the risk for deployments that do not need this. - **Compatibility versus ambiguity**: This feature intentionally rejects non-empty `ip` overrides while disabled, rather than silently ignoring them. This makes configuration support transparent to clients, but is a documented HTTP announce compatibility change for 4.0.0. -- **Interaction with reverse proxy mode**: Resolved — when both `use_ip_from_query_string` and `on_reverse_proxy` are enabled, the query string `ip` takes precedence. See "Interaction with `on_reverse_proxy`" above for rationale. +- **Address-resolution interaction**: Resolved — when enabled, a valid query `ip` takes precedence over `external_ip`, reverse-proxy, and connection address resolution. See "Peer address precedence" above for rationale. - **IPv4/IPv6**: The `ip` parameter accepts both IPv4 and IPv6 addresses (via `IpAddr::from_str`). If the tracker is bound to an IPv6-only socket and a client sends an IPv4 `ip`, the address is accepted as-is — the tracker does not validate address family compatibility with the listener binding. ## References diff --git a/packages/axum-http-server/tests/server/v1/contract/for_all_config_modes/receiving_an_announce_request.rs b/packages/axum-http-server/tests/server/v1/contract/for_all_config_modes/receiving_an_announce_request.rs index 82c4e1628..7c4cd1ae0 100644 --- a/packages/axum-http-server/tests/server/v1/contract/for_all_config_modes/receiving_an_announce_request.rs +++ b/packages/axum-http-server/tests/server/v1/contract/for_all_config_modes/receiving_an_announce_request.rs @@ -988,146 +988,155 @@ async fn should_reject_a_valid_ipv4_peer_ip_when_overrides_are_disabled() { env.stop().await; } -#[tokio::test] -async fn when_the_client_ip_is_a_loopback_ipv4_it_should_assign_to_the_peer_ip_the_external_ip_without_an_ip_parameter() { - logging::setup(); - - /* We assume that both the client and tracker share the same public IP. - - client <-> tracker <-> Internet - 127.0.0.1 external_ip = "2.137.87.41" - */ - let cfg = configuration::ephemeral_with_external_ip(IpAddr::from_str("2.137.87.41").unwrap()); - let core_config = Arc::new(cfg.core.clone()); - let http_tracker_config = Arc::new(cfg.http_trackers.unwrap()[0].clone()); - let env = Started::new(&core_config, &http_tracker_config).await; - - let info_hash = InfoHash::from_str("9c38422213e30bff212b30c360d26f9a02136422").unwrap(); // DevSkim: ignore DS173237 - let loopback_ip = IpAddr::from_str("127.0.0.1").unwrap(); - let client_ip = loopback_ip; - - let announce_query = AnnounceBuilder::default().with_info_hash(&info_hash).query(); - - { - let client = Client::bind(env.base_url(), Duration::from_secs(5), client_ip).unwrap(); - let status = client.announce(&announce_query).await.unwrap().status(); +mod when_the_ip_parameter_is_not_accepted { + use super::*; + + // TODO(#1980, #1987): Add `when_the_ip_parameter_is_accepted` after schema + // v3.0.0 becomes runtime-active. Cover query-IP precedence over `external_ip` + // for loopback clients, absent/empty fallback to `external_ip`, and the + // remaining enabled-policy HTTP contract scenarios. + + #[tokio::test] + async fn a_loopback_ipv4_client_uses_the_external_ip_when_ip_is_absent() { + logging::setup(); + + /* We assume that both the client and tracker share the same public IP. + + client <-> tracker <-> Internet + 127.0.0.1 external_ip = "2.137.87.41" + */ + let cfg = configuration::ephemeral_with_external_ip(IpAddr::from_str("2.137.87.41").unwrap()); + let core_config = Arc::new(cfg.core.clone()); + let http_tracker_config = Arc::new(cfg.http_trackers.unwrap()[0].clone()); + let env = Started::new(&core_config, &http_tracker_config).await; + + let info_hash = InfoHash::from_str("9c38422213e30bff212b30c360d26f9a02136422").unwrap(); // DevSkim: ignore DS173237 + let loopback_ip = IpAddr::from_str("127.0.0.1").unwrap(); + let client_ip = loopback_ip; + + let announce_query = AnnounceBuilder::default().with_info_hash(&info_hash).query(); + + { + let client = Client::bind(env.base_url(), Duration::from_secs(5), client_ip).unwrap(); + let status = client.announce(&announce_query).await.unwrap().status(); + + assert_eq!(status, StatusCode::OK); + } + + let peers = env + .container + .tracker_core_container + .in_memory_torrent_repository + .get_torrent_peers(&info_hash, usize::MAX) + .await; + let peer_addr = peers[0].peer_addr; + + let ext_ip: IpAddr = env + .container + .tracker_core_container + .core_config + .net + .external_ip + .unwrap() + .into(); + assert_eq!(peer_addr.ip(), ext_ip); - assert_eq!(status, StatusCode::OK); + env.stop().await; } - let peers = env - .container - .tracker_core_container - .in_memory_torrent_repository - .get_torrent_peers(&info_hash, usize::MAX) - .await; - let peer_addr = peers[0].peer_addr; - - let ext_ip: IpAddr = env - .container - .tracker_core_container - .core_config - .net - .external_ip - .unwrap() - .into(); - assert_eq!(peer_addr.ip(), ext_ip); - - env.stop().await; -} - -#[tokio::test] -async fn when_the_client_ip_is_a_loopback_ipv6_it_should_assign_to_the_peer_ip_the_external_ip_without_an_ip_parameter() { - logging::setup(); - - /* We assume that both the client and tracker share the same public IP. - - client <-> tracker <-> Internet - ::1 external_ip = "2345:0425:2CA1:0000:0000:0567:5673:23b5" - */ - - let cfg = configuration::ephemeral_with_external_ip(IpAddr::from_str("2345:0425:2CA1:0000:0000:0567:5673:23b5").unwrap()); - let core_config = Arc::new(cfg.core.clone()); - let http_tracker_config = Arc::new(cfg.http_trackers.unwrap()[0].clone()); - let env = Started::new(&core_config, &http_tracker_config).await; - - let info_hash = InfoHash::from_str("9c38422213e30bff212b30c360d26f9a02136422").unwrap(); // DevSkim: ignore DS173237 - let loopback_ip = IpAddr::from_str("127.0.0.1").unwrap(); - let client_ip = loopback_ip; - - let announce_query = AnnounceBuilder::default().with_info_hash(&info_hash).query(); - - { - let client = Client::bind(env.base_url(), Duration::from_secs(5), client_ip).unwrap(); - let status = client.announce(&announce_query).await.unwrap().status(); + #[tokio::test] + async fn a_loopback_ipv6_client_uses_the_external_ip_when_ip_is_absent() { + logging::setup(); - assert_eq!(status, StatusCode::OK); - } + /* We assume that both the client and tracker share the same public IP. - let peers = env - .container - .tracker_core_container - .in_memory_torrent_repository - .get_torrent_peers(&info_hash, usize::MAX) - .await; - let peer_addr = peers[0].peer_addr; - - let ext_ip: IpAddr = env - .container - .tracker_core_container - .core_config - .net - .external_ip - .unwrap() - .into(); - assert_eq!(peer_addr.ip(), ext_ip); + client <-> tracker <-> Internet + ::1 external_ip = "2345:0425:2CA1:0000:0000:0567:5673:23b5" + */ - env.stop().await; -} + let cfg = configuration::ephemeral_with_external_ip(IpAddr::from_str("2345:0425:2CA1:0000:0000:0567:5673:23b5").unwrap()); + let core_config = Arc::new(cfg.core.clone()); + let http_tracker_config = Arc::new(cfg.http_trackers.unwrap()[0].clone()); + let env = Started::new(&core_config, &http_tracker_config).await; -#[tokio::test] -async fn when_the_tracker_is_behind_a_reverse_proxy_it_should_assign_to_the_peer_ip_the_ip_in_the_x_forwarded_for_http_header() { - logging::setup(); + let info_hash = InfoHash::from_str("9c38422213e30bff212b30c360d26f9a02136422").unwrap(); // DevSkim: ignore DS173237 + let loopback_ip = IpAddr::from_str("127.0.0.1").unwrap(); + let client_ip = loopback_ip; - /* - client <-> http proxy <-> tracker <-> Internet - ip: header: config: peer addr: - 145.254.214.256 X-Forwarded-For = 145.254.214.256 on_reverse_proxy = true 145.254.214.256 - */ + let announce_query = AnnounceBuilder::default().with_info_hash(&info_hash).query(); - let cfg = configuration::ephemeral_with_reverse_proxy(); - let core_config = Arc::new(cfg.core.clone()); - let http_tracker_config = Arc::new(cfg.http_trackers.unwrap()[0].clone()); - let env = Started::new(&core_config, &http_tracker_config).await; + { + let client = Client::bind(env.base_url(), Duration::from_secs(5), client_ip).unwrap(); + let status = client.announce(&announce_query).await.unwrap().status(); - let info_hash = InfoHash::from_str("9c38422213e30bff212b30c360d26f9a02136422").unwrap(); // DevSkim: ignore DS173237 + assert_eq!(status, StatusCode::OK); + } - let announce_query = AnnounceBuilder::default().with_info_hash(&info_hash).query(); + let peers = env + .container + .tracker_core_container + .in_memory_torrent_repository + .get_torrent_peers(&info_hash, usize::MAX) + .await; + let peer_addr = peers[0].peer_addr; - { - let client = Client::new(env.base_url(), Duration::from_secs(5)).unwrap(); - let status = client - .announce_with_header( - &announce_query, - "X-Forwarded-For", - "203.0.113.195,2001:db8:85a3:8d3:1319:8a2e:370:7348,150.172.238.178", - ) - .await + let ext_ip: IpAddr = env + .container + .tracker_core_container + .core_config + .net + .external_ip .unwrap() - .status(); + .into(); + assert_eq!(peer_addr.ip(), ext_ip); - assert_eq!(status, StatusCode::OK); + env.stop().await; } - let peers = env - .container - .tracker_core_container - .in_memory_torrent_repository - .get_torrent_peers(&info_hash, usize::MAX) - .await; - let peer_addr = peers[0].peer_addr; - - assert_eq!(peer_addr.ip(), IpAddr::from_str("150.172.238.178").unwrap()); - - env.stop().await; + #[tokio::test] + async fn a_reverse_proxy_client_uses_the_x_forwarded_for_ip_when_ip_is_absent() { + logging::setup(); + + /* + client <-> http proxy <-> tracker <-> Internet + ip: header: config: peer addr: + 145.254.214.256 X-Forwarded-For = 145.254.214.256 on_reverse_proxy = true 145.254.214.256 + */ + + let cfg = configuration::ephemeral_with_reverse_proxy(); + let core_config = Arc::new(cfg.core.clone()); + let http_tracker_config = Arc::new(cfg.http_trackers.unwrap()[0].clone()); + let env = Started::new(&core_config, &http_tracker_config).await; + + let info_hash = InfoHash::from_str("9c38422213e30bff212b30c360d26f9a02136422").unwrap(); // DevSkim: ignore DS173237 + + let announce_query = AnnounceBuilder::default().with_info_hash(&info_hash).query(); + + { + let client = Client::new(env.base_url(), Duration::from_secs(5)).unwrap(); + let status = client + .announce_with_header( + &announce_query, + "X-Forwarded-For", + "203.0.113.195,2001:db8:85a3:8d3:1319:8a2e:370:7348,150.172.238.178", + ) + .await + .unwrap() + .status(); + + assert_eq!(status, StatusCode::OK); + } + + let peers = env + .container + .tracker_core_container + .in_memory_torrent_repository + .get_torrent_peers(&info_hash, usize::MAX) + .await; + let peer_addr = peers[0].peer_addr; + + assert_eq!(peer_addr.ip(), IpAddr::from_str("150.172.238.178").unwrap()); + + env.stop().await; + } } diff --git a/packages/http-core/src/services/announce.rs b/packages/http-core/src/services/announce.rs index af4ddb471..1935aefd1 100644 --- a/packages/http-core/src/services/announce.rs +++ b/packages/http-core/src/services/announce.rs @@ -681,6 +681,69 @@ mod tests { assert!(result.is_ok()); } + #[tokio::test] + async fn it_should_prefer_a_query_string_peer_ip_over_the_external_ip_for_a_loopback_client_when_overrides_are_enabled() { + // Arrange + let external_ip = "203.0.113.195".parse().unwrap(); + let query_string_peer_ip = "198.51.100.42".parse().unwrap(); + let configuration = configuration::ephemeral_with_external_ip(external_ip); + let (core_tracker_services, mut core_http_tracker_services) = + initialize_core_tracker_services_with_config(&configuration).await; + let server_service_binding = + ServiceBinding::new(Protocol::HTTP, SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 7070)).unwrap(); + let server_service_binding_for_event = server_service_binding.clone(); + let peer = sample_peer(); + let peer_port = peer.peer_addr.port(); + let (announce_request, _) = sample_announce_request_for_peer(peer); + let announce_request = Announce { + ip: PeerIp::Literal(query_string_peer_ip), + ..announce_request + }; + let client_ip_sources = ClientIpSources { + right_most_x_forwarded_for: None, + connection_info_socket_address: Some(SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 8080)), + }; + + let mut http_stats_event_sender_mock = MockHttpStatsEventSender::new(); + http_stats_event_sender_mock + .expect_send() + .with(predicate::function(move |event| { + let mut announcement = peer; + announcement.peer_addr = SocketAddr::new(query_string_peer_ip, peer_port); + + let expected_event = Event::TcpAnnounce { + connection: ConnectionContext::new( + RemoteClientAddr::new(ResolvedIp::FromSocketAddr(IpAddr::V4(Ipv4Addr::LOCALHOST)), Some(8080)), + server_service_binding_for_event.clone(), + ), + info_hash: sample_info_hash(), + announcement, + }; + + announce_events_match(event, &expected_event) + })) + .times(1) + .returning(|_| Box::pin(future::ready(Some(Ok(1))))); + core_http_tracker_services.http_stats_event_sender = Some(Arc::new(http_stats_event_sender_mock)); + + let announce_service = AnnounceService::new_with_peer_ip_selection_policy( + core_tracker_services.core_config, + core_tracker_services.announce_handler, + core_tracker_services.authentication_service, + core_tracker_services.whitelist_authorization, + core_http_tracker_services.http_stats_event_sender, + PeerIpSelectionPolicy::enabled(), + ); + + // Act + let result = announce_service + .handle_announce(&announce_request, &client_ip_sources, &server_service_binding, None) + .await; + + // Assert + assert!(result.is_ok()); + } + #[tokio::test] async fn it_should_send_the_tcp_4_announce_event_when_the_peer_uses_ipv4() { let server_socket_addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 7070); From ab37b56aab9c40110d67a7eb0a87c846d21cd7bf Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 19 Aug 2026 13:32:24 +0100 Subject: [PATCH 6/7] test(http-protocol): cover IP parser helpers --- .../http-protocol/src/v1/requests/announce.rs | 33 ++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/packages/http-protocol/src/v1/requests/announce.rs b/packages/http-protocol/src/v1/requests/announce.rs index 7b9034a43..3d65fe5d2 100644 --- a/packages/http-protocol/src/v1/requests/announce.rs +++ b/packages/http-protocol/src/v1/requests/announce.rs @@ -658,9 +658,40 @@ mod tests { use crate::v1::query::Query; use crate::v1::requests::announce::{ Announce, AnnounceBuilder, COMPACT, Compact, DOWNLOADED, EVENT, Event, INFO_HASH, IP, LEFT, NUMWANT, NumberOfBytes, - PEER_ID, PORT, PeerIp, UPLOADED, + PEER_ID, PORT, PeerIp, UPLOADED, is_dns_name, percent_decode_ip_parameter, }; + #[test] + fn should_recognize_supported_dns_name_syntax() { + for value in ["localhost", "tracker", "example.com", "a-b.example"] { + assert!(is_dns_name(value), "{value}"); + } + } + + #[test] + fn should_reject_invalid_dns_name_syntax() { + for value in ["", "-example", "example-", "example..com", "example_com", "999.999.999.999"] { + assert!(!is_dns_name(value), "{value}"); + } + } + + #[test] + fn should_percent_decode_a_valid_peer_ip_parameter() { + for (encoded, decoded) in [("192.0.2.1", "192.0.2.1"), ("2001%3Adb8%3A%3A1", "2001:db8::1")] { + assert_eq!(percent_decode_ip_parameter(encoded).unwrap(), decoded); + } + } + + #[test] + fn should_reject_invalid_peer_ip_parameter_encoding() { + for value in ["%", "%ZZ", "%FF"] { + assert!(matches!( + percent_decode_ip_parameter(value), + Err(crate::v1::requests::announce::ParseAnnounceQueryError::MalformedIpEncoding) + )); + } + } + #[test] fn should_be_instantiated_from_the_url_query_with_only_the_mandatory_params() { let raw_query = Query::from(vec![ From d00bd1b2d5f4f673fd448cbdac4f0f714db8bf93 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Wed, 19 Aug 2026 15:50:12 +0100 Subject: [PATCH 7/7] refactor(events): defer rejected request observability --- ...260727000000_events_are_objective_facts.md | 2 + docs/events-architecture.md | 1 + docs/issues/drafts/generalize-error-events.md | 159 ++++++++++++++++++ .../ISSUE.md | 95 ++++++----- .../error-event-observability-analysis.md | 148 ++++++++++++++++ .../manual-verification.md | 33 +--- packages/http-core/src/event.rs | 31 +--- packages/http-core/src/services/announce.rs | 113 +++++-------- packages/http-core/src/services/scrape.rs | 6 +- .../http-core/src/statistics/event/handler.rs | 46 +---- packages/http-core/src/statistics/metrics.rs | 15 +- packages/http-core/src/statistics/mod.rs | 7 - .../http-protocol/src/v1/requests/announce.rs | 5 +- .../http-protocol/src/v1/requests/scrape.rs | 6 + packages/tracker-core/src/error.rs | 6 + packages/udp-core/src/event.rs | 6 +- packages/udp-server/src/error.rs | 4 + packages/udp-server/src/event.rs | 7 +- 18 files changed, 458 insertions(+), 232 deletions(-) create mode 100644 docs/issues/drafts/generalize-error-events.md create mode 100644 docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/error-event-observability-analysis.md diff --git a/docs/adrs/20260727000000_events_are_objective_facts.md b/docs/adrs/20260727000000_events_are_objective_facts.md index 83754e0a6..972eadc45 100644 --- a/docs/adrs/20260727000000_events_are_objective_facts.md +++ b/docs/adrs/20260727000000_events_are_objective_facts.md @@ -7,6 +7,8 @@ semantic-links: - packages/udp-server/src/event.rs - packages/http-core/src/event.rs - packages/swarm-coordination-registry/src/event.rs + - docs/issues/drafts/generalize-error-events.md + - docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/error-event-observability-analysis.md --- # Events Are Objective Facts diff --git a/docs/events-architecture.md b/docs/events-architecture.md index bcc017937..00a78f8b4 100644 --- a/docs/events-architecture.md +++ b/docs/events-architecture.md @@ -110,6 +110,7 @@ to metrics configuration. ## Related Work - Approved specification: [normalize per-instance event metrics policy](issues/open/2039-normalize-per-instance-event-metrics-policy/ISSUE.md) +- Deferred draft EPIC: [define and implement general error events](issues/drafts/generalize-error-events.md) - Bootstrap bug: [#2035][2035] - Runtime identity prerequisite: [#2036][2036] - Shared-services decision: [ADR 20260727180000](adrs/20260727180000_shared_services_across_tracker_instances.md) diff --git a/docs/issues/drafts/generalize-error-events.md b/docs/issues/drafts/generalize-error-events.md new file mode 100644 index 000000000..1265d743f --- /dev/null +++ b/docs/issues/drafts/generalize-error-events.md @@ -0,0 +1,159 @@ +--- +doc-type: epic +status: draft +github-issue: null +spec-path: docs/issues/drafts/generalize-error-events.md +epic-owner: null +last-updated-utc: 2026-08-19 00:00 +semantic-links: + skill-links: + - create-issue + related-artifacts: + - docs/adrs/20260727000000_events_are_objective_facts.md + - docs/events-architecture.md + - docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/error-event-observability-analysis.md + - packages/events/src/bus.rs + - packages/http-core/src/event.rs + - packages/http-core/src/services/announce.rs + - packages/http-core/src/services/scrape.rs + - packages/http-protocol/src/v1/requests/announce.rs + - packages/http-protocol/src/v1/requests/scrape.rs + - packages/tracker-core/src/error.rs + - packages/udp-core/src/event.rs + - packages/udp-server/src/event.rs + - packages/udp-server/src/error.rs +--- + + + +# EPIC #[To be assigned] - Define and Implement General Error Events + +## Goal + +Define a deliberate, stable, privacy-safe error-event contract and implement it +consistently for the tracker services and error paths that the approved design +includes. + +## Why This Is Needed + +The tracker event system decouples producers from metrics, banning, and future +consumers. Adding a one-off event merely to create a counter risks creating an +accidental public event API with incomplete coverage and unclear guarantees. + +Issue #1987 exposed this problem when an event and metric were proposed for a +rejected HTTP announce `ip` parameter. The event and metric were deliberately +removed under Option B. The strict protocol behavior remains, but this EPIC +records the cross-service design work required before similar error events are +introduced. + +## Scope + +### In Scope + +- Define the purpose, audience, compatibility guarantees, and coverage boundary + of error events. +- Define objective, bounded, consumer-safe error reason types rather than + exposing internal error enums or raw client-controlled values. +- Decide how parser/extractor failures, authentication and authorization + denials, service errors, and response-generation failures are represented. +- Audit current HTTP, UDP, tracker-core, and REST error paths against the agreed + boundary; implement events for every in-scope current case. +- Reconsider the rejected HTTP announce `ip` parameter once the general + contract is implemented. Its counter is only added if it follows from that + contract. +- Document source-level semantic links to the governing ADR, this EPIC, and + relevant decision analyses wherever event/error APIs are defined. + +### Out of Scope + +- Reintroducing a rejected-`ip` counter or event before the general contract is + designed and accepted. +- Direct metrics dependencies from request-handling services. +- Defining a new ADR or opening a GitHub issue before this draft is refined and + approved. + +## Subissues + +Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. + +| Order | Issue | Local Spec | Status | Notes | +| ----- | ---------------------------------------------------------------- | ----------- | ------ | -------------------------------------------------------------------------------------------------------------- | +| 1 | #[To be assigned] - Define the error-event contract | Not created | TODO | Establishes scope, reason stability, privacy, and compatibility rules; may require an ADR. | +| 2 | #[To be assigned] - Implement current in-scope error events | Not created | TODO | Audits and implements the contract across the agreed HTTP, UDP, tracker-core, and REST boundaries. | +| 3 | #[To be assigned] - Observe rejected HTTP announce IP parameters | Not created | TODO | Implement only if subissue 1 includes this outcome; expected to be delivered with subissue 2 where applicable. | + +## Delivery Strategy + +The EPIC is intentionally deferred. Before any implementation, refine the +service scope and create subissue 1. The implementation must follow the +approved contract; it must not add isolated event variants simply to support a +single metric. + +For each implementation subissue: + +1. Run `linter all`, relevant tests, and pre-push checks when applicable. +2. Run manual verification scenarios and record evidence. +3. Re-review acceptance criteria against observed behavior before completion. + +## Progress Tracking + +### Workflow Checkpoints + +- [x] Epic draft created in `docs/issues/drafts/` +- [ ] Epic draft reviewed and approved by user/maintainer +- [ ] GitHub epic issue created and issue number added to this spec +- [ ] Error-event contract subissue created and linked +- [ ] Current in-scope error-event implementation subissue created and linked +- [ ] Rejected-`ip` observability decision revisited under the approved contract +- [ ] Epic acceptance criteria reviewed and checked off +- [ ] Epic issue closed and spec moved to `docs/issues/closed/` + +### Progress Log + +- 2026-08-19 00:00 UTC - Maintainer decision - Created draft after selecting #1987 Option B; no implementation is planned yet. + +## Acceptance Criteria + +- [ ] The accepted contract states which services and rejection/error phases + emit events, including explicit exclusions. +- [ ] Event payloads expose only stable bounded reason types and minimum safe + context; raw client-controlled values and implementation error composition do + not become public payloads. +- [ ] The design states compatibility/versioning expectations for consumers. +- [ ] All current error paths within the accepted boundary emit the specified + objective events consistently. +- [ ] Metrics and other consumers remain decoupled from request handling. +- [ ] The rejected HTTP announce `ip` case is either implemented consistently + with the contract or explicitly deferred with a documented rationale. +- [ ] Every modified event/error API has semantic links to the governing design + documents. +- [ ] Automated and manual verification evidence is recorded for each + implementation subissue. + +### Acceptance Verification + +| AC ID | Status (`TODO`/`DONE`) | Evidence | +| ----- | ---------------------- | ---------------------------------- | +| AC1 | TODO | Approved design/ADR and subissue 1 | +| AC2 | TODO | Event payload and privacy review | +| AC3 | TODO | Contract compatibility section | +| AC4 | TODO | Per-service implementation tests | +| AC5 | TODO | Architecture and integration tests | +| AC6 | TODO | Subissue 2/3 decision record | +| AC7 | TODO | Source semantic-link review | +| AC8 | TODO | CI and manual verification records | + +## Risks and Trade-offs + +- "All errors" is too broad without a precise boundary. The contract must name + the included services and phases before implementation begins. +- Error enums often contain wrapped errors, dynamically formatted messages, or + raw client input. Reusing them directly would leak unstable or sensitive data. +- Existing UDP error events and consumers must remain compatible while the + contract is introduced or migrated. + +## References + +- Events architecture: `docs/events-architecture.md` +- Governing ADR: `docs/adrs/20260727000000_events_are_objective_facts.md` +- #1987 analysis: `docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/error-event-observability-analysis.md` diff --git a/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md index 8158991ac..5a053bc44 100644 --- a/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md +++ b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md @@ -11,7 +11,7 @@ depends-on: - docs/issues/open/1985-rename-peer-addr-to-ip-in-http-announce-request/ISSUE.md - docs/issues/open/1980-1978-configuration-overhaul-final-cleanup.md blocks: null -last-updated-utc: 2026-08-18 00:00 +last-updated-utc: 2026-08-19 00:00 semantic-links: skill-links: - create-issue @@ -23,6 +23,8 @@ semantic-links: - docs/issues/open/1640-1978-per-http-tracker-on-reverse-proxy-setting.md - evidence-opentracker-no-dns-support.md - evidence-chihaya-no-dns-support.md + - error-event-observability-analysis.md + - docs/issues/drafts/generalize-error-events.md --- # Issue #1987 - Add per-HTTP-tracker config option to use peer IP from `ip` GET parameter (sub-issue of #1978) @@ -107,9 +109,17 @@ Thus, the query-string `ip` takes precedence over both `external_ip` and `X-Forw Enabling this feature allows a remote client to claim any IP address in its announce request. The tracker would accept that address and include it in the peer list. This is a potential source of IP spoofing in the peer list. The feature must therefore be **opt-in**, disabled by default, and clearly documented as a trust-based setting — suitable only for private/controlled deployments, or as a workaround for peers behind symmetric NAT that cannot be reached via their connection IP. -### Rejection observability scope +### Rejection observability decision -This issue defines observability only for the new peer-IP rejection event and metric. Its debug log records a bounded reason code, not the raw `ip` parameter value. The rejection metric likewise uses bounded labels only. +The initially implemented peer-IP rejection event and metric were removed under +Option B after architectural review. This issue retains strict validation and +precise bencoded failure responses but does not add a dedicated aggregate +counter or rejection-specific event. + +The deferred [general error-events draft EPIC](../../drafts/generalize-error-events.md) +and [Error Event Observability Analysis](error-event-observability-analysis.md) +record the cross-service contract that must be defined before a similar event or +metric is introduced. Existing HTTP request logging, including its request-URI behavior, is outside this issue's scope. This issue does not establish a tracker-wide policy for redacting query parameters, client addresses, peer IDs, or other client-controlled request data. A cross-cutting request-log privacy and diagnostic policy requires a separate issue and, if adopted, an ADR. @@ -123,7 +133,7 @@ Do not add raw invalid values to the new rejection log merely because they are n - Accept an absent or empty `ip` GET parameter in both configuration modes, using the normal connection-derived address. - Reject a non-empty `ip` parameter that is invalid, is a DNS name, or is supplied while the option is disabled, with a precise protocol failure reason. - When the option is enabled and the `ip` GET parameter contains a valid IP address, use that IP as the peer's address instead of the connection IP. -- Record rejected `ip` parameters in an operator-visible bounded-reason counter and rejection-specific debug log without treating them as application errors or logging raw `ip` parameter values. +- Defer rejected-parameter events and metrics until the general error-event contract is designed; retain strict protocol failures and existing diagnostics. - Document the security implications of enabling this option in the configuration schema and in the module documentation. - Preserve the `ip` parameter's raw request state at the HTTP protocol boundary so absent, empty, valid literal, DNS-name, and invalid non-empty values remain distinguishable. - Add exhaustive tests for every raw-parameter validation and address-selection case. Prefer focused unit tests; add contract/integration tests only where HTTP boundary behavior cannot be validated by unit tests. @@ -147,14 +157,14 @@ Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. | T3 | DONE | Preserve the raw `ip` parameter state in the HTTP protocol | Replaced lossy `Option` parsing with `PeerIp`, preserving absent, empty, literal, DNS-name, and invalid states. | | T4 | DONE | Inject the address-selection policy into the announce service | Production constructs an explicit disabled policy pending #1980; unit tests inject both policy values. | | T5 | DONE | Validate and select the peer IP | Implemented strict failures and enabled literal selection. A valid enabled query IP overrides `external_ip`, reverse-proxy, and connection-derived addresses; absent/empty values preserve normal resolution. | -| T6 | DONE | Add rejected-parameter observability | Added a bounded-reason counter and rejection-specific debug event logging without raw `ip` parameter values. Existing request-URI logging is outside this issue's scope. | +| T6 | DONE | Decide rejected-parameter observability | Selected Option B: removed the #1987-specific event and metric; documented the deferred general error-events EPIC. | | T7 | DONE | Add exhaustive tests for validation and selection | Added protocol/service unit tests and HTTP contract coverage for raw states and failure responses. | | T8 | DONE | Update configuration documentation | Documented the v3 field and staged activation. The active v2 default config is intentionally unchanged pending #1980. | | T9 | DONE | Run `cargo test --workspace` — no regressions | Full workspace test suite passed on 2026-08-19 after updating the scaffold fixture to omit the now-disallowed non-empty `ip` override. | | T10 | DONE | Run `linter all` | Passed through the pre-commit gate on 2026-08-18. | | T11 | DONE | Update migration guide if this subissue affects the config public API | Updated `docs/issues/open/1978-configuration-overhaul-epic/configuration-v2-to-v3-migration.md`. | | T12 | DONE | Capture baseline behavior locally | Recorded in `manual-verification.md`. | -| T13 | DONE | Manually verify disabled behavior locally | Recorded successful fallback, strict failures, client response, metric, and safe debug evidence in `manual-verification.md`. | +| T13 | DONE | Manually verify disabled behavior locally | Recorded successful fallback, strict failures, and client response in `manual-verification.md`; rejection-specific observability was deferred under Option B. | | T14 | BLOCKED | Manually verify enabled behavior locally with active v3 configuration | After #1980 activates schema v3.0.0 at runtime, enable `use_ip_from_query_string` in a local per-HTTP-tracker config and run the enabled-mode scenarios with the local tracker and tracker client. Append reproducible evidence to `manual-verification.md`. | ## Progress Tracking @@ -181,9 +191,10 @@ Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. - 2026-08-18 00:00 UTC - Copilot/User - Required post-implementation manual verification against a local tracker using the local tracker client, with reproducible evidence retained in this issue directory. - 2026-08-18 00:00 UTC - Copilot/User - Chose staged delivery while v2 remains the active runtime schema: production wiring remains explicitly disabled; unit tests cover both policies; enabled-mode local manual verification is deferred until #1980 activates v3.0.0 configuration. - 2026-08-18 00:00 UTC - Copilot/User - Required a three-phase local manual verification record: baseline behavior before implementation, disabled-policy behavior after implementation, and enabled-v3 behavior after #1980. The baseline documents the intentional change from silently ignoring non-empty `ip` values to rejecting them when overrides are disabled. -- 2026-08-19 00:00 UTC - Copilot/User - Implemented the staged disabled-policy behavior, v3 schema field, strict raw `ip` parsing, bounded observability, automated coverage, and baseline/disabled local verification. Enabled-v3 manual verification remains blocked on #1980. -- 2026-08-19 00:00 UTC - Copilot/User - Clarified observability scope: the new rejection event and metric use bounded reason data only; existing HTTP request-URI logging is out of scope. A tracker-wide logging privacy/diagnostic policy requires a separate issue and ADR decision. +- 2026-08-19 00:00 UTC - Copilot/User - Implemented the staged disabled-policy behavior, v3 schema field, strict raw `ip` parsing, automated coverage, and baseline/disabled local verification. Enabled-v3 manual verification remains blocked on #1980. - 2026-08-19 00:00 UTC - Copilot/User - Clarified future enabled-policy precedence: a valid query `ip` overrides loopback `external_ip`, `X-Forwarded-For`, and the direct connection address; absent or empty `ip` preserves normal address resolution. Grouped disabled-policy HTTP contract tests and reserved the enabled-policy group for #1980 runtime activation. +- 2026-08-19 00:00 UTC - Copilot/User - Added error-event observability analysis to evaluate whether the #1987 rejection metric/event should remain or be deferred pending a cross-service event API design. +- 2026-08-19 00:00 UTC - Copilot/User - Selected Option B: removed the #1987-specific rejection event and metric, retained strict validation, and created a deferred draft EPIC for a cross-service error-event contract. ## Acceptance Criteria @@ -193,7 +204,7 @@ Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. - [x] AC4: The default configuration file (`share/default/`) has `use_ip_from_query_string` set to `false` (or omitted, defaulting to `false`). Evidence: v3 schema field defaults to `false`; active v2 default file intentionally remains unchanged pending #1980. - [x] AC5: The configuration schema documentation clearly states the security implications of enabling this option. - [x] AC6: Focused unit tests cover every `ip` parameter validation and address-selection case; minimum contract/integration tests verify HTTP failure responses and configuration wiring where unit tests cannot. -- [x] AC6a: A counter records rejected non-empty `ip` parameters using bounded reason labels, and the rejection-specific debug log records only a bounded reason without raw `ip` parameter values or application error logs. Existing request-URI logging is outside scope. Evidence: `manual-verification.md` Phase 2. +- [x] AC6a: The #1987-specific rejection event and counter are absent; strict rejection behavior remains. Any future error observability must follow the deferred general error-events contract. Evidence: `error-event-observability-analysis.md` and `docs/issues/drafts/generalize-error-events.md`. - [x] AC7: `linter all` exits with code `0`. Evidence: pre-commit gate passed on 2026-08-18. - [x] AC8: Relevant tests pass with no regressions. Evidence: `cargo +1.88.0 test --workspace` passed on 2026-08-19. - [x] AC9: Baseline manual verification runs a local tracker and local tracker client before implementation; reproducible commands, output, expected/actual results, and environment details are recorded in `manual-verification.md` in this issue directory. @@ -214,22 +225,21 @@ Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. The implementation must add automated coverage for every row in the parameter contract. Prefer unit tests at the validation and peer-address selection boundaries. Use contract/integration tests only for behavior that requires the HTTP transport boundary. -| ID | `ip` value | Setting | Expected outcome | Preferred test level | -| --- | -------------------------------------------------------------------- | -------- | ----------------------------------------------------------------------------------------------------- | -------------------------------------- | -| A1 | Raw state: absent | Disabled | Accept; use connection/reverse-proxy address | Protocol unit + service unit | -| A2 | Raw state: empty (`ip=`) | Disabled | Accept; treat as absent | Protocol unit + service unit | -| A3 | Valid IPv4 literal | Disabled | Reject with a disabled-override failure reason | Unit + HTTP contract response | -| A4 | Valid IPv6 literal | Disabled | Reject with a disabled-override failure reason | Unit + HTTP contract response | -| A5 | Raw state: DNS name | Disabled | Reject with a DNS-not-supported failure reason | Protocol unit + HTTP contract response | -| A6 | Raw state: invalid non-empty value | Disabled | Reject with an invalid-IP failure reason | Protocol unit + HTTP contract response | -| A7 | Raw state: absent | Enabled | Accept; use connection/reverse-proxy address | Protocol unit + service unit | -| A8 | Raw state: empty (`ip=`) | Enabled | Accept; treat as absent | Protocol unit + service unit | -| A9 | Valid IPv4 literal | Enabled | Accept; use supplied address | Unit | -| A10 | Valid IPv6 literal | Enabled | Accept; use supplied address | Unit | -| A11 | Raw state: DNS name | Enabled | Reject with a DNS-not-supported failure reason | Protocol unit + HTTP contract response | -| A12 | Raw state: invalid non-empty value | Enabled | Reject with an invalid-IP failure reason | Protocol unit + HTTP contract response | -| A13 | Valid IPv4/IPv6 literal with reverse proxy or loopback `external_ip` | Enabled | Accept; supplied address takes precedence over `X-Forwarded-For` and `external_ip` | Unit + minimum integration coverage | -| A14 | Rejected value | Either | Increment the bounded-reason counter and emit a rejection-specific debug entry with no raw `ip` value | Unit | +| ID | `ip` value | Setting | Expected outcome | Preferred test level | +| --- | -------------------------------------------------------------------- | -------- | ---------------------------------------------------------------------------------- | -------------------------------------- | +| A1 | Raw state: absent | Disabled | Accept; use connection/reverse-proxy address | Protocol unit + service unit | +| A2 | Raw state: empty (`ip=`) | Disabled | Accept; treat as absent | Protocol unit + service unit | +| A3 | Valid IPv4 literal | Disabled | Reject with a disabled-override failure reason | Unit + HTTP contract response | +| A4 | Valid IPv6 literal | Disabled | Reject with a disabled-override failure reason | Unit + HTTP contract response | +| A5 | Raw state: DNS name | Disabled | Reject with a DNS-not-supported failure reason | Protocol unit + HTTP contract response | +| A6 | Raw state: invalid non-empty value | Disabled | Reject with an invalid-IP failure reason | Protocol unit + HTTP contract response | +| A7 | Raw state: absent | Enabled | Accept; use connection/reverse-proxy address | Protocol unit + service unit | +| A8 | Raw state: empty (`ip=`) | Enabled | Accept; treat as absent | Protocol unit + service unit | +| A9 | Valid IPv4 literal | Enabled | Accept; use supplied address | Unit | +| A10 | Valid IPv6 literal | Enabled | Accept; use supplied address | Unit | +| A11 | Raw state: DNS name | Enabled | Reject with a DNS-not-supported failure reason | Protocol unit + HTTP contract response | +| A12 | Raw state: invalid non-empty value | Enabled | Reject with an invalid-IP failure reason | Protocol unit + HTTP contract response | +| A13 | Valid IPv4/IPv6 literal with reverse proxy or loopback `external_ip` | Enabled | Accept; supplied address takes precedence over `X-Forwarded-For` and `external_ip` | Unit + minimum integration coverage | ### Manual Verification Scenarios @@ -239,7 +249,7 @@ Run the same applicable request matrix against a local tracker in three phases: - date/time, commit SHA, OS, Rust toolchain, and effective local tracker configuration; - exact tracker and client commands, with sensitive values redacted; -- relevant client output and metric/debug-log evidence; +- relevant client output and diagnostics evidence; - expected and actual results for every executed scenario. | ID | Scenario | Command/Steps | Expected Result | Status | Evidence | @@ -249,28 +259,27 @@ Run the same applicable request matrix against a local tracker in three phases: | M3 | Opt-in config: absent or empty `ip` — fallback | Enable `use_ip_from_query_string`; announce without `ip` and with `ip=` | Peer is registered with the connection IP in both cases | TODO | | | M4 | Opt-in + resolved-address fallbacks: `ip` takes precedence | Enable `use_ip_from_query_string` with either `on_reverse_proxy` or loopback `external_ip`; announce with `ip=1.2.3.4` | Peer is registered with `1.2.3.4` (query string wins over `X-Forwarded-For` and `external_ip`) | TODO | | | M5 | Non-empty invalid or DNS `ip` is rejected | Announce with enabled and disabled configurations using `ip=invalid_ip` and `ip=example.com` | Announce fails with the specific validation reason | IN_PROGRESS | Disabled-mode evidence complete in `manual-verification.md`; enabled-mode verification awaits #1980 | -| M6 | Rejected parameters are observable | Submit rejected non-empty `ip` parameters and inspect the rejection metric and rejection-specific debug log | Counter increments by bounded reason; rejection-specific debug log has no raw `ip` value | DONE | `manual-verification.md` Phase 2 | -**Baseline expectation:** Before implementation, use M1–M5 as an address-selection request matrix. Valid, DNS-name, and invalid non-empty `ip` values are expected to be silently ignored and the announce is expected to succeed using the connection-derived address. Empty and absent values are expected to succeed. M6 is post-implementation only because its metric and debug log do not yet exist. +**Baseline expectation:** Before implementation, use M1–M5 as an address-selection request matrix. Valid, DNS-name, and invalid non-empty `ip` values are expected to be silently ignored and the announce is expected to succeed using the connection-derived address. Empty and absent values are expected to succeed. -**Post-implementation disabled-policy expectation:** M1, the disabled-mode portion of M5, and M6 apply. M2–M4 and the enabled-mode portion of M5 remain blocked until #1980 activates schema v3.0.0 configuration at runtime. Execute and document them under T14 once the setting can be enabled in the local tracker configuration. +**Post-implementation disabled-policy expectation:** M1 and the disabled-mode portion of M5 apply. M2–M4 and the enabled-mode portion of M5 remain blocked until #1980 activates schema v3.0.0 configuration at runtime. Execute and document them under T14 once the setting can be enabled in the local tracker configuration. ### Acceptance Verification -| AC ID | Status (`TODO`/`DONE`) | Evidence | -| ----- | ---------------------- | ------------------------------------------------------------------------------------- | -| AC1 | DONE | `manual-verification.md` Phase 2 | -| AC2 | DONE | Focused enabled-policy service tests; runtime verification deferred to #1980 | -| AC3 | DONE | Focused enabled-policy service/protocol tests; runtime verification deferred to #1980 | -| AC4 | DONE | v3 schema default is `false`; active v2 default config unchanged pending #1980 | -| AC5 | DONE | v3 `HttpTracker` field documentation | -| AC6 | DONE | Focused protocol, service, and Axum HTTP contract tests | -| AC6a | DONE | `manual-verification.md` Phase 2 | -| AC7 | DONE | Pre-commit gate passed 2026-08-18 | -| AC8 | DONE | `cargo +1.88.0 test --workspace` passed 2026-08-19 | -| AC9 | DONE | `manual-verification.md` Phase 1 | -| AC10 | DONE | `manual-verification.md` Phase 2 | -| AC11 | BLOCKED | Requires #1980 to activate v3.0.0 configuration at runtime. | +| AC ID | Status (`TODO`/`DONE`) | Evidence | +| ----- | ---------------------- | ---------------------------------------------------------------------------------------- | +| AC1 | DONE | `manual-verification.md` Phase 2 | +| AC2 | DONE | Focused enabled-policy service tests; runtime verification deferred to #1980 | +| AC3 | DONE | Focused enabled-policy service/protocol tests; runtime verification deferred to #1980 | +| AC4 | DONE | v3 schema default is `false`; active v2 default config unchanged pending #1980 | +| AC5 | DONE | v3 `HttpTracker` field documentation | +| AC6 | DONE | Focused protocol, service, and Axum HTTP contract tests | +| AC6a | DONE | `error-event-observability-analysis.md`; `docs/issues/drafts/generalize-error-events.md` | +| AC7 | DONE | Pre-commit gate passed 2026-08-18 | +| AC8 | DONE | `cargo +1.88.0 test --workspace` passed 2026-08-19 | +| AC9 | DONE | `manual-verification.md` Phase 1 | +| AC10 | DONE | `manual-verification.md` Phase 2 | +| AC11 | BLOCKED | Requires #1980 to activate v3.0.0 configuration at runtime. | ## Risks and Trade-offs diff --git a/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/error-event-observability-analysis.md b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/error-event-observability-analysis.md new file mode 100644 index 000000000..4abe1f47c --- /dev/null +++ b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/error-event-observability-analysis.md @@ -0,0 +1,148 @@ +# Error Event Observability Analysis + +**Decision:** Option B was selected on 2026-08-19. The #1987-specific rejection +event and metric were removed. The strict validation behavior remains. + +The deferred cross-service work is recorded in the draft EPIC +[`generalize-error-events.md`](../../drafts/generalize-error-events.md). No +GitHub issue or additional ADR will be created until that draft is refined. + +## Context + +Issue #1987 introduced a bounded metric for rejected non-empty HTTP announce +`ip` parameters. The initial implementation emits +`Event::TcpAnnouncePeerIpRejected` so the statistics listener can increment the +metric. + +This adds a rejected-request outcome to the HTTP-core event enum. The existing +[Events Are Objective Facts ADR](../../../adrs/20260727000000_events_are_objective_facts.md) +requires event variants to describe objective facts rather than consumer-specific +policy decisions. The proposed event must therefore be evaluated as a potential +public event-stream contract, not merely as a metrics implementation detail. + +## Problem + +The tracker needs to decide whether rejected requests should be exposed as events +and, if so, establish a coherent contract for all services. Introducing only one +rejection event for one HTTP announce validation rule could mislead consumers into +thinking that the event stream exposes every rejected request. + +The metric is operationally useful: it could show whether stricter handling of +the optional BEP 3 `ip` parameter rejects clients in practice. However, it is a +convenience for operators, not a prerequisite for the core correctness of strict +validation. Existing request logs can be used to investigate problematic client +usage while a broader observability design is deferred. + +## Questions Requiring a Decision + +### 1. Which rejected requests emit events? + +Possible scopes include: + +- only selected protocol-validation rejections; +- every announce rejection after request parsing; +- every HTTP request rejection, including announce and scrape; +- all rejected requests across HTTP, UDP, REST, and future services. + +A partial scope must be explicit. Otherwise consumers cannot distinguish an +unobserved rejection from a service failure or a missing event implementation. + +### 2. Do parser failures emit events? + +Some errors occur before `AnnounceService::handle_announce`, while a request is +being parsed or extracted. A complete rejected-request event contract must decide +whether those failures emit events and how request context is represented when +no valid request DTO exists. + +### 3. Are authentication and authorization denials included? + +Authentication-key failures, private-mode authentication failures, whitelist +denials, malformed requests, and tracker-core announce failures have different +context and privacy properties. Omitting them from a supposedly general rejection +contract would create an inconsistent interface; including them expands the work +substantially. + +### 4. What is the stable reason API? + +The service return type `HttpAnnounceError` is not a suitable event payload. It +contains internal error composition and wrapped implementation details that may +change independently of an event contract. + +If rejection events are exposed, they should use dedicated, bounded, +consumer-safe reason types. The design must decide whether those enums are: + +- exhaustive and changed only in a major version; or +- explicitly non-exhaustive/extensible, with consumer guidance for unknown + future values. + +### 5. What privacy constraints apply? + +Event payloads must not include raw client-controlled query values by default. +Raw values may contain addresses, hostnames, identifiers, or arbitrary text. A +stable event contract should carry only the minimum request context and bounded +reason classifications required by consumers. + +### 6. Who are event-stream consumers? + +The event stream currently decouples internal metrics and future consumers from +request handling. Before exposing rejected outcomes, the project must state +whether the stream is: + +- an internal implementation mechanism; +- a supported API for in-process or external consumers; or +- both, with versioning and compatibility guarantees. + +## Options + +### Option A: Keep the #1987 rejection event and metric + +Treat `TcpAnnouncePeerIpRejected` as a narrow, supported event contract. + +**Advantages:** preserves the immediate operational metric and event-based +decoupling. + +**Disadvantages:** establishes a one-off error-observability precedent without +answering the questions above. Consumers may incorrectly infer comprehensive +rejection coverage. + +### Option B: Remove the #1987 rejection event and metric + +Keep strict `ip` validation and bencoded failure responses. Defer rejected +request event/metric design to a dedicated cross-service effort. + +**Advantages:** keeps #1987 focused on its protocol and configuration behavior; +avoids an accidental public event API; preserves the existing event architecture +without directly coupling announce handling to metrics. + +**Disadvantages:** operators do not receive a dedicated aggregate rejection +counter initially. They must use existing request logs and normal diagnostics to +assess client compatibility. + +### Option C: Design a general rejected-request event contract now + +Create an ADR and implement a coherent event family across relevant HTTP and UDP +request paths. + +**Advantages:** provides a deliberate, homogeneous observable interface. + +**Disadvantages:** significantly expands scope, requires decisions for all +questions above, and should not be implemented only for HTTP announce `ip` +validation. + +## Decision + +**Option B** was selected: `TcpAnnouncePeerIpRejected`, its bounded reason type, +and its metric were removed. Strict protocol validation remains intact. + +The future work is documented as a local draft EPIC rather than an open GitHub +issue. It must define the public rejected-request event contract before adding +similar metrics or events. The future contract should cover its explicitly +chosen service/method boundaries consistently, define reason stability and +privacy rules, and retain the objective-fact principles in the Events ADR. + +## Relationship to the Events ADR + +The existing ADR remains applicable: events must be objective facts and not +consumer-specific policy decisions. This analysis identifies an additional +unresolved boundary: even an objective rejection outcome needs a deliberate, +complete, stable contract before it is added to a shared event enum. diff --git a/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/manual-verification.md b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/manual-verification.md index a4e3052bb..bedd651ee 100644 --- a/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/manual-verification.md +++ b/docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/manual-verification.md @@ -122,30 +122,15 @@ The same raw HTTP announce matrix from Phase 1 was run after rebuilding the trac This verifies the intentional baseline change: absent and empty values remain successful, while every non-empty override is explicitly rejected until schema v3.0.0 can activate the opt-in policy. -### Observability Evidence - -The tracker was restarted with a temporary local debug logging override, then received `ip=1.2.3.4` while the policy remained disabled. - -```sh -TORRUST_TRACKER_CONFIG_OVERRIDE_LOGGING__THRESHOLD=debug \ - cargo +1.88.0 run --bin torrust-tracker -``` - -The rejection-specific debug output used only the bounded reason code and did not contain the raw submitted value (`1.2.3.4`): - -```text -DEBUG torrust_tracker_http_core::statistics::event::handler: Recorded rejected HTTP announce peer IP parameter reason="override_disabled" -``` - -The authenticated local metrics endpoint reported one rejection with the bounded `reason="override_disabled"` label and no raw submitted-IP label: - -```text -# HELP http_tracker_core_announce_peer_ip_rejections_total Total rejected HTTP announce peer IP parameters -# TYPE http_tracker_core_announce_peer_ip_rejections_total counter -http_tracker_core_announce_peer_ip_rejections_total{client_address_ip_family="inet",client_address_ip_type="plain",reason="override_disabled",server_binding_address_ip_family="inet",server_binding_address_ip_type="plain",server_binding_ip="0.0.0.0",server_binding_port="7070",server_binding_protocol="http"}1 -``` - -Existing HTTP request middleware logs the full request URI at `info`, including query values. That established behavior is outside #1987's rejection-observability scope and is not evidence that raw values are globally absent from tracker logs. +### Observability Decision + +The initially tested rejection-specific event and metric were deliberately +removed under Option B after architectural review. The tracker therefore has no +dedicated aggregate counter for rejected announce `ip` parameters in #1987. +Existing request logs and normal diagnostics remain available to investigate +client compatibility. A future general error-event contract may introduce a +counter only when it is consistent with the documented cross-service design in +[`generalize-error-events.md`](../../drafts/generalize-error-events.md). ### Local Tracker-Client Result diff --git a/packages/http-core/src/event.rs b/packages/http-core/src/event.rs index 7df267cc1..bae81c385 100644 --- a/packages/http-core/src/event.rs +++ b/packages/http-core/src/event.rs @@ -7,8 +7,13 @@ //! should not do in response. Policy decisions belong in the consumer or the //! enforcement point, never in the event definition. //! -//! See [ADR-20260727000000](../../../../docs/adrs/20260727000000_events_are_objective_facts.md) +//! See [ADR-20260727000000](../../../docs/adrs/20260727000000_events_are_objective_facts.md) //! for the full rationale, the concrete counter-example, and naming heuristics. +//! +//! Rejected-request/error events require an additional deliberate contract. Do +//! not add one-off variants solely to support a metric; see the deferred +//! [general error-events EPIC](../../../docs/issues/drafts/generalize-error-events.md) +//! and the [#1987 analysis](../../../docs/issues/open/1987-add-config-option-to-use-ip-from-announce-query-string/error-event-observability-analysis.md). use std::net::{IpAddr, SocketAddr}; use torrust_info_hash::InfoHash; @@ -29,30 +34,6 @@ pub enum Event { TcpScrape { connection: ConnectionContext, }, - /// A non-empty announce `ip` parameter was rejected by address-selection policy. - TcpAnnouncePeerIpRejected { - connection: ConnectionContext, - reason: PeerIpRejectionReason, - }, -} - -/// Bounded reasons for rejecting an announce `ip` parameter. -#[derive(Debug, PartialEq, Eq, Clone, Copy)] -pub enum PeerIpRejectionReason { - OverrideDisabled, - DnsNameUnsupported, - InvalidIpAddress, -} - -impl PeerIpRejectionReason { - #[must_use] - pub const fn as_str(self) -> &'static str { - match self { - Self::OverrideDisabled => "override_disabled", - Self::DnsNameUnsupported => "dns_name_unsupported", - Self::InvalidIpAddress => "invalid_ip_address", - } - } } #[derive(Debug, PartialEq, Eq, Clone)] diff --git a/packages/http-core/src/services/announce.rs b/packages/http-core/src/services/announce.rs index 1935aefd1..8a3d547c7 100644 --- a/packages/http-core/src/services/announce.rs +++ b/packages/http-core/src/services/announce.rs @@ -29,7 +29,7 @@ use torrust_tracker_primitives::peer::PeerAnnouncement; use torrust_tracker_primitives::{AnnounceData, AnnounceEvent, NumberOfBytes}; use crate::event; -use crate::event::{Event, PeerIpRejectionReason}; +use crate::event::Event; use crate::services::error_mapping::protocol_error_from_tracker_core_error; /// The HTTP tracker `announce` service. @@ -129,14 +129,7 @@ impl AnnounceService { let remote_client_addr = resolve_remote_client_addr(&self.core_config.net.on_reverse_proxy.into(), client_ip_sources)?; - let peer_ip = match self.select_peer_ip(announce_request, remote_client_addr.ip()) { - Ok(peer_ip) => peer_ip, - Err(reason) => { - self.send_peer_ip_rejection_event(remote_client_addr, server_service_binding.clone(), reason) - .await; - return Err(reason.into()); - } - }; + let peer_ip = self.select_peer_ip(announce_request, remote_client_addr.ip())?; let mut peer = Self::peer_from_request(announce_request, &peer_ip); @@ -190,7 +183,7 @@ impl AnnounceService { &self, announce_request: &Announce, connection_peer_ip: std::net::IpAddr, - ) -> Result { + ) -> Result { Self::select_peer_ip_with_policy(self.peer_ip_selection_policy, announce_request, connection_peer_ip) } @@ -198,15 +191,15 @@ impl AnnounceService { peer_ip_selection_policy: PeerIpSelectionPolicy, announce_request: &Announce, connection_peer_ip: std::net::IpAddr, - ) -> Result { + ) -> Result { match &announce_request.ip { PeerIp::Absent | PeerIp::Empty => Ok(connection_peer_ip), PeerIp::Literal(_) if !peer_ip_selection_policy.use_ip_from_query_string => { - Err(PeerIpRejectionReason::OverrideDisabled) + Err(HttpAnnounceError::PeerIpOverrideDisabled) } PeerIp::Literal(ip) => Ok(*ip), - PeerIp::DnsName => Err(PeerIpRejectionReason::DnsNameUnsupported), - PeerIp::Invalid => Err(PeerIpRejectionReason::InvalidIpAddress), + PeerIp::DnsName => Err(HttpAnnounceError::PeerIpDnsNameUnsupported), + PeerIp::Invalid => Err(HttpAnnounceError::PeerIpInvalid), } } @@ -253,27 +246,14 @@ impl AnnounceService { http_stats_event_sender.send(event).await; } } - - async fn send_peer_ip_rejection_event( - &self, - remote_client_addr: RemoteClientAddr, - server_service_binding: ServiceBinding, - reason: PeerIpRejectionReason, - ) { - tracing::debug!(reason = reason.as_str(), "Rejected HTTP announce peer IP parameter"); - - if let Some(http_stats_event_sender) = self.opt_http_stats_event_sender.as_deref() { - http_stats_event_sender - .send(Event::TcpAnnouncePeerIpRejected { - connection: event::ConnectionContext::new(remote_client_addr, server_service_binding), - reason, - }) - .await; - } - } } /// Errors related to announce requests. +/// +/// This internal error type is not an event payload: variants may compose +/// implementation errors and client-visible text. Any future rejected-request +/// event must use a stable, bounded, consumer-safe reason type defined by the +/// [general error-events EPIC](../../../../docs/issues/drafts/generalize-error-events.md). #[derive(thiserror::Error, Debug, Clone)] pub enum HttpAnnounceError { #[error("Error resolving peer IP: {source}")] @@ -292,16 +272,6 @@ pub enum HttpAnnounceError { PeerIpInvalid, } -impl From for HttpAnnounceError { - fn from(reason: PeerIpRejectionReason) -> Self { - match reason { - PeerIpRejectionReason::OverrideDisabled => Self::PeerIpOverrideDisabled, - PeerIpRejectionReason::DnsNameUnsupported => Self::PeerIpDnsNameUnsupported, - PeerIpRejectionReason::InvalidIpAddress => Self::PeerIpInvalid, - } - } -} - impl From for HttpAnnounceError { fn from(peer_ip_resolution_error: PeerIpResolutionError) -> Self { Self::PeerIpResolutionError { @@ -513,12 +483,12 @@ mod tests { use torrust_tracker_test_helpers::configuration; use crate::event::test::announce_events_match; - use crate::event::{ConnectionContext, Event, PeerIpRejectionReason}; + use crate::event::{ConnectionContext, Event}; use crate::services::announce::tests::{ MockHttpStatsEventSender, initialize_core_tracker_services, initialize_core_tracker_services_with_config, sample_announce_request_for_peer, }; - use crate::services::announce::{AnnounceService, PeerIpSelectionPolicy}; + use crate::services::announce::{AnnounceService, HttpAnnounceError, PeerIpSelectionPolicy}; use crate::tests::{sample_info_hash, sample_peer, sample_peer_using_ipv4, sample_peer_using_ipv6}; #[test] @@ -532,10 +502,10 @@ mod tests { let request = sample_announce_request_for_peer(sample_peer()).0; let request = Announce { ip, ..request }; - assert_eq!( + assert!(matches!( AnnounceService::select_peer_ip_with_policy(policy, &request, connection_ip), - Ok(connection_ip) - ); + Ok(peer_ip) if peer_ip == connection_ip + )); } } @@ -544,37 +514,38 @@ mod tests { // Arrange let connection_ip = IpAddr::V4(Ipv4Addr::LOCALHOST); let cases = [ - ( - PeerIp::Literal("192.0.2.1".parse().unwrap()), - Ok("192.0.2.1".parse().unwrap()), - ), - ( - PeerIp::Literal("2001:db8::1".parse().unwrap()), - Ok("2001:db8::1".parse().unwrap()), - ), - (PeerIp::DnsName, Err(PeerIpRejectionReason::DnsNameUnsupported)), - (PeerIp::Invalid, Err(PeerIpRejectionReason::InvalidIpAddress)), + PeerIp::Literal("192.0.2.1".parse().unwrap()), + PeerIp::Literal("2001:db8::1".parse().unwrap()), + PeerIp::DnsName, + PeerIp::Invalid, ]; - // Act / Assert - for (ip, enabled_result) in cases { + for ip in cases { let request = Announce { ip, ..sample_announce_request_for_peer(sample_peer()).0 }; - assert_eq!( - AnnounceService::select_peer_ip_with_policy(PeerIpSelectionPolicy::enabled(), &request, connection_ip), - enabled_result - ); - assert_eq!( - AnnounceService::select_peer_ip_with_policy(PeerIpSelectionPolicy::disabled(), &request, connection_ip), - match request.ip { - PeerIp::Literal(_) => Err(PeerIpRejectionReason::OverrideDisabled), - PeerIp::DnsName => Err(PeerIpRejectionReason::DnsNameUnsupported), - PeerIp::Invalid => Err(PeerIpRejectionReason::InvalidIpAddress), - PeerIp::Absent | PeerIp::Empty => Ok(connection_ip), + + let enabled_result = + AnnounceService::select_peer_ip_with_policy(PeerIpSelectionPolicy::enabled(), &request, connection_ip); + let disabled_result = + AnnounceService::select_peer_ip_with_policy(PeerIpSelectionPolicy::disabled(), &request, connection_ip); + + match request.ip { + PeerIp::Literal(ip) => { + assert!(matches!(enabled_result, Ok(peer_ip) if peer_ip == ip)); + assert!(matches!(disabled_result, Err(HttpAnnounceError::PeerIpOverrideDisabled))); + } + PeerIp::DnsName => { + assert!(matches!(enabled_result, Err(HttpAnnounceError::PeerIpDnsNameUnsupported))); + assert!(matches!(disabled_result, Err(HttpAnnounceError::PeerIpDnsNameUnsupported))); } - ); + PeerIp::Invalid => { + assert!(matches!(enabled_result, Err(HttpAnnounceError::PeerIpInvalid))); + assert!(matches!(disabled_result, Err(HttpAnnounceError::PeerIpInvalid))); + } + PeerIp::Absent | PeerIp::Empty => unreachable!(), + } } } diff --git a/packages/http-core/src/services/scrape.rs b/packages/http-core/src/services/scrape.rs index fa5b7dfe9..ac1d644f2 100644 --- a/packages/http-core/src/services/scrape.rs +++ b/packages/http-core/src/services/scrape.rs @@ -115,7 +115,11 @@ impl ScrapeService { } } -/// Errors related to announce requests. +/// Errors related to scrape requests. +/// +/// This internal error type is not an event payload. A future rejected-request +/// event must use the stable, bounded, consumer-safe reason types defined by +/// the [general error-events EPIC](../../../../docs/issues/drafts/generalize-error-events.md). #[derive(thiserror::Error, Debug, Clone)] pub enum HttpScrapeError { #[error("Error resolving peer IP: {source}")] diff --git a/packages/http-core/src/statistics/event/handler.rs b/packages/http-core/src/statistics/event/handler.rs index f25564411..96dfc3cc4 100644 --- a/packages/http-core/src/statistics/event/handler.rs +++ b/packages/http-core/src/statistics/event/handler.rs @@ -5,8 +5,8 @@ use torrust_metrics::label::{LabelSet, LabelValue}; use torrust_metrics::{label_name, metric_name}; use crate::event::Event; +use crate::statistics::HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL; use crate::statistics::repository::Repository; -use crate::statistics::{HTTP_TRACKER_CORE_ANNOUNCE_PEER_IP_REJECTIONS_TOTAL, HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL}; pub async fn handle_event(event: Event, stats_repository: &Arc, now: DurationSinceUnixEpoch) { match event { @@ -44,22 +44,6 @@ pub async fn handle_event(event: Event, stats_repository: &Arc, now: Err(err) => tracing::error!("Failed to increase the counter: {}", err), } } - Event::TcpAnnouncePeerIpRejected { connection, reason } => { - let mut label_set = LabelSet::from(connection); - label_set.upsert(label_name!("reason"), LabelValue::new(reason.as_str())); - - match stats_repository - .increase_counter( - &metric_name!(HTTP_TRACKER_CORE_ANNOUNCE_PEER_IP_REJECTIONS_TOTAL), - &label_set, - now, - ) - .await - { - Ok(()) => tracing::debug!(reason = reason.as_str(), "Recorded rejected HTTP announce peer IP parameter"), - Err(err) => tracing::error!("Failed to increase the counter: {}", err), - } - } } tracing::debug!("stats: {:?}", stats_repository.get_stats().await); @@ -75,7 +59,7 @@ mod tests { use torrust_tracker_http_protocol::v1::services::peer_ip_resolver::{RemoteClientAddr, ResolvedIp}; use crate::CurrentClock; - use crate::event::{ConnectionContext, Event, PeerIpRejectionReason}; + use crate::event::{ConnectionContext, Event}; use crate::statistics::event::handler::handle_event; use crate::statistics::repository::Repository; use crate::tests::{sample_info_hash, sample_peer_using_ipv4, sample_peer_using_ipv6}; @@ -179,30 +163,4 @@ mod tests { assert_eq!(stats.tcp6_scrapes_handled(), 1); } - - #[tokio::test] - async fn it_should_increase_the_peer_ip_rejection_counter_for_the_rejection_reason() { - // Arrange - let stats_repository = Arc::new(Repository::new()); - let reason = PeerIpRejectionReason::DnsNameUnsupported; - - // Act - handle_event( - Event::TcpAnnouncePeerIpRejected { - connection: ConnectionContext::new( - RemoteClientAddr::new(ResolvedIp::FromSocketAddr(IpAddr::V4(Ipv4Addr::LOCALHOST)), Some(8080)), - ServiceBinding::new(Protocol::HTTP, SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 7070)).unwrap(), - ), - reason, - }, - &stats_repository, - CurrentClock::now(), - ) - .await; - - // Assert - let stats = stats_repository.get_stats().await; - assert_eq!(stats.announce_peer_ip_rejections_total(reason.as_str()), 1); - assert_eq!(stats.announce_peer_ip_rejections_total("invalid_ip_address"), 0); - } } diff --git a/packages/http-core/src/statistics/metrics.rs b/packages/http-core/src/statistics/metrics.rs index 03e206b09..acb67d4bf 100644 --- a/packages/http-core/src/statistics/metrics.rs +++ b/packages/http-core/src/statistics/metrics.rs @@ -6,7 +6,7 @@ use torrust_metrics::metric_collection::aggregate::sum::Sum; use torrust_metrics::metric_collection::{Error, MetricCollection}; use torrust_metrics::metric_name; -use crate::statistics::{HTTP_TRACKER_CORE_ANNOUNCE_PEER_IP_REJECTIONS_TOTAL, HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL}; +use crate::statistics::HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL; /// Metrics collected by the tracker. #[derive(Debug, Clone, PartialEq, Default, Serialize)] @@ -94,17 +94,4 @@ impl Metrics { ) .unwrap_or_default() as u64 } - - /// Total rejected HTTP announce `ip` parameters for a bounded rejection reason. - #[must_use] - #[allow(clippy::cast_sign_loss)] - #[allow(clippy::cast_possible_truncation)] - pub fn announce_peer_ip_rejections_total(&self, reason: &str) -> u64 { - self.metric_collection - .sum( - &metric_name!(HTTP_TRACKER_CORE_ANNOUNCE_PEER_IP_REJECTIONS_TOTAL), - &[("reason", reason)].into(), - ) - .unwrap_or_default() as u64 - } } diff --git a/packages/http-core/src/statistics/mod.rs b/packages/http-core/src/statistics/mod.rs index d8fedbc48..96102395f 100644 --- a/packages/http-core/src/statistics/mod.rs +++ b/packages/http-core/src/statistics/mod.rs @@ -8,7 +8,6 @@ use torrust_metrics::metric_name; use torrust_metrics::unit::Unit; pub const HTTP_TRACKER_CORE_REQUESTS_RECEIVED_TOTAL: &str = "http_tracker_core_requests_received_total"; -pub const HTTP_TRACKER_CORE_ANNOUNCE_PEER_IP_REJECTIONS_TOTAL: &str = "http_tracker_core_announce_peer_ip_rejections_total"; #[must_use] pub fn describe_metrics() -> Metrics { @@ -19,11 +18,5 @@ pub fn describe_metrics() -> Metrics { Some(Unit::Count), Some(MetricDescription::new("Total number of HTTP requests received")), ); - metrics.metric_collection.describe_counter( - &metric_name!(HTTP_TRACKER_CORE_ANNOUNCE_PEER_IP_REJECTIONS_TOTAL), - Some(Unit::Count), - Some(MetricDescription::new("Total rejected HTTP announce peer IP parameters")), - ); - metrics } diff --git a/packages/http-protocol/src/v1/requests/announce.rs b/packages/http-protocol/src/v1/requests/announce.rs index 3d65fe5d2..b5c378c2a 100644 --- a/packages/http-protocol/src/v1/requests/announce.rs +++ b/packages/http-protocol/src/v1/requests/announce.rs @@ -171,7 +171,10 @@ pub struct Announce { /// /// The `info_hash` and `peer_id` query params are special because they contain /// binary data. The `info_hash` is a 20-byte SHA1 hash and the `peer_id` is a -/// 20-byte array. +/// 20-byte array. This parser error includes raw query values and is not a +/// suitable event payload. See the [general error-events +/// EPIC](../../../../../docs/issues/drafts/generalize-error-events.md) before +/// exposing parser failures through an event stream. #[derive(Error, Debug)] pub enum ParseAnnounceQueryError { /// A mandatory param is missing. diff --git a/packages/http-protocol/src/v1/requests/scrape.rs b/packages/http-protocol/src/v1/requests/scrape.rs index 54c57c082..71dc35b13 100644 --- a/packages/http-protocol/src/v1/requests/scrape.rs +++ b/packages/http-protocol/src/v1/requests/scrape.rs @@ -19,6 +19,12 @@ pub struct Scrape { pub info_hashes: Vec, } +/// Errors that can occur while parsing a scrape request. +/// +/// Some variants retain raw query values, so this type must not be reused as an +/// event payload. See the [general error-events +/// EPIC](../../../../../docs/issues/drafts/generalize-error-events.md) before +/// exposing parser failures through an event stream. #[derive(Error, Debug)] pub enum ParseScrapeQueryError { #[error("missing query params for scrape request in {location}")] diff --git a/packages/tracker-core/src/error.rs b/packages/tracker-core/src/error.rs index fb2f71bda..70632b85e 100644 --- a/packages/tracker-core/src/error.rs +++ b/packages/tracker-core/src/error.rs @@ -17,6 +17,12 @@ use super::databases; use crate::authentication; /// Wrapper for all errors returned by the tracker core. +/// +/// This internal composition type is not an event payload: it can expose +/// implementation details and context that are unsuitable for a stable event +/// API. See the [general error-events +/// EPIC](../../../docs/issues/drafts/generalize-error-events.md) before adding +/// error events derived from it. #[derive(thiserror::Error, Debug, Clone)] pub enum TrackerCoreError { /// Error returned when there was an error with the tracker core announce handler. diff --git a/packages/udp-core/src/event.rs b/packages/udp-core/src/event.rs index 358270287..e54b8c112 100644 --- a/packages/udp-core/src/event.rs +++ b/packages/udp-core/src/event.rs @@ -7,8 +7,12 @@ //! should not do in response. Policy decisions belong in the consumer or the //! enforcement point, never in the event definition. //! -//! See [ADR-20260727000000](../../../../docs/adrs/20260727000000_events_are_objective_facts.md) +//! See [ADR-20260727000000](../../../docs/adrs/20260727000000_events_are_objective_facts.md) //! for the full rationale, the concrete counter-example, and naming heuristics. +//! +//! Error-event coverage is intentionally deferred until the [general +//! error-events EPIC](../../../docs/issues/drafts/generalize-error-events.md) +//! defines a stable cross-service contract. use std::net::{IpAddr, SocketAddr}; use torrust_info_hash::InfoHash; diff --git a/packages/udp-server/src/error.rs b/packages/udp-server/src/error.rs index e5f889cd9..9f1a53181 100644 --- a/packages/udp-server/src/error.rs +++ b/packages/udp-server/src/error.rs @@ -13,6 +13,10 @@ use torrust_tracker_udp_protocol::{ConnectionId, RequestParseError, TransactionI pub struct ConnectionCookie(pub ConnectionId); /// Error returned by the UDP server. +/// +/// This internal type carries implementation details and must not be used as a +/// new event payload without the stable reason classification required by the +/// [general error-events EPIC](../../../docs/issues/drafts/generalize-error-events.md). #[derive(Error, Debug, Clone)] pub enum Error { /// Error returned when the request is invalid. diff --git a/packages/udp-server/src/event.rs b/packages/udp-server/src/event.rs index 8499a2b26..125f0e330 100644 --- a/packages/udp-server/src/event.rs +++ b/packages/udp-server/src/event.rs @@ -19,8 +19,13 @@ //! to an existing one but named differently so a listener ignores it — stop and //! change the listener or the enforcement point instead. //! -//! See [ADR-20260727000000](../../../../../docs/adrs/20260727000000_events_are_objective_facts.md) +//! See [ADR-20260727000000](../../../docs/adrs/20260727000000_events_are_objective_facts.md) //! for the full rationale, the concrete counter-example, and naming heuristics. +//! +//! The existing [`Event::UdpError`] and [`ErrorKind`] predate a general +//! rejected-request event contract. Do not add ad hoc error variants or reuse +//! internal error types as new payloads; see the [general error-events +//! EPIC](../../../docs/issues/drafts/generalize-error-events.md). use std::fmt; use std::time::Duration;