fix: complete DNSSEC validation correctness#128
Conversation
Zone invalidation: when qType is empty, publish zone name only (no ":qType" suffix) so other cluster nodes correctly flush their entire L1 cache instead of calling Invalidate with a malformed key. startInvalidationListener: detect zone-level messages (no colon) and flush the full L1 cache rather than attempting a per-record invalidation. Also restore doc comments on PushToDLQ and PopFromDLQ that were removed during prior cleanup.
- ValidateDNSKEYFormat: validate RSA and Ed25519 keys in addition to ECDSA
Previously only extractECDSAPublicKey was called, allowing malformed
RSA/Ed25519 keys to pass validation silently.
- NSEC3 owner name errors: propagate VerifyNSEC3OwnerName errors instead
of discarding them. Owner name hash mismatches are fatal for validation.
- Trust anchor matching: verify public key bytes in addition to key tag
and algorithm. Prevents key-tag collision attacks bypassing trust anchors.
- SignRRSet ZSK hardcode: use key.KeyType ("KSK" vs "ZSK") to set correct
DNSKEY Flags (257 for KSK/SEP, 256 for ZSK). Previously always used 256.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches🧪 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. Review rate limit: 0/1 reviews remaining, refill in 24 minutes and 1 second.Comment |
- Add ValidateNSEC3Proof tests (WildcardMatch, ValidNXDOMAIN, NoData, InvalidOwnerName) to cover the NXDOMAIN/no-data validation path - Add ValidateNSEC3WildcardProof tests (Valid, EmptyRecords, WrongWildcardHash) - Add nsec3CoversHash and decodeBase32Hash tests (already at 100%) - Add TestValidateWithTrustAnchor_PublicKeyMismatch to cover the public key comparison branch added in PR #128 (keytag + algorithm + public key) - Fix TestValidateDNSKEYFormat_RSASHA256_TooSmall to use truncated bytes instead of small generated key (crypto/rsa rejects < 1024-bit keys) - Replace impossible TestValidateNSEC3Proof_NoCoveringNSEC3 with comment explaining why owner==queryHash+wrap always covers - Add missing bytes import to dnssec_verify_test.go Coverage: ValidateNSEC3Proof 0%->72%, ValidateNSEC3WildcardProof 0%->85%, nsec3CoversHash 0%->100%, decodeBase32Hash 0%->100%
1 participant
Summary
Fixes 5 DNSSEC validation correctness issues across 3 files:
ValidateDNSKEYFormatonly validated ECDSA keys, skipped RSA/Ed25519SignRRSethardcodedFlags=256(ZSK), ignored KSK vs ZSK distinctionChanges
ValidateDNSKEYFormatnow validates RSA SHA-256 and Ed25519 key formats in addition to ECDSA. NSEC3 owner name errors are now propagated instead of silently discarded.SignRRSetuseskey.KeyTypeto set correct DNSKEY flags (257 for KSK/SEP, 256 for ZSK).Test plan
go build ./...passesgo test ./... -racepasses (all 17 packages)