test: add dns resolver tests

[tynes]

This PR includes a test that purchases names and then writes one of each of the different Handshake records into the tree. A DNS stub resolver then queries for the name and asserts on the correct data being returned.

The DNSSEC test tests that all resource records sets in the answer and authority sections are signed. If all resource record sets in the additional section were signed as well, it could cause problems because there could potentially be many signatures and would force the DNS server to fall back to TCP. Ideally a different authentication mechanism is devised, because the DNSSEC key is public information anyways.

@boymanjor happy to split this up into smaller PRs if it would make it easier to review

Closes #264

[codecov-io]

Codecov Report

Merging #265 into master will increase coverage by 3.6%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #265     +/-   ##
=========================================
+ Coverage   54.86%   58.47%   +3.6%     
=========================================
  Files         129      129             
  Lines       35786    35790      +4     
  Branches     6032     6032             
=========================================
+ Hits        19635    20929   +1294     
+ Misses      16151    14861   -1290

Impacted Files	Coverage Δ
lib/dns/records.js	77.23% <100%> (+66.43%)	⬆️
lib/dns/resource.js	78.8% <100%> (+59.07%)	⬆️
lib/utils/binary.js	56.41% <0%> (-2.57%)	⬇️
lib/node/fullnode.js	56.16% <0%> (+0.45%)	⬆️
lib/dns/key.js	100% <0%> (+18.18%)	⬆️
lib/dns/server.js	58.51% <0%> (+26.59%)	⬆️
lib/dns/compress.js	78.22% <0%> (+28.04%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a323da8...4ea41aa. Read the comment docs.

[tynes]

I think the question that we need to answer is if all Resource Record sets in the response need to be authenticated. I do not feel like it is acceptable to have only part of the response authenticated. Based on experience playing with dig, I have only gotten RRSIG records back in the answer section. When there are additional records included, there is no RRSIG record that commits to them.

I suppose the combination of sig0 and DNSSEC helps, but the signing keys are public. Resolving Handshake names over the clearweb should be considered just as safe as resolving traditional DNS over clearweb (not safe). hsd should consider using DNS over HTTPS or another type of encryption scheme for its DNS resolution. It is particularly important to prevent man in the middle attacks, especially if cryptocurrency addresses can be resolved.

I think moving forwards with this PR, I can remove the DNSSEC checks in the tests and then remove the additional DNSSEC code and then open an issue to determine how we want to be authenticate our requests. @boymanjor thoughts?

[tynes]

I'd like to turn this into a helper function as I think I'd find it useful building applications on top of Handshake. Not quite sure where it would live or how to generalize it correctly - notice that it uses a magic index into an array.

[tuxcanfly]

Could it be part of utils or some common package?

[tynes]

I'd like to check through hsd and bns to make sure that it already doesn't exist. Would the fn signature be:

name (str), hsd resource (Resource), dns type (int) -> List[str] (list of query strings)

A Resource can have multiple query strings depending on the Records inside of it, but this also may not return the name number of strings in the return value as the number of Records in the Resource which may be confusing to a library consumer.

[tuxcanfly]

OK if we not maybe test/util/common is a good place.

[boymanjor]

I think we should leave this as a specific test helper for now. We can abstract this into a proper library method when the functionality is needed elsewhere.

[tynes]

Ok this makes sense to me. Its not clearly documented anywhere in the Handshake docs that you need to use a particular domain name (with subdomains) to query for some of the rr types. Maybe the bns stub resolver could be extended for this repo where its wrapped with an easier to use API that will handle these cases as well as use handshake native types instead of dns rr types. I could imagine that being very helpful for onboarding new developers, especially if it works in the browser.

[boymanjor]

@tynes I think this is basically ready to go. I left a comment about separating out the TXT record signing into a separate PR. I also think we can clean up the commit history. Basically this PR adds the helper types and then introduces the tests. All other commits can be squashed once we pull out the signing logic. I'd say don't worry about cleaning things up until everything thing that this depends on is merged.

[boymanjor]

I think we should leave this as a specific test helper for now. We can abstract this into a proper library method when the functionality is needed elsewhere.

[tynes]

I also think we can clean up the commit history.

I can rebase on master and rebase out a bunch of the intermediate commits after #292 gets merged.

Thanks for the review @boymanjor

Note that I wanted to add SIG0 validation to this PR as well, I started in #289 but ran into a bug with KEY records. Right now I do not see a way to validate the SIG0 signature without being able to query for the key, which the RFC defines KEY as the RR type used to return the SIG0 key. The authoritative resolver doesn't have logic for KEY RR types at the name .

See: https://github.com/handshake-org/hsd/blob/master/lib/dns/server.js#L228

[tynes]

Needs rebase now that 91f1c27 has been merged

[tynes]

@boymanjor Has been rebased to remove intermediate commits

[boymanjor]

LGTM