Adds a function for encoding a Merkle tree leaf in TLS wire format.

Adds a function for encoding a Merkle tree leaf in TLS wire format.

This will be useful for producing a leaf hash, as required for requesting audit
proofs from Certificate Transparency logs. A leaf hash is a SHA-256 hash of a
Merkle tree leaf encoded in TLS wire format.

BUG=608868
Committed: https://crrev.com/1b1732191f54e031e740a6907dc5608c8d32ecfd
Cr-Commit-Position: refs/heads/master@{#392636}

[Rob Percival, 2016-05-03 23:14:32]

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

[Rob Percival, 2016-05-03 23:38:27]

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

[commit-bot: I haz the power, 2016-05-03 23:39:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1943313003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1943313003/20001

[commit-bot: I haz the power, 2016-05-04 00:59:02]

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

[commit-bot: I haz the power, 2016-05-04 00:59:03]

Dry run: This issue passed the CQ dry run.

[Eran Messeri, 2016-05-04 10:29:23]

Overall looks good, a few minor comments.

https://codereview.chromium.org/1943313003/diff/20001/net/cert/ct_serializati...
File net/cert/ct_serialization.cc (right):

https://codereview.chromium.org/1943313003/diff/20001/net/cert/ct_serializati...
net/cert/ct_serialization.cc:24: const size_t kSha256HashLength = 32;
This is redundant, you can use the definition from
https://code.google.com/p/chromium/codesearch#chromium/src/crypto/sha2.h&q=cr...

https://codereview.chromium.org/1943313003/diff/20001/net/cert/ct_serializati...
File net/cert/ct_serialization.h (right):

https://codereview.chromium.org/1943313003/diff/20001/net/cert/ct_serializati...
net/cert/ct_serialization.h:42: NET_EXPORT_PRIVATE bool EncodeTreeLeaf(const
MerkleTreeLeaf& leaf,
NET_EXPORT as I expect it'll be used from components/certificate_transparency.

https://codereview.chromium.org/1943313003/diff/20001/net/cert/merkle_tree_le...
File net/cert/merkle_tree_leaf.h (right):

https://codereview.chromium.org/1943313003/diff/20001/net/cert/merkle_tree_le...
net/cert/merkle_tree_leaf.h:25: // Version enum in RFC 6962, Section 3.2.
Nit: I'd inline those in ct_serialization.cc and avoid changing
merkle_tree_leaf.h entirely.
You'd point out that we did this differently for SignedCertificateTimestamp -
and you're right - but now I'm fairly confident there won't be another version /
type of MerkleTreeLeaf for RFC6962.
Since we only care about these values during serialization, then it's simpler to
have them in the serialization code.

[Rob Percival, 2016-05-05 15:45:31]

https://codereview.chromium.org/1943313003/diff/20001/net/cert/ct_serializati...
File net/cert/ct_serialization.cc (right):

https://codereview.chromium.org/1943313003/diff/20001/net/cert/ct_serializati...
net/cert/ct_serialization.cc:24: const size_t kSha256HashLength = 32;
On 2016/05/04 10:29:23, Eran Messeri wrote:
> This is redundant, you can use the definition from
>
https://code.google.com/p/chromium/codesearch#chromium/src/crypto/sha2.h&q=cr...

Done.

https://codereview.chromium.org/1943313003/diff/20001/net/cert/ct_serializati...
File net/cert/ct_serialization.h (right):

https://codereview.chromium.org/1943313003/diff/20001/net/cert/ct_serializati...
net/cert/ct_serialization.h:42: NET_EXPORT_PRIVATE bool EncodeTreeLeaf(const
MerkleTreeLeaf& leaf,
On 2016/05/04 10:29:23, Eran Messeri wrote:
> NET_EXPORT as I expect it'll be used from components/certificate_transparency.

Done.

https://codereview.chromium.org/1943313003/diff/20001/net/cert/merkle_tree_le...
File net/cert/merkle_tree_leaf.h (right):

https://codereview.chromium.org/1943313003/diff/20001/net/cert/merkle_tree_le...
net/cert/merkle_tree_leaf.h:25: // Version enum in RFC 6962, Section 3.2.
On 2016/05/04 10:29:23, Eran Messeri wrote:
> Nit: I'd inline those in ct_serialization.cc and avoid changing
> merkle_tree_leaf.h entirely.
> You'd point out that we did this differently for SignedCertificateTimestamp -
> and you're right - but now I'm fairly confident there won't be another version
/
> type of MerkleTreeLeaf for RFC6962.
> Since we only care about these values during serialization, then it's simpler
to
> have them in the serialization code.

Done.

[Rob Percival, 2016-05-05 15:45:52]

robpercival@chromium.org changed reviewers:
+ eroman@chromium.org

[Eran Messeri, 2016-05-05 15:59:34]

LGTM for CT compliance, other than two minor comments.

https://codereview.chromium.org/1943313003/diff/60001/net/cert/ct_serializati...
File net/cert/ct_serialization.h (right):

https://codereview.chromium.org/1943313003/diff/60001/net/cert/ct_serializati...
net/cert/ct_serialization.h:13: #include "net/cert/merkle_tree_leaf.h"
Nit: Can you get away with forward-declaring MerkleTreeLeaf? I would expect so,
given you only refer to it by const &.
You could add a TODO here to forward-declare everything possible.

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

https://codereview.chromium.org/1943313003/diff/60001/net/cert/signed_certifi...
net/cert/signed_certificate_timestamp.h:96: // TODO(robpercival): Fix
inconsistent naming between this and
Not convinced about the value of this TODO - if it's a simple fix as suggested,
simply do it in a follow-up CL, if not, please expand the description of this
TODO to indicate what exactly you'd want to fix.

https://codereview.chromium.org/1943313003/diff/60001/net/test/ct_test_util.cc
File net/test/ct_test_util.cc (right):

https://codereview.chromium.org/1943313003/diff/60001/net/test/ct_test_util.c...
net/test/ct_test_util.cc:103: const int64_t kTestTimestamp =
INT64_C(1396877277237);
Given the value here is exactly the same as of kSampleSTHTimestamp defined in
line 173, why not use that instead?

[Rob Percival, 2016-05-05 17:31:44]

https://codereview.chromium.org/1943313003/diff/60001/net/cert/ct_serializati...
File net/cert/ct_serialization.h (right):

https://codereview.chromium.org/1943313003/diff/60001/net/cert/ct_serializati...
net/cert/ct_serialization.h:13: #include "net/cert/merkle_tree_leaf.h"
On 2016/05/05 15:59:33, Eran Messeri wrote:
> Nit: Can you get away with forward-declaring MerkleTreeLeaf? I would expect
so,
> given you only refer to it by const &.
> You could add a TODO here to forward-declare everything possible.

I can; I only didn't because nothing else was. I've created a new change that
forward-declares everything rather than adding a TODO:
https://codereview.chromium.org/1953773002.

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

https://codereview.chromium.org/1943313003/diff/60001/net/cert/signed_certifi...
net/cert/signed_certificate_timestamp.h:96: // TODO(robpercival): Fix
inconsistent naming between this and
On 2016/05/05 15:59:33, Eran Messeri wrote:
> Not convinced about the value of this TODO - if it's a simple fix as
suggested,
> simply do it in a follow-up CL, if not, please expand the description of this
> TODO to indicate what exactly you'd want to fix. 

Done in https://codereview.chromium.org/1951233003.

https://codereview.chromium.org/1943313003/diff/60001/net/test/ct_test_util.cc
File net/test/ct_test_util.cc (right):

https://codereview.chromium.org/1943313003/diff/60001/net/test/ct_test_util.c...
net/test/ct_test_util.cc:103: const int64_t kTestTimestamp =
INT64_C(1396877277237);
On 2016/05/05 15:59:33, Eran Messeri wrote:
> Given the value here is exactly the same as of kSampleSTHTimestamp defined in
> line 173, why not use that instead?

Done.

[eroman, 2016-05-07 00:20:53]

lgtm

https://codereview.chromium.org/1943313003/diff/80001/net/cert/ct_serializati...
File net/cert/ct_serialization.cc (right):

https://codereview.chromium.org/1943313003/diff/80001/net/cert/ct_serializati...
net/cert/ct_serialization.cc:351: return false;
run "git cl format" -- need to remove some spaces of indentation here.

https://codereview.chromium.org/1943313003/diff/80001/net/cert/ct_serializati...
net/cert/ct_serialization.cc:353: return false;
Same

https://codereview.chromium.org/1943313003/diff/80001/net/cert/ct_serializati...
File net/cert/ct_serialization.h (right):

https://codereview.chromium.org/1943313003/diff/80001/net/cert/ct_serializati...
net/cert/ct_serialization.h:42: // Note that the log ID is not serialized.
Not quite sure what this line means.

Does this mean the log ID is not included in |output|?

https://codereview.chromium.org/1943313003/diff/80001/net/cert/ct_serializati...
net/cert/ct_serialization.h:43: NET_EXPORT bool EncodeTreeLeaf(const
MerkleTreeLeaf& leaf, std::string* output);
Please clarify in the comments that this *appends* to |output|.

Otherwise clear |output| at the start of the implementation.

[Rob Percival, 2016-05-08 04:08:18]

https://codereview.chromium.org/1943313003/diff/80001/net/cert/ct_serializati...
File net/cert/ct_serialization.cc (right):

https://codereview.chromium.org/1943313003/diff/80001/net/cert/ct_serializati...
net/cert/ct_serialization.cc:351: return false;
On 2016/05/07 00:20:52, eroman wrote:
> run "git cl format" -- need to remove some spaces of indentation here.

Oddly, "git cl format" doesn't make any changes. Fixed manually.

https://codereview.chromium.org/1943313003/diff/80001/net/cert/ct_serializati...
net/cert/ct_serialization.cc:353: return false;
On 2016/05/07 00:20:52, eroman wrote:
> Same

Done.

https://codereview.chromium.org/1943313003/diff/80001/net/cert/ct_serializati...
File net/cert/ct_serialization.h (right):

https://codereview.chromium.org/1943313003/diff/80001/net/cert/ct_serializati...
net/cert/ct_serialization.h:42: // Note that the log ID is not serialized.
On 2016/05/07 00:20:52, eroman wrote:
> Not quite sure what this line means.
> 
> Does this mean the log ID is not included in |output|?

Yes - I've clarified this.

https://codereview.chromium.org/1943313003/diff/80001/net/cert/ct_serializati...
net/cert/ct_serialization.h:43: NET_EXPORT bool EncodeTreeLeaf(const
MerkleTreeLeaf& leaf, std::string* output);
On 2016/05/07 00:20:52, eroman wrote:
> Please clarify in the comments that this *appends* to |output|.
> 
> Otherwise clear |output| at the start of the implementation.

Clarified. The same ought to be done for the other function documentation in
this file, albeit not as part of this change.

[Rob Percival, 2016-05-10 15:00:27]

The CQ bit was checked by robpercival@chromium.org

[Rob Percival, 2016-05-10 15:00:28]

The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org,
eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1943313003/#ps100001
(title: "Addresses eroman review comments")

[commit-bot: I haz the power, 2016-05-10 15:00:42]

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

[commit-bot: I haz the power, 2016-05-10 15:00:43]

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

[commit-bot: I haz the power, 2016-05-10 15:00:44]

CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true
and NOPRESUBMIT=true in order for the CQ to process them

[Rob Percival, 2016-05-10 16:12:29]

The CQ bit was checked by robpercival@chromium.org

[Rob Percival, 2016-05-10 16:12:30]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
eranm@chromium.org
Link to the patchset: https://codereview.chromium.org/1943313003/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-05-10 16:12:48]

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

[commit-bot: I haz the power, 2016-05-10 17:24:49]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-10 17:27:38]

Description was changed from

==========
Adds a function for encoding a Merkle tree leaf in TLS wire format.

This will be useful for producing a leaf hash, as required for requesting audit
proofs from Certificate Transparency logs. A leaf hash is a SHA-256 hash of a
Merkle tree leaf encoded in TLS wire format.

BUG=608868
==========

to

==========
Adds a function for encoding a Merkle tree leaf in TLS wire format.

This will be useful for producing a leaf hash, as required for requesting audit
proofs from Certificate Transparency logs. A leaf hash is a SHA-256 hash of a
Merkle tree leaf encoded in TLS wire format.

BUG=608868

Committed: https://crrev.com/1b1732191f54e031e740a6907dc5608c8d32ecfd
Cr-Commit-Position: refs/heads/master@{#392636}
==========

[commit-bot: I haz the power, 2016-05-10 17:27:39]

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