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

Improve errors for non-HTTP/2 DoH clients #979

Closed
jsha opened this issue Dec 14, 2023 · 1 comment · Fixed by #980
Closed

Improve errors for non-HTTP/2 DoH clients #979

jsha opened this issue Dec 14, 2023 · 1 comment · Fixed by #980

Comments

@jsha
Copy link
Contributor

jsha commented Dec 14, 2023

Unbound requires HTTP/2 to serve DoH, which makes sense for performance reasons.

However, sometimes a client may be misconfigured to only send HTTP/1.1. For instance we recently ran into such a problem in Boulder: letsencrypt/boulder#7215.

When that happens, Unbound's debug logs show:

http2: session_recv from 192.168.4.146 failed, error: Received bad client magic byte string

And the client sees the connection closed abruptly with no response. In Go that looks like an EOF error. In curl it looks like:

curl: (52) Empty reply from server

It would be nice to have more detailed information on what went wrong. For instance, on Unbound's side, rather than logging about the client magic string, we could notice that the client did not send h2 as one of its ALPN protocols, and thus the client doesn't support HTTP/2.

Also, Unbound could support HTTP/1.1 handshakes to get as far as serving a HTTP 400 with an appropriate error message saying "Unbound requires HTTP/2 for DoH but client only supports HTTP/1.1."

@wcawijngaards wcawijngaards linked a pull request Jan 3, 2024 that will close this issue
@wcawijngaards
Copy link
Member

Merged the patch to fix the issue. The reason that HTTP 2.0 is required is that dns over https requires at least 2.0, this is, for example, explained in RFC 8484.

wcawijngaards added a commit that referenced this issue Jan 3, 2024
- Merge #980: DoH: reject non-h2 early. To fix #979: Improve errors
  for non-HTTP/2 DoH clients.
jedisct1 added a commit to jedisct1/unbound that referenced this issue Jan 7, 2024
* nlnet/master: (40 commits)
  - Fix unit test for NLnetLabs#987 change in udp1xxx retry packet send.
  Changelog note for NLnetLabs#987 - Merge NLnetLabs#987: skip edns frag retry if advertised udp payload size is   not smaller.
  skip edns frag retry if advertised udp payload size is not smaller
  - Remove unneeded newlines and improve indentation in remote control   code.
  - Fix NLnetLabs#983: Sha1 runtime insecure change was incomplete.
  Changelog note for NLnetLabs#985. - Merge NLnetLabs#985: Add DoH and DoT to dnstap message.
  Changelog note for NLnetLabs#979 and NLnetLabs#980. - Merge NLnetLabs#980: DoH: reject non-h2 early. To fix NLnetLabs#979: Improve errors   for non-HTTP/2 DoH clients.
  Add DoH and DoT to dnstap message
  - Update example.conf with cookie options.
  DoH: reject non-h2 early
  Fixup doc/Changelog.
  - Fix root_zonemd unit test, it checks that the root ZONEMD verifies,   now that the root has a valid ZONEMD.
  Changelog note for NLnetLabs#975 - Merge NLnetLabs#975: Fixed some syntax errors in rpl files.
  Fixed some syntax errors in rpl files.
  - Fix NLnetLabs#974: doc: default number of outgoing ports without libevent.
  - Use the origin (DNAME) TTL for syntesized CNAMEs as per RFC 6672.
  - Fix tests to use new common.sh functions, wait_logfile and   kill_from_pidfile.
  - Update test script file common.sh.
  - Updated IPv4 and IPv6 address for b.root-servers.net in root hints.
  - iana portlist update.
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants