mafintosh / dns-packet

An abstract-encoding compliant module for encoding / decoding DNS packets
MIT License
207 stars 71 forks source link

Add support for RCODE and other header flag decoding. #15

Closed pusateri closed 6 years ago

silverwind commented 6 years ago

Is this done yet? It seems to be missing tests and the linter is failing.

pusateri commented 6 years ago

Sorry, still learning. I will fix the lint issue and add a test case.

silverwind commented 6 years ago

I'd suggest you use editorconfig. With it, things like trailing newlines will be handled automatically in by your editor.

pusateri commented 6 years ago

Ok, I think I have handled all of your concerns. Travis-ci seems to be having trouble right now with the node 0.12 version because of a key error unrelated to my changes though.

silverwind commented 6 years ago

Hmm, this just tests the conversion. Do you think it would be possible to test the properties of a decoded header? (Sorry, it's been a while since I've worked on this module)

silverwind commented 6 years ago

Thanks for the test. I've compared the output to dig and would have to suggestions:

dig output

$ dig soa google.com

; <<>> DiG 9.9.7-P3 <<>> soa google.com
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 62316
;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 0, ADDITIONAL: 1

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
;; QUESTION SECTION:
;google.com.      IN  SOA

;; ANSWER SECTION:
google.com.   53  IN  SOA ns1.google.com. dns-admin.google.com. 181011351 900 900 1800 60

dns-socket with dns-packet from your branch

> util.inspect.defaultOptions.depth = null
null
> require('.')().query({
...   questions: [{
.....     type: 'SOA',
.....     name: 'google.com'
.....   }]
... }, 53, '8.8.8.8', console.log)
17586
> null { id: 17586,
  type: 'response',
  flags: 384,
  opcode: 0,
  flag_auth: 0,
  flag_trunc: 0,
  flag_rd: 1,
  flag_ra: 1,
  flag_z: 0,
  flag_ad: 0,
  flag_cd: 0,
  rcode: 'NOERROR',
  questions: [ { name: 'google.com', type: 'SOA', class: 1 } ],
  answers:
   [ { name: 'google.com',
       type: 'SOA',
       class: 1,
       ttl: 59,
       flush: false,
       data:
        { mname: 'ns1.google.com',
          rname: 'dns-admin.google.com',
          serial: 181011351,
          refresh: 900,
          retry: 900,
          expire: 1800,
          minimum: 60 } } ],
  authorities: [],
  additionals: [] } { questions: [ { type: 'SOA', name: 'google.com' } ],
  type: 'query',
  flags: 256,
  id: 17586,
  answers: [],
  authorities: [],
  additionals: [] } 53 '8.8.8.8'
  1. The qr flag that dig displays is missing, should it be added?
  2. I'd change 0/1 in the flags to true/false which seems more appropriate in a language like JavaScript to me.
silverwind commented 6 years ago

Oh, and

  1. I'd suggest resolving opcode to a string e.g. 0 for 'QUERY', like dig does it.
pusateri commented 6 years ago
  1. The QR flag is currently showed as the type field (was there before I made any changes). But I can also add a boolean in addition.
  2. I will change the integers to booleans.
  3. Do I continue creating separate files for translating opcode values to strings like types.js and rcodes.js?

To be clear, your requests are inconsistent in format (even if they are consistent with dig). You are asking me to add a boolean for QR flag from a string that already exists and to change the integer value of opcode to a string.

pusateri commented 6 years ago

By the way, why do commits for pull request #14 show up here in pull request #15 ?

silverwind commented 6 years ago

The QR flag is currently showed as the type field (was there before I made any changes). But I can also add a boolean in addition.

Looking at the RFC, I think it's actually fine the way you have it with the type string property. It seems more natural then having a boolean indicate query vs. response, so lets keep it.

Do I continue creating separate files for translating opcode values to strings like types.js and rcodes.js?

Yeah, I think we should keep that tradition.

By the way, why do commits for pull request #14 show up here in pull request #15 ?

Because you're making the common novice mistake of issueing pull requests from your master branch. I strongly advice to create separate feature branches for every pull request you make, so you can manage them independently. Also see here. Feel free to close this PR and open a new one from your branch.

pusateri commented 6 years ago

Ok. Do you want me to now remove the Q/R boolean from my commit above or leave them both in?

silverwind commented 6 years ago

Hmm, I guess it won't hurt to keep it because it's technically a flag like the others. Let's keep it in.

pusateri commented 6 years ago

Closing this pull request. Will reopen a new one on a separate branch.