Certificate Transparency: Add observer for Signed Certificate Timestamps

Certificate Transparency: Add observer for Signed Certificate Timestamps

This will allow hooking a monitor that will check inclusion of
certificates that were accompanied by SCTs in CT logs.

BUG=506227
Committed: https://crrev.com/b68c27e0dfd59fe943aba065d724fcb8b565a679
Cr-Commit-Position: refs/heads/master@{#337250}

[Eran Messeri, 2015-07-02 10:58:43]

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

[Ryan Sleevi, 2015-07-02 13:23:08]

https://codereview.chromium.org/1216153010/diff/1/net/cert/ct_verifier.h
File net/cert/ct_verifier.h (right):

https://codereview.chromium.org/1216153010/diff/1/net/cert/ct_verifier.h#newc...
net/cert/ct_verifier.h:35: // checking functionality.
Why not have this interface provide the certificate now? Doesn't seem to be
coupled to inclusion checking.

https://codereview.chromium.org/1216153010/diff/1/net/cert/ct_verifier.h#newc...
net/cert/ct_verifier.h:37: scoped_refptr<CTLogVerifier> log_verifier) = 0;
FIRST ORDER DESIGN: Why pass this as a scoped_refptr<> [if it was, prefer
const-ref]

That is, why not pass it as CTLogVerifier*? Is there an asynchronous delivery
issue (doesn't seem to be? Certainly not from the base interface)

SECOND ORDER DESIGN: Why pass the verifier at all? This presumes a particular
implementation strategy (that some higher-order consumer will re-use the same
Observer across multiple Verifiers), but it's questionable about whether or not
that should influence the base design.

If it does, then it arguably warrants a discussion via comments on the Observer
that the same Observer may be used across multiple verifiers, so as to make it
obvious/intuitive to the reader that providing the Verifier via OnSCTVerified is
an intentional design decision of an intended API contract, rather than just an
incidental effect of weird layering.

nit: Assuming that the second order doesn't change, I'm slightly inclined to
suggest the order is |log_verifier|, |sct| rather than |sct|, |log_verifier|,
since if there's an argument for keeping |log_verifier| in the API signature,
it's likely the high-order bit for processing, and thus somewhat argues for
providing first.

https://codereview.chromium.org/1216153010/diff/1/net/cert/ct_verifier.h#newc...
net/cert/ct_verifier.h:40: virtual ~CTVerifier();
It's unclear why you're no longer inlining this? This class remains a pure
interface class, thus inlining the dtor is fine.

https://codereview.chromium.org/1216153010/diff/1/net/cert/ct_verifier.h#newc...
net/cert/ct_verifier.h:58: virtual void StopNotifications() = 0;
Document.

Seems like this would be more logical after SetObserver.

DESIGN: Does it make more sense to do SetObserver(nullptr) as the unregistration
action? Why or why not?

[Eran Messeri, 2015-07-02 14:12:52]

All done, comments do simplify the overall design quite a bit, thanks! PTAL.

https://codereview.chromium.org/1216153010/diff/1/net/cert/ct_verifier.h
File net/cert/ct_verifier.h (right):

https://codereview.chromium.org/1216153010/diff/1/net/cert/ct_verifier.h#newc...
net/cert/ct_verifier.h:35: // checking functionality.
On 2015/07/02 13:23:08, Ryan Sleevi (slow through 7-15 wrote:
> Why not have this interface provide the certificate now? Doesn't seem to be
> coupled to inclusion checking.

Done.

https://codereview.chromium.org/1216153010/diff/1/net/cert/ct_verifier.h#newc...
net/cert/ct_verifier.h:37: scoped_refptr<CTLogVerifier> log_verifier) = 0;
On 2015/07/02 13:23:08, Ryan Sleevi (slow through 7-15 wrote:
> FIRST ORDER DESIGN: Why pass this as a scoped_refptr<> [if it was, prefer
> const-ref]
> 
> That is, why not pass it as CTLogVerifier*? Is there an asynchronous delivery
> issue (doesn't seem to be? Certainly not from the base interface)
> 
> SECOND ORDER DESIGN: Why pass the verifier at all? This presumes a particular
> implementation strategy (that some higher-order consumer will re-use the same
> Observer across multiple Verifiers), but it's questionable about whether or
not
> that should influence the base design.
> 
> If it does, then it arguably warrants a discussion via comments on the
Observer
> that the same Observer may be used across multiple verifiers, so as to make it
> obvious/intuitive to the reader that providing the Verifier via OnSCTVerified
is
> an intentional design decision of an intended API contract, rather than just
an
> incidental effect of weird layering.
> 
> nit: Assuming that the second order doesn't change, I'm slightly inclined to
> suggest the order is |log_verifier|, |sct| rather than |sct|, |log_verifier|,
> since if there's an argument for keeping |log_verifier| in the API signature,
> it's likely the high-order bit for processing, and thus somewhat argues for
> providing first.

Good point, the observer can hold a list of CTLogVerifiers and match the right
one based on the log_id in the SCT, so it's unnecessary here. Removed.

https://codereview.chromium.org/1216153010/diff/1/net/cert/ct_verifier.h#newc...
net/cert/ct_verifier.h:40: virtual ~CTVerifier();
On 2015/07/02 13:23:08, Ryan Sleevi (slow through 7-15 wrote:
> It's unclear why you're no longer inlining this? This class remains a pure
> interface class, thus inlining the dtor is fine.

Reverted.

https://codereview.chromium.org/1216153010/diff/1/net/cert/ct_verifier.h#newc...
net/cert/ct_verifier.h:58: virtual void StopNotifications() = 0;
On 2015/07/02 13:23:08, Ryan Sleevi (slow through 7-15 wrote:
> Document.
> 
> Seems like this would be more logical after SetObserver.
> 
> DESIGN: Does it make more sense to do SetObserver(nullptr) as the
unregistration
> action? Why or why not?

Using SetObserver(nullptr) would definitely do the job, removing this method.

[Ryan Sleevi, 2015-07-02 14:25:47]

LGTM % nits

https://codereview.chromium.org/1216153010/diff/20001/net/cert/ct_verifier.h
File net/cert/ct_verifier.h (right):

https://codereview.chromium.org/1216153010/diff/20001/net/cert/ct_verifier.h#...
net/cert/ct_verifier.h:10: #include "base/memory/ref_counted.h"
Unused

https://codereview.chromium.org/1216153010/diff/20001/net/cert/ct_verifier.h#...
net/cert/ct_verifier.h:58: // URLRequests which have to be cancelled before this
object is destroyed.
comment nit:
// Setting |observer| to nullptr has the effect of stopping all notifications.

https://codereview.chromium.org/1216153010/diff/20001/net/cert/multi_log_ct_v...
File net/cert/multi_log_ct_verifier_unittest.cc (right):

https://codereview.chromium.org/1216153010/diff/20001/net/cert/multi_log_ct_v...
net/cert/multi_log_ct_verifier_unittest.cc:321:
Mock::VerifyAndClearExpectations(&observer);
This isn't needed if you remove 322/323, since this is implicitly handled as the
mock object goes out of scope.

https://codereview.chromium.org/1216153010/diff/20001/net/cert/multi_log_ct_v...
net/cert/multi_log_ct_verifier_unittest.cc:323: verifier_->SetObserver(nullptr);
This isn't necessary - is it? verifier_ gets deleted as soon as this test
completed.

https://codereview.chromium.org/1216153010/diff/20001/net/cert/multi_log_ct_v...
net/cert/multi_log_ct_verifier_unittest.cc:332: verifier_->SetObserver(nullptr);
335 isn't strictly necessary, but it seems like you're not testing the ordering
of the calls (namely, that the delivery of notifications happens prior to the
SetObserver(nullptr)

Suggested restructuring:

331 ASSERT_TRUE(...);
332 Mock::VerifyAndClearExpectations(&observer);
333
334 verifier_->SetObserver(nullptr)
335 ASSERT_TRUE(...);
336 }

[Eran Messeri, 2015-07-02 15:04:39]

https://codereview.chromium.org/1216153010/diff/20001/net/cert/ct_verifier.h
File net/cert/ct_verifier.h (right):

https://codereview.chromium.org/1216153010/diff/20001/net/cert/ct_verifier.h#...
net/cert/ct_verifier.h:10: #include "base/memory/ref_counted.h"
On 2015/07/02 14:25:46, Ryan Sleevi (slow through 7-15 wrote:
> Unused

Done.

https://codereview.chromium.org/1216153010/diff/20001/net/cert/ct_verifier.h#...
net/cert/ct_verifier.h:58: // URLRequests which have to be cancelled before this
object is destroyed.
On 2015/07/02 14:25:46, Ryan Sleevi (slow through 7-15 wrote:
> comment nit:
> // Setting |observer| to nullptr has the effect of stopping all notifications.

Done.

https://codereview.chromium.org/1216153010/diff/20001/net/cert/multi_log_ct_v...
File net/cert/multi_log_ct_verifier_unittest.cc (right):

https://codereview.chromium.org/1216153010/diff/20001/net/cert/multi_log_ct_v...
net/cert/multi_log_ct_verifier_unittest.cc:321:
Mock::VerifyAndClearExpectations(&observer);
On 2015/07/02 14:25:46, Ryan Sleevi (slow through 7-15 wrote:
> This isn't needed if you remove 322/323, since this is implicitly handled as
the
> mock object goes out of scope.

Done.

https://codereview.chromium.org/1216153010/diff/20001/net/cert/multi_log_ct_v...
net/cert/multi_log_ct_verifier_unittest.cc:323: verifier_->SetObserver(nullptr);
On 2015/07/02 14:25:46, Ryan Sleevi (slow through 7-15 wrote:
> This isn't necessary - is it? verifier_ gets deleted as soon as this test
> completed.

Done.

https://codereview.chromium.org/1216153010/diff/20001/net/cert/multi_log_ct_v...
net/cert/multi_log_ct_verifier_unittest.cc:332: verifier_->SetObserver(nullptr);
On 2015/07/02 14:25:46, Ryan Sleevi (slow through 7-15 wrote:
> 335 isn't strictly necessary, but it seems like you're not testing the
ordering
> of the calls (namely, that the delivery of notifications happens prior to the
> SetObserver(nullptr)
> 
> Suggested restructuring:
> 
> 331 ASSERT_TRUE(...);
> 332 Mock::VerifyAndClearExpectations(&observer);
> 333
> 334 verifier_->SetObserver(nullptr)
> 335 ASSERT_TRUE(...);
> 336 }

Done, though I had to add:
EXPECT_CALL(observer, OnSCTVerified(...)).Times(0);
after VerifyAndClearExpectations otherwise the call would be allowed (and a
warning would be emitted about an uninteresting mock function call)

[Eran Messeri, 2015-07-02 15:35:11]

The CQ bit was checked by eranm@chromium.org

[Eran Messeri, 2015-07-02 15:35:12]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1216153010/#ps40001
(title: "Addressing nits")

[commit-bot: I haz the power, 2015-07-02 15:35:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1216153010/40001

[commit-bot: I haz the power, 2015-07-02 16:01:59]

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

[commit-bot: I haz the power, 2015-07-02 16:02:00]

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

[Eran Messeri, 2015-07-02 19:59:54]

The CQ bit was checked by eranm@chromium.org

[commit-bot: I haz the power, 2015-07-02 20:00:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1216153010/40001

[commit-bot: I haz the power, 2015-07-02 20:05:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-02 20:06:26]

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