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 doh [DO NOT MERGE] #6911

Closed
wants to merge 53 commits into from
Closed

Conversation

@ahupowerdns
Copy link
Contributor

@ahupowerdns ahupowerdns commented Aug 31, 2018

Short description

This PR adds DNS over HTTPS support to dnsdist, based on libh2o.

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)
  • checked that this code was merged to master
@ahupowerdns ahupowerdns changed the title Dnsdist doh Dnsdist doh [DO NOT MERGE] Aug 31, 2018
@johndickinson
Copy link

@johndickinson johndickinson commented Sep 6, 2018

I had to upgrade to bionic (from trusty) to get libh2o-evloop and also had to specify LIBS=-lwslay to fix link errors like

/usr/lib/gcc/x86_64-linux-gnu/7/../../../x86_64-linux-gnu/libh2o-evloop.so: undefined reference to `wslay_event_want_read’

The full configure line I used was ./configure --enable-dns-over-tls --enable-dns-over-https --with-libcrypto=/opt/openssl-1.1.1-pre9 --enable-re2 CXXFLAGS=-I/opt/openssl-1.1.1-pre9/include CPPFLAGS=-I/opt/openssl-1.1.1-pre9/include LDFLAGS=-L/opt/openssl-1.1.1-pre9/lib LIBS=-lwslay

Loading

@pieterlexis
Copy link
Member

@pieterlexis pieterlexis commented Sep 6, 2018

@johndickinson,

What does config.log say about libh2o? And can you share the libh2o-evloop.pc? On arch linux, the -lwslay is not needed (and not in the .pc file). I think this is because the arch PKGBUILD installs the library as a .a and Debian/ubuntu as a .so.

Loading

@johndickinson
Copy link

@johndickinson johndickinson commented Sep 6, 2018

config.log:
...
configure:20525: checking for LIBH2OEVLOOP
configure:20532: $PKG_CONFIG --exists --print-errors "libh2o-evloop"
configure:20535: $? = 0
configure:20549: $PKG_CONFIG --exists --print-errors "libh2o-evloop"
configure:20552: $? = 0
configure:20590: result: yes
...
ac_cv_env_LIBH2OEVLOOP_CFLAGS_set=
ac_cv_env_LIBH2OEVLOOP_CFLAGS_value=
ac_cv_env_LIBH2OEVLOOP_LIBS_set=
ac_cv_env_LIBH2OEVLOOP_LIBS_value=
...
pkg_cv_LIBH2OEVLOOP_CFLAGS=
pkg_cv_LIBH2OEVLOOP_LIBS=-lh2o-evloop
...
HAVE_LIBH2OEVLOOP_FALSE='#'
HAVE_LIBH2OEVLOOP_TRUE=''
...
LIBH2OEVLOOP_CFLAGS=''
LIBH2OEVLOOP_LIBS='-lh2o-evloop'
...
#define HAVE_LIBH2OEVLOOP 1

$ cat /usr/lib/x86_64-linux-gnu/pkgconfig/libh2o-evloop.pc
prefix=/usr
exec_prefix=${prefix}
includedir=${prefix}/include
libdir=/usr/lib/x86_64-linux-gnu

Name: libh2o-evloop
Description: An optimized HTTP/1.x & HTTP/2 library
URL: https://h2o.examp1e.net/
Version: 0.13.4
Libs: -L${libdir} -lh2o-evloop
Cflags: -I${includedir}

Loading

@zeha
Copy link
Collaborator

@zeha zeha commented Sep 6, 2018

Loading

publicarray referenced this issue in DNSCrypt/dnscrypt-website Oct 31, 2018
Signed-off-by: Frank Denis <github@pureftpd.org>
@ahupowerdns ahupowerdns force-pushed the dnsdist-doh branch 2 times, most recently from c631ea0 to f5c2664 Nov 9, 2018
uint16_t qtype, qclass;
unsigned int consumed = 0;
DNSName qname(query, len, sizeof(dnsheader), false, &qtype, &qclass, &consumed);
DNSQuestion dq(&qname, qtype, qclass, consumed, &du->dest, &du->remote, dh, 1500, len, false, &queryRealTime);
Copy link
Member

@rgacogne rgacogne Nov 22, 2018

Choose a reason for hiding this comment

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

The 1500 here is wrong, it pretends that there is at least 1500 bytes available from the address pointed to by dh, which is not true. Rules and handleEDNSClientSubnet() will very likely trigger an heap-based out-of-bounds write.

Loading

uint32_t allowExpired = ss ? 0 : g_staleCacheEntriesTTL;
boost::optional<Netmask> subnet;
char cquery[1500];
memcpy(cquery, query, du->query.length());
Copy link
Member

@rgacogne rgacogne Nov 22, 2018

Choose a reason for hiding this comment

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

I don't think you need to copy the query here, unless I missing something, since either it will be overwritten by the cached response on a hit, or we won't use cquery again before leaving this block.

Loading

Copy link
Member

@rgacogne rgacogne Nov 22, 2018

Choose a reason for hiding this comment

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

I also did not check whether the query could be larger than 1500 bytes, which would trigger a stack-based out-of-bounds write.

Loading

vector<uint8_t> stripped;
rewriteResponseWithoutEDNS(du->query, stripped);
du->query.assign((const char*)&stripped[0], stripped.size());
}
Copy link
Member

@rgacogne rgacogne Nov 22, 2018

Choose a reason for hiding this comment

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

The case of an incoming query with EDNS but without ECS is not handled here.

Loading

@rgacogne
Copy link
Member

@rgacogne rgacogne commented Mar 12, 2019

Unless I'm mistaken, we always send the request received over DoH to the backend over UDP, advertising an EDNS Payload Size of 4096 (unless there was an existing EDNS OPT RR). Doesn't that mean that we will never be able to handle responses larger than 4096 bytes over DoH?

Loading

@MaxWichern
Copy link
Contributor

@MaxWichern MaxWichern commented Apr 3, 2019

Starting dnsdist one way results in an error, while another way it works fine.
"nohup dnsdist --supervised &" works without any issues.
"systemctl start dnsdist" results in:
"dnsdist: what(): DOH thread failed to launch: DOH server failed to listen on 10.142.0.3:443: Permission denied
systemd: dnsdist.service: main process exited, code=killed, status=6/ABRT
"

The error message itself should be more specific.
And the error should not end with an abort.

Loading

@rgacogne
Copy link
Member

@rgacogne rgacogne commented Apr 3, 2019

@MaxWichern It should have been fixed by 9bad969, is that not the case?

Loading

@MaxWichern
Copy link
Contributor

@MaxWichern MaxWichern commented Apr 3, 2019

@MaxWichern It should have been fixed by 9bad969, is that not the case?

That was the initial thought, but it didn't.

Loading

@Habbie
Copy link
Member

@Habbie Habbie commented Apr 5, 2019

When #7676 gets merged, we need to extend that code to reload h2o certs as well.

Loading

@rgacogne
Copy link
Member

@rgacogne rgacogne commented Apr 23, 2019

Superseded by #7726.

Loading

@rgacogne rgacogne closed this Apr 23, 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.

None yet

7 participants