Issue 88643002: SignedCertificateTimestamp storing & serialization code.

Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+399 lines, -39 lines)			Patch
content/browser/loader/resource_loader.h	View	1 2 3 4 5 6 7	2 chunks	+9 lines, -0 lines	0 comments	Download
content/browser/loader/resource_loader.cc	View	1 2 3 4 5 6 7	4 chunks	+31 lines, -2 lines	0 comments	Download
content/browser/ssl/ssl_manager.cc	View	1 2 3 4 5	2 chunks	+6 lines, -1 line	0 comments	Download
content/browser/web_contents/web_contents_impl.cc	View	1 2 3 4 5 6 7	1 chunk	+4 lines, -1 line	0 comments	Download
content/common/ssl_status_serialization.h	View	1 2 3 4 5 6 7	1 chunk	+16 lines, -10 lines	0 comments	Download
content/common/ssl_status_serialization.cc	View	1 2 3 4 5 6 7	3 chunks	+43 lines, -10 lines	0 comments	Download
content/content_common.gypi	View	1 2 3	2 chunks	+3 lines, -0 lines	0 comments	Download
content/public/common/signed_certificate_timestamp_id_and_status.h	View	1 2 3 4	1 chunk	+32 lines, -0 lines	0 comments	Download
content/public/common/signed_certificate_timestamp_id_and_status.cc	View	1 2 3 4 5 6 7	1 chunk	+18 lines, -0 lines	0 comments	Download
content/public/common/ssl_status.h	View	1 2 3 4 5 6 7 8	3 chunks	+6 lines, -1 line	0 comments	Download
content/public/common/ssl_status.cc	View	1 2 3 4 5 6 7 8	1 chunk	+2 lines, -0 lines	0 comments	Download
content/renderer/context_menu_params_builder.cc	View		1 chunk	+4 lines, -4 lines	0 comments	Download
content/renderer/render_view_browsertest.cc	View	1 2 3 4 5 6 7 8	1 chunk	+2 lines, -1 line	0 comments	Download
content/renderer/render_view_impl.cc	View	1 2 3 4 5 6 7	1 chunk	+2 lines, -1 line	0 comments	Download
net/cert/ct_verify_result.h	View	1 2 3 4 5 6	1 chunk	+2 lines, -2 lines	0 comments	Download
net/cert/multi_log_ct_verifier.cc	View	1 2 3 4 5 6 7	2 chunks	+3 lines, -3 lines	0 comments	Download
net/cert/multi_log_ct_verifier_unittest.cc	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download
net/cert/sct_status_flags.h	View	1 2 3 4 5 6	1 chunk	+32 lines, -0 lines	0 comments	Download
net/cert/signed_certificate_timestamp.h	View	1 2 3 4 5 6	2 chunks	+7 lines, -0 lines	0 comments	Download
net/cert/signed_certificate_timestamp.cc	View	1 2 3 4 5 6	2 chunks	+41 lines, -0 lines	0 comments	Download
net/http/http_response_info.cc	View	1 2 3 4 5 6	5 chunks	+31 lines, -0 lines	0 comments	Download
net/net.gyp	View	1 2 3 4 5 6 7	2 chunks	+3 lines, -0 lines	0 comments	Download
net/socket/ssl_client_socket_nss.h	View	1 2 3 4 5 6 7	1 chunk	+7 lines, -0 lines	0 comments	Download
net/socket/ssl_client_socket_nss.cc	View	1 2 3 4 5 6 7	4 chunks	+28 lines, -1 line	0 comments	Download
net/ssl/signed_certificate_timestamp_and_status.h	View	1 2 3 4 5 6 7 8	1 chunk	+35 lines, -0 lines	0 comments	Download
net/ssl/signed_certificate_timestamp_and_status.cc	View	1 2 3 4 5 6 7 8	1 chunk	+18 lines, -0 lines	0 comments	Download
net/ssl/ssl_info.h	View	1 2 3 4	2 chunks	+9 lines, -0 lines	0 comments	Download
net/ssl/ssl_info.cc	View	1 2 3 4 5 6	3 chunks	+4 lines, -1 line	0 comments	Download

Messages

Total messages: 15 (0 generated)

Expand Messages | Collapse Messages

Eran M. (Google)

LGTM from the Certificate Transparency side of things - with this info in place we ...

7 years ago (2013-11-26 21:33:21 UTC) #1

alcutter

https://codereview.chromium.org/88643002/diff/60001/content/browser/loader/resource_loader.cc File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/88643002/diff/60001/content/browser/loader/resource_loader.cc#newcode505 content/browser/loader/resource_loader.cc:505: request_->ssl_info().signed_certificate_timestamps, info->GetChildID(), On 2013/11/26 21:33:22, eranm wrote: > Nit: ...

7 years ago (2013-11-26 23:00:05 UTC) #2

jam

lgtm with these nits https://codereview.chromium.org/88643002/diff/80001/content/browser/loader/resource_loader.cc File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/88643002/diff/80001/content/browser/loader/resource_loader.cc#newcode479 content/browser/loader/resource_loader.cc:479: int process_id, content::SignedCertificateTimestampIDStatusList* ditto https://codereview.chromium.org/88643002/diff/80001/content/browser/loader/resource_loader.cc#newcode502 ...

7 years ago (2013-11-27 01:07:50 UTC) #4

alcutter

https://codereview.chromium.org/88643002/diff/80001/content/browser/loader/resource_loader.cc File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/88643002/diff/80001/content/browser/loader/resource_loader.cc#newcode479 content/browser/loader/resource_loader.cc:479: int process_id, content::SignedCertificateTimestampIDStatusList* On 2013/11/27 01:07:50, jam wrote: > ...

7 years ago (2013-11-27 12:17:55 UTC) #5

wtc

Review comments on patch set 4: I only reviewed the files in src/net. I didn't ...

7 years ago (2013-11-27 16:32:40 UTC) #6

wtc

https://codereview.chromium.org/88643002/diff/100001/net/cert/signed_certificate_timestamp.cc File net/cert/signed_certificate_timestamp.cc (right): https://codereview.chromium.org/88643002/diff/100001/net/cert/signed_certificate_timestamp.cc#newcode52 net/cert/signed_certificate_timestamp.cc:52: std::string sig_data; On 2013/11/27 16:32:41, wtc wrote: > > ...

7 years ago (2013-11-27 17:29:18 UTC) #7

alcutter

https://codereview.chromium.org/88643002/diff/100001/net/cert/sct_status_flags.h File net/cert/sct_status_flags.h (right): https://codereview.chromium.org/88643002/diff/100001/net/cert/sct_status_flags.h#newcode1 net/cert/sct_status_flags.h:1: // Copyright (c) 2013 The Chromium Authors. All rights ...

7 years ago (2013-11-27 18:05:54 UTC) #8

https://codereview.chromium.org/88643002/diff/100001/net/cert/sct_status_flags.h
File net/cert/sct_status_flags.h (right):

https://codereview.chromium.org/88643002/diff/100001/net/cert/sct_status_flag...
net/cert/sct_status_flags.h:1: // Copyright (c) 2013 The Chromium Authors. All
rights reserved.
On 2013/11/27 16:32:41, wtc wrote:
> 
> Although it's not required for the build, please add this new header to
> net/net.gyp.

Done.

https://codereview.chromium.org/88643002/diff/100001/net/cert/sct_status_flag...
net/cert/sct_status_flags.h:6: #define NET_CERT_SCT_STATUS_H_
On 2013/11/27 16:32:41, wtc wrote:
> 
> The include guard macro should be updated to match the header file's pathname.

Ay ay, done.

https://codereview.chromium.org/88643002/diff/100001/net/cert/sct_status_flag...
net/cert/sct_status_flags.h:10: enum
SignedCertificateTimestampVerificationStatus {
On 2013/11/27 16:32:41, wtc wrote:
> 
> Please document this enum type and its four values.

Done.
> 
> Nit: the enum type's name could be shorter. Perhaps we can put this inside the
> "ct" namespace and name this enum type "ct::SCTStatus" or
"ct::SCTVerifyStatus".

Done.

https://codereview.chromium.org/88643002/diff/100001/net/cert/sct_status_flag...
net/cert/sct_status_flags.h:11: SCT_STATUS_UNSET = 0,
On 2013/11/27 16:32:41, wtc wrote:
> 
> Nit: it's not clear what "unset" means.

This was a change in response to Eran's comment earlier, just to avoid a default
value of 0 ever being mis-interpreted as a real state, I was going to use
INVALID but that's already taken and means something else.

https://codereview.chromium.org/88643002/diff/100001/net/cert/signed_certific...
File net/cert/signed_certificate_timestamp.cc (right):

https://codereview.chromium.org/88643002/diff/100001/net/cert/signed_certific...
net/cert/signed_certificate_timestamp.cc:52: std::string sig_data;
On 2013/11/27 16:32:41, wtc wrote:
> 
> Nit: you can store the new SignedCertificateTimestamp in a scoped_ptr. Then,
> pass its std::string members to iter->ReadString(). This will avoid string
> copying.

That occurred to me too, but I thought the split of setting some things directly
and other via locals might be a bit confusing.  I've done it anyway, but added a
small comment.

https://codereview.chromium.org/88643002/diff/100001/net/cert/signed_certific...
net/cert/signed_certificate_timestamp.cc:52: std::string sig_data;
On 2013/11/27 17:29:18, wtc wrote:
> 
> On 2013/11/27 16:32:41, wtc wrote:
> > 
> > Nit: you can store the new SignedCertificateTimestamp in a scoped_ptr.
> 
> I just realized that SignedCertificateTimestamp is a referenced-counted type,
so
> we can't store it in a scoped_ptr. My suggestion needs more work.

Ah, I'd assumed you meant a scoped_refptr in the original comment anyway.

> 
> Maybe this function needs to return a
scoped_refptr<SignedCertificateTimestamp>.
> Then we can store the new SignedCertificateTimestamp in a scoped_refptr.

This was loosely modelled on the x509_certificate CreateFromPickle which returns
a raw ptr, but I guess a scoped_refptr works too possibly at the cost of an
extra AddRef()/Release().

I've changed to return a scoped_refptr for now, but I'm happy to do whatever you
recommend.

https://codereview.chromium.org/88643002/diff/100001/net/http/http_response_i...
File net/http/http_response_info.cc (right):

https://codereview.chromium.org/88643002/diff/100001/net/http/http_response_i...
net/http/http_response_info.cc:222: if (sct.get() == NULL ||
!pickle.ReadUInt16(&iter, &status))
On 2013/11/27 16:32:41, wtc wrote:
> 
> Nit: sct.get() == NULL => !sct.get()

Done.

https://codereview.chromium.org/88643002/diff/100001/net/http/http_response_i...
net/http/http_response_info.cc:226: sct,
static_cast<SignedCertificateTimestampVerificationStatus>(
On 2013/11/27 16:32:41, wtc wrote:
> 
> Nit: this is a sign that the enum type's name is too long :-)

BahIDoNotKnowWhatYouAreTalkingAboutSeemsJustFineToMe :)

https://codereview.chromium.org/88643002/diff/100001/net/http/http_response_i...
net/http/http_response_info.cc:310: if
(ssl_info.signed_certificate_timestamps.size() > 0)
On 2013/11/27 16:32:41, wtc wrote:
> 
> Nit: test !ssl_info.signed_certificate_timestamps.empty(). This is the
preferred
> style in Chromium.
> 
> Also change line 339.

Done.

https://codereview.chromium.org/88643002/diff/100001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/88643002/diff/100001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:2798: void
SSLClientSocketNSS::AddSCTInfoToSSLInfo(SSLInfo* ssl_info) const {
On 2013/11/27 16:32:41, wtc wrote:
> 
> The Style Guide recommends that functions be defined in the .cc file in the
same
> order that the functions are declared in the .h file.

Sorry, done.

https://codereview.chromium.org/88643002/diff/100001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.cc:2809:
SignedCertificateTimestampAndStatus(*iter, SCT_STATUS_INVALID));
On 2013/11/27 16:32:41, wtc wrote:
> 
> Nit: we should use the same term to describe this case. Now we have two:
> "unverified" and "INVALID".

I've changed "unverified_scts" to "invalid_scts" in ct_verify_result.h (mainly
because "unverified" could be mistaken for "SCTs for which verification has not
yet been attempted").

https://codereview.chromium.org/88643002/diff/100001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_nss.h (right):

https://codereview.chromium.org/88643002/diff/100001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_nss.h:151: void AddSCTInfoToSSLInfo(SSLInfo*
ssl_info) const;
On 2013/11/27 16:32:41, wtc wrote:
> 
> Nit: add a short comment to describe what this function does.

Done.

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

https://codereview.chromium.org/88643002/diff/100001/net/ssl/ssl_info.cc#newc...
net/ssl/ssl_info.cc:55: signed_certificate_timestamps.clear();
On 2013/11/27 16:32:41, wtc wrote:
> 
> Move this up, between lines 46 and 47, to match the order in which these
members
> are declared.

Exchanged with the public_key_hashes.clear() line to match the new location in
the decl.

> 
> Can you also delete the blank line? Thanks.

Done.

https://codereview.chromium.org/88643002/diff/100001/net/ssl/ssl_info.h
File net/ssl/ssl_info.h (right):

https://codereview.chromium.org/88643002/diff/100001/net/ssl/ssl_info.h#newco...
net/ssl/ssl_info.h:27: struct SignedCertificateTimestampAndStatus {
On 2013/11/27 16:32:41, wtc wrote:
> 
> Nit: this struct probably should be declared in a separate file.

Done.

https://codereview.chromium.org/88643002/diff/100001/net/ssl/ssl_info.h#newco...
net/ssl/ssl_info.h:68: SignedCertificateTimestampAndStatusList
signed_certificate_timestamps;
On 2013/11/27 16:32:41, wtc wrote:
> 
> I suggest moving this member after |cert_status|, so that |cert| and
> |cert_status| are not separated.
> 
> Maybe we can move this member to the end of the SSLInfo struct?

Done.

wtc

Patch set 6 LGTM. I only reviewed the net directory. https://codereview.chromium.org/88643002/diff/100001/net/cert/signed_certificate_timestamp.cc File net/cert/signed_certificate_timestamp.cc (right): https://codereview.chromium.org/88643002/diff/100001/net/cert/signed_certificate_timestamp.cc#newcode52 ...

7 years ago (2013-11-28 01:28:02 UTC) #9

alcutter

7 years ago (2013-11-28 12:08:19 UTC) #10

https://codereview.chromium.org/88643002/diff/100001/net/cert/signed_certific...
File net/cert/signed_certificate_timestamp.cc (right):

https://codereview.chromium.org/88643002/diff/100001/net/cert/signed_certific...
net/cert/signed_certificate_timestamp.cc:52: std::string sig_data;
On 2013/11/28 01:28:02, wtc wrote:
> 
> On 2013/11/27 18:05:55, alcutter wrote:
> >
> > This was loosely modelled on the x509_certificate CreateFromPickle which
> returns
> > a raw ptr, but I guess a scoped_refptr works too possibly at the cost of an
> > extra AddRef()/Release().
> > 
> > I've changed to return a scoped_refptr for now, but I'm happy to do whatever
> you
> > recommend.
> 
> The new convention in Chromium seems to recommend we always use a
scoped_refptr
> with a reference-counted type. X509Certificate::CreateFromPickle was written
> before the new convention was formed, which is why it returns a raw pointer.
> 
> Personally, I think the new convention can result in verbose code and extra
> AddRef()/Release() calls, which may incur atomic instruction overhead, but I
> understand its benefits.

Ack.

https://codereview.chromium.org/88643002/diff/130001/content/browser/loader/r...
File content/browser/loader/resource_loader.h (right):

https://codereview.chromium.org/88643002/diff/130001/content/browser/loader/r...
content/browser/loader/resource_loader.h:94: signed_certificate_timestamp_ids);
On 2013/11/28 01:28:02, wtc wrote:
> 
> Nit: can the last two lines be formatted this way?
> 
>       int process_id,
>       SignedCertificateTimestampIDStatusList*
signed_certificate_timestamp_ids);
> 
> Maybe this calls for using the "sct" abbreviation as you did with "sct_list".

Maybe I follow the "don't abbreviate" doctrine too strictly :)
Done.

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

https://codereview.chromium.org/88643002/diff/130001/net/cert/ct_verify_resul...
net/cert/ct_verify_result.h:28: SCTList invalid_scts;
On 2013/11/28 01:28:02, wtc wrote:
> 
> Nit: Please discuss with Eran to standardize these two member names. It seems
> that ideally they should look opposite: "verified vs. unverified" or "valid
vs.
> invalid".

The problem is that "verified" is the right word for SCTs which have been found
to be valid, but the opposite of that isn't "unverified".

Equally, SCTs for which we don't have the public key may well also in fact be
valid, but we can't verify them so calling the list of SCTs we were able to
check valid_scts feels wrong too.

Since it's effectively tri-state logic, trying to name with opposites is always
going to end up with one list with an incongruous name. So maybe the right
answer is something along the lines of:  verified_scts, invalid_scts,
unverified_scts ?

I'll prepare a follow up patch for that combo so as to not block this on names,
I hope that's ok.

https://codereview.chromium.org/88643002/diff/130001/net/cert/sct_status_flags.h
File net/cert/sct_status_flags.h (right):

https://codereview.chromium.org/88643002/diff/130001/net/cert/sct_status_flag...
net/cert/sct_status_flags.h:16: SCT_STATUS_UNSET = 0,
On 2013/11/28 01:28:02, wtc wrote:
> 
> Nit: we can also consider "NONE", "UNINITIALIZED", or "UNUSED".

changed to NONE, I like that, thanks.

https://codereview.chromium.org/88643002/diff/130001/net/cert/signed_certific...
File net/cert/signed_certificate_timestamp.cc (right):

https://codereview.chromium.org/88643002/diff/130001/net/cert/signed_certific...
net/cert/signed_certificate_timestamp.cc:52: std::string sig_data;
On 2013/11/28 01:28:02, wtc wrote:
> 
> Remove the local variables log_id, extensions, sig_data.

Sorry, done.

https://codereview.chromium.org/88643002/diff/130001/net/cert/signed_certific...
net/cert/signed_certificate_timestamp.cc:64: // Now set the rest of the member
vars:
On 2013/11/28 01:28:02, wtc wrote:
> 
> Nit: vars => variables
> 
> Our Style Guide seems to recommend unnecessary abbreviation in general. It's
> best to avoid them to stay away from the style police :-)

Done.

https://codereview.chromium.org/88643002/diff/130001/net/cert/signed_certific...
File net/cert/signed_certificate_timestamp.h (right):

https://codereview.chromium.org/88643002/diff/130001/net/cert/signed_certific...
net/cert/signed_certificate_timestamp.h:101: CreateFromPickle(PickleIterator*
iter);
On 2013/11/28 01:28:02, wtc wrote:
> 
> Nit: if this works, this line folding might look nicer:
> 
>   static scoped_refptr<SignedCertificateTimestamp> CreateFromPickle(
>       PickleIterator* iter);
> 
> Please pick the form you prefer.

Done.

https://codereview.chromium.org/88643002/diff/130001/net/http/http_response_i...
File net/http/http_response_info.cc (right):

https://codereview.chromium.org/88643002/diff/130001/net/http/http_response_i...
net/http/http_response_info.cc:339:
pickle->WriteInt(!ssl_info.signed_certificate_timestamps.empty());
On 2013/11/28 01:28:02, wtc wrote:
> 
> 1. BUG: the argument to pickle->WriteInt() should be
>   ssl_info.signed_certificate_timestamps.size()
> 
> 2. Nit: but the conditional expression on the previous line should be
>   !ssl_info.signed_certificate_timestamps.empty()

Blimey ! O_O
Good catch, thank you.

Done.

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

https://codereview.chromium.org/88643002/diff/130001/net/ssl/ssl_info.cc#newc...
net/ssl/ssl_info.cc:27: signed_certificate_timestamps =
info.signed_certificate_timestamps;
On 2013/11/28 01:28:02, wtc wrote:
> 
> Nit: move this to the end of the function, after public_key_hashes.

Done.

https://codereview.chromium.org/88643002/diff/130001/net/ssl/ssl_info.h
File net/ssl/ssl_info.h (right):

https://codereview.chromium.org/88643002/diff/130001/net/ssl/ssl_info.h#newco...
net/ssl/ssl_info.h:85: SignedCertificateTimestampAndStatusList
signed_certificate_timestamps;
On 2013/11/28 01:28:02, wtc wrote:
> 
> Another way to store the SCTs in an SSLInfo is to have three vectors of
> SignedCertificateTimestamp, one for each validation status, just like
> CTVerifyResult. Then you don't need the SignedCertificateTimestampAndStatus
> struct.
> 
> I'll let you and Eran decide which form is better.

Spoke with Eran about this, we figured that it's better this way since it's
possible we'll need to make the status richer in the future (e.g. with gossip
info, etc.)

Thank you for the suggestion, though.

wtc

Patch set 7 LGTM. I reviewed the entire CL this time, hence the additional comments. ...

7 years ago (2013-11-28 16:15:07 UTC) #11

alcutter

https://codereview.chromium.org/88643002/diff/130001/net/cert/ct_verify_result.h File net/cert/ct_verify_result.h (right): https://codereview.chromium.org/88643002/diff/130001/net/cert/ct_verify_result.h#newcode28 net/cert/ct_verify_result.h:28: SCTList invalid_scts; On 2013/11/28 16:15:07, wtc wrote: > > ...

7 years ago (2013-11-28 16:42:17 UTC) #12

alcutter

https://codereview.chromium.org/88643002/diff/80001/content/public/common/ssl_status.h File content/public/common/ssl_status.h (right): https://codereview.chromium.org/88643002/diff/80001/content/public/common/ssl_status.h#newcode50 content/public/common/ssl_status.h:50: ~SSLStatus(); On 2013/11/27 12:17:56, alcutter wrote: > On 2013/11/27 ...

7 years ago (2013-11-28 18:44:10 UTC) #13

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alcutter@google.com/88643002/370001

7 years ago (2013-11-28 21:08:45 UTC) #14

Message was sent while issue was closed.

Change committed as 237849

Expand Messages | Collapse Messages

Issue 88643002: SignedCertificateTimestamp storing & serialization code. (Closed)

Description

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : Fixes for eran #

Patch Set 4 : Fixes for jam. #

Patch Set 5 : Fixes for wtc. #

Patch Set 6 : remove a spurious content:: #

Patch Set 7 : Fixes for wtc. #

Patch Set 8 : Fixes for wtc #

Patch Set 9 : fixes #

Messages