Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not follow CNAME records for ANY or CNAME queries #15008

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

miodvallat
Copy link
Contributor

Short description

This is an attempt at fixing #5769 (not addressing the RRSIG concern at the moment, though).

Disclaimer: I have no idea what I am doing.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@coveralls
Copy link

coveralls commented Jan 3, 2025

Pull Request Test Coverage Report for Build 12887559896

Details

  • 3 of 7 (42.86%) changed or added relevant lines in 1 file are covered.
  • 5187 unchanged lines in 72 files lost coverage.
  • Overall coverage decreased (-3.0%) to 61.744%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pdns/packethandler.cc 3 7 42.86%
Files with Coverage Reduction New Missed Lines %
modules/gpgsqlbackend/gpgsqlbackend.cc 1 88.62%
modules/pipebackend/coprocess.hh 1 0.0%
modules/gsqlite3backend/gsqlite3backend.cc 2 94.44%
pdns/recursordist/sortlist.cc 2 72.94%
ext/lmdb-safe/lmdb-typed.hh 2 71.86%
pdns/webserver.hh 3 66.36%
pdns/dnspacket.hh 3 77.78%
pdns/remote_logger.cc 3 53.81%
pdns/auth-zonecache.cc 3 91.07%
pdns/distributor.hh 3 50.62%
Totals Coverage Status
Change from base Build 12884379895: -3.0%
Covered Lines: 122613
Relevant Lines: 166406

💛 - Coveralls

@miodvallat miodvallat force-pushed the cname_withheld_to_protect_the_innocent branch 2 times, most recently from e47038f to 6f7e056 Compare January 3, 2025 09:58
@Habbie
Copy link
Member

Habbie commented Jan 7, 2025

not addressing the RRSIG concern at the moment, though

reading back what I wrote there, it turns out I did not include enough information for anybody (including myself) to see what that was about. In any case the RRSIG behaviour I see today (without your PR) looks just fine.

@Habbie
Copy link
Member

Habbie commented Jan 7, 2025

This patch looks right. I wonder if any the wildcard, LUA and DNAME paths contain a similar bug, though. But this is an improvement in any case. Can you add a test?

@miodvallat
Copy link
Contributor Author

Can you add a test?

I am struggling with getting the existing tests to pass at the moment - a couple of the existing tests need oracle changes, but then further steps in the CI fail because they reuse the same tests and apparently don't need any change...

@miodvallat miodvallat force-pushed the cname_withheld_to_protect_the_innocent branch from 6f7e056 to 32f3173 Compare January 20, 2025 07:33
@@ -1,5 +1,7 @@
0 nxd.example.com. 120 IN CNAME nxdomain.example.com.
0 nxd.example.com. 120 IN RRSIG CNAME 13 3 120 [expiry] [inception] [keytag] example.com. ...
0 nxd.example.com. 86400 IN NSEC outpost.example.com. CNAME RRSIG NSEC
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this extra output is unexpected. If anything I would expect some responses to become smaller!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if we should be setting weDone when we match-but-don't-follow a CNAME?

@miodvallat miodvallat force-pushed the cname_withheld_to_protect_the_innocent branch from 32f3173 to 46928eb Compare January 21, 2025 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants