Add support for DNS-over-TLS upstream resolvers

[gdm85]

This PR adds support for DNS-over-TLS (DoT) upstream resolvers.

It adds a new configuration option which affects declared resolvers globally:

# Only use DNS-over-TLS for upstream DNS queries
tls_only = false

A different approach, more invasive but which gives more flexibility, would be to get away from the global type booleans and instead support configuration declarations like:

# Upstream DNS resolver
# If multiple servers are specified, a random one will be chosen each time.
upstream = [
    "tcp:1.1.1.1:53",
    "tcp-tls:1.0.0.1:853",
    "udp:8.8.8.8:53",
]

Let me know if you prefer the latter instead, I could rewrite this patch.

Edit: rewritten the patch to use above approach.

[m13253]

Thank you for your contribution!

I personally prefer your approach written below, so we can configure the protocol individually for each server. Let's make it: udp: First try UDP, then fall back to TCP if TC bit received. tcp: Only use TCP. tcp-tls: Only use DoT.

After the patch is tested thoroughly, I will perhaps bump a version number to indicate an incompatible configuration file change.

[gdm85]

@m13253 I have followed-up; the rationale for the breaking change is that this way users can "discover" about the new options.

We could also change the defaults to include some DoT address, or even some from https://dnsprivacy.org/wiki/display/DP/DNS+Privacy+Test+Servers

Please note that this PR has a kinda-unrelated commit 36012e2 that improves error logging/handling; I needed that to troubleshoot some issue (and actually would like to improve all the cases of log.Println(err) and unchecked Write calls.

[m13253]

the rationale for the breaking change is that this way users can "discover" about the new options.

I guess simply write some comments in the configuration file. And we can then make the program reject the old configuration file. (That's why I also need to bump the version number to indicate the user to change the configuration.)

We could also change the defaults to include some DoT address, or even some from https://dnsprivacy.org/wiki/display/DP/DNS+Privacy+Test+Servers

I agree, we can have some 1.1.1.1, 8.8.8.8, 9.9.9.9 preloaded in the default configuration files. I prefer to only include very-large-scale and geographical distributed providers to minimize the disturbance to the service providers and to improve global access time.

Please note that this PR has a kinda-unrelated commit 36012e2 that improves error logging/handling; I needed that to troubleshoot some issue (and actually would like to improve all the cases of log.Println(err) and unchecked Write calls.

Thank you for that commit. It is always better to have prettier debugging outputs. 👍

[gdm85]

I guess simply write some comments in the configuration file. And we can then make the program reject the old configuration file. (That's why I also need to bump the version number to indicate the user to change the configuration.)

There are already some comments, please check whether they are sufficient or not? It will already reject the old configuration because it validates the upstream entries on load and the old one are not valid second the new format.

[gdm85]

On a side note: this format could (and maybe should?) be extended to the bootstrap servers specified for the client.

[m13253]

There are already some comments, please check whether they are sufficient or not?

Thank you for that. I suggest writing the comments in more details:

# You can use "udp", "tcp" or "tcp-tls" for the type prefix.
# For "udp", UDP will first be used, and switch to TCP when the server asks to
# or the response is too large.
# For "tcp", only TCP will be used.
# For "tcp-tls", DNS-over-TLS (RFC 7858) will be used to secure the upstream
# connection.

On a side note: this format could (and maybe should?) be extended to the bootstrap servers specified for the client.

From an aesthetic perspective, I would like to. But technically a bootstrap server can't be a DoT server, or that would lead to a chicken-or-the-egg problem because DoT itself may require bootstrapping.

[gdm85]

But technically a bootstrap server can't be a DoT server, or that would lead to a chicken-or-the-egg problem because DoT itself may require bootstrapping.

I assume you mean that only some DoT servers use an IP address in their TLS certificate, thus the others still need name resolution first.

However, I think that even now - for both doh-client and doh-server - calling net.ResolveUDPAddr or net.ResolveTCPAddr will use Go's DNS resolver (either Go-native or CGO-type)? In this sense the "level-0" resolution (upstreams for doh-server or bootstrap entries for doh-client) is always happening with Go's resolver.

[gdm85]

Thank you for that. I suggest writing the comments in more details:

Updated.

[m13253]

I assume you mean that only some DoT servers use an IP address in their TLS certificate, thus the others still need name resolution first.

Yes. And if the DoT server is already using IP-based TLS certificate, that provider can also provide a DoH server with IP-based TLS certificate, and then we don't need bootstrapping at all now. (And of course we also need OCSP stapling to avoid boostrapping.)

However, I think that even now - for both doh-client and doh-server - calling net.ResolveUDPAddr or net.ResolveTCPAddr will use Go's DNS resolver (either Go-native or CGO-type)? In this sense the "level-0" resolution (upstreams for doh-server or bootstrap entries for doh-client) is always happening with Go's resolver.

Exactly. Eventually we go down to the Go runtime resolver or libc resolver for the last resolving.

[m13253]

Merge complete! Thank you for your awesome work! Later (around 1-2 days) I will publish a new version after some testing.

[gdm85]

Great! Thanks, it has been a pleasure :)

[m13253]

I assume you mean that only some DoT servers use an IP address in their TLS certificate, thus the others still need name resolution first.

Yes. And if the DoT server is already using IP-based TLS certificate, that provider can also provide a DoH server with IP-based TLS certificate, and then we don't need bootstrapping at all now. (And of course we also need OCSP stapling to avoid bootrapping.)

... so it is not meaningful to have a DoH server requiring bootstrapping but a DoT server which does not require bootstrapping. That's why I assume we only need plain unencrypted DNS for bootstrapping.

I think, if you want to completely eliminate any packets going through port 53, if may also want to use /etc/hosts to fix the server address.

[gdm85]

... so it is not meaningful to have a DoH server requiring bootstrapping but a DoT server which does not require bootstrapping. That's why I assume we only need plain unencrypted DNS for bootstrapping.

My point was more along the lines that if someone currently specifies a hostname for the upstreams or the bootstrap servers, they will rely on Go/system's resolution for it, and if they were to rely on some DoT server it would be as good as that one (from the MiTM part) plus the encryption part.

IMO the real bootstrap problem with DoT is that you need to have a good bundle of certificates and good time synchronisation, but that problem (or chain of trust) is "out of the hands" of doh-server or doh-client, so to speak.

[m13253]

My point was more along the lines that if someone currently specifies a hostname for the upstreams or the bootstrap servers, they will rely on Go/system's resolution for it, and if they were to rely on some DoT server it would be as good as that one (from the MiTM part) plus the encryption part.

Oh, now I got your idea.

IMO the real bootstrap problem with DoT is that you need to have a good bundle of certificates and good time synchronisation, but that problem (or chain of trust) is "out of the hands" of doh-server or doh-client, so to speak.

I agree. And it is actually the same problem with DoH. You need to have a good bundle of certificates and good time synchronisation for either DoT or DoH, since they both run on top of TLS. And that's why I have to passthrough several common NTP servers (e.g. pool.ntp.org) in the default configuration file.

[m13253]

Please have a look at issue #101. You introduced a lot of useless debug info. Please clean them up and submit me a separate PR.

Try to remove some, and reformat others, and check for the verbose flag.

[gdm85]

@m13253 your tone is not so nice, and you are removing yourself from the responsibility of reviewing/merging this PR; look for example how you could have worded it:

It looks like (see #101) that unnecessary debug information was introduced in this PR and I did not notice it when reviewing and merging this PR. Would you be interested in contributing a PR to address some of these problems? Thanks

Otherwise it sounds like you are requesting or ordering me something, which will not work.

[m13253]

your tone is not so nice, and you are removing yourself from the responsibility of reviewing/merging this PR; look for example how you could have worded it:

I am really sorry. I was studying for an exam and suddenly received the update from this repo, so I didn't spend too much time thinking before posting. I apologize for my rudeness.

It should be my responsibility for not checking the code before merging.

Otherwise it sounds like you are requesting or ordering me something, which will not work.

Now I understand. I will do this myself.

[gdm85]

No worries, I was in fact thinking to contribute an unrelated PR for caching support and that is why I found the comment off-putting.