-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
setopt: add CURLOPT_DOH_URL #2668
Conversation
6bf97fc
to
eca3d0b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few scattered comments from skimming the code.
lib/doh.c
Outdated
|
||
static const char *doh_strerror(DOHcode code) | ||
{ | ||
return errors[code]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this do bounds checking? While not at risk in the current coding, belts are stylish when coupled with suspenders..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're totally correct of course. I'll fix!
lib/doh.c
Outdated
result = CURLE_OUT_OF_MEMORY; | ||
goto error; | ||
} | ||
p->serverdoh.size = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be set to 1
per the above allocation? (the addition in doh_write_cb()
relies on this being "incorrect" but that seems a bit magic) On that note, is there a reason to allocate 1 rather than use a NULL ptr to indicate "reallocation required" since realloc()
on a NULL ptr is identical to malloc()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right on the size assign. But I think it should rather allocate a sensible default size that can make it avoid the realloc in typical responses. I'll make it a 100 bytes I think.
docs/libcurl/opts/CURLOPT_DOH_URL.3
Outdated
heap space. | ||
|
||
Note that \fIcurl_easy_setopt(3)\fP won't actually parse the given string so | ||
given a bad DOH URL, curl will not be detected a problem until it tries to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this intended to say "will not detect a problem"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
lib/doh.c
Outdated
} | ||
|
||
#define ERROR_CHECK_SETOPT(x,y) result = curl_easy_setopt(doh, x, y); \ | ||
if(result) goto error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will trigger -Wextra-semi
warnings with clang.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll fix!
lib/urldata.h
Outdated
size_t size; | ||
}; | ||
|
||
/* one of these for each DOH HTTP request */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick, but should this say "DOH HTTPS request"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or perhaps just "each DoH request" ...
The short name of the protocol became |
On 04 Sep 2018, at 20:08, Daniel Stenberg ***@***.***> wrote:
@bagder commented on this pull request.
In lib/doh.c:
> + CURLcode result = CURLE_OK;
+ timediff_t timeout_ms;
+ DOHcode d = doh_encode(host, dnstype, p->dohbuffer, sizeof(p->dohbuffer),
+ &p->dohlen);
+ if(d) {
+ failf(data, "Failed to encode DOH packet [%d]\n", d);
+ return CURLE_OUT_OF_MEMORY;
+ }
+
+ p->dnstype = dnstype;
+ p->serverdoh.memory = NULL;
+ /* the memory will be grown as needed by realloc in the doh_write_cb
+ function */
+ p->serverdoh.size = 0;
+
+ if(data->set.doh_get) {
I didn't really make up my mind on this. The spec says a server MUST support both methods, but I'm not sure we necessarily have to offer this option. It's not clear why a user would select to use one or the other... So I sort of left it a bit half-way. =)
Oh, didn't realize that, shame on me for not reading the spec. Maybe a comment stating the above is a good enough compromise until someone has a usecase for GET?
|
Support for DNS-over-HTTPS for name resolving when doing transfers.
Complies with dns-over-https-14, tested a bit against the cloudflare end-point.
Wiki page with status, TODO and more.
This is still early days.
simplest example: