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

auth udp: use stubDoResolve for ALIAS #14594

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

Conversation

zeha
Copy link
Collaborator

@zeha zeha commented Aug 27, 2024

Short description

Implements @Habbie's "alternate outcome" from #13039.

Somebody please tell me what is wrong with this :)

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 Aug 27, 2024

Pull Request Test Coverage Report for Build 11049952298

Details

  • 25 of 54 (46.3%) changed or added relevant lines in 2 files are covered.
  • 53 unchanged lines in 14 files lost coverage.
  • Overall coverage increased (+0.1%) to 64.7%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pdns/stubresolver.cc 1 5 20.0%
pdns/packethandler.cc 24 49 48.98%
Files with Coverage Reduction New Missed Lines %
pdns/auth-main.cc 1 61.47%
pdns/packethandler.cc 1 72.31%
modules/gpgsqlbackend/gpgsqlbackend.cc 1 88.62%
pdns/tcpreceiver.cc 1 66.92%
pdns/sstuff.hh 2 56.83%
pdns/backends/gsql/gsqlbackend.hh 2 96.57%
pdns/misc.cc 2 63.66%
pdns/stubresolver.cc 3 75.58%
pdns/tsigverifier.cc 3 77.22%
pdns/axfr-retriever.cc 3 67.07%
Totals Coverage Status
Change from base Build 11049514595: 0.1%
Covered Lines: 124664
Relevant Lines: 161993

💛 - Coveralls

@zeha zeha force-pushed the zeha-13039-udp-dostubresolve branch from e9c86c0 to 69fde7f Compare August 27, 2024 16:05
@zeha zeha changed the title auth udp: use stubDoResolve auth udp: use stubDoResolve for ALIAS Aug 27, 2024
@zeha zeha force-pushed the zeha-13039-udp-dostubresolve branch 9 times, most recently from 95880fa to 312119e Compare August 28, 2024 13:43
@zeha zeha marked this pull request as ready for review August 28, 2024 14:37
@zeha zeha requested a review from Habbie September 2, 2024 14:02
@Habbie
Copy link
Member

Habbie commented Sep 3, 2024

this emits A+RRSIG for me, very nice. Did not verify the signature but I'm confident.

@Habbie
Copy link
Member

Habbie commented Sep 3, 2024

Somebody please tell me what is wrong with this :)

haven't found anything yet! We'll need tests though

@Habbie Habbie added this to the auth-5 milestone Sep 3, 2024
@zeha
Copy link
Collaborator Author

zeha commented Sep 3, 2024

Somebody please tell me what is wrong with this :)

haven't found anything yet! We'll need tests though

I took the passing of the existing ALIAS tests as good enough. If you have something specific to test in mind, please let me know!

@Habbie
Copy link
Member

Habbie commented Sep 3, 2024

I took the passing of the existing ALIAS tests as good enough. If you have something specific to test in mind, please let me know!

oh. I didn't even check if we had those :D I'll have a look, testing the DNSSEC bits would probably be nice now.

@klaus-nicat
Copy link
Contributor

klaus-nicat commented Sep 3, 2024

this emits A+RRSIG for me, very nice. Did not verify the signature but I'm confident.

Does that mean that this PR also solves the problem of signing ALIAS responses?

@Habbie
Copy link
Member

Habbie commented Sep 6, 2024

Does that mean that this PR also solves the problem of signing ALIAS responses?

sure feels that way. Please test and give us your opinion :)

I took the passing of the existing ALIAS tests as good enough. If you have something specific to test in mind, please let me know!

the existing tests are good besides, indeed, the DNSSEC thing. We test plenty of DNSSEC in regression-tests/ but not so much in .auth-py/ currently. I'm not sure what's the easiest path is here. Perhaps just start with +DO on a query from test_ALIAS.py and see if an RRSIG appears.

@Habbie
Copy link
Member

Habbie commented Sep 6, 2024

oohh "Dnspython can do simple DNSSEC signature validation and signing."

@zeha
Copy link
Collaborator Author

zeha commented Sep 7, 2024

Now with DNSSEC tests using dnspython.

@zeha zeha force-pushed the zeha-13039-udp-dostubresolve branch 4 times, most recently from 6ae28d7 to d127b24 Compare September 8, 2024 13:48
@mind04
Copy link
Contributor

mind04 commented Sep 9, 2024

I like the idea of dnssec and ALIAS.

There is however something you cannot ignore. Using stubDoResolve will make ALIAS much more sensitive to exhausting all available backends.

There are similarities wit LUA but the big difference with ALIAS is it's legacy. ALIAS is deployed in many places and is hand out to customers. Hopefully this didn't happen for LUA records ;)

@zeha
Copy link
Collaborator Author

zeha commented Sep 9, 2024

Using stubDoResolve will make ALIAS much more sensitive to exhausting all available backends.

Yeah that was also my concern. But I guess one can a) get more backends and b) employ ratelimiting in front and also behind the auth.

@Habbie
Copy link
Member

Habbie commented Sep 9, 2024

Given that the diff to packethandler itself is quite small, I wonder if we can give users the choice between 'old' (dnsproxy) and 'new' (stubDoResolve)

@zeha
Copy link
Collaborator Author

zeha commented Sep 9, 2024

I wonder if we can give users the choice

Seems possible, but code and test complexity will go up.

I can type it in, if we know we want this.

@Habbie
Copy link
Member

Habbie commented Sep 9, 2024

Seems possible, but code and test complexity will go up.

understood

I can type it in, if we know we want this.

if people have existing deployments where users have the ability to enter ALIASed names (which I'm sure they do), I'm afraid @mind04's concerns are relevant. I did not think about existing users enough, before.

@klaus-nicat
Copy link
Contributor

What exactly is the problem with stubDoResolve and the Backends? Why does resolving a domain requires a backend?

@Habbie
Copy link
Member

Habbie commented Sep 9, 2024

What exactly is the problem with stubDoResolve and the Backends? Why does resolving a domain requires a backend?

in this PR, the resolving happens synchronously inside the distributor thread, which has a database connection open

@klaus-nicat
Copy link
Contributor

in this PR, the resolving happens synchronously inside the distributor thread, which has a database connection open

"synchronously" sounds bad. Was the old code also synchronously or asynchronous?

@Habbie
Copy link
Member

Habbie commented Sep 12, 2024

"synchronously" sounds bad. Was the old code also synchronously or asynchronous?

The old code was asynchronous, which is why doing DNSSEC there was so hard it never happened.

@klaus-nicat
Copy link
Contributor

klaus-nicat commented Sep 12, 2024

That's bad. Having a zone with

*.example.com.  ALIAS zone.with.nonresponding.nameservers.

and someone doing <random>.example.com with a few queries per second can cause a DoS attack.

Do we have the same problem already if the incoming queries use TCP?

@zeha
Copy link
Collaborator Author

zeha commented Sep 22, 2024

Do we have the same problem already if the incoming queries use TCP?

No, only in the AXFR path.

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.

5 participants