Adding support for large UDP responses · Pull Request #684 · libevent/libevent

ghost · 2018-08-19T21:40:58Z

This adds support for DNS records larger than 512 bytes.

I ran into an issue where a domain was failing to resolve because their DNS record was larger than 512 bytes. It is enabled by setting the "max-record-len" option to greater than 512 bytes. By default nothing changes. I tested it with some common public DNS servers (google, cloudflare, Dyn) and they all seemed to work correctly.

Relevant RFC https://tools.ietf.org/html/rfc6891

coveralls · 2018-08-19T22:11:01Z

Coverage increased (+80.5%) to 80.519% when pulling 80f25b6 on dpayne:master into 8483c53 on libevent:master.

ploxiln · 2018-08-19T22:25:22Z

evdns.c

 	base->global_max_nameserver_timeout = 3;
 	base->global_search_state = NULL;
 	base->global_randomize_case = 1;
+	base->global_max_record_len = 512;


could also use DEFAULT_MAX_PACKET_LENGTH here

ghost · 2018-08-23T16:08:26Z

@ploxiln I'm not that familiar with the PR process for libevent. Is there anything else needed here?

ploxiln · 2018-08-23T16:51:21Z

I'm not a maintainer or anything like that.

Just some more patience is needed - the maintainer will probably be around in a week or so, and will probably have some of his own questions and comments.

ghost · 2018-08-23T18:18:58Z

Ah ok didn't realize you weren't a maintainer, I'll wait it out then.

azat

Great stuff, here are some comments, can you take a look?

Also, can you please add some unit test for this? (that will help a lot)

P.S. I just briefly looked and didn't check the protocol, going to do this tomorrow

azat · 2018-08-23T23:26:39Z

evdns.c

 		    sizeof(tv));
-	}
-	return 0;
+    } else if (str_matches_option(option, "max-record-len:")) {


should be enumerated in dns.h (and also feel free to wrap each available option on new line)

azat · 2018-08-23T23:27:05Z

evdns.c

+	* | RDLEN      | u_int16_t    | length of all RDATA          |
+	* | RDATA      | octet stream | {attribute,value} pairs      |
+	* +------------+--------------+------------------------------+ */
+	if ( max_record_len > DEFAULT_MAX_PACKET_LENGTH ) {


this does not suite evdns.c coding style (and in few other places too)

ghost · 2018-08-24T16:47:56Z

Thanks for the comments! I tried to make all the changes you requested. I modified the docs to describe all the options, let me know if you want me to revert that. I also tried to add a unit test. Unfortunately I cannot get tests to pass on my local machine, existing test also fail. Is there something required to get it working?

azat · 2018-08-26T16:03:12Z

Unfortunately I cannot get tests to pass on my local machine, existing test also fail

There are some time-related failures.

I tried to run you test regress --no-fork dns/dns_large_udp and it fails because of evdns_get_global_base()->global_max_record_len;. And using global dns base is a not a good idea, you should use req.base there (and also this got me thinking about proper locking there in you patch, but I will get back with this once I will look at it precisely).

Is there something required to get it working?

Nope.

azat · 2018-08-26T16:10:15Z

And using global dns base is a not a good idea, you should use req.base

Sorry, disinformation. But dns server do not need/have evdns_base, hence this option should be added to evdns_server_port

ghost · 2018-08-27T21:28:29Z

@azat I updated the pull request. I had been looking at the wrong place in the earlier version, instead of changing evdns_server_request_format_response I needed to change nameserver_read. I didn't move global_max_record_len to evdns_server_port since I don't think it's needed there anymore. If you want me to move it somewhere else I can. Sorry for mis-understanding the code in the previous commits, I am still unfamiliar with the internals.

ghost · 2018-09-08T18:37:19Z

The dns regression tests seem to pass now

                                                      ./regress dns/search_empty
                                                      ./regress dns/search
                                                      ./regress dns/search_lower
                                                      ./regress dns/search_cancel
                                                      ./regress dns/dns_large_udp
                                                      ./regress dns/retry
                                                      ./regress dns/retry_disable_when_inactive
                                                      ./regress dns/reissue
                                                      ./regress dns/reissue_disable_when_inactive
                                                      ./regress dns/inflight
                                                      ./regress dns/disable_when_inactive
                                                      ./regress dns/disable_when_inactive_no_ns
                                                      ./regress dns/client_fail_requests

dns/resolve_reverse: DISABLED
0 tests ok.  (1 skipped)
dns/search_empty: [forking] OK
1 tests ok.  (0 skipped)
dns/search: [forking] OK
1 tests ok.  (0 skipped)
dns/search_lower: [forking] OK
1 tests ok.  (0 skipped)
dns/search_cancel: [forking] OK
1 tests ok.  (0 skipped)
dns/dns_large_udp: [forking] OK
1 tests ok.  (0 skipped)
dns/retry: [forking] OK
1 tests ok.  (0 skipped)
dns/retry_disable_when_inactive: [forking] OK
1 tests ok.  (0 skipped)
dns/reissue: [forking] OK
1 tests ok.  (0 skipped)
dns/reissue_disable_when_inactive: [forking] OK
1 tests ok.  (0 skipped)
dns/inflight: [forking] OK
1 tests ok.  (0 skipped)
dns/disable_when_inactive: [forking] OK
1 tests ok.  (0 skipped)
dns/disable_when_inactive_no_ns: [forking] [msg] Nameserver 127.0.0.1:36140 has failed: request timed out.
[msg] All nameservers have failed
OK
1 tests ok.  (0 skipped)
dns/client_fail_requests: [forking] OK
1 tests ok.  (0 skipped)

azat · 2018-09-13T23:11:11Z

I just looked over your changes, and I have some comments: - Maybe it will be better to rename DEFAULT_MAX_PACKET_LENGTH to RFC_MAX_PACKET_LENGTH? - about dns_large_udp test -- it does not check "max-record-len" actually, i.e. simply removing this line and tests passes, while it shoudn't (since there is no requests whose len > DEFAULT_MAX_PACKET_LENGTH) - different style in you new code: tt_assert(! evdns_base_set_option(dns, "max-record-len", "4096")); tt_assert(!evdns_base_nameserver_ip_add(dns, buf)); - different style in your new code against existing code, this is mostly c++ style comments (the one that starts with "//" in evdns.c) putting labels with tabulations in some places, extra white spaces - and don't forget to polish the history once you will finish with the patch-set

ghost · 2018-09-19T02:07:00Z

Hi @azat. I tried to make all the suggested changes.

Currently the unit test dns_large_udp is making sure that the request is not broken. With max-record-len set to > 512, it adds the extended dns record to the dns request. The unit test makes sure that the request does not break.

Unfortunately I wasn't able to add a unit test where the response was larger than 512 bytes. This is because I couldn't find a public url that was larger than this. I ran into this issue because I was running a local dns resolver, unbound, that by default is configured to return UDP responses up to 4096 bytes. Unbound would resolve it through TCP, but return the response in UDP to libevent.

azat · 2018-10-01T08:06:31Z

Unfortunately I wasn't able to add a unit test where the response was larger than 512 bytes.

But you can create answer >512 bytes by your self, could you?

ghost · 2018-10-01T19:45:00Z

Unfortunately I wasn't able to add a unit test where the response was larger than 512 bytes.
But you can create answer >512 bytes by your self, could you?

Yes I can, and I did this on my local box to verify that it worked. I setup a fake dns server to return a large response, but obviously this would only work on my local box. I could be missing something, but I don't how I would do that within a unit without drastically re-writing how the unit tests run.

ghost · 2018-11-30T19:12:56Z

The project I was working that was using libevent ended up going in a different direction and we're no longer using libevent. Since I won't be actively working on libevent I don't think I would be able to actively support any changes, including this one. Feel free to re-use any of the code if someone else wants to try a different approach or can support it.

azat · 2018-12-02T14:41:55Z

Thanks for let me know and for you contribution!
Reopening this, since I this can be interesting.

azat · 2020-05-25T00:19:53Z

@seleznevae are you interesting in this?

seleznevae · 2020-05-25T18:39:16Z

Looks interesting. I'll take a look.

ploxiln reviewed Aug 19, 2018

View reviewed changes

azat reviewed Aug 23, 2018

View reviewed changes

Adding support for large UDP responses

80f25b6

ghost closed this Nov 30, 2018

azat reopened this Dec 2, 2018

azat added the subsystem:dns label Feb 17, 2019

azat mentioned this pull request Apr 28, 2020

evdns: Add support for DNS requests via TCP #1004

Merged

seleznevae mentioned this pull request May 31, 2020

evdns: Add support for setting maximum UDP DNS message size. #1032

Merged

azat closed this in #1032 Jun 2, 2020

Conversation

ghost commented Aug 19, 2018

Uh oh!

coveralls commented Aug 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ploxiln Aug 19, 2018

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 23, 2018

Uh oh!

ploxiln commented Aug 23, 2018

Uh oh!

ghost commented Aug 23, 2018

Uh oh!

azat left a comment

Choose a reason for hiding this comment

Uh oh!

azat Aug 23, 2018

Choose a reason for hiding this comment

Uh oh!

azat Aug 23, 2018

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 24, 2018

Uh oh!

azat commented Aug 26, 2018

Uh oh!

azat commented Aug 26, 2018

Uh oh!

ghost commented Aug 27, 2018

Uh oh!

ghost commented Sep 8, 2018

Uh oh!

azat commented Sep 13, 2018 via email

Uh oh!

ghost commented Sep 19, 2018

Uh oh!

azat commented Oct 1, 2018 via email

Uh oh!

ghost commented Oct 1, 2018

Uh oh!

ghost commented Nov 30, 2018

Uh oh!

azat commented Dec 2, 2018

Uh oh!

azat commented May 25, 2020

Uh oh!

seleznevae commented May 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

coveralls commented Aug 19, 2018 •

edited

Loading