Log whether DNS queries were DNSSEC-validated#8459
Conversation
9cb7334 to
baea2d0
Compare
jsha
left a comment
There was a problem hiding this comment.
Having a long list of positional return values is a bit messy, and having a boolean in there can be a bit confusing since it's not obvious at the call site what the boolean is for.
That said, I don't see an alternative that is clearly better. I considered returning the whole *dns.Msg so it's clear at the call site we're just taking .AuthenticatedData from it; but that passes around way more information than needed and makes bdns less of an abstraction boundary.
I do think each of the functions that is getting a new return value needs a comment indicating what that value means.
| } | ||
|
|
||
| return append(addrsA, addrsAAAA...), resolvers, nil | ||
| return append(addrsA, addrsAAAA...), resolvers, adA && adAAAA, nil |
There was a problem hiding this comment.
This has somewhat confusing semantics in this case:
- Response for
Ais successful, with AD bit set. - Response for
AAAAis an error.
This results in returning false for the ad boolean.
This makes me think maybe LookupHost combining A and AAAA lookups is the wrong abstraction. In the VA, in newHTTPValidationTarget and tryGetChallengeCert, the first thing we do with the combined response is to split it into v4 addresses and v6 addresses. Perhaps we can simplify things with LookupA and LookupAAAA being separate.
When we look up TXT records for dns-01 and dns-account-01 validation, copy the Authenticated Data (AD) bit into the ValidationRecord that we log and store in the database. Similarly, when we look up A/AAAA records for http-01 and tls-alpn-01 validation, copy the AD bit into the ValidationRecord (indirectly via the httpValidationTarget, for http-01). Finally, when we look up CAA records, keep track of the AD bit for each record we find while tree-climbing, and add it to the line we audit-log.
This comes with two caveats:
Fixes #2700