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

curl v7.66 onwards does not apply --resolve for the --doh-url #6589

Closed
mbhatia opened this issue Feb 10, 2021 · 3 comments
Closed

curl v7.66 onwards does not apply --resolve for the --doh-url #6589

mbhatia opened this issue Feb 10, 2021 · 3 comments
Labels
name lookup DNS and related tech

Comments

@mbhatia
Copy link

mbhatia commented Feb 10, 2021

I have been trying to test a DNS-over-HTTPS implementation and ran into issues with the curl command line not applying the --resolve option for the --doh-url itself.

Seems like this broke in curl v7.66. It's difficult to show a fully working example because of TLS, but see below for the DNS resolution of --doh-url.

Expected Output (curl v7.65)

doh.example.com successfully resolved to 1.1.1.1

$ docker run -it curlimages/curl:7.65.3 --verbose --resolve 'doh.example.com:443:1.1.1.1' --doh-url "https://doh.example.com:443/dns-query" google.com
* Added doh.example.com:443:1.1.1.1 to DNS cache
* Hostname doh.example.com was found in DNS cache
*   Trying 1.1.1.1:443...
* TCP_NODELAY set
* Found bundle for host doh.example.com: 0x559c0ec1c380 [serially]
* Server doesn't support multiplex (yet)
* Connection #1 is still name resolving, can't reuse
* Hostname doh.example.com was found in DNS cache
*   Trying 1.1.1.1:443...
* TCP_NODELAY set
* Connected to doh.example.com (1.1.1.1) port 443 (#1)
* ALPN, offering h2
* ALPN, offering http/1.1
...

Incorrect Output (curl v7.66)

doh.example.com is not resolved.

$ docker run -it curlimages/curl:7.66.0 --verbose --resolve 'doh.example.com:443:1.1.1.1' --doh-url "https://doh.example.com:443/dns-query" google.com
* Added doh.example.com:443:1.1.1.1 to DNS cache
* Found bundle for host doh.example.com: 0x5634d7899640 [serially]
* Server doesn't support multiplex (yet)
* Connection #0 is still name resolving, can't reuse
* Could not resolve host: doh.example.com
* Closing connection 0
* a DOH request is completed, 1 to go
* DOH request Couldn't resolve host name
* Could not resolve host: doh.example.com
* Closing connection 1
* a DOH request is completed, 0 to go
* DOH request Couldn't resolve host name
* DOH: Too small type A for google.com
* DOH: Too small type AAAA for google.com
* Closing connection 0
curl: (6) Couldn't resolve host name
@bagder bagder added the name lookup DNS and related tech label Feb 10, 2021
@jay
Copy link
Member

jay commented Feb 11, 2021

bisected to b889408 curl: support parallel transfers. Note in debug output it says adding connection #0 twice:

* Added doh.example.com:443:1.1.1.1 to DNS cache
* STATE: INIT => CONNECT handle 0x6955a0; line 1646 (connection #-5000)
* Added connection 0. The cache now contains 1 members
* STATE: CONNECT => WAITRESOLVE handle 0x6955a0; line 1692 (connection #0)
* STATE: INIT => CONNECT handle 0x6b1970; line 1646 (connection #-5000)
* Added connection 0. The cache now contains 1 members
* STATE: CONNECT => WAITRESOLVE handle 0x6b1970; line 1692 (connection #0)
* STATE: INIT => CONNECT handle 0x6b2900; line 1646 (connection #-5000)
* Found bundle for host doh.example.com: 0x201e458 [serially]
* Added connection 1. The cache now contains 2 members
* STATE: CONNECT => WAITRESOLVE handle 0x6b2900; line 1692 (connection #1)
* Could not resolve host: doh.example.com
* The cache now contains 1 members
* Closing connection 0

I tried it as --libcurl output but I could not reproduce until I found this:

curl/src/tool_operate.c

Lines 1250 to 1253 in 2f33be8

/* Avoid having this setopt added to the --libcurl source output. */
result = curl_easy_setopt(curl, CURLOPT_SHARE, share);
if(result)
break;

curl/src/tool_operate.c

Lines 2603 to 2607 in 2f33be8

curl_share_setopt(share, CURLSHOPT_SHARE, CURL_LOCK_DATA_COOKIE);
curl_share_setopt(share, CURLSHOPT_SHARE, CURL_LOCK_DATA_DNS);
curl_share_setopt(share, CURLSHOPT_SHARE, CURL_LOCK_DATA_SSL_SESSION);
curl_share_setopt(share, CURLSHOPT_SHARE, CURL_LOCK_DATA_CONNECT);
curl_share_setopt(share, CURLSHOPT_SHARE, CURL_LOCK_DATA_PSL);

If I comment out CURL_LOCK_DATA_CONNECT then I no longer see the double connection 0. If I comment out CURL_LOCK_DATA_DNS (regardless of if CURL_LOCK_DATA_CONNECT ) it uses the resolve, so I suspect something to do with sharing the DNS cache. I can reproduce with --libcurl output once I add this:

  CURLSH *share = curl_share_init();
  curl_share_setopt(share, CURLSHOPT_SHARE, CURL_LOCK_DATA_DNS);
  curl_easy_setopt(hnd, CURLOPT_SHARE, share);

@jay
Copy link
Member

jay commented Feb 11, 2021

I suspect something to do with sharing the DNS cache

What's happening is the doh easy handles do not use the share from the user's easy handle. There are two hostcaches, the multi hostcache (ie the default hostcache if the user did not specify one, this is used by doh easy handles) and the user's share hostcache (which contains the fake resolve, this is used only by the user's easy handle).

If I change doh to use the share from the user's easy handle then it works but I don't think it can be done safely since I'd guess it's possible that the user can destroy the share while the two doh handles are still in the multi and using it.

diff --git a/lib/doh.c b/lib/doh.c
index 004244c..1c572d1 100644
--- a/lib/doh.c
+++ b/lib/doh.c
@@ -354,6 +354,7 @@ static CURLcode dohprobe(struct Curl_easy *data,
       ERROR_CHECK_SETOPT(CURLOPT_SSL_EC_CURVES,
         data->set.str[STRING_SSL_EC_CURVES]);
     }
+    ERROR_CHECK_SETOPT(CURLOPT_SHARE, data->share);

     doh->set.fmultidone = doh_done;
     doh->set.dohfor = data; /* identify for which transfer this is done */

@bagder
Copy link
Member

bagder commented Feb 14, 2021

I'd guess it's possible that the user can destroy the share while the two doh handles are still in the multi and using it.

I don't think that should be possible (if it is, that's a bug). Those two transfers are "sub-handles" that should get removed and killed if the "main" easy handle that created them is done/removed/killed.

I think the change you suggest here @jay is a sensible one and is in line with what users would expect libcurl to do. Make a PR out of it!

jay added a commit to jay/curl that referenced this issue Feb 15, 2021
- Share the shared object from the user's easy handle with the DOH
  handles.

Prior to this change if the user had set a shared object with shared
cached DNS (CURL_LOCK_DATA_DNS) for their easy handle then that wasn't
used by any associated DOH handles, since they used the multi's default
hostcache.

This change means all the handles now use the same hostcache, which is
either the shared hostcache from the user created shared object if it
exists or if not then the multi's default hostcache.

Fixes curl#6589
Closes #xxxx
@jay jay closed this as completed in e68ee39 Feb 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
name lookup DNS and related tech
Development

Successfully merging a pull request may close this issue.

3 participants