test(dane): cover the fail-closed paths in certificate verification - #34
Merged
Conversation
dane.js was the least covered file in lib, at 83.76 percent of branches, and the gaps were concentrated in exactly the wrong place: the paths that decide what happens when certificate data is malformed. A bug in those does not make verification throw, it makes verification succeed, so an untested branch there is worth more than a percentage point. Five properties are pinned now, each of which has to fail closed: - a chain entry that cannot be parsed refuses the whole chain, rather than being skipped over. That is a deliberate decision in buildVerifiedPath, since the entry skipped might be the link joining the leaf to the pinned anchor - an unparseable leaf refuses a trust anchor record for the same reason - certificate data that cannot be read or hashed does not match, whether the read throws or the data is of a type no hash accepts - a TLSA record that throws while being read is reported as a failed match instead of escaping into the TLS handshake, where it would surface as an unrelated crash - association data supplied as a typed array or ArrayBuffer matches exactly as a Buffer does, and still fails on a wrong digest The first was checked by mutation: weakening the chain refusal to skip past the unparseable entry fails the new test and nothing else. dane.js goes from 94.70 to 99.27 percent of lines and 83.76 to 93.55 percent of branches; across lib it is now 99.59 and 95.36. The two branches left in dane.js are a resolver fallback that cannot be reached while every supported Node has native TLSA resolution, and a catch around Node's own checkIssued, which would need a certificate that makes Node throw. Both are defensive, and reaching them would take inputs no resolver or TLS stack produces.
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
1 participant
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
Tests only.
dane.jswas the least covered file inlib/, at 83.76% of branches, and the gaps sat in the paths that decide what happens when certificate data is malformed. A bug in those does not make verification throw, it makes verification succeed, so an untested branch there is worth more than a percentage point.Properties pinned
Each of these has to fail closed:
buildVerifiedPath: the entry you skip might be the link joining the leaf to the pinned trust anchorArrayBuffermatches exactly as aBufferdoes, and still fails on a wrong digest. This one is realistic rather than synthetic: a custom resolver could plausibly return eitherVerified by mutation
The first property was checked rather than assumed. Weakening the chain refusal to
continuepast the unparseable entry fails the new test and nothing else, so it pins the behaviour rather than passing incidentally.Coverage
Measured with Node's built-in coverage across both suites.
dane.jslib/(lines / branches; functions were already 100%.)
Deliberately not chased
Two branches remain in
dane.js: a resolver fallback unreachable while every supported Node has native TLSA resolution, and acatcharound Node's owncheckIssued, which would need a certificate that makes Node itself throw. Reaching either takes inputs no resolver or TLS stack produces, which adds test weight without adding confidence.196 unit and 7 integration tests pass, lint and prettier clean. No code changed.