[Feature] rbl: merge duplicate DNS queries produced by multiple checks - #6218
c-rosenberg wants to merge 2 commits into
Conversation
Requests that resolve to the same DNS query (case-insensitive, ignoring a trailing dot) are now deduplicated and sent once instead of once per check. By default, checks sharing a query also share a single symbol insertion: their labels are combined into one comma-separated option and the symbol's weight is counted only once. Since default_merge_checks is true, this changes existing scoring: a rule that previously scored once per check for the same value now scores once per matched value instead. Set merge_checks = false (or default_merge_checks = false) to keep the pre-4.1.6 behaviour of one insertion per check. Also: - whitelisting now tracks every RBL rule that whitelists a given value instead of only the last one processed - resolve_ip carries origins/labels from all merged sender-side checks through to the second-phase query and to failure reports - empty lookup values are skipped instead of being queried against the bare RBL suffix
Covers merge_checks scoring and disabling it, symbols_prefixes grouping, case-insensitive and trailing-dot deduplication, resolve_ip origin propagation, and whitelisting of merged requests.
|
@review deep |
There was a problem hiding this comment.
Review complete: no actionable defects found.
Verified the merged-request bookkeeping (origins/what merging in add_dns_request and the pre-resolve dedup pass), the weight-0.0 follow-up insertions (options are still recorded by lua_task_insert_result_common; the idiom is used across rspamd plugins), the lowercase normalization of query names (rspamd's default base32 alphabet and hex output are already lowercase, so hashed lookups are unaffected), the whitelist entry list shape (the rbl_whitelisted cache is only consumed within src/plugins/lua/rbl.lua), and the test fixtures (sha1 of example.org matches the mock DNS entry, and differs from the dotted spelling, so the dedot-before-hash path is genuinely exercised). The nil-nreq path in gen_rbl_ip_dns_callback when process_script returns falsy predates this change and has unchanged reachability, so it is not reported.
Automated review by Claude Code (glm-5.3 via z.ai).
vstakhov
left a comment
There was a problem hiding this comment.
I found three reproducible correctness gaps in the new canonicalization/scoring paths. The PR's focused suite passes 26/26 on an exact ARM64 build, both Lua files pass luacheck, diff-check is clean, all CI jobs are green, and both commits are GitHub-verified. I also audited the new conversions and aggregation: the built-in inputs are strings or small IP userdata, so I found no message-sized tostring materialization or performance blocker; the extra lowercase/dedup pass is over bounded DNS request names. The negative controls below fail specifically at the documented opt-out, process_script, and malformed-input boundaries.
| local failed_origins = lua_util.keys(resolve_table_elt.origins) | ||
| table.sort(failed_origins) | ||
| for i, origin in ipairs(failed_origins) do | ||
| task:insert_result(rbl.symbol .. '_FAIL', i == 1 and 1.0 or 0.0, |
There was a problem hiding this comment.
[P2] Honor merge_checks = false for failure symbols too. This branch always gives score weight only to the first merged origin, independently of merge_checks, although line 270 documents the option as restoring one insertion per check. With the existing two-selector SERVFAIL fixture plus merge_checks = false, an exact ARM64 run reports both options but scores RBL_MERGED_ERR_FAIL as 1.0 instead of the pre-change/opt-out value 2.0. Please apply the same merge policy here and cover the failing-query opt-out case.
| nreq = { | ||
| forced = forced, | ||
| n = processed, | ||
| n = tostring(processed):lower(), |
There was a problem hiding this comment.
[P2] Canonicalize process_script output consistently. The non-script path removes the DNS root dot, but this path only lowercases, and the final dedupe pass also only lowercases. A temporary rule whose script returns example.org.test9.uribl for one selector and the same name with a trailing dot for another issued two equivalent queries and scored the symbol 4.0 instead of 2.0. That defeats both the default scoring change and the DNS-query reduction for supported scripted rules. Please normalize the single root dot after process_script as well, ideally through one shared query-canonicalization helper.
| local to_resolve | ||
| local origin = req | ||
| -- Strip trailing dots first so hashing treats "foo." and "foo" alike | ||
| local origin = tostring(req):gsub('%.+$', '') |
There was a problem hiding this comment.
[P2] Do not sanitize invalid repeated dots into another valid name. gsub('%.+$', '') removes every trailing dot before validate_dns, so malformed.. becomes malformed and bypasses the validator's explicit .. rejection. In a negative control with only malformed.test9.uribl listed, the selector value malformed.. produced RBL_SELECTOR_MALFORMED. Strip at most the one legal root dot (and re-check for an empty result) so malformed values cannot be reinterpreted as a different lookup.
Summary
Requests that resolve to the same DNS query (case-insensitive, ignoring a
trailing dot) are now deduplicated and sent once instead of once per check.
By default, checks sharing a query also share a single symbol insertion:
their labels are combined into one comma-separated option and the symbol's
weight is counted only once.
Behaviour change
Since
default_merge_checksistrue, this changes existing scoring: a rulethat previously scored once per check for the same value now scores once per
matched value instead. Set
merge_checks = false(ordefault_merge_checks = false) to keep the pre-4.1.6 behaviour of one insertion per check.Additional fixes
instead of only the last one processed.
resolve_ipcarries origins/labels from all merged sender-side checksthrough to the second-phase query and to failure reports.
RBL suffix.
Testing
Added functional coverage for merged DNS requests:
merge_checksscoring anddisabling it,
symbols_prefixesgrouping, case-insensitive and trailing-dotdeduplication,
resolve_iporigin propagation, and whitelisting of mergedrequests.