Panic when using `-H` with non-https nameserver

[Nemo157]

I have a standard UDP/TCP-only nameserver setup, trying to pass -H to dog results in a panic and abort instead of an error message (there's nothing useful in the backtrace):

> dog nemo157.com -H
thread 'main' panicked at 'Invalid HTTPS nameserver', dns-transport/src/https.rs:55:50
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
[1]    2815113 abort (core dumped)  dog nemo157.com -H

Using current master:

> jq -r '.installs | keys[] | select(contains("dog"))' ~/.cargo/.crates2.json
dog 0.1.0 (git+https://github.com/ogham/dog#445ed98bcfd8d2e4cbb8e924e6cf69ad5f9589e1)

[ogham]

You need to pass a HTTPS-responsible nameserver when using the -H or --https flags. (This is still a bug, but that's how to get the error to go away: see https://dns.lookup.dog/features/dns-over-https)

I know exactly where this is coming from, so don't worry about the short backtrace: this is the panicking line. We need to extract the domain from the URL, and if the nameserver isn't a valid URL, then we can't send the request. In order to get the release out last weekend I hurried and put a panic! call in there, because even if dog handles the error it would just exit immediately anyway, and I didn't think people would be trying their own HTTPS nameservers — but what's happening here is that dog notices you aren't passing in a nameserver, so it uses the system default one, which is an IP address, which isn't a URL, so it crashes. I didn't think about that.

The primary fix here is probably to handle the case where the user uses --https with no nameserver, rather than attempting to connect to the default one and failing. The secondary fix is to remove the panic.

[dbrgn]

I assume you've chosen panic = "abort" as panic handling strategy to get smaller binaries? The difference is 120 KiB (6% of the total size). Switching back to unwind panic behavior would get you regular stack traces without core dumps, and slightly less confused users when an error occurs.

(Of course, once the software is mature, panics should not really happen anymore, so that might be a good tradeoff.)

[ogham]

I added a check (the primary fix), so if you don't pass a nameserver, you at least get a warning rather than a crash.

[nicolasff]

I thought that 1.1.1.1 supported DNS over HTTPS, but when I tried it I also got a panic. Stack trace on a debug target:

$ RUST_BACKTRACE=full ./target/debug/dog @1.1.1.1 -H google.com
thread 'main' panicked at 'Invalid HTTPS nameserver', dns-transport/src/https.rs:55:50
stack backtrace:
   0:        0x10e7685ce - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h0afb3dc3ec8cd05f
   1:        0x10e792a3e - core::fmt::write::h39441ef24fae20ea
   2:        0x10e768369 - std::io::Write::write_fmt::h2ffecc964e3c3ddd
   3:        0x10e782125 - std::panicking::default_hook::{{closure}}::h1a491655bcf6394f
   4:        0x10e781e4c - std::panicking::default_hook::h038c301fad559a62
   5:        0x10e782635 - std::panicking::rust_panic_with_hook::h489020cfd35413ea
   6:        0x10e768deb - std::panicking::begin_panic_handler::{{closure}}::he498abc45ca35fbf
   7:        0x10e768748 - std::sys_common::backtrace::__rust_end_short_backtrace::h4a2a0fae6b0989d8
   8:        0x10e782213 - _rust_begin_unwind
   9:        0x10e797e6f - core::panicking::panic_fmt::h0003130af3c08aa0
  10:        0x10e7977da - core::option::expect_failed::hce2e468a386e6fd7
  11:        0x10e6adb83 - core::option::Option<T>::expect::h54432f315cc86993
                               at /private/tmp/rust-20201018-98686-ada2ss/rustc-1.47.0-src/library/core/src/option.rs:333
  12:        0x10e6b0b15 - <dns_transport::https::HttpsTransport as dns_transport::Transport>::send::hff3e6dd414120fd3
                               at /redacted/dog/dns-transport/src/https.rs:55
  13:        0x10e6729e9 - dog::run::h93896b6e9730de3f
                               at /redacted/dog/src/main.rs:106
  14:        0x10e672227 - dog::main::h62af114c71cdaa72
                               at /redacted/dog/src/main.rs:52
  15:        0x10e6316be - core::ops::function::FnOnce::call_once::h1808e7e54c0c73b6
                               at /private/tmp/rust-20201018-98686-ada2ss/rustc-1.47.0-src/library/core/src/ops/function.rs:227
  16:        0x10e670d11 - std::sys_common::backtrace::__rust_begin_short_backtrace::h5c524b58581c973b
                               at /private/tmp/rust-20201018-98686-ada2ss/rustc-1.47.0-src/library/std/src/sys_common/backtrace.rs:137
  17:        0x10e667374 - std::rt::lang_start::{{closure}}::h1fe1510e3453bd58
                               at /private/tmp/rust-20201018-98686-ada2ss/rustc-1.47.0-src/library/std/src/rt.rs:66
  18:        0x10e78295c - std::rt::lang_start_internal::h0a88825a8a52fb96
  19:        0x10e667351 - std::rt::lang_start::h099aa969eaeac164
                               at /private/tmp/rust-20201018-98686-ada2ss/rustc-1.47.0-src/library/std/src/rt.rs:65
  20:        0x10e672d72 - _main

This was built from f756c6769dbdea0661a0ed074568d935f62b3b89.

edit: I see in https.rs that split_domain removes an expected https:// prefix, and then searches for the next /:

    fn split_domain(&self) -> Option<(&str, &str)> {
        if let Some(sp) = self.url.strip_prefix("https://") {
            if let Some(colon_index) = sp.find('/') {
                return Some((&sp[.. colon_index], &sp[colon_index ..]));
            }
        }

        None
    }

By the way, this code seems to have been copied from dns-transport/src/tls.rs:

        if let Some(colon_index) = self.addr.find(':') {
            &self.addr[.. colon_index]
        }

^ the name colon_index is correct here, but not in split_domain, where it should be something like slash_index. Not that it changes the logic or anything.

Based on this information I did manage to pass the argument in the expected format, and figured out why 1.1.1.1 didn't work: their docs say that the endpoint for CloudFlare's DNS over HTTPS is https://cloudflare-dns.com/dns-query.

And it does work fine in this example:

$ dog -H @https://cloudflare-dns.com/dns-query A google.com
A google.com. 42s   172.217.5.110

So things seem to work! That said, I think it might be helpful to provide a bit more information in the error message, maybe explain that it needs to start with https:// and needs at least one / after the host part of the URL. Not only that, but -H uses "the list of resolvers" from the command line, and those also need an @ prefix I guess.

You could have something like:

1. -H @1.1.1.1 – Error: "1.1.1.1" does not look like an HTTPS URL.
2. -H @https://1.1.1.1 – Error: the URL "https://1.1.1.1" does not define a path after the host (either that or add an implicit /)
3. -H https://1.1.1.1 – no resolver found. Provide a resolver with @some.ip.address.here

I don't know… something like that ¯\_(ツ)_/¯

[acdha]

I eventually figured out that the syntax needed to be something like this:

$ dog example.org @https://1.1.1.1/dns-query -H

I support all of @nicolasff's suggestions and would add that there are two heuristics which could make it extra friendly:

1. -H in conjunction with a server value which doesn't start with https could be treated as https://SERVER_VALUE/dns-query (to be pedantically correct this could match only IPv4 or IPv6 addresses since the latter form would need to be bracketed in [ and ]).
2. A server value which supports @https:// could automatically imply -H