docs(readme): correct the TLSA claims and restructure for reading - #33
Merged
Conversation
The custom TLSA resolver was documented as a compatibility shim for old Node, which stopped being true when the floor moved to 22.19.0: native dns.resolveTlsa landed in 22.15.0, so every supported version has it and the option is never required. The section promised "for Node.js versions without native TLSA support", described a situation that can no longer arise, and its example passed an undefined customResolveTlsa, so anyone copying it got a ReferenceError. The same undefined identifier had been pasted into the DNSSEC and MTA-STS examples, where it was noise either way. The option does still have a purpose, just not that one: routing TLSA lookups through a resolver of your own, usually to get DNSSEC validation that Node cannot yet report. It is documented as that now, with a resolver actually written out, and the examples that never needed it no longer carry it. Two other things did not match the code. The family-specific local address options were described as taking precedence over localAddress, where in fact localAddress wins whenever it matches the family of the host and the specific ones only fill in otherwise. The Node requirement sat inside the DANE section although it applies to the whole package, and is now stated once at the top. The rest is structure. Option lists that had grown into paragraphs of prose are tables; the address validation rules, which had become five dense blocks, are a section of their own with a table per tier. Notes and warnings use GitHub alerts, so the two things that can actually cost someone a security property, that rejectUnauthorized must be off for DANE-EE to reach the verifier and that connectHook can rewrite the host after validation, are no longer prose in the middle of a paragraph.
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
Documentation only. Corrects three claims that no longer match the code, and restructures the parts that had grown into walls of prose.
The TLSA resolver was documented as something it is not
dane.resolveTlsawas presented as a compatibility shim for old Node. That stopped being true when the floor moved to 22.19.0: nativedns.resolveTlsalanded in 22.15.0, so every supported version has it,lib/dane.jsalready prefers it automatically, and the option is never required.Verified against real DANE-enabled hosts with nothing configured:
The section also promised "for Node.js versions without native TLSA support", a situation that can no longer arise, and its example passed an undefined
customResolveTlsa, so copying it produced aReferenceError. The same undefined identifier had been pasted into the DNSSEC and MTA-STS examples, where it was noise either way.The option still has a real purpose, just not that one: routing TLSA lookups through a resolver of your own, usually to obtain the DNSSEC validation Node cannot yet report. It is documented as that now, with a resolver actually written out, and the examples that never needed it no longer carry it.
Two further mismatches
The local address precedence was backwards. The docs said the family-specific options take precedence over
localAddress.updateLocalAddressForTargetdoes the opposite, confirmed by running it: withlocalAddress: '192.0.2.1'andlocalAddressIPv4: '198.51.100.1'against an IPv4 host, the connection binds 192.0.2.1.localAddresswins whenever it matches the family of the host; the specific options fill in only when it is unset or the host is the other family.The Node requirement sat inside the DANE section although it applies to the whole package. It is stated once at the top now, and the three-row version table is gone because it had collapsed to "every supported version".
Restructure
Option lists that had grown into paragraphs are tables. Address validation, which had become five dense blocks, is a section of its own with a table per tier: always refused,
blockLocalAddresses,blockReservedNetworks, and the IPv6 forms that carry an IPv4 address. Lists of CIDRs are far easier to scan in a table than in a sentence.Notes and warnings use GitHub alerts. The two that can actually cost someone a security property are no longer prose buried mid-paragraph:
rejectUnauthorizedmust befalseor a self-signed DANE-EE certificate is rejected by PKIX before the verifier ever runsconnectHookreceives the same options object used to open the socket, so a hook rewritingoptions.hostconnects wherever it saysChecks
All nine internal anchors resolve, prettier and lint clean, no code touched.