Wire NetLog into the TreeStateTracker

Wire NetLog into the TreeStateTracker

Pass a NetLog instance from the IOThread/ProfileIOData into the
TreeStateTracker instance created in each, so that CT log auditing
events can be logged into the NetLog and DNS queries related to log
auditing are also logged.

A new NetLog source was created to track NetLog events related to
CT log auditing, since they happen independently of the SSL
connections in which the  certificates and SCTs were observed.

BUG=613495
Review-Url: https://codereview.chromium.org/2650803004
Cr-Commit-Position: refs/heads/master@{#447772}
Committed: https://chromium.googlesource.com/chromium/src/+/bbf5af70f469a5f7807ec4573a65c3710cfbb29a

[Eran Messeri, 2017-01-24 11:30:00]

Description was changed from

==========
Wire NetLog into the TreeStateTracker

Pass a NetLog instance from the IOThread/ProfileIOData into the
TreeStateTracker instance created in each, so that CT log auditing
events can be logged into the NetLog and DNS queries related to log
auditing are also logged.

BUG=613495
==========

to

==========
Wire NetLog into the TreeStateTracker

Pass a NetLog instance from the IOThread/ProfileIOData into the
TreeStateTracker instance created in each, so that CT log auditing
events can be logged into the NetLog and DNS queries related to log
auditing are also logged.

BUG=613495
==========

[Eran Messeri, 2017-01-24 11:30:00]

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

[Eran Messeri, 2017-01-24 11:31:46]

Description was changed from

==========
Wire NetLog into the TreeStateTracker

Pass a NetLog instance from the IOThread/ProfileIOData into the
TreeStateTracker instance created in each, so that CT log auditing
events can be logged into the NetLog and DNS queries related to log
auditing are also logged.

BUG=613495
==========

to

==========
Wire NetLog into the TreeStateTracker

Pass a NetLog instance from the IOThread/ProfileIOData into the
TreeStateTracker instance created in each, so that CT log auditing
events can be logged into the NetLog and DNS queries related to log
auditing are also logged.

A new NetLog source was created to track NetLog events related to CT log
auditing, since they happen independently of the SSL connections in which the 
certificates and SCTs were observed.

BUG=613495
==========

[Eran Messeri, 2017-01-24 11:31:59]

Eric, Ryan: PTAL.

[Ryan Sleevi, 2017-01-24 17:21:41]

Description was changed from

==========
Wire NetLog into the TreeStateTracker

Pass a NetLog instance from the IOThread/ProfileIOData into the
TreeStateTracker instance created in each, so that CT log auditing
events can be logged into the NetLog and DNS queries related to log
auditing are also logged.

A new NetLog source was created to track NetLog events related to CT log
auditing, since they happen independently of the SSL connections in which the 
certificates and SCTs were observed.

BUG=613495
==========

to

==========
Wire NetLog into the TreeStateTracker

Pass a NetLog instance from the IOThread/ProfileIOData into the
TreeStateTracker instance created in each, so that CT log auditing
events can be logged into the NetLog and DNS queries related to log
auditing are also logged.

A new NetLog source was created to track NetLog events related to
CT log auditing, since they happen independently of the SSL
connections in which the  certificates and SCTs were observed.

BUG=613495
==========

[Ryan Sleevi, 2017-01-24 17:23:04]

Where are your unit tests for the TreeStateTracker? :)

[eroman, 2017-01-25 21:23:08]

LGTM.

I am less demanding of a unit-test in this case, but adding one would be nice.

https://codereview.chromium.org/2650803004/diff/1/components/certificate_tran...
File components/certificate_transparency/tree_state_tracker.cc (right):

https://codereview.chromium.org/2650803004/diff/1/components/certificate_tran...
components/certificate_transparency/tree_state_tracker.cc:46: net_log,
net::NetLogSourceType::CT_LOG_AUDITOR),
An alternate name could be CT_TREE_STATE_TRACKER, up to you.

https://codereview.chromium.org/2650803004/diff/1/components/certificate_tran...
File components/certificate_transparency/tree_state_tracker.h (right):

https://codereview.chromium.org/2650803004/diff/1/components/certificate_tran...
components/certificate_transparency/tree_state_tracker.h:49: net::NetLog*
raw_net_log);
net_log

That matches the .cc file, and is the idiomatic way of spelling the parameter.

[Eran Messeri, 2017-01-26 15:08:40]

Thanks for the quick review, I'll add unit-tests and update the CL.

https://codereview.chromium.org/2650803004/diff/1/components/certificate_tran...
File components/certificate_transparency/tree_state_tracker.cc (right):

https://codereview.chromium.org/2650803004/diff/1/components/certificate_tran...
components/certificate_transparency/tree_state_tracker.cc:46: net_log,
net::NetLogSourceType::CT_LOG_AUDITOR),
On 2017/01/25 21:23:08, eroman (slow) wrote:
> An alternate name could be CT_TREE_STATE_TRACKER, up to you.

Done, as it'd be more consistent with the name of the class (but TBH I think the
class is not named particularly well. How about renaming it to CTLogAuditor ?)

https://codereview.chromium.org/2650803004/diff/1/components/certificate_tran...
File components/certificate_transparency/tree_state_tracker.h (right):

https://codereview.chromium.org/2650803004/diff/1/components/certificate_tran...
components/certificate_transparency/tree_state_tracker.h:49: net::NetLog*
raw_net_log);
On 2017/01/25 21:23:08, eroman (slow) wrote:
> net_log
> 
> That matches the .cc file, and is the idiomatic way of spelling the parameter.

My bad, this was a left-over from a previous version of the code. Fixed.

[eroman, 2017-01-26 18:20:39]

lgtm

https://codereview.chromium.org/2650803004/diff/1/components/certificate_tran...
File components/certificate_transparency/tree_state_tracker.cc (right):

https://codereview.chromium.org/2650803004/diff/1/components/certificate_tran...
components/certificate_transparency/tree_state_tracker.cc:46: net_log,
net::NetLogSourceType::CT_LOG_AUDITOR),
On 2017/01/26 15:08:40, Eran Messeri wrote:
> On 2017/01/25 21:23:08, eroman (slow) wrote:
> > An alternate name could be CT_TREE_STATE_TRACKER, up to you.
> 
> Done, as it'd be more consistent with the name of the class (but TBH I think
the
> class is not named particularly well. How about renaming it to CTLogAuditor ?)

sgtm

[Eran Messeri, 2017-01-30 11:21:15]

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

[commit-bot: I haz the power, 2017-01-30 11:21:29]

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, 2017-01-30 12:42:46]

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

[commit-bot: I haz the power, 2017-01-30 12:42:47]

Dry run: This issue passed the CQ dry run.

[Eran Messeri, 2017-01-30 17:44:58]

The CQ bit was checked by eranm@chromium.org

[commit-bot: I haz the power, 2017-01-30 17:45:21]

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, 2017-01-30 17:53:07]

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

[commit-bot: I haz the power, 2017-01-30 17:53:08]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Eran Messeri, 2017-01-30 17:58:12]

The CQ bit was checked by eranm@chromium.org

[Eran Messeri, 2017-01-30 17:58:13]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2650803004/#ps40001
(title: "Merging with master")

[commit-bot: I haz the power, 2017-01-30 17:58: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, 2017-01-30 18:07:15]

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

[commit-bot: I haz the power, 2017-01-30 18:07:18]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Eran Messeri, 2017-01-31 17:25:21]

Ryan, Eric: PTAL.

In the latest patchset:
* The CT log auditing result is logged in the NetLog (added a new netlog event
type for that).
* This is verified in the SingleTreeTracker tests.
* Test for the TreeStateTracker has been added to verify it correctly wires the
NetLog and delegates to the SingleTreeTracker it creates.

[eroman, 2017-01-31 19:24:25]

lgtm

https://codereview.chromium.org/2650803004/diff/60001/components/certificate_...
File components/certificate_transparency/single_tree_tracker.cc (right):

https://codereview.chromium.org/2650803004/diff/60001/components/certificate_...
components/certificate_transparency/single_tree_tracker.cc:445: // XXX(eranm):
Should failures be cached? For now, they are not.
Can you update this to TODO(eranm) ?

https://codereview.chromium.org/2650803004/diff/60001/components/certificate_...
File components/certificate_transparency/single_tree_tracker_unittest.cc
(right):

https://codereview.chromium.org/2650803004/diff/60001/components/certificate_...
components/certificate_transparency/single_tree_tracker_unittest.cc:220:
net::TestNetLogEntry& logged_entry = entries[0];
const

https://codereview.chromium.org/2650803004/diff/60001/net/log/net_log_source_...
File net/log/net_log_source_type_list.h (right):

https://codereview.chromium.org/2650803004/diff/60001/net/log/net_log_source_...
net/log/net_log_source_type_list.h:41: SOURCE_TYPE(CT_TREE_STATE_TRACKER)
Side-note: Technically this is a layering violation, as this is defined in
components/ not net...

However we have been casual about these given the current lack of support for
defining outside of //net (crbug.com/90674). At some point should resolve 90674.

[Eran Messeri, 2017-01-31 21:18:06]

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

[Eran Messeri, 2017-01-31 21:18:25]

Thanks for the quick review Eric, done.

https://codereview.chromium.org/2650803004/diff/60001/components/certificate_...
File components/certificate_transparency/single_tree_tracker.cc (right):

https://codereview.chromium.org/2650803004/diff/60001/components/certificate_...
components/certificate_transparency/single_tree_tracker.cc:445: // XXX(eranm):
Should failures be cached? For now, they are not.
On 2017/01/31 19:24:25, eroman (slow) wrote:
> Can you update this to TODO(eranm) ?

Done.

https://codereview.chromium.org/2650803004/diff/60001/components/certificate_...
File components/certificate_transparency/single_tree_tracker_unittest.cc
(right):

https://codereview.chromium.org/2650803004/diff/60001/components/certificate_...
components/certificate_transparency/single_tree_tracker_unittest.cc:220:
net::TestNetLogEntry& logged_entry = entries[0];
On 2017/01/31 19:24:25, eroman (slow) wrote:
> const

Done.

https://codereview.chromium.org/2650803004/diff/60001/net/log/net_log_source_...
File net/log/net_log_source_type_list.h (right):

https://codereview.chromium.org/2650803004/diff/60001/net/log/net_log_source_...
net/log/net_log_source_type_list.h:41: SOURCE_TYPE(CT_TREE_STATE_TRACKER)
On 2017/01/31 19:24:25, eroman (slow) wrote:
> Side-note: Technically this is a layering violation, as this is defined in
> components/ not net...
> 
> However we have been casual about these given the current lack of support for
> defining outside of //net (crbug.com/90674). At some point should resolve
90674.

Acknowledged.

[commit-bot: I haz the power, 2017-01-31 21:18: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, 2017-01-31 22:56:12]

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

[commit-bot: I haz the power, 2017-01-31 22:56:13]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2017-02-01 23:33:46]

Consider this a nit rather than a blocker, and I think if Eric's happy with the
feedback addressing and his LG stands, use it :)

I'll slap an LGTM if you need this for OWNERS, but I'm comfortable if Eric's
comfortable.

https://codereview.chromium.org/2650803004/diff/80001/components/certificate_...
File components/certificate_transparency/single_tree_tracker_unittest.cc
(right):

https://codereview.chromium.org/2650803004/diff/80001/components/certificate_...
components/certificate_transparency/single_tree_tracker_unittest.cc:220: const
net::TestNetLogEntry& logged_entry = entries[0];
Shouldn't you be searching entries for the event code first, rather than
assuming the first entry will always be the entry you're interested in?

[Eran Messeri, 2017-02-02 10:22:01]

eranm@chromium.org changed reviewers:
+ rch@chromium.org

[Eran Messeri, 2017-02-02 10:22:55]

rch: Please review chrome/browser/profiles/profile_io_data.cc, where I wire a
NetLog instance in the same way as the IOThread (which sleevi approved).

https://codereview.chromium.org/2650803004/diff/80001/components/certificate_...
File components/certificate_transparency/single_tree_tracker_unittest.cc
(right):

https://codereview.chromium.org/2650803004/diff/80001/components/certificate_...
components/certificate_transparency/single_tree_tracker_unittest.cc:220: const
net::TestNetLogEntry& logged_entry = entries[0];
On 2017/02/01 23:33:46, Ryan Sleevi wrote:
> Shouldn't you be searching entries for the event code first, rather than
> assuming the first entry will always be the entry you're interested in?

Done, using net::ExpectLogContainsSomewhere to find the entry.

[Ryan Hamilton, 2017-02-02 14:44:12]

chrome/browser/profiles/profile_io_data.cc: LGTM

[Eran Messeri, 2017-02-02 14:55:01]

The CQ bit was checked by eranm@chromium.org

[Eran Messeri, 2017-02-02 14:55:01]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/2650803004/#ps100001
(title: "Looking for entry first")

[commit-bot: I haz the power, 2017-02-02 14:55:17]

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, 2017-02-02 16:06:22]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486047301096400,
"parent_rev": "a41714caed0f89bf9e0924f5d4c242ec70ce3fec", "commit_rev":
"bbf5af70f469a5f7807ec4573a65c3710cfbb29a"}

[commit-bot: I haz the power, 2017-02-02 16:07:06]

Description was changed from

==========
Wire NetLog into the TreeStateTracker

Pass a NetLog instance from the IOThread/ProfileIOData into the
TreeStateTracker instance created in each, so that CT log auditing
events can be logged into the NetLog and DNS queries related to log
auditing are also logged.

A new NetLog source was created to track NetLog events related to
CT log auditing, since they happen independently of the SSL
connections in which the  certificates and SCTs were observed.

BUG=613495
==========

to

==========
Wire NetLog into the TreeStateTracker

Pass a NetLog instance from the IOThread/ProfileIOData into the
TreeStateTracker instance created in each, so that CT log auditing
events can be logged into the NetLog and DNS queries related to log
auditing are also logged.

A new NetLog source was created to track NetLog events related to
CT log auditing, since they happen independently of the SSL
connections in which the  certificates and SCTs were observed.

BUG=613495

Review-Url: https://codereview.chromium.org/2650803004
Cr-Commit-Position: refs/heads/master@{#447772}
Committed:
https://chromium.googlesource.com/chromium/src/+/bbf5af70f469a5f7807ec4573a65...
==========

[commit-bot: I haz the power, 2017-02-02 16:07:09]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/bbf5af70f469a5f7807ec4573a65...