Rename net::ct::LogEntry to SignedEntryData and clarify the comment.

Rename net::ct::LogEntry to SignedEntryData and clarify the comment.

BUG=none

Review-Url: https://codereview.chromium.org/2824983002
Cr-Commit-Position: refs/heads/master@{#466527}
Committed: https://chromium.googlesource.com/chromium/src/+/e97f8a266e080209cfdbb23cb75d6f29d466eac1

[mattm, 2017-04-18 02:07:37]

Patchset #2 (id:20001) has been deleted

[mattm, 2017-04-18 02:08:44]

Description was changed from

==========
Clarify comment for net::ct::LogEntry.

BUG=none
==========

to

==========
Clarify comment for net::ct::LogEntry regarding its relationship to the struct
from the RFC of the same name.

BUG=none
==========

[mattm, 2017-04-18 02:09:08]

Description was changed from

==========
Clarify comment for net::ct::LogEntry regarding its relationship to the struct
from the RFC of the same name.

BUG=none
==========

to

==========
Clarify comment for net::ct::LogEntry regarding its relationship to the LogEntry
struct from the RFC.

BUG=none
==========

[mattm, 2017-04-18 02:27:15]

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

[mattm, 2017-04-18 02:27:16]

While trying to get better familiarized with CT stuff to do a codereview, I
found the it somewhat confusing that it has a LogEntry type but doesn't use
Precertificates. This tries to at least make it more explicit. wdyt?

[Ryan Sleevi, 2017-04-18 14:53:28]

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

[Ryan Sleevi, 2017-04-18 14:53:29]

https://codereview.chromium.org/2824983002/diff/40001/net/cert/signed_certifi...
File net/cert/signed_certificate_timestamp.h (right):

https://codereview.chromium.org/2824983002/diff/40001/net/cert/signed_certifi...
net/cert/signed_certificate_timestamp.h:32: //     PreCert = issuer_key_hash +
TBSCertificate without poison extension)
Right, this is actually the signed_entry of an SCT from
https://tools.ietf.org/html/rfc6962#section-3.2

Maybe this should be renamed SignedEntryData?

Eran, what do you think?

It'd be much larger to rename, so I'm not necessarily requesting that in this
CL, but perhaps that context is worth updating the comment to reflect that.

// Contains the data necessary to reconstruct the signed_entry
// of a SignedCertificateTimestamp, from RFC 6962, Section 3.2.
//
// All the data necessary to validate a SignedCertificateTimestamp
// is present within the SignedCertificateTimestamp, except for
// the signature_type, entry_type, and the actual entry. The only
// supported signature_type at present is certificate_timestamp.
// The entry_type is implicit from the context in which it is
// received (those in the X.509 extension are precert_entry, all
// others are x509_entry). The signed_entry itself is
// reconstructed from the certificate (or precertificate) being
// verified.
// The SignedEntryData contains this reconstructed data, and can
// be used to either generate or verify SCTs.


(Is the very wordy answer)

WDYT?

[notusing2468, 2017-04-19 04:03:19]

notusing2468@gmail.com changed reviewers:
+ notusing2468@gmail.com

[Eran Messeri, 2017-04-19 10:42:07]

https://codereview.chromium.org/2824983002/diff/40001/net/cert/signed_certifi...
File net/cert/signed_certificate_timestamp.h (right):

https://codereview.chromium.org/2824983002/diff/40001/net/cert/signed_certifi...
net/cert/signed_certificate_timestamp.h:32: //     PreCert = issuer_key_hash +
TBSCertificate without poison extension)
On 2017/04/18 14:53:29, Ryan Sleevi wrote:
> Right, this is actually the signed_entry of an SCT from
> https://tools.ietf.org/html/rfc6962#section-3.2
> 
> Maybe this should be renamed SignedEntryData?
> 
> Eran, what do you think?
> 
> It'd be much larger to rename, so I'm not necessarily requesting that in this
> CL, but perhaps that context is worth updating the comment to reflect that.
> 
> // Contains the data necessary to reconstruct the signed_entry
> // of a SignedCertificateTimestamp, from RFC 6962, Section 3.2.
> //
> // All the data necessary to validate a SignedCertificateTimestamp
> // is present within the SignedCertificateTimestamp, except for
> // the signature_type, entry_type, and the actual entry. The only
> // supported signature_type at present is certificate_timestamp.
> // The entry_type is implicit from the context in which it is
> // received (those in the X.509 extension are precert_entry, all
> // others are x509_entry). The signed_entry itself is
> // reconstructed from the certificate (or precertificate) being
> // verified.
Nit: The precertificate isn't being verified - it could be a certificate
corresponding to a precertificate that's being verified. That bit in the comment
seems to me a bit confusing.
> // The SignedEntryData contains this reconstructed data, and can
> // be used to either generate or verify SCTs.
Nit: Verify the signature in the SCT? 
> 
> 
> (Is the very wordy answer)
> 
> WDYT?

I agree - SignedEntryData sounds like a much better name. The assessment that
this is not really a 'LogEntry' struct as defined in 6962 Section 3.1. is
accurate, this structure is indeed only useful for reconstructing the data which
is signed by the signature in the SignedCertificateTimestamp.

[mattm, 2017-04-21 21:10:07]

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

[commit-bot: I haz the power, 2017-04-21 21:10:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-04-21 21:12:15]

mattm@chromium.org changed reviewers:
- notusing2468@gmail.com

[mattm, 2017-04-21 21:12:16]

Thanks for the suggestions on better text for the comment.
For some reason I decided to do the rename, ptal.

https://codereview.chromium.org/2824983002/diff/40001/net/cert/signed_certifi...
File net/cert/signed_certificate_timestamp.h (right):

https://codereview.chromium.org/2824983002/diff/40001/net/cert/signed_certifi...
net/cert/signed_certificate_timestamp.h:32: //     PreCert = issuer_key_hash +
TBSCertificate without poison extension)
On 2017/04/19 10:42:07, Eran Messeri wrote:
> On 2017/04/18 14:53:29, Ryan Sleevi wrote:
> > Right, this is actually the signed_entry of an SCT from
> > https://tools.ietf.org/html/rfc6962#section-3.2
> > 
> > Maybe this should be renamed SignedEntryData?
> > 
> > Eran, what do you think?
> > 
> > It'd be much larger to rename, so I'm not necessarily requesting that in
this
> > CL, but perhaps that context is worth updating the comment to reflect that.
> > 
> > // Contains the data necessary to reconstruct the signed_entry
> > // of a SignedCertificateTimestamp, from RFC 6962, Section 3.2.
> > //
> > // All the data necessary to validate a SignedCertificateTimestamp
> > // is present within the SignedCertificateTimestamp, except for
> > // the signature_type, entry_type, and the actual entry. The only
> > // supported signature_type at present is certificate_timestamp.
> > // The entry_type is implicit from the context in which it is
> > // received (those in the X.509 extension are precert_entry, all
> > // others are x509_entry). The signed_entry itself is
> > // reconstructed from the certificate (or precertificate) being
> > // verified.
> Nit: The precertificate isn't being verified - it could be a certificate
> corresponding to a precertificate that's being verified. That bit in the
comment
> seems to me a bit confusing.
> > // The SignedEntryData contains this reconstructed data, and can
> > // be used to either generate or verify SCTs.
> Nit: Verify the signature in the SCT? 
> > 
> > 
> > (Is the very wordy answer)
> > 
> > WDYT?
> 
> I agree - SignedEntryData sounds like a much better name. The assessment that
> this is not really a 'LogEntry' struct as defined in 6962 Section 3.1. is
> accurate, this structure is indeed only useful for reconstructing the data
which
> is signed by the signature in the SignedCertificateTimestamp.
> 

Done.

https://codereview.chromium.org/2824983002/diff/80001/components/certificate_...
File components/certificate_transparency/single_tree_tracker.cc (left):

https://codereview.chromium.org/2824983002/diff/80001/components/certificate_...
components/certificate_transparency/single_tree_tracker.cc:38: // it.
Feel free to suggest better text to put here too.

[mattm, 2017-04-21 21:13:02]

Description was changed from

==========
Clarify comment for net::ct::LogEntry regarding its relationship to the LogEntry
struct from the RFC.

BUG=none
==========

to

==========
Rename net::ct::LogEntry to SignedEntryData and clarify the comment.

BUG=none
==========

[Ryan Sleevi, 2017-04-21 21:20:13]

LGTM % nits :)

Thanks for doing this :)

https://codereview.chromium.org/2824983002/diff/100001/net/cert/ct_serializat...
File net/cert/ct_serialization.h (right):

https://codereview.chromium.org/2824983002/diff/100001/net/cert/ct_serializat...
net/cert/ct_serialization.h:23: struct SignedEntryData;
Sort ;)

https://codereview.chromium.org/2824983002/diff/100001/net/test/ct_test_util.h
File net/test/ct_test_util.h (right):

https://codereview.chromium.org/2824983002/diff/100001/net/test/ct_test_util....
net/test/ct_test_util.h:23: struct SignedEntryData;
sort

[mattm, 2017-04-21 21:26:04]

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

[commit-bot: I haz the power, 2017-04-21 21:26:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 22:38:21]

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

[commit-bot: I haz the power, 2017-04-21 22:38:22]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-04-22 02:42:16]

https://codereview.chromium.org/2824983002/diff/100001/net/cert/ct_serializat...
File net/cert/ct_serialization.h (right):

https://codereview.chromium.org/2824983002/diff/100001/net/cert/ct_serializat...
net/cert/ct_serialization.h:23: struct SignedEntryData;
On 2017/04/21 21:20:13, Ryan Sleevi wrote:
> Sort ;)

Done.

https://codereview.chromium.org/2824983002/diff/100001/net/test/ct_test_util.h
File net/test/ct_test_util.h (right):

https://codereview.chromium.org/2824983002/diff/100001/net/test/ct_test_util....
net/test/ct_test_util.h:23: struct SignedEntryData;
On 2017/04/21 21:20:13, Ryan Sleevi wrote:
> sort

Done.

[mattm, 2017-04-22 02:42:20]

The CQ bit was checked by mattm@chromium.org

[mattm, 2017-04-22 02:42:21]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/2824983002/#ps120001
(title: "sort forward decls")

[commit-bot: I haz the power, 2017-04-22 02:42:39]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-22 02:46:37]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492828940777280,
"parent_rev": "e814d1f84152018b469fbe6e39412111afa31aa0", "commit_rev":
"e97f8a266e080209cfdbb23cb75d6f29d466eac1"}

[commit-bot: I haz the power, 2017-04-22 02:46:46]

Description was changed from

==========
Rename net::ct::LogEntry to SignedEntryData and clarify the comment.

BUG=none
==========

to

==========
Rename net::ct::LogEntry to SignedEntryData and clarify the comment.

BUG=none

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

[commit-bot: I haz the power, 2017-04-22 02:46:47]

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