mafintosh / dns-packet

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

parse and generate EDNS0 options #46

Closed hildjj closed 5 years ago

hildjj commented 5 years ago

Better support for "popular" EDNS0 options from the IANA list. I've the start of a patch for ECS that allows this:

    options: [ {
      code: 8,
      ip: 'fe80::/64'
    } ]

on parse, you get this:

       options:
        [ { code: 8,
            data: <Buffer 00 02 40 00 fe 80 00 00 00 00 00 00>,
            family: 2,
            sourcePrefixLength: 64,
            scopePrefixLength: 0,
            ip: 'fe80::' } ] } ] }

note that the data field is retained for backward compatibility.

If this approach makes sense and is interesting to the maintainers, I'll send a PR eventually with support for all of the options in the IANA list that have RFCs that are clear enough to make sense to me, including tests.

silverwind commented 5 years ago

Better parsing for the options is welcome. Not sure about parsing the CIDR in this module. I'd suggest returning just the parsed string instead (I'd like to eventually get rid of the ip dependency because it is unmaintained and for the IPv6 case, full of bugs).

hildjj commented 5 years ago

Nod, I'm ok pushing the CIDR calculation to the caller, and just using the sourcePrefixLength to select the number of IP bytes to send.

I wonder if it's worth starting a new project to replace ip. Seems like a thing that's needed.

silverwind commented 5 years ago

Light usage of ip like this is fine.

Yes, a ip/cidr library similar to Python's netaddr is on my wishlist as well. So far, I've been keeping on top of things using https://github.com/whitequark/ipaddr.js and https://github.com/silverwind/cidr-tools mostly.

hildjj commented 5 years ago

ipaddr.js at least appears to be actively maintained and have unit tests.

hildjj commented 5 years ago

Anything else you'd like to see on this before I send a PR? Want it squashed?

silverwind commented 5 years ago

README needs to be updated with the new format. Also left a comment on the option names. Other than that, I'd say go ahead and submit.