Certificate Transparency: Per-profile CT verification

Certificate Transparency: Per-profile CT verification

Prepare for auditing Signed Certificate Timestamps (which are tied to
specific certificates) by having a per-profile CT verification.
That would allow auditing SCTs separately for separate profiles.

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

[Eran Messeri, 2015-11-11 17:40:36]

Description was changed from

==========
Certificate Transparency: Per-profile CT verification

Prepare for auditing Signed Certificate Timestamps (which are tied to
specific certificates) by having a per-profile CT verification.
That would allow auditing SCTs separately for separate profiles.

BUG=506227
==========

to

==========
Certificate Transparency: Per-profile CT verification

Prepare for auditing Signed Certificate Timestamps (which are tied to
specific certificates) by having a per-profile CT verification.
That would allow auditing SCTs separately for separate profiles.

BUG=506227
==========

[Eran Messeri, 2015-11-11 17:40:36]

eranm@chromium.org changed reviewers:
+ davidben@chromium.org, mmenke@chromium.org

[Eran Messeri, 2015-11-11 17:41:39]

Matt / David: Wasn't sure who is more suitable for reviewing this change - any
input is appreciated.

[davidben, 2015-11-11 22:39:02]

On 2015/11/11 17:41:39, Eran Messeri wrote:
> Matt / David: Wasn't sure who is more suitable for reviewing this change - any
> input is appreciated.

(Matt, I'll leave this to you since it's ProfileIOData. I can look at it if you
like, but I imagine if you want to load balance, it's best directed to something
who is neither you or I.)

[mmenke, 2015-11-17 17:38:32]

What's the motivation of doing this, instead of just sharing the IOThread's
verifier?  Being able to add more CTLogVerifiers on a per-profile basis?

Disclaimer:  I know very little about certificates, or certificate transparency.

https://codereview.chromium.org/1440643002/diff/1/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/1440643002/diff/1/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:27: #include "net/cert/ct_verifier.h"
Why can't this still be forward declared?

https://codereview.chromium.org/1440643002/diff/1/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1440643002/diff/1/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:1159: net::MultiLogCTVerifier*
ct_verifier = new net::MultiLogCTVerifier();
scoped_refptr?  Think that makes the ownership model here clearer, and not a big
fan of raw pointers.

[Eran Messeri, 2015-11-17 17:59:44]

On 2015/11/17 17:38:32, mmenke wrote:
> What's the motivation of doing this, instead of just sharing the IOThread's
> verifier?  Being able to add more CTLogVerifiers on a per-profile basis?
> 
> Disclaimer:  I know very little about certificates, or certificate
transparency.
Two reasons:
(1) We may perform outbound HTTPS connections (to Google-operated CT log
mirrors). I understand that in that case we should use the per-profile
URLRequestContext (https://codereview.chromium.org/1405293009/, for example).
(2) The goal is auditing CT logs by verifying that certificates which claim to
be logged (by being served with Signed Certificate Timestamps) are indeed
present in the log. This information is profile-specific (equivalent to browsing
history) and so we'd like to handle it separately for each profile. Having a
per-profile CTVerifier will allow us to add per-profile hooks and storage
mechanism for this information.
Does that make sense? I'll be happy to provide more context if needed.  

I'll address the rest of your comments shortly.

> 
> https://codereview.chromium.org/1440643002/diff/1/chrome/browser/io_thread.h
> File chrome/browser/io_thread.h (right):
> 
>
https://codereview.chromium.org/1440643002/diff/1/chrome/browser/io_thread.h#...
> chrome/browser/io_thread.h:27: #include "net/cert/ct_verifier.h"
> Why can't this still be forward declared?
> 
>
https://codereview.chromium.org/1440643002/diff/1/chrome/browser/profiles/pro...
> File chrome/browser/profiles/profile_io_data.cc (right):
> 
>
https://codereview.chromium.org/1440643002/diff/1/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_io_data.cc:1159: net::MultiLogCTVerifier*
> ct_verifier = new net::MultiLogCTVerifier();
> scoped_refptr?  Think that makes the ownership model here clearer, and not a
big
> fan of raw pointers.

[mmenke, 2015-11-17 18:15:38]

On 2015/11/17 17:59:44, Eran Messeri wrote:
> On 2015/11/17 17:38:32, mmenke wrote:
> > What's the motivation of doing this, instead of just sharing the IOThread's
> > verifier?  Being able to add more CTLogVerifiers on a per-profile basis?
> > 
> > Disclaimer:  I know very little about certificates, or certificate
> transparency.
> Two reasons:
> (1) We may perform outbound HTTPS connections (to Google-operated CT log
> mirrors). I understand that in that case we should use the per-profile
> URLRequestContext (https://codereview.chromium.org/1405293009/, for example).

This makes sense, but for this, you could either share a verifier and pass in
the URLRequestContext, or you'd need separate verifiers, as opposed to a
Multi*Verifier that shares a list of verifiers.  I don't think exposing a global
list in IOThread really helps you with either approach.

> (2) The goal is auditing CT logs by verifying that certificates which claim to
> be logged (by being served with Signed Certificate Timestamps) are indeed
> present in the log. This information is profile-specific (equivalent to
browsing
> history) and so we'd like to handle it separately for each profile. Having a
> per-profile CTVerifier will allow us to add per-profile hooks and storage
> mechanism for this information.
> Does that make sense? I'll be happy to provide more context if needed.  

I think my above concern applies here, too.

> I'll address the rest of your comments shortly.
> 
> > 
> > https://codereview.chromium.org/1440643002/diff/1/chrome/browser/io_thread.h
> > File chrome/browser/io_thread.h (right):
> > 
> >
>
https://codereview.chromium.org/1440643002/diff/1/chrome/browser/io_thread.h#...
> > chrome/browser/io_thread.h:27: #include "net/cert/ct_verifier.h"
> > Why can't this still be forward declared?
> > 
> >
>
https://codereview.chromium.org/1440643002/diff/1/chrome/browser/profiles/pro...
> > File chrome/browser/profiles/profile_io_data.cc (right):
> > 
> >
>
https://codereview.chromium.org/1440643002/diff/1/chrome/browser/profiles/pro...
> > chrome/browser/profiles/profile_io_data.cc:1159: net::MultiLogCTVerifier*
> > ct_verifier = new net::MultiLogCTVerifier();
> > scoped_refptr?  Think that makes the ownership model here clearer, and not a
> big
> > fan of raw pointers.

[Eran Messeri, 2015-11-18 14:11:06]

On 2015/11/17 18:15:38, mmenke wrote:
> On 2015/11/17 17:59:44, Eran Messeri wrote:
> > On 2015/11/17 17:38:32, mmenke wrote:
> > > What's the motivation of doing this, instead of just sharing the
IOThread's
> > > verifier?  Being able to add more CTLogVerifiers on a per-profile basis?
> > > 
> > > Disclaimer:  I know very little about certificates, or certificate
> > transparency.
> > Two reasons:
> > (1) We may perform outbound HTTPS connections (to Google-operated CT log
> > mirrors). I understand that in that case we should use the per-profile
> > URLRequestContext (https://codereview.chromium.org/1405293009/, for
example).
> 
> This makes sense, but for this, you could either share a verifier and pass in
> the URLRequestContext, or you'd need separate verifiers, as opposed to a
> Multi*Verifier that shares a list of verifiers.  I don't think exposing a
global
> list in IOThread really helps you with either approach.

Sorry, the class hierarchy with the CT verifiers is not very clear, I suspect
that contributes to the confusion.
CTLogVerifier: Stateless class for verifying signatures over data from a
particular, single CT log. Each instance holds a log's key (and other
information). Sharing instances of this class between profiles should be safe.
CTVerifier: The interface used during the certificate validation process to
verify a collection of Signed Certificate Timestapms over a particular X.509
certificate. Also defines an observer for reporting observation of valid SCTs.
MultiLogCTVerifier: Implements CTVerifier, validates the SCTs collection by
delegating individual checks to CTLogVerifier instances it holds (the only
non-test implementation of CTVerifier).  

In this change the CTLogVerifier instances are still shared between profiles;
each profile (including the system one) gets its own CTVerifier instance - each
instance will hold pointer to an observer unique to that profile (because, for
example, it will persist observed SCTs in the profile directory or use the
profile's URLRequestContext to obtain information needed to verify inclusion in
the CT log).

If I understand your suggestion correctly I could pass in the URLRequestContext
much later on, during certificate validation, if I encounter Certificate
Transparency information and need to do something with it. 
I think it's suboptimal because:
(1) Complex plumbing: The CTVerifier is used in the SSLClientSocket
(https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/ssl_cli...),
far from where the URLRequestContext is available. I can pass it along the
CTVerifier - it will make the code quite messy though.
(2) Asynchronous nature of inclusion check in CT logs: The URLRequestContext
will be needed independently of the SCTs obtained during certificate validation
(not every SCT encountered will require communication with the log).
(3) Need to store stateful information regarding CT logs: as mentioned earlier,
the "view" of each CT log may be different between profiles based on SCTs from
that particular log observed while browsing. 
 
The IOThread globals end up with a list of CTLogVerifier instances and a
separate CTVerifier because the CTVerifier will be used for verifying CT-related
information observed while making SSL connections using the
system_request_context, while the list of CTLogVerifiers is used to construct
per-profile CTVerifier instances. Couldn't think of a better way to split it (I
can make the CTVerifier instance have a Clone() method if you think that's
better).
  
> 
> > (2) The goal is auditing CT logs by verifying that certificates which claim
to
> > be logged (by being served with Signed Certificate Timestamps) are indeed
> > present in the log. This information is profile-specific (equivalent to
> browsing
> > history) and so we'd like to handle it separately for each profile. Having a
> > per-profile CTVerifier will allow us to add per-profile hooks and storage
> > mechanism for this information.
> > Does that make sense? I'll be happy to provide more context if needed.  
> 
> I think my above concern applies here, too.
> 
> > I'll address the rest of your comments shortly.
> > 
> > > 
> > >
https://codereview.chromium.org/1440643002/diff/1/chrome/browser/io_thread.h
> > > File chrome/browser/io_thread.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1440643002/diff/1/chrome/browser/io_thread.h#...
> > > chrome/browser/io_thread.h:27: #include "net/cert/ct_verifier.h"
> > > Why can't this still be forward declared?
> > > 
> > >
> >
>
https://codereview.chromium.org/1440643002/diff/1/chrome/browser/profiles/pro...
> > > File chrome/browser/profiles/profile_io_data.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1440643002/diff/1/chrome/browser/profiles/pro...
> > > chrome/browser/profiles/profile_io_data.cc:1159: net::MultiLogCTVerifier*
> > > ct_verifier = new net::MultiLogCTVerifier();
> > > scoped_refptr?  Think that makes the ownership model here clearer, and not
a
> > big
> > > fan of raw pointers.

[Eran Messeri, 2015-11-18 14:16:22]

Also, addressed review comments.

https://codereview.chromium.org/1440643002/diff/1/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/1440643002/diff/1/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:27: #include "net/cert/ct_verifier.h"
On 2015/11/17 17:38:32, mmenke wrote:
> Why can't this still be forward declared?

Done - it can.

https://codereview.chromium.org/1440643002/diff/1/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1440643002/diff/1/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:1159: net::MultiLogCTVerifier*
ct_verifier = new net::MultiLogCTVerifier();
On 2015/11/17 17:38:32, mmenke wrote:
> scoped_refptr?  Think that makes the ownership model here clearer, and not a
big
> fan of raw pointers.

Done.

[mmenke, 2015-11-18 16:19:54]

On 2015/11/18 14:11:06, Eran Messeri wrote:
> On 2015/11/17 18:15:38, mmenke wrote:
> > On 2015/11/17 17:59:44, Eran Messeri wrote:
> > > On 2015/11/17 17:38:32, mmenke wrote:
> > > > What's the motivation of doing this, instead of just sharing the
> IOThread's
> > > > verifier?  Being able to add more CTLogVerifiers on a per-profile basis?
> > > > 
> > > > Disclaimer:  I know very little about certificates, or certificate
> > > transparency.
> > > Two reasons:
> > > (1) We may perform outbound HTTPS connections (to Google-operated CT log
> > > mirrors). I understand that in that case we should use the per-profile
> > > URLRequestContext (https://codereview.chromium.org/1405293009/, for
> example).
> > 
> > This makes sense, but for this, you could either share a verifier and pass
in
> > the URLRequestContext, or you'd need separate verifiers, as opposed to a
> > Multi*Verifier that shares a list of verifiers.  I don't think exposing a
> global
> > list in IOThread really helps you with either approach.
> 
> Sorry, the class hierarchy with the CT verifiers is not very clear, I suspect
> that contributes to the confusion.
> CTLogVerifier: Stateless class for verifying signatures over data from a
> particular, single CT log. Each instance holds a log's key (and other
> information). Sharing instances of this class between profiles should be safe.
> CTVerifier: The interface used during the certificate validation process to
> verify a collection of Signed Certificate Timestapms over a particular X.509
> certificate. Also defines an observer for reporting observation of valid SCTs.
> MultiLogCTVerifier: Implements CTVerifier, validates the SCTs collection by
> delegating individual checks to CTLogVerifier instances it holds (the only
> non-test implementation of CTVerifier).  
> 
> In this change the CTLogVerifier instances are still shared between profiles;
> each profile (including the system one) gets its own CTVerifier instance -
each
> instance will hold pointer to an observer unique to that profile (because, for
> example, it will persist observed SCTs in the profile directory or use the
> profile's URLRequestContext to obtain information needed to verify inclusion
in
> the CT log).
> 
> If I understand your suggestion correctly I could pass in the
URLRequestContext
> much later on, during certificate validation, if I encounter Certificate
> Transparency information and need to do something with it. 
> I think it's suboptimal because:
> (1) Complex plumbing: The CTVerifier is used in the SSLClientSocket
>
(https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/ssl_cli...),
> far from where the URLRequestContext is available. I can pass it along the
> CTVerifier - it will make the code quite messy though.
> (2) Asynchronous nature of inclusion check in CT logs: The URLRequestContext
> will be needed independently of the SCTs obtained during certificate
validation
> (not every SCT encountered will require communication with the log).
> (3) Need to store stateful information regarding CT logs: as mentioned
earlier,
> the "view" of each CT log may be different between profiles based on SCTs from
> that particular log observed while browsing.

So you're saying the MultiLogCTVerifier contains / may contain / will contain
per-profile state, but the CTLogVerifiers do not?  I don't suppose we can make
the CTLogVerifier const (That doesn't necessarily follow from just having no
per-profile state, but think it makes the situation clearer if we can)?

Also, is this safe to share with isolate app URLRequestContexts?  Eventually we
want to make them completely independent of the main context, anyways, just want
to know what kind of sidechannel information we're enabling here.
 
> The IOThread globals end up with a list of CTLogVerifier instances and a
> separate CTVerifier because the CTVerifier will be used for verifying
CT-related
> information observed while making SSL connections using the
> system_request_context, while the list of CTLogVerifiers is used to construct
> per-profile CTVerifier instances. Couldn't think of a better way to split it
(I
> can make the CTVerifier instance have a Clone() method if you think that's
> better).

I wonder if we can make this state clearer by class docs (Or a rename?).  Maybe
the code makes it obvious, and it's just my ignorance of CTs that makes this
non-obvious.

> > 
> > > (2) The goal is auditing CT logs by verifying that certificates which
claim
> to
> > > be logged (by being served with Signed Certificate Timestamps) are indeed
> > > present in the log. This information is profile-specific (equivalent to
> > browsing
> > > history) and so we'd like to handle it separately for each profile. Having
a
> > > per-profile CTVerifier will allow us to add per-profile hooks and storage
> > > mechanism for this information.
> > > Does that make sense? I'll be happy to provide more context if needed.  
> > 
> > I think my above concern applies here, too.
> > 
> > > I'll address the rest of your comments shortly.
> > > 
> > > > 
> > > >
> https://codereview.chromium.org/1440643002/diff/1/chrome/browser/io_thread.h
> > > > File chrome/browser/io_thread.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1440643002/diff/1/chrome/browser/io_thread.h#...
> > > > chrome/browser/io_thread.h:27: #include "net/cert/ct_verifier.h"
> > > > Why can't this still be forward declared?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1440643002/diff/1/chrome/browser/profiles/pro...
> > > > File chrome/browser/profiles/profile_io_data.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1440643002/diff/1/chrome/browser/profiles/pro...
> > > > chrome/browser/profiles/profile_io_data.cc:1159:
net::MultiLogCTVerifier*
> > > > ct_verifier = new net::MultiLogCTVerifier();
> > > > scoped_refptr?  Think that makes the ownership model here clearer, and
not
> a
> > > big
> > > > fan of raw pointers.

[Eran Messeri, 2015-11-18 18:07:19]

On 2015/11/18 16:19:54, mmenke wrote:
> On 2015/11/18 14:11:06, Eran Messeri wrote:
> > On 2015/11/17 18:15:38, mmenke wrote:
> > > On 2015/11/17 17:59:44, Eran Messeri wrote:
> > > > On 2015/11/17 17:38:32, mmenke wrote:
> > > > > What's the motivation of doing this, instead of just sharing the
> > IOThread's
> > > > > verifier?  Being able to add more CTLogVerifiers on a per-profile
basis?
> > > > > 
> > > > > Disclaimer:  I know very little about certificates, or certificate
> > > > transparency.
> > > > Two reasons:
> > > > (1) We may perform outbound HTTPS connections (to Google-operated CT log
> > > > mirrors). I understand that in that case we should use the per-profile
> > > > URLRequestContext (https://codereview.chromium.org/1405293009/, for
> > example).
> > > 
> > > This makes sense, but for this, you could either share a verifier and pass
> in
> > > the URLRequestContext, or you'd need separate verifiers, as opposed to a
> > > Multi*Verifier that shares a list of verifiers.  I don't think exposing a
> > global
> > > list in IOThread really helps you with either approach.
> > 
> > Sorry, the class hierarchy with the CT verifiers is not very clear, I
suspect
> > that contributes to the confusion.
> > CTLogVerifier: Stateless class for verifying signatures over data from a
> > particular, single CT log. Each instance holds a log's key (and other
> > information). Sharing instances of this class between profiles should be
safe.
> > CTVerifier: The interface used during the certificate validation process to
> > verify a collection of Signed Certificate Timestapms over a particular X.509
> > certificate. Also defines an observer for reporting observation of valid
SCTs.
> > MultiLogCTVerifier: Implements CTVerifier, validates the SCTs collection by
> > delegating individual checks to CTLogVerifier instances it holds (the only
> > non-test implementation of CTVerifier).  
> > 
> > In this change the CTLogVerifier instances are still shared between
profiles;
> > each profile (including the system one) gets its own CTVerifier instance -
> each
> > instance will hold pointer to an observer unique to that profile (because,
for
> > example, it will persist observed SCTs in the profile directory or use the
> > profile's URLRequestContext to obtain information needed to verify inclusion
> in
> > the CT log).
> > 
> > If I understand your suggestion correctly I could pass in the
> URLRequestContext
> > much later on, during certificate validation, if I encounter Certificate
> > Transparency information and need to do something with it. 
> > I think it's suboptimal because:
> > (1) Complex plumbing: The CTVerifier is used in the SSLClientSocket
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/ssl_cli...),
> > far from where the URLRequestContext is available. I can pass it along the
> > CTVerifier - it will make the code quite messy though.
> > (2) Asynchronous nature of inclusion check in CT logs: The URLRequestContext
> > will be needed independently of the SCTs obtained during certificate
> validation
> > (not every SCT encountered will require communication with the log).
> > (3) Need to store stateful information regarding CT logs: as mentioned
> earlier,
> > the "view" of each CT log may be different between profiles based on SCTs
from
> > that particular log observed while browsing.
> 
> So you're saying the MultiLogCTVerifier contains / may contain / will contain
> per-profile state, but the CTLogVerifiers do not?  I don't suppose we can make
> the CTLogVerifier const (That doesn't necessarily follow from just having no
> per-profile state, but think it makes the situation clearer if we can)?
Correct - I will look into making the CTLogVerifiers const as a part of this
change.

> 
> Also, is this safe to share with isolate app URLRequestContexts?  Eventually
we
> want to make them completely independent of the main context, anyways, just
want
> to know what kind of sidechannel information we're enabling here.
TBH I don't know a lot about isolate app ULRequestContexts (where can I read
more?) but I believe it is safe to share as information would only be going into
the CTVerifier, the CTVerifier should not be exposing the CT information it has 
gathered except for logging situations where different items of CT information
cannot be reconciled.
If in doubt, we can leave it out (by setting null observer on the CTVerifier
provided to the isolate app URLRequestContext).

>  
> > The IOThread globals end up with a list of CTLogVerifier instances and a
> > separate CTVerifier because the CTVerifier will be used for verifying
> CT-related
> > information observed while making SSL connections using the
> > system_request_context, while the list of CTLogVerifiers is used to
construct
> > per-profile CTVerifier instances. Couldn't think of a better way to split it
> (I
> > can make the CTVerifier instance have a Clone() method if you think that's
> > better).
> 
> I wonder if we can make this state clearer by class docs (Or a rename?). 
Maybe
> the code makes it obvious, and it's just my ignorance of CTs that makes this
> non-obvious.
I will improve the class docs or propose better class names.

> 
> > > 
> > > > (2) The goal is auditing CT logs by verifying that certificates which
> claim
> > to
> > > > be logged (by being served with Signed Certificate Timestamps) are
indeed
> > > > present in the log. This information is profile-specific (equivalent to
> > > browsing
> > > > history) and so we'd like to handle it separately for each profile.
Having
> a
> > > > per-profile CTVerifier will allow us to add per-profile hooks and
storage
> > > > mechanism for this information.
> > > > Does that make sense? I'll be happy to provide more context if needed.  
> > > 
> > > I think my above concern applies here, too.
> > > 
> > > > I'll address the rest of your comments shortly.
> > > > 
> > > > > 
> > > > >
> > https://codereview.chromium.org/1440643002/diff/1/chrome/browser/io_thread.h
> > > > > File chrome/browser/io_thread.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1440643002/diff/1/chrome/browser/io_thread.h#...
> > > > > chrome/browser/io_thread.h:27: #include "net/cert/ct_verifier.h"
> > > > > Why can't this still be forward declared?
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1440643002/diff/1/chrome/browser/profiles/pro...
> > > > > File chrome/browser/profiles/profile_io_data.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1440643002/diff/1/chrome/browser/profiles/pro...
> > > > > chrome/browser/profiles/profile_io_data.cc:1159:
> net::MultiLogCTVerifier*
> > > > > ct_verifier = new net::MultiLogCTVerifier();
> > > > > scoped_refptr?  Think that makes the ownership model here clearer, and
> not
> > a
> > > > big
> > > > > fan of raw pointers.

[mmenke, 2015-11-18 18:45:07]

On 2015/11/18 18:07:19, Eran Messeri wrote:
> On 2015/11/18 16:19:54, mmenke wrote:
> > On 2015/11/18 14:11:06, Eran Messeri wrote:
> > > On 2015/11/17 18:15:38, mmenke wrote:
> > > > On 2015/11/17 17:59:44, Eran Messeri wrote:
> > > > > On 2015/11/17 17:38:32, mmenke wrote:
> > > > > > What's the motivation of doing this, instead of just sharing the
> > > IOThread's
> > > > > > verifier?  Being able to add more CTLogVerifiers on a per-profile
> basis?
> > > > > > 
> > > > > > Disclaimer:  I know very little about certificates, or certificate
> > > > > transparency.
> > > > > Two reasons:
> > > > > (1) We may perform outbound HTTPS connections (to Google-operated CT
log
> > > > > mirrors). I understand that in that case we should use the per-profile
> > > > > URLRequestContext (https://codereview.chromium.org/1405293009/, for
> > > example).
> > > > 
> > > > This makes sense, but for this, you could either share a verifier and
pass
> > in
> > > > the URLRequestContext, or you'd need separate verifiers, as opposed to a
> > > > Multi*Verifier that shares a list of verifiers.  I don't think exposing
a
> > > global
> > > > list in IOThread really helps you with either approach.
> > > 
> > > Sorry, the class hierarchy with the CT verifiers is not very clear, I
> suspect
> > > that contributes to the confusion.
> > > CTLogVerifier: Stateless class for verifying signatures over data from a
> > > particular, single CT log. Each instance holds a log's key (and other
> > > information). Sharing instances of this class between profiles should be
> safe.
> > > CTVerifier: The interface used during the certificate validation process
to
> > > verify a collection of Signed Certificate Timestapms over a particular
X.509
> > > certificate. Also defines an observer for reporting observation of valid
> SCTs.
> > > MultiLogCTVerifier: Implements CTVerifier, validates the SCTs collection
by
> > > delegating individual checks to CTLogVerifier instances it holds (the only
> > > non-test implementation of CTVerifier).  
> > > 
> > > In this change the CTLogVerifier instances are still shared between
> profiles;
> > > each profile (including the system one) gets its own CTVerifier instance -
> > each
> > > instance will hold pointer to an observer unique to that profile (because,
> for
> > > example, it will persist observed SCTs in the profile directory or use the
> > > profile's URLRequestContext to obtain information needed to verify
inclusion
> > in
> > > the CT log).
> > > 
> > > If I understand your suggestion correctly I could pass in the
> > URLRequestContext
> > > much later on, during certificate validation, if I encounter Certificate
> > > Transparency information and need to do something with it. 
> > > I think it's suboptimal because:
> > > (1) Complex plumbing: The CTVerifier is used in the SSLClientSocket
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/ssl_cli...),
> > > far from where the URLRequestContext is available. I can pass it along the
> > > CTVerifier - it will make the code quite messy though.
> > > (2) Asynchronous nature of inclusion check in CT logs: The
URLRequestContext
> > > will be needed independently of the SCTs obtained during certificate
> > validation
> > > (not every SCT encountered will require communication with the log).
> > > (3) Need to store stateful information regarding CT logs: as mentioned
> > earlier,
> > > the "view" of each CT log may be different between profiles based on SCTs
> from
> > > that particular log observed while browsing.
> > 
> > So you're saying the MultiLogCTVerifier contains / may contain / will
contain
> > per-profile state, but the CTLogVerifiers do not?  I don't suppose we can
make
> > the CTLogVerifier const (That doesn't necessarily follow from just having no
> > per-profile state, but think it makes the situation clearer if we can)?
> Correct - I will look into making the CTLogVerifiers const as a part of this
> change.

Thanks!  If that doesn't seem workable, we can hash out some comments instead.

> > 
> > Also, is this safe to share with isolate app URLRequestContexts?  Eventually
> we
> > want to make them completely independent of the main context, anyways, just
> want
> > to know what kind of sidechannel information we're enabling here.
> TBH I don't know a lot about isolate app ULRequestContexts (where can I read
> more?) but I believe it is safe to share as information would only be going
into
> the CTVerifier, the CTVerifier should not be exposing the CT information it
has 
> gathered except for logging situations where different items of CT information
> cannot be reconciled.
> If in doubt, we can leave it out (by setting null observer on the CTVerifier
> provided to the isolate app URLRequestContext).

Erm...We don't have a whole lot of documentation on them, to the extent of my
knowledge.  They're basically used by "extensions" that are stand alone-ish
"apps" and are supposed to be separate from the main request contexts.  Doesn't
sound like sharing would be an issue here, just wanted to make sure.

> >  
> > > The IOThread globals end up with a list of CTLogVerifier instances and a
> > > separate CTVerifier because the CTVerifier will be used for verifying
> > CT-related
> > > information observed while making SSL connections using the
> > > system_request_context, while the list of CTLogVerifiers is used to
> construct
> > > per-profile CTVerifier instances. Couldn't think of a better way to split
it
> > (I
> > > can make the CTVerifier instance have a Clone() method if you think that's
> > > better).
> > 
> > I wonder if we can make this state clearer by class docs (Or a rename?). 
> Maybe
> > the code makes it obvious, and it's just my ignorance of CTs that makes this
> > non-obvious.
> I will improve the class docs or propose better class names.
> 
> > 
> > > > 
> > > > > (2) The goal is auditing CT logs by verifying that certificates which
> > claim
> > > to
> > > > > be logged (by being served with Signed Certificate Timestamps) are
> indeed
> > > > > present in the log. This information is profile-specific (equivalent
to
> > > > browsing
> > > > > history) and so we'd like to handle it separately for each profile.
> Having
> > a
> > > > > per-profile CTVerifier will allow us to add per-profile hooks and
> storage
> > > > > mechanism for this information.
> > > > > Does that make sense? I'll be happy to provide more context if needed.
 
> > > > 
> > > > I think my above concern applies here, too.
> > > > 
> > > > > I'll address the rest of your comments shortly.
> > > > > 
> > > > > > 
> > > > > >
> > >
https://codereview.chromium.org/1440643002/diff/1/chrome/browser/io_thread.h
> > > > > > File chrome/browser/io_thread.h (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1440643002/diff/1/chrome/browser/io_thread.h#...
> > > > > > chrome/browser/io_thread.h:27: #include "net/cert/ct_verifier.h"
> > > > > > Why can't this still be forward declared?
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1440643002/diff/1/chrome/browser/profiles/pro...
> > > > > > File chrome/browser/profiles/profile_io_data.cc (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1440643002/diff/1/chrome/browser/profiles/pro...
> > > > > > chrome/browser/profiles/profile_io_data.cc:1159:
> > net::MultiLogCTVerifier*
> > > > > > ct_verifier = new net::MultiLogCTVerifier();
> > > > > > scoped_refptr?  Think that makes the ownership model here clearer,
and
> > not
> > > a
> > > > > big
> > > > > > fan of raw pointers.

[Eran Messeri, 2015-11-20 13:28:10]

As discussed, made the CTLogVerifier const throughout (it is now not possible to
hold a non-const reference to it as CTLogVerifier::Create returns a
scoped_refptr<const CTLogVerifier>).

I've also fixed the documentation to reflect the intended use.

That increased the number of files changed in this change but they are all to do
with the type change and so are safe.

[mmenke, 2015-11-20 17:23:59]

Thanks for the change!  This looks good.

https://codereview.chromium.org/1440643002/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1440643002/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1146:
cert_transparency_verifier_.reset(ct_verifier.release());
cert_transparency_verifier_ = ct_verifier.Pass()?

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

https://codereview.chromium.org/1440643002/diff/40001/net/cert/ct_verifier.h#...
net/cert/ct_verifier.h:35: // tied to a specific user profile, so it must be
profile-specific.
It's a layering violation for net/ to know about profiles...  So wait, this is
solely a (potential) property of the (Not yet written) Observers, and not the
MultiCTVerifier itself?

https://codereview.chromium.org/1440643002/diff/40001/net/cert/multi_log_ct_v...
File net/cert/multi_log_ct_verifier.h (right):

https://codereview.chromium.org/1440643002/diff/40001/net/cert/multi_log_ct_v...
net/cert/multi_log_ct_verifier.h:28: // of this class must exist for each
profile.
See other comment about layering violation.

[Eran Messeri, 2015-11-23 12:34:39]

Matt, changed the documentation to better reflect the expected dependency.
If that helps, Ryan suggested this pattern of observer interface in net/ &
implementation outside of net/ to be able to handle data tied to particular
profiles.

https://codereview.chromium.org/1440643002/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1440643002/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1146:
cert_transparency_verifier_.reset(ct_verifier.release());
On 2015/11/20 17:23:58, mmenke wrote:
> cert_transparency_verifier_ = ct_verifier.Pass()?

Done.

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

https://codereview.chromium.org/1440643002/diff/40001/net/cert/ct_verifier.h#...
net/cert/ct_verifier.h:35: // tied to a specific user profile, so it must be
profile-specific.
On 2015/11/20 17:23:58, mmenke wrote:
> It's a layering violation for net/ to know about profiles...  So wait, this is
> solely a (potential) property of the (Not yet written) Observers, and not the
> MultiCTVerifier itself?

Correct - the observer implementation will live outside net/ (in
components/certificate_transparency)  and will be aware of profiles. Hopefully
that is sensible?
(Likely that the observer implementation will only get the URLRequestContext and
storage  path from the profile and will not be tightly coupled with code in
chrome/browser/profiles)

I have amended the documentation to reflect that.

https://codereview.chromium.org/1440643002/diff/40001/net/cert/multi_log_ct_v...
File net/cert/multi_log_ct_verifier.h (right):

https://codereview.chromium.org/1440643002/diff/40001/net/cert/multi_log_ct_v...
net/cert/multi_log_ct_verifier.h:28: // of this class must exist for each
profile.
On 2015/11/20 17:23:58, mmenke wrote:
> See other comment about layering violation.  

Removed this text. I'm not sure how to phrase it in the documentation, but this
class is no more "profile specific" than the URLRequestContext.
Hopefully this is evident by the fact there's only one observer allowed per
CTVerifier instance.

[mmenke, 2015-11-23 16:09:21]

LGTM

[Eran Messeri, 2015-11-23 21:55:28]

The CQ bit was checked by eranm@chromium.org

[Eran Messeri, 2015-11-23 21:55:29]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1440643002/#ps80001
(title: "Fixing iOS compilation")

[commit-bot: I haz the power, 2015-11-23 21:58:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1440643002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1440643002/80001

[commit-bot: I haz the power, 2015-11-23 22:23:40]

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

[commit-bot: I haz the power, 2015-11-23 22:23:48]

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

[Eran Messeri, 2015-11-24 10:21:42]

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

[droger, 2015-11-24 10:24:59]

//ios lgtm

[Eran Messeri, 2015-11-24 10:25:25]

The CQ bit was checked by eranm@chromium.org

[commit-bot: I haz the power, 2015-11-24 10:25:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1440643002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1440643002/80001

[commit-bot: I haz the power, 2015-11-24 10:56:57]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-24 10:57:51]

Patchset 5 (id:??) landed as
https://crrev.com/1a79db29a0ac4d6936581e0d1a82c267721841cf
Cr-Commit-Position: refs/heads/master@{#361311}