LogDnsClient now rejects responses unless they contain exactly one TXT RDATA string

LogDnsClient now rejects responses unless they contain exactly one TXT RDATA string

Previously, multiple TXT RDATA strings would be concatenated if present,
allowing a larger number of audit proof nodes to be delivered. However,
the CT-over-DNS draft RFC
(https://github.com/google/certificate-transparency-rfcs/blob/master/dns/draft-ct-over-dns.md)
explicitly states that the response should contain only one string, and trying
to handle other cases only causes confusion.

BUG=624894
Committed: https://crrev.com/9dc75a8c1abe75bf6dd4674090f7b7591d1ea0f9
Cr-Commit-Position: refs/heads/master@{#423140}

[Rob Percival, 2016-09-27 17:45:13]

The CQ bit was checked by robpercival@chromium.org to run a CQ dry run

[Rob Percival, 2016-09-27 17:45:31]

robpercival@chromium.org changed reviewers:
+ eranm@chromium.org, rsleevi@chromium.org

[Rob Percival, 2016-09-27 17:45:32]

PTAL.

[commit-bot: I haz the power, 2016-09-27 17:45:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 18:38:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 18:38:45]

Dry run: This issue passed the CQ dry run.

[Eran Messeri, 2016-09-27 19:44:16]

On 2016/09/27 17:45:32, Rob Percival wrote:
> PTAL.

LGTM, handing over to Ryan for owners approval.

[Eran Messeri, 2016-09-27 19:44:40]

Is it worth having a test for this case?

[Rob Percival, 2016-09-28 13:08:14]

On 2016/09/27 19:44:40, Eran Messeri wrote:
> Is it worth having a test for this case?

Tests coming right up...

[Rob Percival, 2016-09-28 16:20:38]

On 2016/09/28 13:08:14, Rob Percival wrote:
> On 2016/09/27 19:44:40, Eran Messeri wrote:
> > Is it worth having a test for this case?
> 
> Tests coming right up...

PTAL.

[Rob Percival, 2016-09-28 16:20:44]

The CQ bit was checked by robpercival@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 16:21:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 16:35:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 16:35:34]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Eran Messeri, 2016-09-30 10:02:40]

still lgtm % compilation fix (see comment)

https://codereview.chromium.org/2375693002/diff/20001/components/certificate_...
File components/certificate_transparency/log_dns_client_unittest.cc (right):

https://codereview.chromium.org/2375693002/diff/20001/components/certificate_...
components/certificate_transparency/log_dns_client_unittest.cc:464: std::string
first_chunk_of_proof = std::accumulate(
You may be missing an include, as compilation fails on some platforms with:
../../components/certificate_transparency/log_dns_client_unittest.cc:464:43:
error: no member named 'accumulate' in namespace 'std'
  std::string first_chunk_of_proof = std::accumulate(

[Rob Percival, 2016-09-30 12:04:44]

The CQ bit was checked by robpercival@chromium.org to run a CQ dry run

[Rob Percival, 2016-09-30 12:04:50]

https://codereview.chromium.org/2375693002/diff/20001/components/certificate_...
File components/certificate_transparency/log_dns_client_unittest.cc (right):

https://codereview.chromium.org/2375693002/diff/20001/components/certificate_...
components/certificate_transparency/log_dns_client_unittest.cc:464: std::string
first_chunk_of_proof = std::accumulate(
On 2016/09/30 10:02:40, Eran Messeri wrote:
> You may be missing an include, as compilation fails on some platforms with:
> ../../components/certificate_transparency/log_dns_client_unittest.cc:464:43:
> error: no member named 'accumulate' in namespace 'std'
>   std::string first_chunk_of_proof = std::accumulate(

Done.

[commit-bot: I haz the power, 2016-09-30 12:04:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 12:46:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 12:46:20]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2016-10-03 23:18:15]

lgtm

[Ryan Sleevi, 2016-10-03 23:20:12]

https://codereview.chromium.org/2375693002/diff/40001/components/certificate_...
File components/certificate_transparency/mock_log_dns_traffic.cc (right):

https://codereview.chromium.org/2375693002/diff/40001/components/certificate_...
components/certificate_transparency/mock_log_dns_traffic.cc:203:
CHECK_LE(str.size(), 0xFFul);
nit: The 0xFFul is weird to see here. C++11 will auto-sign extend to ul/ull, so
you could just state 0xFFu.

However, CHECK'ing during test code is very much an anti-pattern that should not
be done. Tests should fail, not crash.

https://codereview.chromium.org/2375693002/diff/40001/components/certificate_...
components/certificate_transparency/mock_log_dns_traffic.cc:204:
answer.insert(answer.end(), static_cast<char>(str.size()));
Why not use the safe-numerics here, if you want the CHECKing behaviour?

[Rob Percival, 2016-10-04 11:46:47]

https://codereview.chromium.org/2375693002/diff/40001/components/certificate_...
File components/certificate_transparency/mock_log_dns_traffic.cc (right):

https://codereview.chromium.org/2375693002/diff/40001/components/certificate_...
components/certificate_transparency/mock_log_dns_traffic.cc:203:
CHECK_LE(str.size(), 0xFFul);
On 2016/10/03 23:20:12, Ryan Sleevi (slow) wrote:
> nit: The 0xFFul is weird to see here. C++11 will auto-sign extend to ul/ull,
so
> you could just state 0xFFu.
> 
> However, CHECK'ing during test code is very much an anti-pattern that should
not
> be done. Tests should fail, not crash.

Would you recommend instead making all of the methods in this class that
currently ASSERT/EXPECT/CHECK return bool and ASSERTing on that in the tests?

https://codereview.chromium.org/2375693002/diff/40001/components/certificate_...
components/certificate_transparency/mock_log_dns_traffic.cc:204:
answer.insert(answer.end(), static_cast<char>(str.size()));
On 2016/10/03 23:20:12, Ryan Sleevi (slow) wrote:
> Why not use the safe-numerics here, if you want the CHECKing behaviour?

It seemed like overkill for a scenario that shouldn't occur unless someone sets
up a test with an excessively long string, in which case crashing appeared
reasonable.

[Rob Percival, 2016-10-04 15:11:05]

https://codereview.chromium.org/2375693002/diff/40001/components/certificate_...
File components/certificate_transparency/mock_log_dns_traffic.cc (right):

https://codereview.chromium.org/2375693002/diff/40001/components/certificate_...
components/certificate_transparency/mock_log_dns_traffic.cc:204:
answer.insert(answer.end(), static_cast<char>(str.size()));
On 2016/10/04 11:46:47, Rob Percival wrote:
> On 2016/10/03 23:20:12, Ryan Sleevi (slow) wrote:
> > Why not use the safe-numerics here, if you want the CHECKing behaviour?
> 
> It seemed like overkill for a scenario that shouldn't occur unless someone
sets
> up a test with an excessively long string, in which case crashing appeared
> reasonable.

Done.

[Rob Percival, 2016-10-04 15:11:12]

The CQ bit was checked by robpercival@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 15:11:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 16:28:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-04 16:28:59]

Dry run: This issue passed the CQ dry run.

[Rob Percival, 2016-10-05 09:27:06]

https://codereview.chromium.org/2375693002/diff/40001/components/certificate_...
File components/certificate_transparency/mock_log_dns_traffic.cc (right):

https://codereview.chromium.org/2375693002/diff/40001/components/certificate_...
components/certificate_transparency/mock_log_dns_traffic.cc:203:
CHECK_LE(str.size(), 0xFFul);
On 2016/10/04 11:46:47, Rob Percival wrote:
> On 2016/10/03 23:20:12, Ryan Sleevi (slow) wrote:
> > nit: The 0xFFul is weird to see here. C++11 will auto-sign extend to ul/ull,
> so
> > you could just state 0xFFu.
> > 
> > However, CHECK'ing during test code is very much an anti-pattern that should
> not
> > be done. Tests should fail, not crash.
> 
> Would you recommend instead making all of the methods in this class that
> currently ASSERT/EXPECT/CHECK return bool and ASSERTing on that in the tests?

Here's an implementation of that: https://codereview.chromium.org/2389993003/

[Rob Percival, 2016-10-05 13:25:58]

The CQ bit was checked by robpercival@chromium.org

[Rob Percival, 2016-10-05 13:25:59]

The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org,
rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/2375693002/#ps60001
(title: "Use checked_cast")

[commit-bot: I haz the power, 2016-10-05 13:26:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 13:30:12]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-05 13:31:54]

Description was changed from

==========
LogDnsClient now rejects responses unless they contain exactly one TXT RDATA
string

Previously, multiple TXT RDATA strings would be concatenated if present,
allowing a larger number of audit proof nodes to be delivered. However,
the CT-over-DNS draft RFC
(https://github.com/google/certificate-transparency-rfcs/blob/master/dns/draft...)
explicitly states that the response should contain only one string, and trying
to handle other cases only causes confusion.

BUG=624894
==========

to

==========
LogDnsClient now rejects responses unless they contain exactly one TXT RDATA
string

Previously, multiple TXT RDATA strings would be concatenated if present,
allowing a larger number of audit proof nodes to be delivered. However,
the CT-over-DNS draft RFC
(https://github.com/google/certificate-transparency-rfcs/blob/master/dns/draft...)
explicitly states that the response should contain only one string, and trying
to handle other cases only causes confusion.

BUG=624894

Committed: https://crrev.com/9dc75a8c1abe75bf6dd4674090f7b7591d1ea0f9
Cr-Commit-Position: refs/heads/master@{#423140}
==========

[commit-bot: I haz the power, 2016-10-05 13:31:57]

Patchset 4 (id:??) landed as
https://crrev.com/9dc75a8c1abe75bf6dd4674090f7b7591d1ea0f9
Cr-Commit-Position: refs/heads/master@{#423140}