json-dns/response.go: Fix variant question response in Response.Question

leiless commented 4 years ago

HOWTO reproduce

Use https://dns.alidns.com/resolve in doh-client.conf


diff --git a/doh-client/doh-client.conf b/doh-client/doh-client.conf
index b6eff00..da0eb4d 100644
--- a/doh-client/doh-client.conf
+++ b/doh-client/doh-client.conf
@@ -1,9 +1,6 @@
# DNS listen port
listen = [
-    "127.0.0.1:53",
 "127.0.0.1:5380",
-    "[::1]:53",
-    "[::1]:5380",

 ## To listen on both 0.0.0.0:53 and [::]:53, use the following line
 # ":53",
@@ -25,8 +22,12 @@ upstream_selector = "random"

## CloudFlare's resolver, bad ECS, good DNSSEC
## ECS is disabled for privacy by design: https://developers.cloudflare.com/1.1.1.1/nitty-gritty-details/#edns-client-subnet
-[[upstream.upstream_ietf]]
-    url = "https://cloudflare-dns.com/dns-query"
+#[[upstream.upstream_ietf]]
+#    url = "https://cloudflare-dns.com/dns-query"
+#    weight = 50
+
+[[upstream.upstream_google]]
+    url = "https://dns.alidns.com/resolve"
 weight = 50

## CloudFlare's resolver, bad ECS, good DNSSEC
@@ -131,4 +132,4 @@ no_ipv6 = false
no_user_agent = false

# Enable logging
-verbose = false
+verbose = true


2. Compile and run
```bash
$ make doh-client/doh-client

$ ./doh-client
2020/08/01 13:34:29 random mode start
127.0.0.1:43635 - - [01/Aug/2020:13:34:33 +0800] "baidu.com. IN A"
2020/08/01 13:34:33 choose upstream: upstream type: Google, upstream url: https://dns.alidns.com/resolve
2020/08/01 13:34:33 json: cannot unmarshal object into Go struct field Response.Question of type []jsonDNS.Question

$ dig @127.0.0.1 -p5380 baidu.com

; <<>> DiG 9.16.1-Ubuntu <<>> @127.0.0.1 -p5380 baidu.com
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: SERVFAIL, id: 45660
;; flags: qr rd ra; QUERY: 1, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 0

;; QUESTION SECTION:
;baidu.com.         IN  A

;; Query time: 163 msec
;; SERVER: 127.0.0.1#5380(127.0.0.1)
;; WHEN: Sat Aug 01 13:34:33 CST 2020
;; MSG SIZE  rcvd: 27

Verify manually

$ curl -4sL 'https://dns.alidns.com/resolve?ct=application/dns-json&name=t.cn&type=A' | jq
{
  "Status": 0,
  "TC": false,
  "RD": true,
  "RA": true,
  "AD": false,
  "CD": false,
  "Question": {
    "name": "t.cn.",
    "type": 1
  },
  "Answer": [
    {
      "name": "t.cn.",
      "TTL": 23,
      "type": 1,
      "data": "121.29.19.162"
    }
  ]
}

Peer behaviour

$ curl -4sL 'https://cloudflare-dns.com/dns-query?ct=application/dns-json&name=t.cn&type=A' | jq
{
  "Status": 0,
  "TC": false,
  "RD": true,
  "RA": true,
  "AD": false,
  "CD": false,
  "Question": [
    {
      "name": "t.cn",
      "type": 1
    }
  ],
  "Answer": [
    {
      "name": "t.cn",
      "type": 1,
      "TTL": 49,
      "data": "116.211.169.137"
    }
  ]
}

Root cause

AliDNS JSON DoH will return a flattened Question response if there is only one question section.

Affected DoH server

AFAIK, https://www.alidns.com/faqs/#dns-safe

NOTE

While there is unclear whether flattened Question is acceptable, https://developers.google.com/speed/public-dns/docs/doh/json says nothing about this. It's very likely being a bug in AliDNS DoH server.

leiless commented 4 years ago

BTW, the go.sum file can be put into .gitignore, since it's derived from go.mod.

m13253 commented 4 years ago

Thank you for your contribution!

The JSON-based DoH protocol isn't standardized yet. So this issue is expected to happen. I am delighted to see Alibaba (and more companies) adopting DoH in their services, which is a good sign.

BTW, the go.sum file can be put into .gitignore, since it's derived from go.mod.

I will do this before my next commit. Thank you for telling me this!

m13253 commented 4 years ago

BTW, the go.sum file can be put into .gitignore, since it's derived from go.mod.

Update: I will not put go.sum into .gitignore for security and safety reasons, see this post for why.

leiless commented 4 years ago

Update: I will not put go.sum into .gitignore for security and safety reasons, see this post for why.

Gotcha 👍

m13253 / dns-over-https