Boulder: va: log CNAMEs in addition to CAA records

[jsha]

When the VA encounters CAA records, it logs the contents of those records. When those records were the result of following a chain of CNAMEs, the CNAMEs are included as part of the response from our recursive resolver. However, the current flow for logging the responses logs only the CAA records, not the CNAMEs. The additional detail would be informative when examining records of CAA checks that involve CNAMEs.

[aarongable]

Notes from our deep dive through this today:

The point where we want to log the CNAMEs is
https://github.com/letsencrypt/boulder/blob/63eafa4a3b3c736b2ef4844d71a125036b1c2faa/va/caa.go#L67-L68

The call stack from there to the actual DNS CAA lookup is pretty deep:
va.checkCAA --> va.checkCAARecords --> va.getCAASet --> va.parallelCAALookup --> dns.LookupCAA --> dns.exchangeOne

There are two problems here:

• Even the lowest level of that call stack assumes that the remote dns resolver (unbound) will automatically follow all CNAMEs it encounters while trying to look up the requested records
• Even if unbound did return the set of CNAMEs that it followed it the process of getting its answer, that's a big stack to return them all the way up

Potential solution (predicated on learning a lot more about how unbound actually works, and how to get info out of it):

1. Add a new LookupCNAME method to boulder's dns.go library
2. Call that method from va.checkCAA, all the way at the top of the stack, to get any CNAMEs that need to be logged
   a. Potential premature optimization: only do this call if the CAA records returned are for a different name than the one we requested, indicating that CNAMEs were followed
   b. Potential pitfall: depending on how CNAME records are returned by unbound, this might potentially involve a significant number of consecutive calls to unbound, getting one CNAME hop at a time

[jsha]

Even the lowest level of that call stack assumes that the remote dns resolver (unbound) will automatically follow all CNAMEs it encounters while trying to look up the requested records

I believe this is part of the RFC in terms of what a recursive resolver is specified to return to its clients. Worth double checking. But it is definitely true in practice.

Add a new LookupCNAME method to boulder's dns.go library

I would prefer to avoid this solution, since we will wind up reimplementing the CNAME chasing part of DNS in Boulder; right now we get to leave most of the DNS semantics to our recursive resolver. This would also mean additional queries (potentially a lot of them), and the possibility of getting different answers for those queries than the ones Unbound relied on.

I mentioned on a call a few weeks back, but should have written in the issue: This is not so much "log CNAMEs" as "log the whole DNS response that Boulder received in response to a CAA query, not just the CAA records."

I definitely agree that there's a lot of call stack to pass the whole response through - that makes this a challenging task. But I think it's worth the effort to save on the network calls.

[beautifulentropy]

bdns.LookupCAA:
https://github.com/letsencrypt/boulder/blob/aa09c1661cd1d313d0a0d41b93c38407f4314713/bdns/dns.go#L153

• Refactor the DNSClient Interface such that LookupCAA returns CAAs, response, nil
• Adjust any tests to account for this

https://github.com/letsencrypt/boulder/blob/aa09c1661cd1d313d0a0d41b93c38407f4314713/bdns/dns.go#L488-L506

• Assign the output of r.Msg.String() to a new var response (docs: https://godoc.org/github.com/miekg/dns#Msg.String)
• Refactor LookupCAA to return CAAs, response, nil
• Adjust any tests to account for this

va.caaResult and va.parallelCAALookup:
https://github.com/letsencrypt/boulder/blob/aa09c1661cd1d313d0a0d41b93c38407f4314713/va/caa.go#L122-L125

• Refactor caaResult struct adding field response, a string containing the response returned by bdns.LookupCAA
• Adjust any tests to account for this

https://github.com/letsencrypt/boulder/blob/aa09c1661cd1d313d0a0d41b93c38407f4314713/va/caa.go#L140-L156

• Refactor the tuple assignment from bdns.LookupCAA to add response
• Adjust any tests to account for this

va.getCAASet:
https://github.com/letsencrypt/boulder/blob/aa09c1661cd1d313d0a0d41b93c38407f4314713/va/caa.go#L158-L172

• Refactor the return line, adding response
• Adjust any tests to account for this

va.CheckCAARecords:
https://github.com/letsencrypt/boulder/blob/aa09c1661cd1d313d0a0d41b93c38407f4314713/va/caa.go#L184-L200

• Reactor the assignment from va.getCAASet, adding response
• Refactor the return line, adding response
• Adjust any tests to account for this

va.checkCAA:
https://github.com/letsencrypt/boulder/blob/aa09c1661cd1d313d0a0d41b93c38407f4314713/va/caa.go#L49-L52

• Reactor the assignment from va.CheckCAARecords, adding response
• Adjust any tests to account for this

https://github.com/letsencrypt/boulder/blob/aa09c1661cd1d313d0a0d41b93c38407f4314713/va/caa.go#L67-L70

• Add quoted response to the va.log.AuditInfof call
• Adjust any tests to account for this

[jsha]

I like this approach! One thing to watch out for: The String() output for the message may be kind of verbose. I think that's fine on balance, since nonempty CAA responses are fairly rare, and the String() output (i.e. dig-like output) is more readable than we we currently emit. I would just make sure that in the empty case we log something that's quite compact, or we'll increase our log size a lot.