Add support for DNSSEC types from RFC4034

[Arachnid]

Also adds support for decoding 'UNKNOWN_x' type names to the appropriate integer value.

[Arachnid]

Ping! It'd be nice to know if you're happy to have these types added.

[pusateri]

This is awesome to have. I'm trying some sample code now. Getting a decode error. Could be my error though. I'll let you know.

[Arachnid]

I just encountered a decode error myself. I'll debug it tomorrow.

[pusateri]

Looks like it's the RRSIG decoding length:

If I set rrrsig.decode.bytes = length + 2, it works. This isn't probably the best way to fix it but it shows the problem.

printing the response doesn't include the RRSIG RDATA fields though. I first printed them in the RRSIG decoder and then printed the decoded packet. Those fields should show up there.

Make sure you add a new test to catch this case before fixing it.

% node tcp.js
Connected
Received response: 679 bytes
{ typeCovered: 'A',
  algorithm: 7,
  labels: 2,
  originalTTL: 450,
  expiration: 1527605128,
  inception: 1525771795,
  keyTag: 13950,
  signersName: 'getdnsapi.net',
  signature: <Buffer 79 b6 03 e2 43 1f 48 b4 99 5f 50 11 4c 6a 01 99 e5 69 a8 2f 62 a8 6a 38 9a dd ac c1 3b 88 ab 8e af e6 50 77 f4 a7 62 ef 3b c2 c1 8d 0c 9a d5 68 52 f4 ... > }
{ typeCovered: 'NS',
  algorithm: 7,
  labels: 2,
  originalTTL: 450,
  expiration: 1526688235,
  inception: 1524828595,
  keyTag: 13950,
  signersName: 'getdnsapi.net',
  signature: <Buffer 66 1b c8 68 f1 6f d0 a4 f5 ee ff 5d 76 1f 68 66 03 1c 4a e3 36 30 80 63 9b 78 cd 1a 0e 69 44 37 48 30 4f 00 13 59 2f 95 3f a7 cf 45 59 7c a9 8a 61 2b ... > }
{ typeCovered: 'AAAA',
  algorithm: 7,
  labels: 2,
  originalTTL: 450,
  expiration: 1527493410,
  inception: 1525678195,
  keyTag: 13950,
  signersName: 'getdnsapi.net',
  signature: <Buffer 19 a0 65 16 a5 9c 17 89 a2 98 81 64 3a 10 bc ac ed 91 86 44 ab cd 4c f3 2a 54 b6 3f 3e d5 b3 2e 5e 53 a1 e1 20 05 fb ae 10 35 2f fc 3b 1e d1 76 3e 49 ... > }
{ id: 53366,
  type: 'response',
  flags: 1024,
  flag_qr: true,
  opcode: 'QUERY',
  flag_aa: true,
  flag_tc: false,
  flag_rd: false,
  flag_ra: false,
  flag_z: false,
  flag_ad: false,
  flag_cd: false,
  rcode: 'NOERROR',
  questions: [ { name: 'getdnsapi.net', type: 'A', class: 'IN' } ],
  answers: 
   [ { name: 'getdnsapi.net',
       type: 'A',
       ttl: 450,
       class: 'IN',
       flush: false,
       data: '185.49.141.37' },
     { name: 'getdnsapi.net',
       type: 'RRSIG',
       ttl: 450,
       class: 'IN',
       flush: false,
       data: [Object] } ],
  authorities: 
   [ { name: 'getdnsapi.net',
       type: 'NS',
       ttl: 450,
       class: 'IN',
       flush: false,
       data: 'getdnsapi.net' },
     { name: 'getdnsapi.net',
       type: 'NS',
       ttl: 450,
       class: 'IN',
       flush: false,
       data: 'puck.nether.net' },
     { name: 'getdnsapi.net',
       type: 'NS',
       ttl: 450,
       class: 'IN',
       flush: false,
       data: 'dicht.nlnetlabs.nl' },
     { name: 'getdnsapi.net',
       type: 'RRSIG',
       ttl: 450,
       class: 'IN',
       flush: false,
       data: [Object] } ],
  additionals: 
   [ { name: 'getdnsapi.net',
       type: 'AAAA',
       ttl: 450,
       class: 'IN',
       flush: false,
       data: '2a04:b900:0:100::37' },
     { name: 'getdnsapi.net',
       type: 'RRSIG',
       ttl: 450,
       class: 'IN',
       flush: false,
       data: [Object] },
     { name: '.',
       type: 'OPT',
       udpPayloadSize: 4096,
       extendedRcode: 0,
       ednsVersion: 0,
       flags: 32768,
       flag_do: true,
       options: [] } ] }
Connection closed

[Arachnid]

I've identified the bug, and written a patch for it (as well as a test that fails pre-patch).

[pusateri]

RRSIG packet decoding works now for me. The decoded JSON object still looks incomplete:

data: [Object] in RRSIG and data: <Buffer ...> in NSEC3

% node tcp-dnssec.js
Connected
Received response: 1059 bytes
{ id: 22741,
  type: 'response',
  flags: 1027,
  flag_qr: true,
  opcode: 'QUERY',
  flag_aa: true,
  flag_tc: false,
  flag_rd: false,
  flag_ra: false,
  flag_z: false,
  flag_ad: false,
  flag_cd: false,
  rcode: 'NXDOMAIN',
  questions: [ { name: 'foo.getdnsapi.net', type: 'A', class: 'IN' } ],
  answers: [],
  authorities: 
   [ { name: 'eaevgg7iah97si9katcsi3l3am3bj3u4.getdnsapi.net',
       type: 'NSEC3',
       ttl: 3600,
       class: 'IN',
       flush: false,
       data: <Buffer 01 00 00 05 08 94 af 89 0f 02 80 e2 14 14 74 25 57 7b 9d 75 5a 2a 51 62 d8 8b e4 27 ab e6 6c 78 04 41 00 06 04 00 00 00 00 02> },
     { name: 'eaevgg7iah97si9katcsi3l3am3bj3u4.getdnsapi.net',
       type: 'RRSIG',
       ttl: 3600,
       class: 'IN',
       flush: false,
       data: [Object] },
     { name: 'sqa9s0bfkhdjmndls5u4b27javrv1tmh.getdnsapi.net',
       type: 'NSEC3',
       ttl: 3600,
       class: 'IN',
       flush: false,
       data: <Buffer 01 00 00 05 08 94 af 89 0f 02 80 e2 14 14 e9 44 f5 2d 6c 64 61 10 74 22 b3 6d 7b bf 66 a9 68 4e 33 27 00 07 62 01 00 08 00 0a 90> },
     { name: 'sqa9s0bfkhdjmndls5u4b27javrv1tmh.getdnsapi.net',
       type: 'RRSIG',
       ttl: 3600,
       class: 'IN',
       flush: false,
       data: [Object] },
     { name: 'e3itoje4r1dni9vqcglbbni7glrnp1e4.getdnsapi.net',
       type: 'NSEC3',
       ttl: 3600,
       class: 'IN',
       flush: false,
       data: <Buffer 01 00 00 05 08 94 af 89 0f 02 80 e2 14 14 72 9d f8 40 f2 54 52 7e 49 34 57 59 c9 0e a3 55 86 b9 8f c4 00 06 20 00 00 00 00 12> },
     { name: 'e3itoje4r1dni9vqcglbbni7glrnp1e4.getdnsapi.net',
       type: 'RRSIG',
       ttl: 3600,
       class: 'IN',
       flush: false,
       data: [Object] },
     { name: 'getdnsapi.net',
       type: 'SOA',
       ttl: 3600,
       class: 'IN',
       flush: false,
       data: [Object] },
     { name: 'getdnsapi.net',
       type: 'RRSIG',
       ttl: 3600,
       class: 'IN',
       flush: false,
       data: [Object] } ],
  additionals: 
   [ { name: '.',
       type: 'OPT',
       udpPayloadSize: 4096,
       extendedRcode: 0,
       ednsVersion: 0,
       flags: 32768,
       flag_do: true,
       options: [] } ] }
Connection closed

[Arachnid]

@pusateri That's down to the way the prettyprinter you're using is summarising deeply nested Javascript objects.

Correction: That's because the data is a buffer and can't be serialized naively as JSON.

[pusateri]

ok, agreed for RRSIG and NSEC3 isn't covered by these changes so that also makes sense. Thanks for the follow-up.

[pusateri]

I don't have merge permissions but those that do may want some var declarations changed to const. I've been knocked for that in the past.

The README needs updated with the new record types. Other than that, it looks good to me.

% git diff index.js
diff --git a/index.js b/index.js
index eb6fa59..03890ad 100644
--- a/index.js
+++ b/index.js
@@ -780,7 +780,7 @@ rdnskey.decode = function (buf, offset) {
   if (!offset) offset = 0
   const oldOffset = offset

-  var key = {}
+  const key = {}
   var length = buf.readUInt16BE(offset)
   offset += 2
   key.flags = buf.readUInt16BE(offset)
@@ -846,7 +846,7 @@ rrrsig.decode = function (buf, offset) {
   if (!offset) offset = 0
   const oldOffset = offset

-  var sig = {}
+  const sig = {}
   var length = buf.readUInt16BE(offset)
   offset += 2
   sig.typeCovered = types.toString(buf.readUInt16BE(offset))
@@ -917,11 +917,11 @@ typebitmap.decode = function (buf, offset, length) {
   if (!offset) offset = 0
   const oldOffset = offset

-  var typelist = []
+  const typelist = []
   while (offset - oldOffset < length) {
-    var window = buf.readUInt8(offset)
+    const window = buf.readUInt8(offset)
     offset += 1
-    var windowLength = buf.readUInt8(offset)
+    const windowLength = buf.readUInt8(offset)
     offset += 1
     for (var i = 0; i < windowLength; i++) {
       var b = buf.readUInt8(offset + i)
@@ -942,7 +942,7 @@ typebitmap.decode = function (buf, offset, length) {
 typebitmap.decode.bytes = 0

 typebitmap.encodingLength = function (typelist) {
-  var extents = []
+  const extents = []
   for (var i = 0; i < typelist.length; i++) {
     var typeid = types.toType(typelist[i])
     extents[typeid >> 8] |= Math.max(extents[typeid >> 8] || 0, typeid & 0xFF)
@@ -982,7 +982,7 @@ rnsec.decode = function (buf, offset) {
   if (!offset) offset = 0
   const oldOffset = offset

-  var record = {}
+  const record = {}
   var length = buf.readUInt16BE(offset)
   offset += 2
   record.nextDomain = name.decode(buf, offset)
@@ -1035,7 +1035,7 @@ rds.decode = function (buf, offset) {
   if (!offset) offset = 0
   const oldOffset = offset

-  var digest = {}
+  const digest = {}
   var length = buf.readUInt16BE(offset)
   offset += 2
   digest.keyTag = buf.readUInt16BE(offset)

[pusateri]

Silverwind/mafintosh: I would like to see this merged. Any review comments?

[silverwind]

I see you represented signatures/digests/keys as Buffer. While it's probably fine for a low-level library to do so, we could also do what the "presentation" sections of RFC4034 recommends:

DS: The Signature field is represented as a Base64 encoding of the signature.
RRSIG: The Digest MUST be represented as a sequence of case-insensitive hexadecimal digits.
DNSKEY: The Public Key field MUST be represented as a Base64 encoding of the Public Key.

We've been avoiding returning buffers to users because in most cases they are not really useful directly, thought as those buffers represent opaque binary data, I guess we could make an exception. Open to opinions here.

As mentioned, I'd like to see README updated with those new record types. Otherwise, it's looking fine, but I have not completely tested it yet.

@mafintosh could you please add @pusateri as a contributor?

[Arachnid]

@silverwind We've been avoiding returning buffers to users because in most cases they are not really useful directly, thought as those buffers represent opaque binary data, I guess we could make an exception. Open to opinions here.

That was more or less my thinking; elsewhere in supported RRTypes the data is all text already. I was reluctant to use presentation encoding, because that requires applications that want the raw data to decode it again themselves.

As mentioned, I'd like to see README updated with those new record types. Otherwise, it's looking fine, but I have not completely tested it yet.

I'll add that to the PR over the weekend. Thanks for the review!

[pusateri]

I think the Buffer representation is the minimum we need. Adding the presentation form could be done in the future as additional fields without removing the Buffers.

[silverwind]

I think the Buffer representation is the minimum we need. Adding the presentation form could be done in the future as additional fields without removing the Buffers.

I'm just a bit worried because one can not visually compare the buffers with the base64 presentation formats. Maybe add a short note to README on how to convert to presentation formats on each affected property.

[pusateri]

We probably need some usage experience with it to know if it's useful like this or if it needs the presentation format to be useful. If you are just validating the results, it doesn't seem like the presentation format helps. But since it's a new feature, we aren't breaking anything by merging it. It just might not be that useful yet.

[Arachnid]

Sorry for the delay; I've added the new RRTypes to the readme (and sorted the existing ones alphabetically).

[pusateri]

I just tested the NSEC3 decoding and it works for me. Thanks!

[silverwind]

Merged, thanks. I'll be doing some tweaks (removing that dependency and readme things).