feat(udp-server): add configurable connection ID validation policy (#1136) - #2032
Merged
josecelano merged 5 commits intoJul 28, 2026
Merged
Conversation
…on policy - Clarify that cookie-error metrics are emitted in disabled mode so operators can observe non-compliant clients; IP-ban counters are not incremented when validation is disabled - Connect action continues to issue valid connection IDs in both modes; BEP 15-compliant clients are unaffected in disabled mode - Expand startup warning to WARN level, identifying the service binding and the security implication - Rewrite test strategy: disabled policy is a distinct scenario group (connect, announce, scrape) analogous to private/public scenarios - Add AC12 covering metrics/connect/BEP-15 client behaviour - Add M3 manual scenario for connect on a disabled listener; renumber M4/M5 accordingly - Document feature motivation: operator flexibility for real-world non-compliant clients; encourage strict BEP 15 compliance - Mark EPIC subissue 7 as IN_PROGRESS
josecelano
added a commit
to josecelano/torrust-tracker
that referenced
this pull request
Jul 27, 2026
All 12 ACs met; manual verification deferred to torrust#1980.
There was a problem hiding this comment.
Pull request overview
This PR introduces a per-listener UDP connection ID validation policy (schema v3) with a runtime propagation path that allows bypassing connection ID validation for announce/scrape (while keeping connect issuing IDs), plus an ADR documenting the event-design principle used to preserve observability.
Changes:
- Add
ConnectionIdValidationPolicy { strict, disabled }to config schema v3 and mirror it inudp-core, then thread the policy through UDP server startup → processing → handlers. - Update announce/scrape handling to optionally skip cookie validation, while still emitting cookie-error observations/events in disabled mode.
- Add contract tests for disabled-policy behavior and document the “events are objective facts” principle via a new ADR and event module docs.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/bootstrap/jobs/udp_tracker.rs | Passes a (currently hardcoded) Strict policy through the bootstrap UDP job until v3 config is wired in. |
| project-words.txt | Adds unstarted for spell-checking. |
| packages/udp-server/tests/server/contract.rs | Adds contract tests covering disabled-policy connect/announce/scrape success paths. |
| packages/udp-server/src/testing/environment.rs | Adds connection_id_validation to the test environment plus an Unstarted alias and builder method. |
| packages/udp-server/src/server/states.rs | Extends server startup signature to accept the policy. |
| packages/udp-server/src/server/spawner.rs | Threads the policy through the spawner → launcher. |
| packages/udp-server/src/server/processor.rs | Stores the policy in the processor and passes it down to packet handling. |
| packages/udp-server/src/server/mod.rs | Updates server tests to pass Strict explicitly. |
| packages/udp-server/src/server/launcher.rs | Adds startup warning for disabled mode and gates ban enforcement on policy. |
| packages/udp-server/src/handlers/scrape.rs | Adds policy-aware cookie validation behavior for scrape, including disabled-mode observation emission. |
| packages/udp-server/src/handlers/mod.rs | Introduces CookieValidationContext to carry valid range + policy through handler calls. |
| packages/udp-server/src/handlers/announce.rs | Adds policy-aware cookie validation behavior for announce, including disabled-mode observation emission. |
| packages/udp-server/src/event.rs | Adds module-level documentation linking event design to the new ADR. |
| packages/udp-core/src/services/scrape.rs | Adds validate_cookie: bool to allow callers to bypass cookie validation. |
| packages/udp-core/src/services/announce.rs | Adds validate_cookie: bool to allow callers to bypass cookie validation. |
| packages/udp-core/src/lib.rs | Adds ConnectionIdValidationPolicy enum mirrored from configuration v3. |
| packages/udp-core/src/event.rs | Adds module-level documentation referencing the ADR principle. |
| packages/swarm-coordination-registry/src/event.rs | Adds module-level documentation referencing the ADR principle. |
| packages/http-core/src/event.rs | Adds module-level documentation referencing the ADR principle. |
| packages/configuration/src/v3_0_0/udp_tracker.rs | Adds the v3 schema enum + per-listener field with serde defaults and round-trip tests. |
| docs/issues/open/1980-1978-configuration-overhaul-final-cleanup.md | Adds a deferred cleanup task to remove the test-environment hardcoded policy after v3 migration. |
| docs/issues/open/1978-configuration-overhaul-epic.md | Updates epic progress/status entries for this subissue. |
| docs/issues/open/1136-1978-configurable-udp-connection-id-validation-policy.md | Updates the implementation spec, tasks, and verification notes for the new policy. |
| docs/adrs/index.md | Registers the new ADR in the index table. |
| docs/adrs/20260727000000_events_are_objective_facts.md | New ADR documenting the “events are objective facts” design contract. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #2032 +/- ##
===========================================
- Coverage 81.46% 81.46% -0.01%
===========================================
Files 344 344
Lines 24527 24601 +74
Branches 24527 24601 +74
===========================================
+ Hits 19981 20041 +60
- Misses 4246 4252 +6
- Partials 300 308 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
packages/udp-server/src/server/launcher.rs:218
Disabledmode currently skips ban enforcement (is_banned), but cookie errors are still emitted asEvent::UdpError { error: ConnectionCookie(..) }from the handlers. Because the banning event handler incrementsBanServicecounters on everyConnectionCookieerror, a disabled listener will still accumulate ban state and increase theUDP_TRACKER_SERVER_IPS_BANNED_TOTALgauge, which contradicts the docs/spec text that IP-ban counters are not incremented in disabled mode and can cause unnecessary memory growth.
// When connection ID validation is disabled, the tracker is
// intentionally accepting requests with invalid or arbitrary
// connection IDs. Enforcing IP bans in that mode is
// contradictory — the banning listener still counts invalid
// cookies for observability, but the ban is not acted upon.
let ban_enforcement_active = connection_id_validation == ConnectionIdValidationPolicy::Strict;
if ban_enforcement_active && udp_tracker_core_container.ban_service.read().await.is_banned(&req.from.ip()) {
packages/udp-server/tests/server/contract.rs:556
- Same issue as the announce test: asserting only
response.is_ok()doesn’t prove the scrape succeeded (anErrorresponse is stillOk(Response)). The test should assert the decoded response is aScraperesponse (and notError).
client.send(scrape_request.into()).await.unwrap();
let response = client.receive().await;
assert!(
response.is_ok(),
"scrape should succeed even with an invalid connection ID when validation is disabled"
);
…orrust#1136) Implement issue torrust#1136 which adds a `connection_id_validation` option to the schema v3 UDP tracker server configuration. **Configuration (T1–T2)** - Add `ConnectionIdValidationPolicy { Strict, Disabled }` enum to `packages/configuration/src/v3_0_0/udp_tracker_server.rs` with `#[serde(rename_all = "kebab-case")]` and `Strict` as the default. - Add the `connection_id_validation` field to `UdpTrackerServer` (global setting, not per-instance — the BanService is shared across all listeners). - Add 6 serialization/deserialization tests. **Runtime type (T3)** - Mirror the enum in `packages/udp-core/src/lib.rs` so the service layer does not need to depend on the configuration crate. - Add `validate_cookie: bool` parameter to `AnnounceService::handle_announce` and `ScrapeService::handle_scrape`; when `false` the cookie check is skipped. **Policy propagation (T4–T5)** - Thread the policy from the launcher through the processor to handlers. - Bootstrap job hardcodes `Strict` (v2 config compat). - Test environment defaults to `Strict` with builder override. - `CookieValidationContext` struct groups `valid_range` + `connection_id_validation` to stay within clippy's `too_many_arguments` limit. **Observability and banning (T6)** - Disabled mode still validates the cookie and emits `UdpError { ConnectionCookie }` (objective fact) so the ban listener always counts invalid IDs. - Ban enforcement is gated in the main loop: `is_banned` is checked only when `connection_id_validation == Strict`. **Startup warning (T7)** - `WARN`-level log at startup when the policy is `Disabled`. **Integration tests (T8)** - 3 contract tests: connect still issues valid ID, announce/scrape succeed with arbitrary ID in disabled mode. **ADR-20260727000000** — Events are objective facts (design principle). **ADR-20260727180000** — Shared services across tracker instances.
josecelano
force-pushed
the
1136-connection-id-validation-policy
branch
from
July 28, 2026 09:47
8a46ff4 to
ca35387
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
packages/udp-core/src/lib.rs:40
- The
Disabledpolicy docs say IP-ban counters are not incremented, but the udp-server banning listener still increments the sharedBanServicecounter onEvent::UdpError { ConnectionCookie }even when validation is disabled; only enforcement is skipped in the main loop. Please update the docs to match the actual behavior (or adjust the implementation if the intent is truly "no counter increments").
/// Skip connection ID validation for announce and scrape requests.
/// Cookie-error metrics are still emitted; IP-ban counters are not incremented.
Disabled,
packages/configuration/src/v3_0_0/udp_tracker_server.rs:58
- This field doc claims that in
disabledmode IP-ban counters are not incremented, but the current server-side banning listener still increments the ban counter (it just doesn’t enforce bans when validation is disabled). Update the comment to reflect enforcement gating rather than counter suppression.
/// `strict` (default) preserves all existing validation.
/// `disabled` skips validation so non-compliant clients that reuse
/// expired or arbitrary connection IDs can still connect. Cookie-error
/// metrics are still emitted in disabled mode; IP-ban counters are not
/// incremented.
packages/configuration/src/v3_0_0/udp_tracker_server.rs:37
ConnectionIdValidationPolicy::Disabledis documented as not incrementing IP-ban counters, but the current udp-server implementation still increments the shared ban counter onUdpError { ConnectionCookie }and only skips enforcement. The docstring should be updated to avoid misleading operators.
This issue also appears on line 54 of the same file.
/// Skip connection ID validation for announce and scrape requests.
/// The connect action continues to issue valid connection IDs.
/// Cookie-error metrics are still emitted; IP-ban counters are not
/// incremented.
Disabled,
docs/issues/open/1136-1978-configurable-udp-connection-id-validation-policy.md:359
- AC2 evidence references a
connection_id_validationfield onv3_0_0::UdpTracker, but the implementation and earlier design decision in this spec place the field onv3_0_0::udp_tracker_server::UdpTrackerServer. The evidence line should be corrected to match the code.
| AC2 | DONE | Field `connection_id_validation` on `v3_0_0::UdpTracker` struct |
docs/issues/open/1136-1978-configurable-udp-connection-id-validation-policy.md:366
- AC9 evidence currently describes per-processor/per-listener isolation, but this PR’s design pivot (and the config placement on
UdpTrackerServer) makes the policy global due to the sharedBanService. Update the evidence to reflect that mixed per-listener policies are intentionally unsupported and enforcement is gated when disabled.
| AC9 | DONE | Policy is per-processor-instance; tests pass per-listener isolation; M4 scenario verified inline |
…t#1136) Update Disabled variant docs to say 'IP-ban enforcement is skipped' instead of 'IP-ban counters are not incremented' — the ban counter still counts invalid IDs for observability, only enforcement is gated.\n\nFix announce and scrape contract tests to decode and assert the response variant instead of just checking response.is_ok().
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (7)
docs/issues/open/1136-1978-configurable-udp-connection-id-validation-policy.md:367
- AC9 evidence says “per-listener isolation”, but the spec text above now states the setting is global (BanService is shared) and strict/disabled cannot coexist on different listeners. The evidence should be updated to reflect the intended global/uniform policy (or the spec should re-justify mixed-policy support).
| AC8 | DONE | `Launcher::run_with_graceful_shutdown` emits `WARN` log on `Disabled`; `Unstarted` type alias |
| AC9 | DONE | Policy is per-processor-instance; tests pass per-listener isolation; M4 scenario verified inline |
| AC10 | DONE | Only `v3_0_0/` touched; bootstrap hardcodes `Strict` for v2 compat |
packages/udp-core/src/lib.rs:41
udp-core’sConnectionIdValidationPolicy::Disableddoc comment claims cookie-error metrics and ban-counter semantics, but udp-core does not emit metrics or manage banning; it only provides the policy/type used by callers. This doc currently over-promises behavior that’s implemented in udp-server (handlers/launcher/banning listener).
/// Skip connection ID validation for announce and scrape requests.
/// Cookie-error metrics are still emitted and the ban counter still counts
/// invalid IDs for observability, but IP-ban enforcement is skipped.
Disabled,
packages/udp-server/src/handlers/announce.rs:65
- In
Disabledmode, this handler still emitsEvent::UdpError { error: ConnectionCookie(..) }whencheck()fails. The banning listener increments the ban counter on everyUdpError::ConnectionCookieevent (packages/udp-server/src/banning/event/handler.rs:19-30), so disabled mode still increments the IP-ban counter/metrics—contradicting issue #1136’s acceptance criteria (“Disabled mode produces no connection-cookie errors, connection-ID metric increments, or IP-ban increments for the bypassed check”).
// When validation is disabled, still perform the cookie check so the
// banning listener can count invalid IDs for observability. Emit the
// same UdpError event (objective fact: a cookie error occurred), but
// do not return an error — the request is allowed to proceed.
// Ban enforcement is skipped in the main loop when validation is
// disabled (see launcher.rs), so the client is never actually blocked.
let validate_cookie = match cookie_validation.connection_id_validation {
ConnectionIdValidationPolicy::Strict => true,
ConnectionIdValidationPolicy::Disabled => {
packages/udp-server/src/handlers/scrape.rs:58
- Same issue as announce: in
Disabledmode this handler emitsEvent::UdpError { ConnectionCookie(..) }on failedcheck(), which drives the banning listener counter/metrics (packages/udp-server/src/banning/event/handler.rs:19-30). That means disabled mode still increments the IP-ban counter, conflicting with issue #1136’s stated acceptance criteria for disabled mode.
let validate_cookie = match cookie_validation.connection_id_validation {
ConnectionIdValidationPolicy::Strict => true,
ConnectionIdValidationPolicy::Disabled => {
if let Err(cookie_error) = check(
&request.connection_id,
gen_remote_fingerprint(&client_socket_addr),
cookie_validation.valid_range.clone(),
) {
tracing::debug!(
docs/issues/open/1136-1978-configurable-udp-connection-id-validation-policy.md:360
- The AC2 evidence line references
v3_0_0::UdpTracker, but this PR movedconnection_id_validationontov3_0_0::udp_tracker_server::UdpTrackerServer(see packages/configuration/src/v3_0_0/udp_tracker_server.rs:44-68). This looks like a leftover from the earlier per-listener design and makes the verification table misleading.
This issue also appears on line 365 of the same file.
| AC1 | DONE | Enum `ConnectionIdValidationPolicy` with `strict`/`disabled` serde values in `v3_0_0/udp_tracker_server.rs` |
| AC2 | DONE | Field `connection_id_validation` on `v3_0_0::UdpTracker` struct |
| AC3 | DONE | `#[serde(default)]` + test `it_should_default_connection_id_validation_to_strict` |
packages/udp-server/src/testing/environment.rs:69
with_connection_id_validationis used by the new contract tests (packages/udp-server/tests/server/contract.rs), so#[allow(dead_code)]is unnecessary here and acts as an avoidable lint suppression.
/// Sets the connection ID validation policy for this test environment.
#[must_use]
#[allow(dead_code)]
pub fn with_connection_id_validation(mut self, policy: ConnectionIdValidationPolicy) -> Self {
self.connection_id_validation = policy;
packages/configuration/src/v3_0_0/udp_tracker_server.rs:67
- This PR makes
connection_id_validationa global policy onUdpTrackerServer, but issue #1136 (as linked in the PR metadata) explicitly calls for a per-listenerconnection_id_validationso strict and disabled listeners can run simultaneously and independently. If the global-only design is intentional (per ADR-20260727180000), the GitHub issue’s Design/Acceptance Criteria likely needs updating before closing it.
/// Connection ID validation policy for all UDP tracker listeners.
///
/// This is a global setting because the ban service is shared across all
/// UDP instances. A per-instance policy would allow one listener's traffic
/// to pollute the shared ban counter that another listener enforces against.
///
/// `strict` (default) preserves all existing validation.
/// `disabled` skips validation so non-compliant clients that reuse
/// expired or arbitrary connection IDs can still connect. Cookie-error
/// metrics are still emitted and the ban counter still counts invalid
/// IDs for observability, but IP-ban enforcement is skipped.
///
/// **Security**: only use `disabled` on deployments where all listeners are
/// isolated through external network controls. Always prefer `strict` in
/// public deployments.
///
/// See ADR-20260727180000 for the rationale behind shared services.
#[serde(default)]
pub connection_id_validation: ConnectionIdValidationPolicy,
josecelano
commented
Jul 28, 2026
josecelano
left a comment
josecelano
left a comment
Member
Author
There was a problem hiding this comment.
Response to Copilot review suggestions:
Addressed in commit 9ce32b5:
-
Doc comments on
Disabledvariant — Updatedpackages/udp-core/src/lib.rsandpackages/configuration/src/v3_0_0/udp_tracker_server.rsto say "IP-ban enforcement is skipped, but the ban counter still counts invalid IDs for observability" instead of "IP-ban counters are not incremented". This matches the actual behaviour: the banning listener always increments the counter, only theis_bannedcheck is gated onStrict. -
Test assertions — Fixed
announce_succeeds_with_an_arbitrary_connection_idandscrape_succeeds_with_an_arbitrary_connection_idto decode the response and assert the correct variant (is_ipv4_announce_response/is_scrape_response) instead of justresponse.is_ok(). -
Launcher comment — The comment at
launcher.rs:218is intentionally correct: "the banning listener still counts invalid cookies for observability, but the ban is not acted upon". This matches the design documented in ADR-20260727000000.
…rrust#1136) Added test `many_invalid_connection_ids_do_not_cause_ban_in_disabled_mode` to verify that ban enforcement is skipped in disabled mode. Requests that exceed the ban threshold still succeed without timeout when validation is disabled. This addresses Copilot's concern about banning semantics in disabled-policy mode.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
packages/udp-server/src/testing/environment.rs:81
startis apubmethod and is called from integration/contract tests, so#[allow(dead_code)]is unnecessary and makes it easier for unused APIs to slip in unnoticed.
/// Starts the test environment and return a running environment.
///
/// # Panics
///
/// Will panic if it cannot start the server.
#[allow(dead_code)]
pub async fn start(self) -> Environment<Running> {
let cookie_lifetime = self.container.udp_tracker_core_container.udp_tracker_config.cookie_lifetime;
packages/udp-server/src/server/launcher.rs:218
- In Disabled mode, ban enforcement is skipped here, but the ban state still grows because handlers continue emitting
Event::UdpError { error: ConnectionCookie(..) }and the banning listener unconditionally callsban_service.increase_counter(...)(seepackages/udp-server/src/banning/event/handler.rs:19-30). Under invalid-ID traffic this can fillBanService::accurate_error_counterwith many unique IPs until the next reset interval, even though the policy is intended as a compatibility bypass.
If Disabled mode is meant to avoid ban-state churn (per issue #1136 acceptance criteria and earlier review discussions), the banning listener needs to avoid calling increase_counter when policy is Disabled, while still allowing separate observability metrics.
// When connection ID validation is disabled, the tracker is
// intentionally accepting requests with invalid or arbitrary
// connection IDs. Enforcing IP bans in that mode is
// contradictory — the banning listener still counts invalid
// cookies for observability, but the ban is not acted upon.
let ban_enforcement_active = connection_id_validation == ConnectionIdValidationPolicy::Strict;
if ban_enforcement_active && udp_tracker_core_container.ban_service.read().await.is_banned(&req.from.ip()) {
docs/issues/open/1136-1978-configurable-udp-connection-id-validation-policy.md:10
- The issue spec frontmatter still points at
related-pr: 2002, but this change set appears to be PR #2032 (seedocs/pr-reviews/pr-2032-copilot-suggestions.mdand the PR metadata). Keeping an outdated PR reference makes it harder to trace implementation history.
spec-path: docs/issues/open/1136-1978-configurable-udp-connection-id-validation-policy.md
branch: "1136-connection-id-validation-policy"
related-pr: 2002
last-updated-utc: 2026-07-27 12:36
docs/issues/open/1136-1978-configurable-udp-connection-id-validation-policy.md:361
- Acceptance Verification evidence for AC2 references
v3_0_0::UdpTracker, but the implementation movedconnection_id_validationtov3_0_0::udp_tracker_server::UdpTrackerServer(global setting). This table entry should match the actual type to avoid confusion for reviewers.
| AC1 | DONE | Enum `ConnectionIdValidationPolicy` with `strict`/`disabled` serde values in `v3_0_0/udp_tracker_server.rs` |
| AC2 | DONE | Field `connection_id_validation` on `v3_0_0::UdpTracker` struct |
| AC3 | DONE | `#[serde(default)]` + test `it_should_default_connection_id_validation_to_strict` |
| AC4 | DONE | Strict mode rejects via `AnnounceService`/`ScrapeService` with `validate_cookie = true`; unit tests in `udp-core` |
packages/udp-server/src/testing/environment.rs:72
with_connection_id_validationis apubmethod and is exercised by the UDP server contract tests, so#[allow(dead_code)]is redundant here (and can mask genuinely dead code in the future).
This issue also appears on line 74 of the same file.
/// Sets the connection ID validation policy for this test environment.
#[must_use]
#[allow(dead_code)]
pub fn with_connection_id_validation(mut self, policy: ConnectionIdValidationPolicy) -> Self {
self.connection_id_validation = policy;
self
}
Clippy pointed out that 'x as i32' was unnecessary when x is already i32. Changed loop to use 'for x in 0i32..=15' and removed the cast.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (8)
docs/issues/open/1136-1978-configurable-udp-connection-id-validation-policy.md:336
- The manual verification scenarios still expect “no ban increment” in Disabled mode (M2) and include “Mixed policies remain isolated” (M4). That conflicts with (a) the current implementation where
Event::UdpError{ConnectionCookie}is emitted and the banning listener increments counters, and (b) the design pivot to a global policy (so strict+disabled can’t run simultaneously). The scenarios should be revised so future manual verification matches the actual deployable behavior.
| ID | Scenario | Command/Steps | Expected Result | Status | Evidence |
| --- | --------------------------------------- | ---------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------- | ------ | -------------------------- |
| M1 | Strict listener rejects an invalid ID | Start a local strict UDP listener; send announce and scrape requests using an expired or zero connection ID | Requests receive the existing connection-ID error; error metrics and ban counters increase | TODO | |
| M2 | Disabled listener accepts an invalid ID | Start a local disabled UDP listener; repeat the same announce and scrape requests with arbitrary connection IDs | Requests pass cookie validation and continue through normal request handling; cookie-error metrics emitted; no ban increment | TODO | |
| M3 | Connect works on a disabled listener | Send a connect request to a disabled listener; then use the returned connection ID in an announce/scrape request | Connect returns a valid connection ID; subsequent announce/scrape succeeds | TODO | |
| M4 | Mixed policies remain isolated | Start strict and disabled listeners in one process; send the same invalid requests to both | Strict listener rejects them; disabled listener accepts them; neither listener changes the other | TODO | |
docs/issues/open/1136-1978-configurable-udp-connection-id-validation-policy.md:366
- The Acceptance Verification evidence table has inconsistencies with the updated design: AC2 evidence mentions
v3_0_0::UdpTracker, but the field is onUdpTrackerServer; AC9 evidence claims per-listener isolation, which conflicts with the global-policy decision documented earlier in this spec.
| AC1 | DONE | Enum `ConnectionIdValidationPolicy` with `strict`/`disabled` serde values in `v3_0_0/udp_tracker_server.rs` |
| AC2 | DONE | Field `connection_id_validation` on `v3_0_0::UdpTracker` struct |
| AC3 | DONE | `#[serde(default)]` + test `it_should_default_connection_id_validation_to_strict` |
| AC4 | DONE | Strict mode rejects via `AnnounceService`/`ScrapeService` with `validate_cookie = true`; unit tests in `udp-core` |
| AC5 | DONE | Handlers call `check()` for observation but pass `validate_cookie = false` to service |
| AC6 | DONE | Connect handler unchanged; test `connect_still_issues_a_valid_connection_id` passes |
| AC7 | DONE | Handlers emit `UdpError { ConnectionCookie }` regardless of mode; ban listener always counts; main loop skips `is_banned` when disabled |
| AC8 | DONE | `Launcher::run_with_graceful_shutdown` emits `WARN` log on `Disabled`; `Unstarted` type alias |
| AC9 | DONE | Policy is per-processor-instance; tests pass per-listener isolation; M4 scenario verified inline |
packages/udp-server/src/handlers/announce.rs:86
- In Disabled mode this still sends
Event::UdpError { error: ConnectionCookie(..) }into the UDP server event bus. The banning listener currently treats anyConnectionCookieerror as bannable and unconditionally callsban_service.increase_counter(...)(packages/udp-server/src/banning/event/handler.rs:19-30), so IP-ban counters andUDP_TRACKER_SERVER_IPS_BANNED_TOTALwill still increase under invalid-ID traffic even though enforcement is skipped in launcher.rs. This conflicts with parts of the issue spec that still say “no ban increment” in Disabled mode and can make the “ips banned” metric misleading (it becomes ‘would-be-banned’).
let announce_data = {
// When validation is disabled, still perform the cookie check so the
// banning listener can count invalid IDs for observability. Emit the
// same UdpError event (objective fact: a cookie error occurred), but
// do not return an error — the request is allowed to proceed.
// Ban enforcement is skipped in the main loop when validation is
// disabled (see launcher.rs), so the client is never actually blocked.
let validate_cookie = match cookie_validation.connection_id_validation {
ConnectionIdValidationPolicy::Strict => true,
ConnectionIdValidationPolicy::Disabled => {
if let Err(cookie_error) = check(
&request.connection_id,
gen_remote_fingerprint(&client_socket_addr),
cookie_validation.valid_range.clone(),
) {
tracing::debug!(
target: UDP_TRACKER_LOG_TARGET,
%client_socket_addr,
error = %cookie_error,
"connection ID validation disabled: invalid connection ID observed (request allowed, ban not enforced)"
);
if let Some(sender) = opt_udp_server_stats_event_sender.as_deref() {
sender
.send(Event::UdpError {
context: ConnectionContext::new(client_socket_addr, server_service_binding.clone()),
kind: Some(UdpRequestKind::Announce {
announce_request: *request,
}),
error: ErrorKind::ConnectionCookie(cookie_error.to_string()),
})
.await;
packages/udp-server/src/handlers/scrape.rs:72
- Same as announce: in Disabled mode this emits
Event::UdpError { ConnectionCookie(..) }, and the banning listener unconditionally increments the ban-service counter for every such event (packages/udp-server/src/banning/event/handler.rs:19-30). That means Disabled mode still changes ban counters/state and the “ips banned” gauge even though enforcement is skipped, which conflicts with parts of the spec that still state “no ban increment” in Disabled mode.
let scrape_data = {
let validate_cookie = match cookie_validation.connection_id_validation {
ConnectionIdValidationPolicy::Strict => true,
ConnectionIdValidationPolicy::Disabled => {
if let Err(cookie_error) = check(
&request.connection_id,
gen_remote_fingerprint(&client_socket_addr),
cookie_validation.valid_range.clone(),
) {
tracing::debug!(
target: UDP_TRACKER_LOG_TARGET,
%client_socket_addr,
error = %cookie_error,
"connection ID validation disabled: invalid connection ID observed (request allowed, ban not enforced)"
);
if let Some(sender) = opt_udp_server_stats_event_sender.as_deref() {
sender
.send(Event::UdpError {
context: ConnectionContext::new(client_socket_addr, server_service_binding.clone()),
kind: Some(UdpRequestKind::Scrape),
error: ErrorKind::ConnectionCookie(cookie_error.to_string()),
})
.await;
}
docs/pr-reviews/pr-2032-copilot-suggestions.md:15
- This file is named for PR #2032, but the header and Source line still contain placeholders (<PR_NUMBER>, <PR_URL>), unlike other entries in docs/pr-reviews/. This makes the audit log ambiguous and harder to link back to the PR.
# PR #<PR_NUMBER> Copilot Suggestions Tracking
Source: Copilot PR review threads for <PR_URL>
packages/udp-core/src/lib.rs:32
- The doc comment says the enum lives in udp-core “so that the service layer does not need to depend on the configuration crate”, but this crate already depends on and uses
torrust_tracker_configuration(e.g. udp-core/src/container.rs) andCargo.tomlincludes it. This reads as an outdated rationale and could confuse maintainers.
/// This mirrors [`torrust_tracker_configuration::v3_0_0::udp_tracker_server::ConnectionIdValidationPolicy`]
/// but lives in `udp-core` so that the service layer does not need to depend on
/// the configuration crate.
packages/udp-server/src/testing/environment.rs:71
#[allow(dead_code)]is used here without an explanation. Given the repo’s strict “deny warnings” posture, it would help to document why this needs to be allowed (typically because integration tests compile the library withoutcfg(test)).
/// Sets the connection ID validation policy for this test environment.
#[must_use]
#[allow(dead_code)]
pub fn with_connection_id_validation(mut self, policy: ConnectionIdValidationPolicy) -> Self {
self.connection_id_validation = policy;
docs/issues/open/1136-1978-configurable-udp-connection-id-validation-policy.md:325
- The required focused coverage section still references mixed-policy listeners and claims “IP-ban counters only in strict mode”, but the spec elsewhere pivots to a global policy (no mixed strict/disabled listeners) and the implementation/ADR describe counting invalid cookies for observability even in Disabled mode. These bullets should be updated to match the current design.
This issue also appears in the following locations of the same file:
- line 331
- line 358
- Scrape succeeds with an arbitrary (invalid) connection ID
- Cookie-error metrics are emitted in both modes; IP-ban counters only in strict mode
- Two simultaneous listeners using different policies
Member
Author
|
ACK 2407165 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Implements issue #1136 — add a global
connection_id_validationoption to the schema v3 UDP tracker server configuration, allowing operators to disable connection ID validation for compatibility with non-compliant BEP 15 clients.Changes
Configuration (T1–T2)
ConnectionIdValidationPolicyenum withstrict(default) anddisabledvariants inv3_0_0/udp_tracker_server.rs(global setting, not per-instance — the BanService is shared across all UDP listeners)Runtime type (T3)
udp-coreto avoid configuration crate dependencyvalidate_cookie: boolparameter onAnnounceService::handle_announceandScrapeService::handle_scrapePolicy propagation (T4–T5)
CookieValidationContextstruct to stay within clippy'stoo_many_argumentslimitObservability and banning (T6)
UdpError { ConnectionCookie }event (objective fact)is_bannedcheck only whenStrict)Startup warning (T7)
WARN-level log at startup when policy isDisabledIntegration tests (T8)
Documentation & Code Quality
Commits
cea84cffca35387f9ce32b50c04d75ea24071657Related
docs/adrs/20260727000000_events_are_objective_facts.mddocs/adrs/20260727180000_shared_services_across_tracker_instances.mdNotes
The per-instance/global configuration pivot was made after discovering that the global BanService would be polluted by per-instance policies. All Copilot suggestions were addressed with documented replies and the branch is linter-clean and test-passing.