Don't compress or downcase the FQDN in LP rdata - #1282
Closed
chuenchen309 wants to merge 1 commit into
Closed
Conversation
`LP._to_wire` forwarded the message compression table and the canonicalize flag to its embedded FQDN, so the name got name-compressed and downcased in the DNSSEC canonical form. LP (RFC 6742) is not an RFC 1035 type: per RFC 3597 section 4 its embedded name must not be compressed, and per RFC 4034 section 6.2 (as amended by RFC 6840 section 5.1) it is not on the downcasing list. Every other modern name-bearing type (DSYNC, SVCB/HTTPS, HIP, IPSECKEY, NSEC) already uses `to_wire(file, None, origin, False)`; LP was the outlier, copied from the MX template where compression is grandfathered.
Collaborator
|
I agree it is a bug. Perhaps the code could get rid of the unnecessary comments and the test could be generalized to all types with uncompressable names? |
Owner
|
I agree with Petr. |
Owner
|
I found a few other similar issues implementing the suggested testing, so I'll just fix everything for downcasing and canonicalization, include updated tests, and be sure to add a Co-Authored-By for you to the commit. |
Owner
|
Closing in favor of [#1284]. You are a co-author. |
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.
LP._to_wire(dns/rdtypes/ANY/LP.py) forwards the message compression table and the canonicalize flag to its embedded FQDN:So an LP record's name gets name-compressed on the wire and downcased in the DNSSEC canonical form. Both are wrong for LP (RFC 6742), which is not an RFC 1035 type:
Every other modern name-bearing type in dnspython (
DSYNC,SVCB/HTTPS,HIP,IPSECKEY,NSEC) already usesto_wire(file, None, origin, False). LP is the lone outlier — it was written from theMXtemplate, where compression is grandfathered.Fix: use the same idiom as the sibling types —
to_wire(file, None, origin, False).Verification (re-runnable from the diff): added
test_LP_name_not_compressedintests/test_rdata.py, modeled on the existingtest_svcb.py::test_alias_not_compressed; it fails on the current tree and passes with the fix.tests/test_rdata.py+tests/test_wire.py→ 108 passed; the wider suite is green apart from an unrelated offline live-network test.(Left out of this PR:
DNAME/UncompressedNSdoes not downcase even though DNAME is on the RFC 4034 list — but that's arguably intentional (RFC 6672) and debated, so I scoped this to the clear-cut LP case.)Disclosure: This PR was authored by an AI coding agent (Claude Code) running on this account: the AI found the bug, ran the repro, wrote the test, and wrote this description. The human account holder reviews every change and is accountable for it. The verification above is real and re-runnable from the diff. If this isn't the kind of contribution you want, say so and I'll close it.