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: Allow accepting DoH queries over HTTP instead of HTTPS #8267

Merged
merged 1 commit into from Sep 18, 2019

Conversation

@rgacogne
Copy link
Member

@rgacogne rgacogne commented Sep 2, 2019

Short description

It allows using dnsdist behind a reverse-proxy or any other device offloading TLS.
Closes #8263.

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)
@Habbie
Copy link
Member

@Habbie Habbie commented Sep 3, 2019

Like

Please use the emoji button instead of comment like, thank you!

@PowerDNS PowerDNS deleted a comment Sep 3, 2019
@@ -103,11 +103,12 @@ Listen Sockets
higher than 0 to enable TCP Fast Open when available.
Default is 0.

.. function:: addDOHLocal(address, certFile(s), keyFile(s) [, urls [, options]])
.. function:: addDOHLocal(address, [certFile(s) [, keyFile(s) [, urls [, options]]]])
Copy link

@Jamesits Jamesits Sep 5, 2019

Choose a reason for hiding this comment

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

I think it should be

function:: addDOHLocal(address, [certFile(s), keyFile(s) [, urls [, options]]])

Since if you have certFile then you should have keyFile too.

Copy link
Member Author

@rgacogne rgacogne Sep 14, 2019

Choose a reason for hiding this comment

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

In practice that's pretty much what the code does, it requires the same number of keys.

@cmouse
Copy link
Contributor

@cmouse cmouse commented Sep 5, 2019

Should this come with some check(s) that the remote actually offloaded SSL? Or at least support for such thing?

HAPROXY supports sending TLS status with in version 2 header. Apache/nginx can send header(s) that inform about TLS usage. Also these connections should not be accepted from untrusted IP sources.

@krombel
Copy link
Contributor

@krombel krombel commented Sep 10, 2019

I would not require such header nor would I add such restriction.
The person which is configuring this should know what it is doing.
I think the log output that encryption is disabled (as done) is sufficient.

What I think is more important:
How do the filter (especially rate limiting) apply?
Is it possible to accept X-Forwared-For or X-Real-IP header in http-only setups?

@rgacogne
Copy link
Member Author

@rgacogne rgacogne commented Sep 14, 2019

Should this come with some check(s) that the remote actually offloaded SSL? Or at least support for such thing?

HAPROXY supports sending TLS status with in version 2 header. Apache/nginx can send header(s) that inform about TLS usage. Also these connections should not be accepted from untrusted IP sources.

I think we should let the administrator configure the ACL properly, and accept plaintext connections when told to do so.

@rgacogne rgacogne merged commit 831f6c1 into PowerDNS:master Sep 18, 2019
26 checks passed
@rgacogne rgacogne deleted the dnsdist-doh-over-http branch Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants