feat(dns): accept a promise-based custom DNS resolver - #35
Merged
Conversation
The only way to supply a resolver was dnsOptions.resolve, which takes a callback, and which omits the record type entirely for A lookups. Everything inside the library has been async/await for a while, so the callback was promisified straight back into a promise, and the arity quirk had to be preserved because resolvers written against it may only accept the two-argument shape. dnsOptions.resolveAsync is the same job without either wart. It receives a domain and a record type, A lookups included, and returns the records. Returning them directly is as acceptable as returning a promise, since awaiting covers both, so a resolver backed by a cache no longer has to pretend to be asynchronous; a synchronous throw surfaces as a rejection for the same reason. resolve keeps working exactly as before, including its two-argument A lookups. When both are set resolveAsync wins and the callback is not consulted, so a deployment can migrate one option at a time. getDnsResolver now takes the whole dnsOptions object rather than one function, since it has three sources to choose between and every call site already had the object to hand. It is internal, not exported from the package. The name is resolveAsync rather than resolver because the latter differs from resolve by one character, and a typo between them would silently select a different code path.
…le resolvers Renamed before it ships, on review, so nothing released is affected. "Async" named the wrong axis: both resolver options are asynchronous, and ironically the suffixed one is the only one allowed to answer synchronously. It also collided with the meaning the suffix already carries in this codebase, where resolveTlsaAsync is an internally promisified callback while the public promise-based options, dane.resolveTlsa and dane.checkDnssecSecure, take no suffix at all. resolveRecords follows those, and says what the function does. Two findings from the security review are fixed with it. A resolver option set to something uncallable used to throw from inside every lookup; the typeof guard added with the new option turned that into a silent fall back to the system resolver. For an MTA that deliberately configures a resolver, say a DNSSEC-validating one, a typo would have quietly sent mail through a resolver nobody chose, with nothing to notice. Both options are now refused up front with a TypeError naming the offender. Unset, or explicitly null, still falls through. The MTA-STS policy filter called .filter on whatever the resolver returned, so an answer supplying its own filter method chose what survived validation, and mailauth connects to whatever comes back. That was the only unvalidated address the review could get near a socket. The filter now runs only over a real array. Also folded the "an omitted type means A" rule, which had spread to four places, into a single default parameter, so the legacy two-argument A lookup lives on one line of one branch. tryResolve no longer needs its ternary and names A explicitly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
dnsOptions.resolveRecords, a DNS resolver that returns the records instead of taking a callback. The existingdnsOptions.resolvekeeps working unchanged.Why
resolvewas the only way to supply a resolver, and it carries two warts. It takes a callback, which the library promisifies straight back into a promise since everything inside has been async/await for a while. And it omits the record type entirely for A lookups, callingresolve(domain, callback)rather thanresolve(domain, type, callback), so every implementation has to handle two arities.resolveRecordsis the same job without either. It receives a domain and an explicit record type, A lookups included, and returns the records:Returning the records directly is as valid as returning a promise, since awaiting covers both, so a cache-backed resolver does not have to pretend to be asynchronous. A synchronous throw becomes a rejection for the same reason.
resolvekeeps working exactly as before, arity quirk included. When both are setresolveRecordswins and the callback is never consulted, so a deployment can migrate one option at a time.Naming
Named
resolveAsyncat first and renamed on review, before it shipped, so nothing released is affected. "Async" named the wrong axis: both options are asynchronous, and the suffixed one is ironically the only one allowed to answer synchronously. It also collided with the meaning the suffix already carries here, whereresolveTlsaAsyncis an internally promisified callback while the public promise-based options,dane.resolveTlsaanddane.checkDnssecSecure, take no suffix at all.resolveRecordsfollows those and says what the function does.Two security fixes found in review
A mistyped resolver no longer silently falls back to system DNS. The
typeofguard added with the new option turned what used to be a loud per-lookup throw into a quiet fallback. For an MTA that deliberately configures a resolver, a DNSSEC-validating one for instance, a typo would have sent mail through a resolver nobody chose with nothing to notice. Both options are now refused up front with aTypeErrornaming the offender; unset ornullstill falls through.The MTA-STS policy filter now only trusts a real array. It called
.filteron whatever the resolver returned, so an answer supplying its ownfiltermethod chose what survived validation, and mailauth connects to whatever comes back. This was the only unvalidated address the review could get near a socket. Verified directly: a duck-typed{length: 1, filter: () => ['127.0.0.1']}previously reachedhttps.requestwith host127.0.0.1and now produces zero requests, while the delivery still completes over the legitimate MX. Covered by a regression test.Review evidence
The security review found no HIGH or MEDIUM issues in the resolver work itself, backed by:
temporary, identically on both paths, so an attacker who can cause SERVFAIL cannot steer resolution to a fallback they controlThe simplify review also caught that one of the new tests was vacuous, asserting only that a resolver came back, which every branch satisfies. It now pins the behaviour and fails when the guard is weakened.
Tests and docs
206 unit and 7 integration tests pass; coverage 99.59% lines, 95.60% branches, 100% functions. README gains a Custom DNS resolver section covering both forms, how to report a lookup failure, precedence, the new refusal, and a note that TLSA lookups bypass
dnsOptionsentirely and needdane.resolveTlsa.