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

dnsdist: doq,doh3 make sure we enforce any ACL #13670

Merged
merged 1 commit into from Jan 8, 2024

Conversation

chbruyand
Copy link
Member

Short description

This makes sure we enforce ACL in the DOQ and DOH3 paths.

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 Dec 28, 2023

Pull Request Test Coverage Report for Build 7350045492

  • 15 of 15 (100.0%) changed or added relevant lines in 2 files are covered.
  • 1965 unchanged lines in 37 files lost coverage.
  • Overall coverage decreased (-0.08%) to 57.622%

Files with Coverage Reduction New Missed Lines %
pdns/sodcrypto.cc 1 90.51%
pdns/rcpgenerator.cc 2 89.95%
pdns/recursordist/rec_metrics.hh 2 0.0%
pdns/version.cc 2 89.92%
modules/gpgsqlbackend/spgsql.cc 3 67.7%
modules/lmdbbackend/lmdbbackend.cc 3 72.43%
pdns/epollmplexer.cc 3 80.72%
pdns/iputils.hh 3 74.93%
pdns/recursordist/test-syncres_cc2.cc 3 89.05%
pdns/tsigverifier.cc 3 77.22%
Totals Coverage Status
Change from base Build 7329684697: -0.08%
Covered Lines: 107562
Relevant Lines: 155335

💛 - Coveralls

Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

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

Looks good! I wonder if it would make sense to drop the DoQ connection earlier in the process, before spending CPU time negotiating the key materials?

@chbruyand
Copy link
Member Author

chbruyand commented Dec 29, 2023

Looks good! I wonder if it would make sense to drop the DoQ connection earlier in the process, before spending CPU time negotiating the key materials?

That would be nice, but I think that if we do so, we cannot properly terminate the connection, but only discard received data ?
Anyway we can indeed do that earlier and avoid some unnecessary processing.

@rgacogne
Copy link
Member

Looks good! I wonder if it would make sense to drop the DoQ connection earlier in the process, before spending CPU time negotiating the key materials?

That would be nice, but I think that if we do so, we cannot properly terminate the connection, but only discard received data ? Anyway we can indeed do that earlier and avoid some unnecessary processing.

You are right! Perhaps we could make it optional? I think proper termination is great most of the time, but when you are under attack you might want to just drop DoQ packets not allowed by the ACL.

@rgacogne
Copy link
Member

rgacogne commented Jan 8, 2024

I'm going to merge this because not enforcing the ACL is a clear issue, and we can look for improvements later.

@rgacogne rgacogne merged commit 48c98b7 into PowerDNS:master Jan 8, 2024
75 checks passed
@chbruyand chbruyand deleted the dnsdist-doq-acl branch January 19, 2024 10:52
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.

None yet

3 participants