fix(dnsbl): preserve TXT wire limits and bounded RRset cache lifetimes - #456
Open
seonghobae wants to merge 6 commits into
Open
seonghobae wants to merge 6 commits into
seonghobae wants to merge 6 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (16)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughDNSBL 존 출력에 TTL과 TXT wire 크기 제한을 적용합니다. 같은 IPv4 owner에는 유효 TTL 중 최솟값을 사용합니다. 유효하지 않은 origin은 ChangesDNSBL 존 게시
CI 러너 계약
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Merge Risk: 🔵 Low · up to The DNSBL TTL, TXT and origin changes look well covered by tests. Before merging, confirm that a runner with the cwlab-ci-isolated label is registered and available to this repository, otherwise the CI jobs will wait and not run. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The DNS publishing repairs improve wire-format and cache-lifetime correctness. However, running pull-request builds on self-hosted machines depends on isolation and cleanup guarantees that are not demonstrated here. Upgrades can also stop publishing older entries with newly disallowed lifetimes, requiring a compatibility check for existing deployments. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
seonghobae
marked this pull request as ready for review
October 2, 2026 12:32
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
seonghobae
changed the title
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
2 participants
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.
Product defects and bounded repairs
Protected base:
f8260f1e03836039ff9463dd99fa982e4e270c4b.Current reviewed and published head:
1c855e93499d47f33496db4e94adbd8beb0ec8de.Full 27-file candidate tree:
270487e7788d13caef7c1bbd57dbac263a540a36.Complete base-to-head patch SHA256:
804a35647e1f43e15ce550fff998072a4366b492a40b6cf7cd2ecd76f722762e.Admitted DNSBL metadata was rendered as one oversized TXT string. A valid 600-byte reason plus source
unitproduced 612 decoded bytes and a real DNS parser rejected it. The Rust exporter now uses lossless adjacent strings at UTF-8 scalar boundaries, each at most 255 decoded bytes, retaining quote/backslash/control escaping.Admitted
ttl_secondswas ignored by zone publication: a 1-second entry produced A/TXT records with the fixed$TTL 300. Valid lifetimes now appear explicitly except the byte-compatible 300-second default. DNSBL admission rejects values above 2147483647, and the untrusted persisted-state export boundary omits zero/out-of-range values rather than silently clamping them. Valid source entries sharing an IPv4 owner use the shortest published TTL consistently for both RRsets, without changing stored evidence or depending on input order.Origin filtering alone produced unloadable output for a 64-byte label, an interior empty label and a 256-byte fully qualified owner. Actual Rust output failed dnspython with
LabelTooLong,EmptyLabelandNameTooLong, while 63-byte label and 255-byte owner controls passed. The sanitizer now retains normal spelling and existing filtering, but uses the existingdnsbl.invalidfallback for invalid labels or origins above 237 ASCII textual bytes. This reserves 16 encoded bytes for four maximal reversed IPv4 labels, satisfying237 + 2 + 16 = 255without changing stored entries, metadata or TTLs.Per-string chunking still admitted and exported an aggregate TXT RDATA of 65,536 octets: text parsing and raw RDATA serialization succeeded, but actual RR serialization returned
FormError. The 65,535-octet positive control succeeds. Admission now counts decoded UTF-8 bytes plus every chunk length octet without allocating an escaped copy and rejects overflow. Persisted oversized entries are omitted from both A/TXT emission and shared-owner TTL selection, retaining stored evidence and otherwise-valid entries. Escaped master-file spelling does not count as wire payload.Scope: DNSBL validation/publication and direct regression consumers only. No API shape/schema/authorization/feed-ownership change, foreign-owner dependency, deployment, central-workflow copy or competitor writer adoption. Existing MISP/shared DNSBL ownership repair remains with PR #167; official-feed/CIDR work remains with PR #115.
Actually executed RED → minimum repair → GREEN → independent review
TXT character string exceeds 255 bytes: 612. Boundaries 254/255/256/510/511, long UTF-8, escapes, HTTP admission-to-export and short-output compatibility are retained.[300,300]instead of[1,1]. Later negative controls separately demonstrated oversized admission/invalid persisted TTL and inconsistent shared-owner RRset TTLs; each failure receipt is preserved.scripts/smoke.shfailed at line212 because its 600-second fixture still expected a no-TTL record. Independent review returned CHANGES_REQUESTED. The minimal smoke correction requires literal600 on A and TXT while retaining the seed300 check. Real smoke then passed through restart/persistence. The negative review and RED receipts remain preserved.b7780764ef7e1901fb62d3c788750cb89c3179317311b579b7ab9d674269ccd6. This is not counted GitHub approval.Subsequent whole-candidate test-sensitivity review
The next substantive reviewer approved the complete 17-file union, reusing the earlier source reasoning only for unchanged bytes and independently judging the six-path test/fuzz/docs delta and composition. Local verdict SHA256:
92e538ad3000171a4dbb2163bce411adceb5381ccd62850dc9623f607b6d5ccf. Its independently executed scratch test passed three positive and rejected fifteen negative controls. Parent executions and reviewer executions are attributed separately; no local verdict is counted GitHub approval. Production code, dependencies and public APIs are unchanged by the subsequent test-only commit.Origin boundary whole-candidate source review
The subsequent read-only substantive reviewer approved the complete 20-file union and independently judged the seven-path origin repair and consumers; earlier reasoning was reused only for identical file bytes. Local verdict SHA256:
7885770b5a9665fc41e72e8edd8284d3fafa44f86cec736138e2412b8548c63e. It relied on official RFC content, not unsupported retrieval-ledger chronology. Parent executions inspected by the reviewer were not claimed as independent reruns. After the review, a new extracted core archive matched all eight changed core source/test files byte-for-byte and passed 36 tests, closing the previous final-fixture/archive mismatch. This is local technical approval, not counted GitHub approval.Aggregate RDATA whole-candidate source review
The read-only substantive reviewer approved the complete 23-file union and directly inspected the twelve-path RDLENGTH increment, with prior reasoning reused only for identical file bytes. Local report SHA256:
8109e8dcb2c6f6669b50d58acc36a053e99a01bac231961d75d9d0cdda6ae72c. It reviewed parent executions separately and did not rerun tests/builds. Parent tests preserved actual admission, persisted-export and oracle REDs before the corresponding repairs; literal limits, UTF-8 overhead, quote/backslash/control bytes, near-boundary properties and real authenticated rejected-write readback/export are covered. The exact safely extracted final core archive passed 42 tests. No local verdict is counted GitHub approval.User-directed self-hosted execution migration
The user requested organization-wide self-hosted transition and actual normalization on October 3, 2026. This owner changed only Wardnet's three retained local workflows and their existing Rust runner contract; central workflows and infrastructure remain with their existing owners. CI/Fuzz/Scorecard now require
[self-hosted, Linux, X64, cwlab-ci-isolated]; CI/Fuzz no longer persist checkout credentials. Triggers, permissions, immutable action pins, concurrency, fuzz budgets and Scorecard v2.4.4 remain unchanged. There is no hosted or privileged-pool fallback.The complete 27-file union received conditional local source approval, report SHA256
641fa5d9f35c117a41f2b0a699972c1200d2d66386b11254e9df0d3ac961a805. The reviewer independently read source and verified immutable bytes, but did not rerun native gates. Two actual new contract REDs preceded the routing/credential repair; all six retained runner/queue checks pass. Real actionlint passes with a scratch config declaring this exact custom label, without ignored findings. The label declaration is a routing requirement, not evidence of a provisioned runner.Normalization is NOT complete: the current operator API inventory has zero matching isolated workers. All nine registered organization workers belong to restricted control/scanner/GPU pools; none is a safe general-CI substitute. Actual isolated provisioning, repository/workflow eligibility, clean per-job lifecycle, Node24/Rust/fuzz/Docker compatibility and exact-head successful jobs are still required. Arbitrary public-PR code must not reach persistent privileged/inference state;
persist-credentials: falseand an isolated-looking label alone are not a sandbox. No runner ACL, credential, billing, protection, inference service or production deployment was changed. The existing central owner/operator route has been notified; broadcast enqueue is not recipient adoption.Exact committed-head local verification
Executed again on
1c855e93499d47f33496db4e94adbd8beb0ec8de, clean checkout:cargo fmt --check— exit0.cargo test --locked --offline --workspace— 219 passed, 0 failed/ignored/filtered, exit0.cargo clippy --locked --offline --workspace --all-targets -- -D warnings— exit0.git diff --check <protected-base> <head>— exit0.bash scripts/smoke.sh— exit0 including real loopback HTTP, management auth, DNSBL A/TXT TTL, restart and persistence checks.cargo check --locked --offline ... --bin fuzz_dnsbl_zone— exit0; compilation only, not an executed fuzz campaign.Parent real dnspython2.8.0 checks exercised exported TTL1/60/300/2147483647 through DNS parsing and message wire roundtrip, with three invalid persisted omissions. Extracted core archive27/27 and isolated synchronized fuzz-target compile passed. The latter is a scratch dependency compile diagnostic, not a canonical locked fuzz campaign.
The later extracted core archive passed 32 tests; its new helper bytes match the reviewed source, and the property file differs only by a descriptive comment. Stable Cargo gates and retained end-to-end smoke were rerun on the exact clean committed head.
Fresh-DB local Trivy vuln/misconfig CRITICAL/HIGH fixable scan of the exact 23-file candidate export returned exit0: Cargo.lock vulnerability0, Dockerfile20 success/0 failure and Kubernetes23 success/0 failure. This is the candidate tree, not a current remote merge-ref scan, hosted Security Scan or normal merge acceptance. Current-head CI/security/review/countable approval retain separate obligations; predecessor/Draft-exempt statuses do not transfer.
Honest remaining acceptance limits
The source-bound unexcluded final-candidate LLVM run reported 9842/10260 lines (95.9259259%), 827/907 functions and 13528/14195 regions. Both production instantiations of the new RDATA helper executed all25/25 regions. This report inventories11 production/unit-source files, not integration-test oracle source; it is a candidate execution, not a fresh committed-head coverage rerun. Full-workspace100% remains unmet; branches/MC-DC are unmeasured. The original100% requirement is not waived.
The earlier broad property-only mutation probes did not reject removal of chunking, explicit TTLs or RRset minimums; those negative observations remain preserved. The new independent package-local publication oracle checks input-derived A/TXT counts, owner/code identity, source order, shortest valid owner TTL and lossless decoded metadata. A dedicated property generates at least two publishable shared-owner records, while the original arbitrary-input path remains. In isolated archives, the exact new property executed one passing baseline test and one failing test for each of the three deliberate production mutations. An earlier replay selected zero tests and is explicitly excluded as invalid harness evidence. The fuzz target retains raw arbitrary inputs and adds a bounded positive projection; compilation does not prove a coverage-guided campaign ran.
Full DNS message fit beyond aggregate TXT RDLENGTH, general DNS name grammar beyond the emitted ASCII origin contract, CIDR/IPv6 publication, authoritative SOA/NS, threat/feed expiration, complete root archive publication, business/performance and rendered8locale acceptance are not established. No operational deployment, paid fallback, credential/profile edits, protection bypass or self-approval. Original dirty checkout13file hashes/status/HEAD remain preserved. This PR does not complete the repository-wide product goal.
Protocol grounding and evidence
docs/doctoring/dnsbl-txt-character-strings.mdanddocs/doctoring/dnsbl-cache-lifetimes.mdanddocs/doctoring/dnsbl-origin-wire-limits.mdanddocs/doctoring/dnsbl-txt-rdata-limits.md.No detection model/load-balancing algorithm or copyrighted-paper redistribution is introduced. Local real receipts, frozen manifests, raw coverage, preserved failures and independent reviews:
/Users/seonghobae/orca/reports/hermes-rolling-migration/fleet/wardnet-hermes-product-evidence-20261002/.Summary by CodeRabbit