Skip to content

Conversation

@aarongable
Copy link
Contributor

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:

  1. There are two possible reasons for the AD bit to be false: either the records were not DNSSEC-signed, or the resolver did not validate DNSSEC. We have our recursive resolvers configured to always validate DNSSEC and to fail the query if validation fails, but Boulder itself does not and cannot enforce that behavior.
  2. For CAA records, we're only logging whether the "Relevant RRSet" had the AD bit; this PR ignores the AD status of earlier queries in the CAA tree-climbing algorithm which had no relevant RRSet.

Fixes #2700

@aarongable aarongable force-pushed the ad-validation-record branch from 9cb7334 to baea2d0 Compare October 24, 2025 23:48
@aarongable aarongable marked this pull request as ready for review October 24, 2025 23:48
@aarongable aarongable requested a review from a team as a code owner October 24, 2025 23:48
@aarongable aarongable requested a review from jsha October 24, 2025 23:48
Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has somewhat confusing semantics in this case:

  • Response for A is successful, with AD bit set.
  • Response for AAAA is 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Log DNSSEC status for CAA queries

3 participants