Certificate Transparency: Change CTVerifyResult to have a single list

Certificate Transparency: Change CTVerifyResult to have a single list

CTVerifyResult currently has 3 lists of SCTs according to their verification
status. As a result, adding a new SCT verification status that doesn't directly
map into one of the existing lists loses information.

This change switches the CTVerifyResult to have a single 'scts' list that
contains SCTs and their statuses, making it easy to extend the SCT
verify status enum and generally unifying handling of the SCTs list
across the different layers of Chrome.

BUG=634006
Committed: https://crrev.com/4bed0b57efe07c94d98836f5ae82fa6557e2b262
Cr-Commit-Position: refs/heads/master@{#411924}

[Eran Messeri, 2016-08-08 23:05:13]

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

[Eran Messeri, 2016-08-08 23:05:43]

eranm@chromium.org changed reviewers:
+ eroman@chromium.org, estark@chromium.org

[commit-bot: I haz the power, 2016-08-08 23:05:58]

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

[Eran Messeri, 2016-08-08 23:07:08]

Eric, Emily,
This is another step on the way to extending the ct::SCTVerifyStatus enum.
There are no functional changes, only refactoring of the three lists to one.

[commit-bot: I haz the power, 2016-08-08 23:25:09]

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

[commit-bot: I haz the power, 2016-08-08 23:25:10]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[estark, 2016-08-09 02:43:22]

https://codereview.chromium.org/2225223002/diff/1/chrome/browser/ssl/chrome_e...
File chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc (right):

https://codereview.chromium.org/2225223002/diff/1/chrome/browser/ssl/chrome_e...
chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc:195:
std::unique_ptr<base::ListValue> sct_list;
Hrmm, not sure I understand this change, could you explain?

https://codereview.chromium.org/2225223002/diff/1/net/cert/ct_verify_result.cc
File net/cert/ct_verify_result.cc (right):

https://codereview.chromium.org/2225223002/diff/1/net/cert/ct_verify_result.c...
net/cert/ct_verify_result.cc:27: for (auto sct_and_status : sct_and_status_list)
const auto& sct_and_status

[Eran Messeri, 2016-08-09 08:13:48]

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

[commit-bot: I haz the power, 2016-08-09 08:14:10]

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

[Eran Messeri, 2016-08-09 08:14:21]

https://codereview.chromium.org/2225223002/diff/1/chrome/browser/ssl/chrome_e...
File chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc (right):

https://codereview.chromium.org/2225223002/diff/1/chrome/browser/ssl/chrome_e...
chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc:195:
std::unique_ptr<base::ListValue> sct_list;
On 2016/08/09 at 02:43:22, estark wrote:
> Hrmm, not sure I understand this change, could you explain?

The goal was to save repeated calls to:

    ASSERT_NO_FATAL_FAILURE(FindSCTInReportList(
        expected_sct.sct, expected_sct.status, *(sct_list.get())));

But this change is not, strictly speaking, necessary, so I've reverted it.

https://codereview.chromium.org/2225223002/diff/1/net/cert/ct_verify_result.cc
File net/cert/ct_verify_result.cc (right):

https://codereview.chromium.org/2225223002/diff/1/net/cert/ct_verify_result.c...
net/cert/ct_verify_result.cc:27: for (auto sct_and_status : sct_and_status_list)
On 2016/08/09 at 02:43:22, estark wrote:
> const auto& sct_and_status

Done.

[commit-bot: I haz the power, 2016-08-09 10:16:50]

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

[commit-bot: I haz the power, 2016-08-09 10:16:51]

Dry run: This issue passed the CQ dry run.

[estark, 2016-08-09 20:02:04]

non-owners LGTM

https://codereview.chromium.org/2225223002/diff/20001/net/cert/ct_signed_cert...
File net/cert/ct_signed_certificate_timestamp_log_param.cc (right):

https://codereview.chromium.org/2225223002/diff/20001/net/cert/ct_signed_cert...
net/cert/ct_signed_certificate_timestamp_log_param.cc:63: // list is a
dictionary created by SCTToDictionary.
nit: perhaps update this comment to note that we only output SCTs matching
|match_status|

[eroman, 2016-08-10 19:00:40]

https://codereview.chromium.org/2225223002/diff/20001/net/cert/ct_signed_cert...
File net/cert/ct_signed_certificate_timestamp_log_param.cc (right):

https://codereview.chromium.org/2225223002/diff/20001/net/cert/ct_signed_cert...
net/cert/ct_signed_certificate_timestamp_log_param.cc:83:
SCTListToPrintableValues(ct_result->scts, ct::SCT_STATUS_OK));
Is there a need to preserve the old format?

AFAICT there isn't a consumer that relies on this structure (just getting dumped
into NetLog right)?

In that case, can you re-structure it to match the code organization?

For instance add an "scts" parameter that is an array of all the SCTs.

This should be more informative when debugging issues (in case order ends up
mattering), and also means if a new veryfstatus is added don't need to remember
to update this.

If you do not make that change, then please add a breadcrumb to inform callers
that this needs to be updated. Something like this would suffice:

static_assert(ct::SCT_STATUS_MAX == 4, "When adding new verify status add it to
the NetLog here");

https://codereview.chromium.org/2225223002/diff/20001/net/cert/ct_verify_resu...
File net/cert/ct_verify_result.h (right):

https://codereview.chromium.org/2225223002/diff/20001/net/cert/ct_verify_resu...
net/cert/ct_verify_result.h:44: // Return a list of SCTs from
|sct_and_status_list| whose status matches
Return --> Returns

https://codereview.chromium.org/2225223002/diff/20001/net/ssl/ssl_info.cc
File net/ssl/ssl_info.cc (right):

https://codereview.chromium.org/2225223002/diff/20001/net/ssl/ssl_info.cc#new...
net/ssl/ssl_info.cc:80: signed_certificate_timestamps.clear();
This behavior is different than previously, where it only appended. Is this
intentional?

If the intent is assignment, perhaps can write it more simply as:

signed_certificate_timestamps = ct_verify_result.scts;

https://codereview.chromium.org/2225223002/diff/20001/net/test/ct_test_util.cc
File net/test/ct_test_util.cc (right):

https://codereview.chromium.org/2225223002/diff/20001/net/test/ct_test_util.c...
net/test/ct_test_util.cc:409: return (result.scts.size() > 0) &&
(result.scts[0].status == SCT_STATUS_OK) &&
What are the guarantees around ordering? For more general code should this
search for a match instead of just looking at the first?

[Eran Messeri, 2016-08-10 22:04:51]

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

[Eran Messeri, 2016-08-10 22:05:07]

Addressed Eric's and Emily's comments, PTAL. Thanks for the thorough review.

https://codereview.chromium.org/2225223002/diff/20001/net/cert/ct_signed_cert...
File net/cert/ct_signed_certificate_timestamp_log_param.cc (right):

https://codereview.chromium.org/2225223002/diff/20001/net/cert/ct_signed_cert...
net/cert/ct_signed_certificate_timestamp_log_param.cc:63: // list is a
dictionary created by SCTToDictionary.
On 2016/08/09 at 20:02:04, estark wrote:
> nit: perhaps update this comment to note that we only output SCTs matching
|match_status|

Done.

https://codereview.chromium.org/2225223002/diff/20001/net/cert/ct_signed_cert...
net/cert/ct_signed_certificate_timestamp_log_param.cc:83:
SCTListToPrintableValues(ct_result->scts, ct::SCT_STATUS_OK));
On 2016/08/10 at 19:00:40, eroman wrote:
> Is there a need to preserve the old format?
> 
> AFAICT there isn't a consumer that relies on this structure (just getting
dumped into NetLog right)?
> 
> In that case, can you re-structure it to match the code organization?
> 
> For instance add an "scts" parameter that is an array of all the SCTs.
> 
> This should be more informative when debugging issues (in case order ends up
mattering), and also means if a new veryfstatus is added don't need to remember
to update this.
> 
> If you do not make that change, then please add a breadcrumb to inform callers
that this needs to be updated. Something like this would suffice:
> 
> static_assert(ct::SCT_STATUS_MAX == 4, "When adding new verify status add it
to the NetLog here");

Re-structured as suggested. Now the status is passed all the way to
SCTToDictionary and is added to the dictionary as an integer value there.

Note this affects the MultiLogCTVerifier unit test.

https://codereview.chromium.org/2225223002/diff/20001/net/cert/ct_verify_resu...
File net/cert/ct_verify_result.h (right):

https://codereview.chromium.org/2225223002/diff/20001/net/cert/ct_verify_resu...
net/cert/ct_verify_result.h:44: // Return a list of SCTs from
|sct_and_status_list| whose status matches
On 2016/08/10 at 19:00:40, eroman wrote:
> Return --> Returns

Done.

https://codereview.chromium.org/2225223002/diff/20001/net/ssl/ssl_info.cc
File net/ssl/ssl_info.cc (right):

https://codereview.chromium.org/2225223002/diff/20001/net/ssl/ssl_info.cc#new...
net/ssl/ssl_info.cc:80: signed_certificate_timestamps.clear();
On 2016/08/10 at 19:00:40, eroman wrote:
> This behavior is different than previously, where it only appended. Is this
intentional?
> 
> If the intent is assignment, perhaps can write it more simply as:
> 
> signed_certificate_timestamps = ct_verify_result.scts;

Reverted to the previous behaviour - of appending at the end of the scts list
without clearing it.

https://codereview.chromium.org/2225223002/diff/20001/net/test/ct_test_util.cc
File net/test/ct_test_util.cc (right):

https://codereview.chromium.org/2225223002/diff/20001/net/test/ct_test_util.c...
net/test/ct_test_util.cc:409: return (result.scts.size() > 0) &&
(result.scts[0].status == SCT_STATUS_OK) &&
On 2016/08/10 at 19:00:40, eroman wrote:
> What are the guarantees around ordering? For more general code should this
search for a match instead of just looking at the first?

No guarantees indeed - Changed the code to look for a match.

[commit-bot: I haz the power, 2016-08-10 22:05:45]

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-08-11 00:13:31]

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

[commit-bot: I haz the power, 2016-08-11 00:13:32]

Dry run: This issue passed the CQ dry run.

[eroman, 2016-08-11 22:55:50]

lgtm

https://codereview.chromium.org/2225223002/diff/40001/net/cert/ct_signed_cert...
File net/cert/ct_signed_certificate_timestamp_log_param.cc (right):

https://codereview.chromium.org/2225223002/diff/40001/net/cert/ct_signed_cert...
net/cert/ct_signed_certificate_timestamp_log_param.cc:45:
out->SetInteger("verification_status", status);
[optional] Unless the integer code is going to be stable, I suggest turning it
into a string here (easier to then read the NetLog). There are other options
like doing the reverse mapping on the NetLog viewer UI that can be explored, but
unless the data size is going to be a problem I suggest just saving a string.

https://codereview.chromium.org/2225223002/diff/40001/net/test/ct_test_util.cc
File net/test/ct_test_util.cc (right):

https://codereview.chromium.org/2225223002/diff/40001/net/test/ct_test_util.c...
net/test/ct_test_util.cc:409: if (result.scts.size() == 0)
[optional] Can delete this line.

[Eran Messeri, 2016-08-13 21:53:25]

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

[Eran Messeri, 2016-08-13 21:53:26]

https://codereview.chromium.org/2225223002/diff/40001/net/cert/ct_signed_cert...
File net/cert/ct_signed_certificate_timestamp_log_param.cc (right):

https://codereview.chromium.org/2225223002/diff/40001/net/cert/ct_signed_cert...
net/cert/ct_signed_certificate_timestamp_log_param.cc:45:
out->SetInteger("verification_status", status);
On 2016/08/11 at 22:55:50, eroman wrote:
> [optional] Unless the integer code is going to be stable, I suggest turning it
into a string here (easier to then read the NetLog). There are other options
like doing the reverse mapping on the NetLog viewer UI that can be explored, but
unless the data size is going to be a problem I suggest just saving a string.

The integer could should be stable as the same enum (SCTVerifyStatus) is used
for UMA and its documentation explicitly states that the values should remain
stable.

I've simply turned this into a string as you suggested, though,  as there's
already a handy function to do this
(https://cs.chromium.org/chromium/src/net/cert/ct_sct_to_string.h?l=33).

As a side note, are there memory constraints around NetLog logging? the reason I
stuck with an integer is the lower memory consumption. Is that a concern when
logging to NetLog?

https://codereview.chromium.org/2225223002/diff/40001/net/test/ct_test_util.cc
File net/test/ct_test_util.cc (right):

https://codereview.chromium.org/2225223002/diff/40001/net/test/ct_test_util.c...
net/test/ct_test_util.cc:409: if (result.scts.size() == 0)
On 2016/08/11 at 22:55:50, eroman wrote:
> [optional] Can delete this line.

Done.

[commit-bot: I haz the power, 2016-08-13 21:53:34]

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-08-13 22:49:13]

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

[commit-bot: I haz the power, 2016-08-13 22:49:13]

Dry run: This issue passed the CQ dry run.

[Eran Messeri, 2016-08-14 20:57:28]

The CQ bit was checked by eranm@chromium.org

[Eran Messeri, 2016-08-14 20:57:29]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org,
eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2225223002/#ps60001
(title: "NetLog int to string")

[commit-bot: I haz the power, 2016-08-14 20:57:37]

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-08-14 21:00:57]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-14 21:02:23]

Description was changed from

==========
Certificate Transparency: Change CTVerifyResult to have a single list

CTVerifyResult currently has 3 lists of SCTs according to their verification
status. As a result, adding a new SCT verification status that doesn't directly
map into one of the existing lists loses information.

This change switches the CTVerifyResult to have a single 'scts' list that
contains SCTs and their statuses, making it easy to extend the SCT
verify status enum and generally unifying handling of the SCTs list
across the different layers of Chrome.

BUG=634006
==========

to

==========
Certificate Transparency: Change CTVerifyResult to have a single list

CTVerifyResult currently has 3 lists of SCTs according to their verification
status. As a result, adding a new SCT verification status that doesn't directly
map into one of the existing lists loses information.

This change switches the CTVerifyResult to have a single 'scts' list that
contains SCTs and their statuses, making it easy to extend the SCT
verify status enum and generally unifying handling of the SCTs list
across the different layers of Chrome.

BUG=634006

Committed: https://crrev.com/4bed0b57efe07c94d98836f5ae82fa6557e2b262
Cr-Commit-Position: refs/heads/master@{#411924}
==========

[commit-bot: I haz the power, 2016-08-14 21:02:24]

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

[eroman, 2016-08-15 17:48:07]

https://codereview.chromium.org/2225223002/diff/40001/net/cert/ct_signed_cert...
File net/cert/ct_signed_certificate_timestamp_log_param.cc (right):

https://codereview.chromium.org/2225223002/diff/40001/net/cert/ct_signed_cert...
net/cert/ct_signed_certificate_timestamp_log_param.cc:45:
out->SetInteger("verification_status", status);
On 2016/08/13 21:53:26, Eran Messeri wrote:
> On 2016/08/11 at 22:55:50, eroman wrote:
> > [optional] Unless the integer code is going to be stable, I suggest turning
it
> into a string here (easier to then read the NetLog). There are other options
> like doing the reverse mapping on the NetLog viewer UI that can be explored,
but
> unless the data size is going to be a problem I suggest just saving a string.
> 
> The integer could should be stable as the same enum (SCTVerifyStatus) is used
> for UMA and its documentation explicitly states that the values should remain
> stable.
> 
> I've simply turned this into a string as you suggested, though,  as there's
> already a handy function to do this
> (https://cs.chromium.org/chromium/src/net/cert/ct_sct_to_string.h?l=33).
> 
> As a side note, are there memory constraints around NetLog logging? the reason
I
> stuck with an integer is the lower memory consumption. Is that a concern when
> logging to NetLog?

<background>....

Memory is always a concern. In the case of NetLog these are serialized to disk
so the important metric is filesize, not so much the amount of RAM (which is
short lived).

That said, there is also a tension with ease-of-use and revisioning. So unless
the status code is going to appear frequently, I discourage against it.

The NetLog format aims to be self-describing (by virtue of being JSON), and
independent of C++ side changes. This makes it easy on the C++ developers as I
don't want them to have to worry about refactoring changes breaking NetLogs
(case in point this CL).

Yet at the same time there is enough information in the log format for it to be
useful when consuming. String statuses are self-describing to the viewer so no
special visualization needs to be added for those events in the NetLog viewer
application.

There are some frequently used enums like load status and net error codes which
are encoded in the logs as integers, however the log file itself also carries
the mapping back to their symbolic name (i.e. like a symbol table) so this
really just serves as a dumb form of compression (effectively their string value
is still encoded in the log).

At some point in the future I hope to make NetLog's use a compressed file
format, so the duplication of string status wasting disk space will be less of a
concern.