Certificate Transparency: Add the high-level interface for verifying SCTs over multiple logs

Add the high-level interface for verifying SCTs over multiple logs
This interface (and the default implementation) verify SCT lists obtained
during the TLS handshake or from OCSP stapling, as well as embedded ones.
The result will be used to modify the ssl_info with indicatior of CT status.

The next, and final, patch will wire the CTVerifier to the SSL client socket.

BUG=309578
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=237008

[Ryan Sleevi, 2013-11-20 01:09:41]

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

https://codereview.chromium.org/67513008/diff/1/net/cert/ct_log_verifier.h#ne...
net/cert/ct_log_verifier.h:30: enum VerifyResult {
nit: Do you think the naming will be confusing with the CTVerifier's result
object that stores the result of all SCTs?

On an API level, how do you expect a caller to treat these differently? A
user-visible action? Something that is logged? Just making sure it's an API
distinction that we *need* and can only be accomplished by returning the
different error codes. This isn't an objection, so much as trying to think
whether it just makes more sense to always use a bool and VLOG, or if it's
something that, say, we'd surface to a NetLog or net-internals

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

https://codereview.chromium.org/67513008/diff/1/net/cert/ct_serialization.cc#...
net/cert/ct_serialization.cc:361: WriteVariableBytes(kSCTListLengthBytes,
encoded_sct, output);
style nit: indent 4 spaces (or use clang-format / git cl format)

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

https://codereview.chromium.org/67513008/diff/1/net/cert/multi_log_ct_verifie...
net/cert/multi_log_ct_verifier.h:24: class NET_EXPORT MultiLogCTVerifier :
public CTVerifier {
Style Nit: Please add documentation about this class and how to use it right.

https://codereview.chromium.org/67513008/diff/1/net/cert/multi_log_ct_verifie...
net/cert/multi_log_ct_verifier.h:26: explicit
MultiLogCTVerifier(scoped_ptr<CTLogVerifier> log_verifier);
API design: Why use two different methods for adding a log (ctor & AddLog),
versus a consistent interface (eg: always using AddLog)

https://codereview.chromium.org/67513008/diff/1/net/cert/multi_log_ct_verifie...
net/cert/multi_log_ct_verifier.h:29: virtual void
AddLog(scoped_ptr<CTLogVerifier> log_verifier);
Why does this need to be virtual?

https://codereview.chromium.org/67513008/diff/1/net/cert/multi_log_ct_verifie...
net/cert/multi_log_ct_verifier.h:30: // CTVerifier implementation:
nit: newline before the comment

https://codereview.chromium.org/67513008/diff/1/net/cert/multi_log_ct_verifie...
net/cert/multi_log_ct_verifier.h:55: ScopedVector<CTLogVerifier> logs_;
API design: Why a vector, when every CTLogVerifier has an associated log id that
may make it more efficient as a map lookup for the given SCT's log id?

https://codereview.chromium.org/67513008/diff/1/net/cert/multi_log_ct_verifie...
File net/cert/multi_log_ct_verifier_unittest.cc (right):

https://codereview.chromium.org/67513008/diff/1/net/cert/multi_log_ct_verifie...
net/cert/multi_log_ct_verifier_unittest.cc:47:
ASSERT_TRUE(result.unknown_logs_scts.empty());
These ASSERTs will not function as you expect, because this is in a void
function return.

That is, it will return from *this* function, but the remaining code of the
calling function will continue executing. You'd need to check the error state in
the parent test on each return or, alternatively, just use a bool here.

Style is up to you.

https://codereview.chromium.org/67513008/diff/1/net/data/ssl/certificates/ct-...
File net/data/ssl/certificates/ct-test-embedded-with-intermediate-chain.pem
(right):

https://codereview.chromium.org/67513008/diff/1/net/data/ssl/certificates/ct-...
net/data/ssl/certificates/ct-test-embedded-with-intermediate-chain.pem:1:
Certificate:
Update the README file with the provenance of these certs and how to
re-generate/reproduce them.

[Eran M. (Google), 2013-11-20 19:45:06]

Addressed Ryan's comments, which also simplified the CL (no need for the
CTLogVerifier to indicate the type of error).

PTAL

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

https://codereview.chromium.org/67513008/diff/1/net/cert/ct_log_verifier.h#ne...
net/cert/ct_log_verifier.h:30: enum VerifyResult {
On 2013/11/20 01:09:42, Ryan Sleevi wrote:
> nit: Do you think the naming will be confusing with the CTVerifier's result
> object that stores the result of all SCTs?
> 
> On an API level, how do you expect a caller to treat these differently? A
> user-visible action? Something that is logged? Just making sure it's an API
> distinction that we *need* and can only be accomplished by returning the
> different error codes. This isn't an objection, so much as trying to think
> whether it just makes more sense to always use a bool and VLOG, or if it's
> something that, say, we'd surface to a NetLog or net-internals

This isn't really necessary anymore now that the MultiLogCTVerifier knows which
log IDs it knows about.

As for the different treatment by the caller - yes, there will be a distinction
in the UI between SCTs from known and unknown logs. I plan to implement a SCT
viewer which will show the exact status for each SCT. We may want to log even
finer details to NetLog or net-internals but I think the basic distinction
between an unverified SCT and an SCT from unknown log is important enough to
remain.

Originally I considered adding a method to CTLogVerifier that will tell, for a
given SCT, if it's from a known log, but that seems superflous as this check is
performed in Verify() anyway.

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

https://codereview.chromium.org/67513008/diff/1/net/cert/ct_serialization.cc#...
net/cert/ct_serialization.cc:361: WriteVariableBytes(kSCTListLengthBytes,
encoded_sct, output);
On 2013/11/20 01:09:42, Ryan Sleevi wrote:
> style nit: indent 4 spaces (or use clang-format / git cl format)

Done.

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

https://codereview.chromium.org/67513008/diff/1/net/cert/multi_log_ct_verifie...
net/cert/multi_log_ct_verifier.h:24: class NET_EXPORT MultiLogCTVerifier :
public CTVerifier {
On 2013/11/20 01:09:42, Ryan Sleevi wrote:
> Style Nit: Please add documentation about this class and how to use it right.

Done, in the header file.

https://codereview.chromium.org/67513008/diff/1/net/cert/multi_log_ct_verifie...
net/cert/multi_log_ct_verifier.h:26: explicit
MultiLogCTVerifier(scoped_ptr<CTLogVerifier> log_verifier);
On 2013/11/20 01:09:42, Ryan Sleevi wrote:
> API design: Why use two different methods for adding a log (ctor & AddLog),
> versus a consistent interface (eg: always using AddLog)

Good point, left AddLog and removed the argument from the c'tor.

https://codereview.chromium.org/67513008/diff/1/net/cert/multi_log_ct_verifie...
net/cert/multi_log_ct_verifier.h:29: virtual void
AddLog(scoped_ptr<CTLogVerifier> log_verifier);
On 2013/11/20 01:09:42, Ryan Sleevi wrote:
> Why does this need to be virtual?

Does not - removed virtual qualifier.

https://codereview.chromium.org/67513008/diff/1/net/cert/multi_log_ct_verifie...
net/cert/multi_log_ct_verifier.h:29: virtual void
AddLog(scoped_ptr<CTLogVerifier> log_verifier);
On 2013/11/20 01:09:42, Ryan Sleevi wrote:
> Why does this need to be virtual?

Does not - removed.

https://codereview.chromium.org/67513008/diff/1/net/cert/multi_log_ct_verifie...
net/cert/multi_log_ct_verifier.h:30: // CTVerifier implementation:
On 2013/11/20 01:09:42, Ryan Sleevi wrote:
> nit: newline before the comment

Done.

https://codereview.chromium.org/67513008/diff/1/net/cert/multi_log_ct_verifie...
net/cert/multi_log_ct_verifier.h:55: ScopedVector<CTLogVerifier> logs_;
On 2013/11/20 01:09:42, Ryan Sleevi wrote:
> API design: Why a vector, when every CTLogVerifier has an associated log id
that
> may make it more efficient as a map lookup for the given SCT's log id?

Good point - a map would make more sense here.
As discussed offline, switching to a map of strings to linked_ptr<CTLogVerifier>

https://codereview.chromium.org/67513008/diff/1/net/data/ssl/certificates/ct-...
File net/data/ssl/certificates/ct-test-embedded-with-intermediate-chain.pem
(right):

https://codereview.chromium.org/67513008/diff/1/net/data/ssl/certificates/ct-...
net/data/ssl/certificates/ct-test-embedded-with-intermediate-chain.pem:1:
Certificate:
On 2013/11/20 01:09:42, Ryan Sleevi wrote:
> Update the README file with the provenance of these certs and how to
> re-generate/reproduce them.

Done.

[Eran M. (Google), 2013-11-20 20:00:13]

Skipping hooks so maybe diff will not be formatted.

[Eran M. (Google), 2013-11-20 20:11:20]

Simply removing the offending text from the pem file to avoid patch application
failure.

[Eran M. (Google), 2013-11-20 23:00:06]

Fixed using of released pointer.

[wtc, 2013-11-21 02:05:01]

Patch set 7 LGTM.

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

https://codereview.chromium.org/67513008/diff/310001/net/cert/ct_verifier.h#n...
net/cert/ct_verifier.h:18: // High-level interface for verifying Signed
Certificate Timestamps

Nit: remove "High-level"? unless there is also a low-level interface for
verifying SCTs.

https://codereview.chromium.org/67513008/diff/310001/net/cert/ct_verifier.h#n...
net/cert/ct_verifier.h:25: // OCSP stapling on the given |verified_cert|

Nit: TLS handshake/OCSP stapling => the signed_certificate_timestamp TLS
extension or OCSP

Update: I see that you already have the enum SCT_FROM_TLS_HANDSHAKE, so you can
ignore this suggested change.

https://codereview.chromium.org/67513008/diff/310001/net/cert/ct_verifier.h#n...
net/cert/ct_verifier.h:28: virtual int Verify(X509Certificate* verified_cert,

Nit: the first parameter should be simply named |cert|.

https://codereview.chromium.org/67513008/diff/310001/net/cert/ct_verifier.h#n...
net/cert/ct_verifier.h:30: const std::string& sct_list_from_tls_handshake,

1. Nit: I suggest changing "from_tls_handshake" to "from_tls_extension".

Note that OCSP stapling is also a TLS extension, but in theory we can get the
OCSP response directly using the OCSP protocol.

Update: I see that you already have the enum SCT_FROM_TLS_HANDSHAKE, so you can
ignore this suggested change.

2. Why are these two parameters std::string instead of
std::vector<SignedCertificateTimestamp>?

https://codereview.chromium.org/67513008/diff/310001/net/cert/ct_verify_resul...
File net/cert/ct_verify_result.cc (right):

https://codereview.chromium.org/67513008/diff/310001/net/cert/ct_verify_resul...
net/cert/ct_verify_result.cc:1: #include "net/cert/ct_verify_result.h"

Add the copyright header.

https://codereview.chromium.org/67513008/diff/310001/net/cert/ct_verify_result.h
File net/cert/ct_verify_result.h (right):

https://codereview.chromium.org/67513008/diff/310001/net/cert/ct_verify_resul...
net/cert/ct_verify_result.h:27: // SCTs from unknown logs

Nit: add a period (.) at the end of line.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.cc (right):

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:12: #include
"net/cert/signed_certificate_timestamp.h"

Nit: already included by the .h file.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:20: MultiLogCTVerifier::~MultiLogCTVerifier()
{ }

Nit: the constructor and destructor should use the same style for an empty
function body.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:20: MultiLogCTVerifier::~MultiLogCTVerifier()
{ }

Nit: the constructor and destructor should use the same style for an empty
function body.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:24: logs_[log_id] =
linked_ptr<CTLogVerifier>(log_verifier.release());

1. Nit: you should be able to do
  logs_[log_id].reset(log_verifier.release());

2. Nit: It seems wrong to mix scoped_ptr and linked_ptr.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:45: &embedded_scts)) {

Nit: indent by four spaces with respect to "ct::ExtractEmbeddedSCTList(".

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:46: ct::LogEntry embedded_log_entry;

Nit: if the |x509_entry| variable you declare below is also an embedded log
entry, then I suggest renaming this variable to "precert_entry".

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:49: ct::GetPrecertLogEntry(

Nit: indent by four spaces.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:62: return has_verified_scts ? OK :
ERR_FAILED;

IMPORTANT: use a more informative error code than ERR_FAILED. You can add a new
error code to net/base/net_error_list.h for CT verification.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:63: }

Nit: omit curly braces.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:119: result->unverified_scts.push_back(sct);

Nit: it seems clearer to just push |sct| to the appropriate member of |result|
before each return statement. This way the person who reads this code doesn't
need to check where we need to pop_back().

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:135: if (sct.timestamp +
base::TimeDelta::FromSeconds(1) > base::Time::Now()) {

Should add a comment to explain why we need to add one second.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.h (right):

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.h:25: // A Certificate Transparency verifier that
can verifies Signed Certificate

Nit: can verifies => can verify

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.h:29: // Log's public key).

Nit: "Log's" doesn't need to be capitalized

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.h:32: explicit MultiLogCTVerifier();

Remove "explicit". It's only necessary for constructors that take one parameter.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.h:38: virtual int Verify(X509Certificate*
verified_cert,

Nit: we should simply name this parameter |cert|.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.h:40: const std::string&
sct_list_from_tls_handshake,

Nit: change "from_tls_handshake" to "from_tls_extension", if you agree.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.h:46: // |origin|.

1. Nit: "Fills in the origin field of each SCT from |origin|" is very confusing
without looking at the code in the .cc file. It would be better to describe at a
more abstract level how |origin| would be used by this function, for example,
  Treat each SCT as being obtained from |origin|.
Or
  The SCTs in the list come from |origin|.

2. Nit: explain what |expected_entry| is for.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.h:58: typedef std::map<std::string,
linked_ptr<CTLogVerifier> > IDToLogMap;

1. Nit: typedef should be listed at the beginning of the "private" section.

Recommended by the Style Guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

2. Nit: document what the std::string "ID" is. I guess it's the ID of the log's
public key?

https://codereview.chromium.org/67513008/diff/310001/net/data/ssl/certificate...
File net/data/ssl/certificates/ct-test-embedded-with-preca-chain.pem (right):

https://codereview.chromium.org/67513008/diff/310001/net/data/ssl/certificate...
net/data/ssl/certificates/ct-test-embedded-with-preca-chain.pem:1: -----BEGIN
CERTIFICATE-----

Nit: this file doesn't have the human-readable form of the certificates.

https://codereview.chromium.org/67513008/diff/310001/net/test/cert_test_util.cc
File net/test/cert_test_util.cc (right):

https://codereview.chromium.org/67513008/diff/310001/net/test/cert_test_util....
net/test/cert_test_util.cc:33: CertificateList certs =
CreateCertificateListFromFile(

This is an inefficient way to implement this function. It is acceptable because
this function is only used in testing.

I was hoping that we could use CreateOSCertHandlesFromBytes, but it doesn't seem
to support the format of multiple CERTIFICATEs in a PEM file.

https://codereview.chromium.org/67513008/diff/310001/net/test/cert_test_util....
net/test/cert_test_util.cc:47: 

Nit: delete one blank line.

https://codereview.chromium.org/67513008/diff/310001/net/test/cert_test_util.h
File net/test/cert_test_util.h (right):

https://codereview.chromium.org/67513008/diff/310001/net/test/cert_test_util....
net/test/cert_test_util.h:21: // Imports all of the certificates in |cert_file|,
a file in |certs_dir|,

Add a blank line before this comment.

https://codereview.chromium.org/67513008/diff/310001/net/test/cert_test_util....
net/test/cert_test_util.h:36: // Imports a single certificate from |cert_file|.

Nit: it seems useful to keep the original comment for clarification:
  If cert_file contains multiple certificates, the first certificate
  found will be returned.

[Eran M. (Google), 2013-11-21 20:06:02]

Addressed all comments - waiting for your review of the changes to
net_error_list.h before submitting.

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

https://codereview.chromium.org/67513008/diff/310001/net/cert/ct_verifier.h#n...
net/cert/ct_verifier.h:18: // High-level interface for verifying Signed
Certificate Timestamps
On 2013/11/21 02:05:02, wtc wrote:
> 
> Nit: remove "High-level"? unless there is also a low-level interface for
> verifying SCTs.

Done.

https://codereview.chromium.org/67513008/diff/310001/net/cert/ct_verifier.h#n...
net/cert/ct_verifier.h:25: // OCSP stapling on the given |verified_cert|
On 2013/11/21 02:05:02, wtc wrote:
> 
> Nit: TLS handshake/OCSP stapling => the signed_certificate_timestamp TLS
> extension or OCSP
> 
> Update: I see that you already have the enum SCT_FROM_TLS_HANDSHAKE, so you
can
> ignore this suggested change.

Changed the comment. Would you like me to change the enum as well?

https://codereview.chromium.org/67513008/diff/310001/net/cert/ct_verifier.h#n...
net/cert/ct_verifier.h:28: virtual int Verify(X509Certificate* verified_cert,
On 2013/11/21 02:05:02, wtc wrote:
> 
> Nit: the first parameter should be simply named |cert|.

Done, in all classes.

https://codereview.chromium.org/67513008/diff/310001/net/cert/ct_verifier.h#n...
net/cert/ct_verifier.h:30: const std::string& sct_list_from_tls_handshake,
On 2013/11/21 02:05:02, wtc wrote:
> 
> 1. Nit: I suggest changing "from_tls_handshake" to "from_tls_extension".
> 
> Note that OCSP stapling is also a TLS extension, but in theory we can get the
> OCSP response directly using the OCSP protocol.
> 
> Update: I see that you already have the enum SCT_FROM_TLS_HANDSHAKE, so you
can
> ignore this suggested change.
> 
> 2. Why are these two parameters std::string instead of
> std::vector<SignedCertificateTimestamp>?
1. Changed the name (in all classes), could also change the enum.

2. Because the parsing is done in this verifier - the strings here are the
encoded SignedCertificateTimestampList as defined in section 3.3. of RFC 6962.
That means the changes to the SSLClientSocket class are minimal - simply
extracting these encoded strings from OCSP or TLS extension and passing them
into the verified.

https://codereview.chromium.org/67513008/diff/310001/net/cert/ct_verify_resul...
File net/cert/ct_verify_result.cc (right):

https://codereview.chromium.org/67513008/diff/310001/net/cert/ct_verify_resul...
net/cert/ct_verify_result.cc:1: #include "net/cert/ct_verify_result.h"
On 2013/11/21 02:05:02, wtc wrote:
> 
> Add the copyright header.

Done, also removed the (c) from all copyright headers in new files.

https://codereview.chromium.org/67513008/diff/310001/net/cert/ct_verify_result.h
File net/cert/ct_verify_result.h (right):

https://codereview.chromium.org/67513008/diff/310001/net/cert/ct_verify_resul...
net/cert/ct_verify_result.h:27: // SCTs from unknown logs
On 2013/11/21 02:05:02, wtc wrote:
> 
> Nit: add a period (.) at the end of line.

Done.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.cc (right):

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:12: #include
"net/cert/signed_certificate_timestamp.h"
On 2013/11/21 02:05:02, wtc wrote:
> 
> Nit: already included by the .h file.

Done.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:20: MultiLogCTVerifier::~MultiLogCTVerifier()
{ }
On 2013/11/21 02:05:02, wtc wrote:
> 
> Nit: the constructor and destructor should use the same style for an empty
> function body.

Done.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:20: MultiLogCTVerifier::~MultiLogCTVerifier()
{ }
On 2013/11/21 02:05:02, wtc wrote:
> 
> Nit: the constructor and destructor should use the same style for an empty
> function body.

Done.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:24: logs_[log_id] =
linked_ptr<CTLogVerifier>(log_verifier.release());
On 2013/11/21 02:05:02, wtc wrote:
> 
> 1. Nit: you should be able to do
>   logs_[log_id].reset(log_verifier.release());
> 
> 2. Nit: It seems wrong to mix scoped_ptr and linked_ptr.

1. Done

2. Will look into changing the type of the input parameter to linked_ptr as well

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:45: &embedded_scts)) {
On 2013/11/21 02:05:02, wtc wrote:
> 
> Nit: indent by four spaces with respect to "ct::ExtractEmbeddedSCTList(".

Done.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:46: ct::LogEntry embedded_log_entry;
On 2013/11/21 02:05:02, wtc wrote:
> 
> Nit: if the |x509_entry| variable you declare below is also an embedded log
> entry, then I suggest renaming this variable to "precert_entry".

Done.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:49: ct::GetPrecertLogEntry(
On 2013/11/21 02:05:02, wtc wrote:
> 
> Nit: indent by four spaces.

Done.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:62: return has_verified_scts ? OK :
ERR_FAILED;
On 2013/11/21 02:05:02, wtc wrote:
> 
> IMPORTANT: use a more informative error code than ERR_FAILED. You can add a
new
> error code to net/base/net_error_list.h for CT verification.

Done - ended up adding another category for the various CT errors. PTAL and let
me know if it better fits one of the existing sections.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:63: }
On 2013/11/21 02:05:02, wtc wrote:
> 
> Nit: omit curly braces.

Done.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:119: result->unverified_scts.push_back(sct);
On 2013/11/21 02:05:02, wtc wrote:
> 
> Nit: it seems clearer to just push |sct| to the appropriate member of |result|
> before each return statement. This way the person who reads this code doesn't
> need to check where we need to pop_back().

Done - that was my attempt at defensive programming, does not sound like it's
necessary.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:135: if (sct.timestamp +
base::TimeDelta::FromSeconds(1) > base::Time::Now()) {
On 2013/11/21 02:05:02, wtc wrote:
> 
> Should add a comment to explain why we need to add one second.

Done, although I'll be the first to admit it's not a convincing one.
The basic reasoning is to not disqualify SCTs which have just been issued in
case the client and log's clocks are not in perfect sync. This is taken from
here:
https://code.google.com/p/certificate-transparency/source/browse/src/log/log_...

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.h (right):

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.h:25: // A Certificate Transparency verifier that
can verifies Signed Certificate
On 2013/11/21 02:05:02, wtc wrote:
> 
> Nit: can verifies => can verify

Done.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.h:29: // Log's public key).
On 2013/11/21 02:05:02, wtc wrote:
> 
> Nit: "Log's" doesn't need to be capitalized

Done.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.h:32: explicit MultiLogCTVerifier();
On 2013/11/21 02:05:02, wtc wrote:
> 
> Remove "explicit". It's only necessary for constructors that take one
parameter.

Done.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.h:38: virtual int Verify(X509Certificate*
verified_cert,
On 2013/11/21 02:05:02, wtc wrote:
> 
> Nit: we should simply name this parameter |cert|.

Done.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.h:40: const std::string&
sct_list_from_tls_handshake,
On 2013/11/21 02:05:02, wtc wrote:
> 
> Nit: change "from_tls_handshake" to "from_tls_extension", if you agree.

Done.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.h:46: // |origin|.
On 2013/11/21 02:05:02, wtc wrote:
> 
> 1. Nit: "Fills in the origin field of each SCT from |origin|" is very
confusing
> without looking at the code in the .cc file. It would be better to describe at
a
> more abstract level how |origin| would be used by this function, for example,
>   Treat each SCT as being obtained from |origin|.
> Or
>   The SCTs in the list come from |origin|.
> 
> 2. Nit: explain what |expected_entry| is for.

Done.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.h:58: typedef std::map<std::string,
linked_ptr<CTLogVerifier> > IDToLogMap;
On 2013/11/21 02:05:02, wtc wrote:
> 
> 1. Nit: typedef should be listed at the beginning of the "private" section.
> 
> Recommended by the Style Guide:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order
> 
> 2. Nit: document what the std::string "ID" is. I guess it's the ID of the
log's
> public key?

1. Done

2. You're right - the ID is a hash of the log's public key, which is defined in
section 3.2. of RFC6962 (CT). Added that information in the comment.

https://codereview.chromium.org/67513008/diff/310001/net/data/ssl/certificate...
File net/data/ssl/certificates/ct-test-embedded-with-preca-chain.pem (right):

https://codereview.chromium.org/67513008/diff/310001/net/data/ssl/certificate...
net/data/ssl/certificates/ct-test-embedded-with-preca-chain.pem:1: -----BEGIN
CERTIFICATE-----
On 2013/11/21 02:05:02, wtc wrote:
> 
> Nit: this file doesn't have the human-readable form of the certificates.

Indeed - the X.509 extension containing the SCT was wrapped by some diff
formatting hook, which meant that applying this patch failed on *all* trybots.
I've decided to simply drop it - but if you can point me to the person who may
know about the hooks that may be formatting my diff, I'll gladly follow it up
with them.

https://codereview.chromium.org/67513008/diff/310001/net/test/cert_test_util.cc
File net/test/cert_test_util.cc (right):

https://codereview.chromium.org/67513008/diff/310001/net/test/cert_test_util....
net/test/cert_test_util.cc:33: CertificateList certs =
CreateCertificateListFromFile(
On 2013/11/21 02:05:02, wtc wrote:
> 
> This is an inefficient way to implement this function. It is acceptable
because
> this function is only used in testing.
> 
> I was hoping that we could use CreateOSCertHandlesFromBytes, but it doesn't
seem
> to support the format of multiple CERTIFICATEs in a PEM file.

Ack - it is indeed only for testing.

https://codereview.chromium.org/67513008/diff/310001/net/test/cert_test_util....
net/test/cert_test_util.cc:47: 
On 2013/11/21 02:05:02, wtc wrote:
> 
> Nit: delete one blank line.

Done.

https://codereview.chromium.org/67513008/diff/310001/net/test/cert_test_util.h
File net/test/cert_test_util.h (right):

https://codereview.chromium.org/67513008/diff/310001/net/test/cert_test_util....
net/test/cert_test_util.h:21: // Imports all of the certificates in |cert_file|,
a file in |certs_dir|,
On 2013/11/21 02:05:02, wtc wrote:
> 
> Add a blank line before this comment.

Done.

https://codereview.chromium.org/67513008/diff/310001/net/test/cert_test_util....
net/test/cert_test_util.h:36: // Imports a single certificate from |cert_file|.
On 2013/11/21 02:05:02, wtc wrote:
> 
> Nit: it seems useful to keep the original comment for clarification:
>   If cert_file contains multiple certificates, the first certificate
>   found will be returned.

Done.

[wtc, 2013-11-21 23:26:33]

Patch set 9 LGTM.

I suggest adding the error codes separately, when you add new bits to
cert_status_flags.h. I also suspect CTVerifier errors don't need to be
fine-grained because the CTVerifyResult has detailed info on failure.

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

https://codereview.chromium.org/67513008/diff/310001/net/cert/ct_verifier.h#n...
net/cert/ct_verifier.h:25: // OCSP stapling on the given |verified_cert|

On 2013/11/21 20:06:02, eranm wrote:
>
> Changed the comment. Would you like me to change the enum as well?

Yes, please. The enum should match the parameter names of this function.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.cc (right):

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:24: logs_[log_id] =
linked_ptr<CTLogVerifier>(log_verifier.release());

On 2013/11/21 20:06:02, eranm wrote:
>
> 2. Will look into changing the type of the input parameter to linked_ptr as
well

You can declare the |log_verifier| parameter as a plain pointer and document
that MultiLogCTVerifier takes ownership of |log_verifier|.

Declaring |log_verifier| as a linked_ptr also seems fine.

I am not an expert in this area, so my opinion may be wrong.

https://codereview.chromium.org/67513008/diff/310001/net/data/ssl/certificate...
File net/data/ssl/certificates/ct-test-embedded-with-preca-chain.pem (right):

https://codereview.chromium.org/67513008/diff/310001/net/data/ssl/certificate...
net/data/ssl/certificates/ct-test-embedded-with-preca-chain.pem:1: -----BEGIN
CERTIFICATE-----

On 2013/11/21 20:06:02, eranm wrote:
> 
> Indeed - the X.509 extension containing the SCT was wrapped by some diff
> formatting hook, which meant that applying this patch failed on *all* trybots.

I see. We can commit this PEM file separately by hand. If you don't have the
privilege to commit CLs manually, I can do that for you.

> I've decided to simply drop it - but if you can point me to the person who may
> know about the hooks that may be formatting my diff, I'll gladly follow it up
> with them.

I don't kno who's the best person. You can ask maruel@ and phajdan.jr@.

https://codereview.chromium.org/67513008/diff/480001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/67513008/diff/480001/net/base/net_error_list....
net/base/net_error_list.h:730: NET_ERROR(NO_SCTS_VERIFIED_OK, -903)

I think CT-related errors seem to belong in the "200-299 Certificate errors"
block.

Please remove this file from this CL. Let's deal with this in your CL that adds
new bits to cert_status_flags.h.

https://codereview.chromium.org/67513008/diff/480001/net/cert/ct_verify_resul...
File net/cert/ct_verify_result.cc (right):

https://codereview.chromium.org/67513008/diff/480001/net/cert/ct_verify_resul...
net/cert/ct_verify_result.cc:3: // found in the LICENSE file.

Add a blank line after the copyright header.

https://codereview.chromium.org/67513008/diff/480001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.cc (right):

https://codereview.chromium.org/67513008/diff/480001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:22: return;

Are you sure we need to allow a null |log_verifier|?

https://codereview.chromium.org/67513008/diff/480001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:86: return ERR_NO_SCTS_PRESENT;

It should be sufficient to just use one error code here because the caller can
inspect |result| to distinguish the three error conditions.

https://codereview.chromium.org/67513008/diff/480001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:142: // been issued.

1. The comment in the reference implementation says:
  / Allow a bit of slack, say 1 second into the future.

If we want to allow 1 second into the future, it seems that we should add 1
second to base::Time::Now():
  if (sct.timestamp > base::Time::Now() + base::TimeDelta::FromSeconds(1)) {

(The reference implementation uses util::TimeInMilliseconds() + 1000.)

Your test actually makes the check stricter rather than slacker.

2. To allow clock skew, a 1-second slack is too short.

https://codereview.chromium.org/67513008/diff/480001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.h (right):

https://codereview.chromium.org/67513008/diff/480001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.h:51: // come from |origin| (as will be indicated
in the origin field of each SCT)

Nit: line 46 and line 51 are missing a period (.) at the end.

[Ryan Sleevi, 2013-11-21 23:37:02]

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.cc (right):

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:24: logs_[log_id] =
linked_ptr<CTLogVerifier>(log_verifier.release());
On 2013/11/21 23:26:34, wtc wrote:
> 
> On 2013/11/21 20:06:02, eranm wrote:
> >
> > 2. Will look into changing the type of the input parameter to linked_ptr as
> well
> 
> You can declare the |log_verifier| parameter as a plain pointer and document
> that MultiLogCTVerifier takes ownership of |log_verifier|.
> 
> Declaring |log_verifier| as a linked_ptr also seems fine.
> 
> I am not an expert in this area, so my opinion may be wrong.

Taking a scoped_ptr<CTLogVerifier> and turning it into a linked_ptr<> is the
"Right Way". The scoped_ptr<> clearly communicates that ownership is
transferred; this is the preferred pattern over documentation. Releasing it into
a linked_ptr<> is also fine, as the ownership is then transferred into a
linked_ptr<>.

So this code looks fine to me.

The only thing I'm not sure of is why you don't do
linked_ptr<CTLogVerifier> log(log_verifier);
logs_[log->key_id()] = log;

I checked the C++03 spec and couldn't find a guarantee that
logs_[log_verifier->key_id()] = linked_ptr<...> was safe (even though I'm 95%
sure it is), but at least with the above construct, you trade a malloc (since
the default small-string optimization size for GCC and MSVC is ~16 bytes, and
ISTR that key IDs are ~20 bytes) for an int inc/dec (to the linked_ptr's
refcount)

[Eran M. (Google), 2013-11-23 21:02:06]

Addressed all comments: Removed the changes to net_error_list.h, left the
pointer initialization as Ryan suggested, changed the enum's value name.

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

https://codereview.chromium.org/67513008/diff/310001/net/cert/ct_verifier.h#n...
net/cert/ct_verifier.h:25: // OCSP stapling on the given |verified_cert|
On 2013/11/21 23:26:34, wtc wrote:
> 
> On 2013/11/21 20:06:02, eranm wrote:
> >
> > Changed the comment. Would you like me to change the enum as well?
> 
> Yes, please. The enum should match the parameter names of this function.

Done.

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.cc (right):

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:24: logs_[log_id] =
linked_ptr<CTLogVerifier>(log_verifier.release());
On 2013/11/21 23:37:02, Ryan Sleevi wrote:
> On 2013/11/21 23:26:34, wtc wrote:
> > 
> > On 2013/11/21 20:06:02, eranm wrote:
> > >
> > > 2. Will look into changing the type of the input parameter to linked_ptr
as
> > well
> > 
> > You can declare the |log_verifier| parameter as a plain pointer and document
> > that MultiLogCTVerifier takes ownership of |log_verifier|.
> > 
> > Declaring |log_verifier| as a linked_ptr also seems fine.
> > 
> > I am not an expert in this area, so my opinion may be wrong.
> 
> Taking a scoped_ptr<CTLogVerifier> and turning it into a linked_ptr<> is the
> "Right Way". The scoped_ptr<> clearly communicates that ownership is
> transferred; this is the preferred pattern over documentation. Releasing it
into
> a linked_ptr<> is also fine, as the ownership is then transferred into a
> linked_ptr<>.
> 
> So this code looks fine to me.
> 
> The only thing I'm not sure of is why you don't do
> linked_ptr<CTLogVerifier> log(log_verifier);
> logs_[log->key_id()] = log;
> 
> I checked the C++03 spec and couldn't find a guarantee that
> logs_[log_verifier->key_id()] = linked_ptr<...> was safe (even though I'm 95%
> sure it is), but at least with the above construct, you trade a malloc (since
> the default small-string optimization size for GCC and MSVC is ~16 bytes, and
> ISTR that key IDs are ~20 bytes) for an int inc/dec (to the linked_ptr's
> refcount)
> 

Done as Ryan suggested - only modification is I've initialized log with
log_verifier.release().

https://codereview.chromium.org/67513008/diff/480001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/67513008/diff/480001/net/base/net_error_list....
net/base/net_error_list.h:730: NET_ERROR(NO_SCTS_VERIFIED_OK, -903)
On 2013/11/21 23:26:34, wtc wrote:
> 
> I think CT-related errors seem to belong in the "200-299 Certificate errors"
> block.
> 
> Please remove this file from this CL. Let's deal with this in your CL that
adds
> new bits to cert_status_flags.h.

Done - reverted changes to this file.
As for including CT-related errors in the mentioned range, the errors included
in this range up to ERR_CERT_END are all included in the checks for
IsCertificateError, which affects the behaviour of error handling in the code in
many places.

Seems safe to include this error at the end of the range, e.g. -299 (since the
common error would be that no SCTs are present).

https://codereview.chromium.org/67513008/diff/480001/net/cert/ct_verify_resul...
File net/cert/ct_verify_result.cc (right):

https://codereview.chromium.org/67513008/diff/480001/net/cert/ct_verify_resul...
net/cert/ct_verify_result.cc:3: // found in the LICENSE file.
On 2013/11/21 23:26:34, wtc wrote:
> 
> Add a blank line after the copyright header.

Done.

https://codereview.chromium.org/67513008/diff/480001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.cc (right):

https://codereview.chromium.org/67513008/diff/480001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:22: return;
On 2013/11/21 23:26:34, wtc wrote:
> 
> Are you sure we need to allow a null |log_verifier|?

No, but I wouldn't want to crash here in production code. Added DCHECK.

https://codereview.chromium.org/67513008/diff/480001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:86: return ERR_NO_SCTS_PRESENT;
On 2013/11/21 23:26:34, wtc wrote:
> 
> It should be sufficient to just use one error code here because the caller can
> inspect |result| to distinguish the three error conditions.

Will be done in the next patch, when I introduce the new error code.

https://codereview.chromium.org/67513008/diff/480001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:142: // been issued.
On 2013/11/21 23:26:34, wtc wrote:
> 
> 1. The comment in the reference implementation says:
>   / Allow a bit of slack, say 1 second into the future.
> 
> If we want to allow 1 second into the future, it seems that we should add 1
> second to base::Time::Now():
>   if (sct.timestamp > base::Time::Now() + base::TimeDelta::FromSeconds(1)) {
> 
> (The reference implementation uses util::TimeInMilliseconds() + 1000.)
> 
> Your test actually makes the check stricter rather than slacker.
> 
> 2. To allow clock skew, a 1-second slack is too short.

I've dropped this 1 second interval entirely. You're right, a 1-second slack is
too short.
On the next iteration we should measure how common is SCT failure due to this
cause and adjust slack accordingly.

https://codereview.chromium.org/67513008/diff/480001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.h (right):

https://codereview.chromium.org/67513008/diff/480001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.h:51: // come from |origin| (as will be indicated
in the origin field of each SCT)
On 2013/11/21 23:26:34, wtc wrote:
> 
> Nit: line 46 and line 51 are missing a period (.) at the end.

Done.

[commit-bot: I haz the power, 2013-11-24 07:31:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eranm@google.com/67513008/810001

[commit-bot: I haz the power, 2013-11-24 09:55:56]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-11-24 21:41:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eranm@google.com/67513008/810001

[Eran M. (Google), 2013-11-24 21:41:31]

wtc@, PTAL at patchset 11, necessary since Al's changes to the
SignedCertificateTimestamp landed before my patch was committed.

[commit-bot: I haz the power, 2013-11-24 22:33:01]

Change committed as 237008

[wtc, 2013-11-25 01:48:31]

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.cc (right):

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:24: logs_[log_id] =
linked_ptr<CTLogVerifier>(log_verifier.release());

On 2013/11/21 23:37:02, Ryan Sleevi wrote:
>
> So this code looks fine to me.
> 
> The only thing I'm not sure of is why you don't do
> linked_ptr<CTLogVerifier> log(log_verifier);
> logs_[log->key_id()] = log;
> 
> I checked the C++03 spec and couldn't find a guarantee that
> logs_[log_verifier->key_id()] = linked_ptr<...> was safe (even though I'm 95%
> sure it is), but at least with the above construct, you trade a malloc (since
> the default small-string optimization size for GCC and MSVC is ~16 bytes, and
> ISTR that key IDs are ~20 bytes) for an int inc/dec (to the linked_ptr's
> refcount)

I think
    logs_[log_id].reset(log_verifier.release());
is cheaper because it avoids a linked_ptr constructor and destructor call, which
is why I suggested it. It seems that the reset() method is intended exactly for
taking a plain pointer. I am curious why you prefer the assignment operator.

[Ryan Sleevi, 2013-11-25 01:56:12]

On 2013/11/25 01:48:31, wtc wrote:
>
https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
> File net/cert/multi_log_ct_verifier.cc (right):
> 
>
https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
> net/cert/multi_log_ct_verifier.cc:24: logs_[log_id] =
> linked_ptr<CTLogVerifier>(log_verifier.release());
> 
> On 2013/11/21 23:37:02, Ryan Sleevi wrote:
> >
> > So this code looks fine to me.
> > 
> > The only thing I'm not sure of is why you don't do
> > linked_ptr<CTLogVerifier> log(log_verifier);
> > logs_[log->key_id()] = log;
> > 
> > I checked the C++03 spec and couldn't find a guarantee that
> > logs_[log_verifier->key_id()] = linked_ptr<...> was safe (even though I'm
95%
> > sure it is), but at least with the above construct, you trade a malloc
(since
> > the default small-string optimization size for GCC and MSVC is ~16 bytes,
and
> > ISTR that key IDs are ~20 bytes) for an int inc/dec (to the linked_ptr's
> > refcount)
> 
> I think
>     logs_[log_id].reset(log_verifier.release());
> is cheaper because it avoids a linked_ptr constructor and destructor call,
which
> is why I suggested it. It seems that the reset() method is intended exactly
for
> taking a plain pointer. I am curious why you prefer the assignment operator.

It's the same cost (aka trivial). However, the syntax of the assignment allows
for an optimization

logs_[log_id].reset() is equivalent to
  logs_.insert(make_pair(log_id,
linked_ptr<LogVerifier>())).second.first.reset()

logs_[log_id] = linked_ptr<...> is equivalent to
  logs_.insert(make_pair(log_id, linked_ptr<LogVerifier>())).second.first = ...

Which is roughly the same, except because of the assignment, may be RVO'd,
whereas .reset() cannot.

[wtc, 2013-11-25 23:01:59]

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.cc (right):

https://codereview.chromium.org/67513008/diff/310001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:24: logs_[log_id] =
linked_ptr<CTLogVerifier>(log_verifier.release());

Thanks for the explanation.

You wrote:
> It's the same cost (aka trivial). However, the syntax of the assignment allows
> for an optimization
> 
> logs_[log_id].reset() is equivalent to
>   logs_.insert(make_pair(log_id,
> linked_ptr<LogVerifier>())).second.first.reset()
> 
> logs_[log_id] = linked_ptr<...> is equivalent to
>   logs_.insert(make_pair(log_id, linked_ptr<LogVerifier>())).second.first =
...
> 
> Which is roughly the same, except because of the assignment, may be RVO'd,
> whereas .reset() cannot.

So the local variable |log| in
  linked_ptr<CTLogVerifier> log(log_verifier.release());
  logs_[log->key_id()] = log;
can used by the compiler (through RVO) as the second argument for the make_pair
in the equivalent code you showed?

I am surprised the compiler can pull that off. That would indeed be cheaper than
using reset(), which doesn't require a link_ptr local variable or temporary
object but requires constructing an empty linked_ptr for make_pair.

[wtc, 2013-11-25 23:15:49]

Patch set 11 LGTM. I reviewed the diffs between patch sets 9 and 11. Please fix
the two nits I found.

https://codereview.chromium.org/67513008/diff/480001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.cc (right):

https://codereview.chromium.org/67513008/diff/480001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:22: return;

On 2013/11/23 21:02:07, eranm wrote:
>
> No, but I wouldn't want to crash here in production code. Added DCHECK.

A crash may help expose a bug sooner. If we know none of the callers will pass
null, the null check is not necessary. Alternatively, the function can return
bool to report a failure.

https://codereview.chromium.org/67513008/diff/810001/net/cert/ct_serializatio...
File net/cert/ct_serialization.cc (right):

https://codereview.chromium.org/67513008/diff/810001/net/cert/ct_serializatio...
net/cert/ct_serialization.cc:320: scoped_refptr<SignedCertificateTimestamp>*
output) {

The function's two parameters should be aligned. (This comes from Al's code, I
think.)

https://codereview.chromium.org/67513008/diff/810001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.cc (right):

https://codereview.chromium.org/67513008/diff/810001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:138: // been issued.

Delete the stale comment about the 1-second slack on lines 137-138.

[Eran M. (Google), 2013-11-26 10:10:23]

Addressed comments in https://codereview.chromium.org/87923002.

https://codereview.chromium.org/67513008/diff/810001/net/cert/ct_serializatio...
File net/cert/ct_serialization.cc (right):

https://codereview.chromium.org/67513008/diff/810001/net/cert/ct_serializatio...
net/cert/ct_serialization.cc:320: scoped_refptr<SignedCertificateTimestamp>*
output) {
On 2013/11/25 23:15:50, wtc wrote:
> 
> The function's two parameters should be aligned. (This comes from Al's code, I
> think.)

Done.

https://codereview.chromium.org/67513008/diff/810001/net/cert/multi_log_ct_ve...
File net/cert/multi_log_ct_verifier.cc (right):

https://codereview.chromium.org/67513008/diff/810001/net/cert/multi_log_ct_ve...
net/cert/multi_log_ct_verifier.cc:138: // been issued.
On 2013/11/25 23:15:50, wtc wrote:
> 
> Delete the stale comment about the 1-second slack on lines 137-138.

Done.