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

DNS Server : bug fix and prettifying #1011

Merged
merged 5 commits into from Mar 4, 2018

Conversation

Projects
None yet
4 participants
@LaurentLouf
Contributor

LaurentLouf commented Jan 19, 2018

I had problem with the last version of the DNS server lib, there were lots of malformed packets on Wireshark and I could not get the library to work. So I checked what was done in the ESP8266 library, integrated some of it and prettified a bit the whole thing. Everything should be a little clearer for someone trying to integrate new functionalities. Tested it with success with WiFiManager.

@me-no-dev

This comment has been minimized.

Member

me-no-dev commented Jan 19, 2018

uhmm I do not really agree. here is what happens in the code:

  • packet is received and checked and domain name is parsed
  • if we use wildcard domain or domain matches what we have in settings, continue
  • start new packet and copy the contents of the received one
  • change Qcount to 0 and Acount to 1 to show that there is one answer in the packet
  • add answer to the end of the packet (after the question)

Now this is all wrong!
Because the question structure is just like the starting of an answer, I just add TTL and data to the end of it, which makes it properly formatted answer.

With the code you included, it did not work for me and nslookup reported malformed packet

@LaurentLouf

This comment has been minimized.

Contributor

LaurentLouf commented Jan 19, 2018

I beg to differ :)
According to the spec (https://tools.ietf.org/pdf/rfc1035.pdf), on page 25, we have the following format for a DNS message :

+---------------------+
| Header |
+---------------------+
| Question | the question for the name server
+---------------------+
| Answer | RRs answering the question
+---------------------+
| Authority | RRs pointing toward an authority
+---------------------+
| Additionall | RRs holding additional information
+---------------------+

So it seems that we agree on the first two, the header and the question. The thing is, you stop there, add a TTL and some data. Okay, but what does the spec say ? Straight to page 29 :

The answer, authority, and additional sections all share the same
format: a variable number of resource records, where the number of
records is specified in the corresponding count field in the header.
Each resource record has the following format:

+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
/ NAME /
+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
| TYPE |
+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
| CLASS |
+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
| TTL |
+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
| RDLENGTH |
+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--|
/ RDATA /
+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+

So, in the reply, if you just copy the question, then add the TTL, RDLength and RData, you just miss 3 fields : the name, the type and the class. You could copy the question again, but to have a complete message, you should, all things considered, copy the question twice instead of once like you do.

My version, which is also the ESP8266 version, use the complete format, with a little trick named message compression described on page 30 (and in my code). I haven't seen anywhere your solution consisting of you just answering with the question padded with the TTL, RDLength and RData, so if you have any reference for that, I'd gladly read it.

@BillyNate

This comment has been minimized.

BillyNate commented Jan 20, 2018

Hi me-no-dev & Loufylouf,

I've checked out both the official master and Loufylouf's.

On my devices (one smartphone and a windows desktop) the captive portal does not work when I'm on espressif:master, but does work when I'm on Loufylouf:master.
I did not do any Wiresharking, but did notice different results when doing an nslookup google.com.

I feel it is safe to say that the captive portal / DNSServer does not fully work for all devices connected to the ESP, and it might be worth looking into this a bit more 😉

@LaurentLouf

This comment has been minimized.

Contributor

LaurentLouf commented Jan 20, 2018

@BillyNate Can you be a bit more specific on the last part of your message ? Did my version work on all of your devices ? Or are you referring to the test of me-no-dev with my version ?

@BillyNate

This comment has been minimized.

BillyNate commented Jan 20, 2018

I added your repository to my remotes, fetched and pulled your master. Once pulled the DNS started to work properly on both devices I tested on.

Does this make it more clear?

@LaurentLouf

This comment has been minimized.

Contributor

LaurentLouf commented Jan 22, 2018

Yeah thanks !

@me-no-dev I now get what you're doing ! I overlooked the Qcount = 0 part. The thing is, while browsing normally on my computer and wiresharking the DNS messages, all query responses carry the question as well ("dns && dns.count.queries == 0" filter returns nothing), so while I agree your solution should be working (after all, there's an ID to match an answer with a question), this does not seem to be the case at least for me (on Ubuntu and Android) and @BillyNate (windows desktop) .

@me-no-dev

This comment has been minimized.

Member

me-no-dev commented Jan 22, 2018

I think the best option here is to create a new answer and do not copy stuff over ;) that whole copy/paste thing sucks :D

@LaurentLouf

This comment has been minimized.

Contributor

LaurentLouf commented Jan 22, 2018

Well, in my code, that's exactly what is done for the answer. Since the format of the question doesn't change, I don't see the problem copy-pasting it. Well, except for the part where we modify the DNS header and then paste the whole buffer, that can be a bit unsettling at first.
I can try to add a commit to clarify this part if you want.

@merlinschumacher merlinschumacher referenced this pull request Jan 22, 2018

Open

Captive portal #16

@tablatronix

This comment has been minimized.

Contributor

tablatronix commented Jan 30, 2018

#1037
Captive portals fail without questions in response.

The compression is a nice touch, was wondering if the entire domain needed to be in response like that.

@tablatronix tablatronix referenced this pull request Jan 31, 2018

Open

ESP32 Compatability #241

Laurent Louf
Add a structure for the DNS question, use it DNS server to store the …
…question data and to create the DNS answer from scratch.
@LaurentLouf

This comment has been minimized.

Contributor

LaurentLouf commented Feb 2, 2018

I've added a commit that creates the DNS question structure and build the response from scratch instead of a dirty copy-paste

@tablatronix

This comment has been minimized.

Contributor

tablatronix commented Feb 11, 2018

@me-no-dev have you had a chance to look this over or document whatever issues you were having?
I would really like to at least see captive portal problems resolved. I could not find ant packet issues with osx, maybe you can detail reproducing your dns issues? What kind of implementation?

@me-no-dev me-no-dev merged commit 694c3a4 into espressif:master Mar 4, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Curclamas added a commit to Curclamas/arduino-esp32 that referenced this pull request Aug 21, 2018

DNS Server : bug fix and prettifying (espressif#1011)
* Retrieve some code from what has been done on the ESP8266. Clarify a bit the signification of several bytes in the response.

* Add the type and class as members of the DNS class for an eventual future use.

* Clarify the sense of a magic number present in DNS server.

* A bit of aesthetics for the DNS server.

* Add a structure for the DNS question, use it DNS server to store the question data and to create the DNS answer from scratch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment