Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
37633002:
CT: First step towards supporting Certificate Transparency in Chrome. (Closed)
|
Created:
7 years, 2 months ago by Eran M. (Google) Modified:
7 years, 1 month ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionCT: First step towards supporting Certificate Transparency in Chrome.
This patch adds Signed Certificate Timestamp (SCT) encoding/decoding.
SCT is the Certificate Transparency (CT) structure containing a proof
of a public log's commitment to adding a certificate to its public
repository.
The next patches would be extracting the SCTs when embedded in
certificates and verifying the signature from the SCT over them.
BUG=309578
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232131
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232267
Patch Set 1 #
Total comments: 82
Patch Set 2 : Addressing review comments #Patch Set 3 : Exporting DigitallySigned #
Total comments: 8
Patch Set 4 : Addressing Ryan's review comments, fixing signed/unsigned comparison. #Patch Set 5 : Fixing ASAN-found error with the tests #Patch Set 6 : Implementing Ryan's suggestion of substr-ing the StringPiece #
Messages
Total messages: 15 (0 generated)
akalin: git-cl suggested you as a reviewer. Ryan could provide more context on this patch if the information & design doc in the attached bug are not enough.
sleevi suggested wtc@ review this instead of me. After taking a quick look, I agree. :)
Considering a good bit was based on my original code, -akalin and +wtc, who is better suited for crypto reviews.
Patch set 1 LGTM. I suggest some changes. After making the changes, if you are happy with the CL, you may commit it without waiting for a new review by me. I will do a post-commit review. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc File net/cert/ct_serialization.cc (right): https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:16: // SCT Version length Nit: define "SCT". https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:17: const size_t kVersionLengthInBytes = 1; Since all of the lengths in this file are in bytes, "InBytes" can be omitted from the names. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:37: SIGNATURE_TYPE_CERTIFICATE_TIMESTAMP = 0, Nit: I suggest also include "tree_hash(1)" for completeness. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:43: // This is essentially log2(max_field_length) It should be (log2(max_field_length) + 7)/8 because log2(max_field_length) is a length in bits. Also, it may be a good idea to just use constants instead of computing the prefix lengths every time. All of the kMax***LengthInBytes constants are 2^n - 1, so their lengths are naturally bounded. More on this in ReadLength(). Note: you can choose to not make this change. I just wanted you to consider this alternative. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:56: // true and stores the result in |*out|. Document how |in| is updated. You may want to point out TLS encodes integers in big-endian order. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:58: bool ReadUint(size_t length, base::StringPiece* in, T* out) { It would be nice to assert that length <= sizeof(T). Otherwise |*out| may receive a truncated value. You have such an assertion in WriteUint. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:64: } Nit: omit curly braces. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:72: // success, returns true and stores the result in |*out|. Document how |in| is updated. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:76: if (!ReadUint(prefix_length, in, &length) || length > max_length) Since we only pass constants of the form 2^n - 1 (where n is 8, 16, or 24) as |max_length| to ReadLength(), |length > max_length| will never be false. I suggest we just pass |prefix_length| directly to ReadLength(). https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:122: } Nit: omit curly braces. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:123: if (list_item.empty()) { Some types may be zero-length. But I agree it doesn't make sense for a list item to be empty. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:159: bool ConvertSigAlgorithm(int in, DigitallySigned::SignatureAlgorithm* out) { Nit: Sig => Signature https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:193: void WriteUint(size_t max_length, T value, std::string* output) { "max_length" is a poor parameter name because it has a different meaning in other functions. This parameter should be named "length". https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:200: >> ((max_length - 1) * 8))); You don't need the left shift of 0xFF. You can do this: (value >> ((max_length - 1) * 8))) & 0xFF https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:214: bool WriteVariableBytes(size_t max_field_length, Nit: this parameter's name should ideally match the equivalent parameter name in ReadVariableBytes. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:227: // Writes a LogEntry of type X509 cert to |output|. Nit: X509 => X.509 https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:252: // If |input| is less than kMaxSignatureLengthInBytes, encodes the |input| Nit: If |input| is => If |input.signature_data| is https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:272: !ReadUint(kSigAlgorithmLengthInBytes, input, &sig_algo) || The third argument of ReadUint (as the function name suggests) should point to an unsigned type, but hash_algo and sig_algo are signed int. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:289: // of |result.signature|. Nit: |result.signature| => |result.signature_data| https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:325: WriteFixedBytes(serialized_log_entry, output); Using the function name "WriteFixedBytes" is a little misleading here because WriteFixedBytes is documented as "Writes a fixed-length array" but serialized_log_entry is not fixed-length. We can use WriteFixedBytes because serialized_log_entry is already encoded. Maybe WriteFixedBytes can be renamed WriteBytes or WriteEncodedBytes, and its comments updated accordingly. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:356: if (!ReadUint(kVersionLengthInBytes, input, &version)) |version| is a signed int, but ReadUint implies we're reading an unsigned integer. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:378: base::TimeDelta::FromMilliseconds(static_cast<int64>(timestamp)); Should we validate |timestamp| before casting it to a signed type and adding it to base::Time::UnixEpoch()? https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.h File net/cert/ct_serialization.h (right): https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.h#n... net/cert/ct_serialization.h:24: NET_EXPORT_PRIVATE bool DecodeDigitallySigned(base::StringPiece* input, Since |input| is not const, I guess it is used as an in/out parameter. You should document how the function updates |input|. Similarly for DecodeSCTList and DecodeSignedCertificateTimestamp. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.h#n... net/cert/ct_serialization.h:37: std::vector<base::StringPiece>* output); This function should be documented. Ideally, every function should be documented. You can just move your comments from the .cc file to this header. At least, document that this is "SignedCertificateTimestampList" because the abbreviation "SCTList" is not used in RFC 6962. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization_uni... File net/cert/ct_serialization_unittest.cc (right): https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization_uni... net/cert/ct_serialization_unittest.cc:27: std::string test_digitally_signed_; Does this member need to be public? https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization_uni... net/cert/ct_serialization_unittest.cc:44: test_digitally_signed_.substr(4), Nit: explain 4 (1 byte hash algorithm, 1 byte signature algorithm, 2 bytes length prefix for the signature). https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization_uni... net/cert/ct_serialization_unittest.cc:77: EXPECT_EQ((size_t) (718 + 5), encoded.size()); We usually deal with this by adding the U suffix to constants: 718U + 5U. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization_uni... net/cert/ct_serialization_unittest.cc:79: // Length is 718 which is 512 + 206, which is 2<<8 + 0xce Nit: we can say "which is 0x2ce". Is there a particular reason to write it as 2<<8 + 0xce? To point out it will be serialized in big-endian order? https://codereview.chromium.org/37633002/diff/1/net/cert/signed_certificate_t... File net/cert/signed_certificate_timestamp.cc (right): https://codereview.chromium.org/37633002/diff/1/net/cert/signed_certificate_t... net/cert/signed_certificate_timestamp.cc:27: DigitallySigned::~DigitallySigned() {} IMPORTANT: None of these constructors initialize any struct members. Then you don't need to declare the constructors and destructors. https://codereview.chromium.org/37633002/diff/1/net/cert/signed_certificate_t... File net/cert/signed_certificate_timestamp.h (right): https://codereview.chromium.org/37633002/diff/1/net/cert/signed_certificate_t... net/cert/signed_certificate_timestamp.h:20: struct NET_EXPORT LogEntry { General suggestion: to help future maintainers of this code, I suggest you include the definitions of the original types in comments. For example, for struct LogEntry, add: enum { x509_entry(0), precert_entry(1), (65535) } LogEntryType; struct { LogEntryType entry_type; select (entry_type) { case x509_entry: X509ChainEntry; case precert_entry: PrecertChainEntry; } entry; } LogEntry; If you think this is too verbose, at least mention the name of the types (for exmple, "enum LogEntryType and struct LogEntry in RFC 6962, Sec. 3.1") in comments. https://codereview.chromium.org/37633002/diff/1/net/cert/signed_certificate_t... net/cert/signed_certificate_timestamp.h:31: Type type; You should use a union below. https://codereview.chromium.org/37633002/diff/1/net/cert/signed_certificate_t... net/cert/signed_certificate_timestamp.h:34: std::string leaf_cert; Nit: leaf_cert => leaf_certificate because you didn't abbreviate "tbs_certificate". https://codereview.chromium.org/37633002/diff/1/net/cert/signed_certificate_t... net/cert/signed_certificate_timestamp.h:66: std::string signature_data; Nit: this field is named "signature" in the RFC. If you deliberately renamed it "signature_data" for clarity, that's fine by me. https://codereview.chromium.org/37633002/diff/1/net/cert/signed_certificate_t... net/cert/signed_certificate_timestamp.h:74: // Source of the SCT - supplamentary, not defined in CT RFC. Typo: supplamentary => supplementary https://codereview.chromium.org/37633002/diff/1/net/cert/signed_certificate_t... net/cert/signed_certificate_timestamp.h:78: FROM_OCSP_RESPONSE = 2, Nit: the naming of the Origin enumerators differs in style from the other enumerators in this header, which all have a consistent prefix that suggests which enum type they belong to. Either style is fine by me. I am just pointing out this inconsistency. https://codereview.chromium.org/37633002/diff/1/net/cert/signed_certificate_t... net/cert/signed_certificate_timestamp.h:90: // As the same SCT can be provided from multiple sources. Nit: As => as https://codereview.chromium.org/37633002/diff/1/net/net.gyp File net/net.gyp (left): https://codereview.chromium.org/37633002/diff/1/net/net.gyp#oldcode291 net/net.gyp:291: 'cert/test_root_certs_android.cc', IMPORTANT: did you remove this file by mistake? https://codereview.chromium.org/37633002/diff/1/net/test/ct_test_util.cc File net/test/ct_test_util.cc (right): https://codereview.chromium.org/37633002/diff/1/net/test/ct_test_util.cc#newc... net/test/ct_test_util.cc:23: std::string B(const char* hex_data) { Nit: The function name "B" is too short and cryptic. https://codereview.chromium.org/37633002/diff/1/net/test/ct_test_util.cc#newc... net/test/ct_test_util.cc:31: std::string H(const base::StringPiece& data) { This H function is not used. https://codereview.chromium.org/37633002/diff/1/net/test/ct_test_util.h File net/test/ct_test_util.h (right): https://codereview.chromium.org/37633002/diff/1/net/test/ct_test_util.h#newco... net/test/ct_test_util.h:22: // Fills |entry| with test data for a PreCert entry. Nit: PreCert => Precertificate PreCert seems to refer to this struct: struct { opaque issuer_key_hash[32]; TBSCertificate tbs_certificate; } PreCert; https://codereview.chromium.org/37633002/diff/1/net/test/ct_test_util.h#newco... net/test/ct_test_util.h:29: std::string GetTestSignedCertificateTimestamp(); Nit: add a blank line after this line.
wtc, thanks for the quick review. I've addressed all comments. sleevi, could you please commit at your leisure, assuming you have no other comments? https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc File net/cert/ct_serialization.cc (right): https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:16: // SCT Version length On 2013/10/24 23:14:23, wtc wrote: > > Nit: define "SCT". Done. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:17: const size_t kVersionLengthInBytes = 1; On 2013/10/24 23:14:23, wtc wrote: > > Since all of the lengths in this file are in bytes, "InBytes" can be omitted > from the names. Done. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:37: SIGNATURE_TYPE_CERTIFICATE_TIMESTAMP = 0, On 2013/10/24 23:14:23, wtc wrote: > > Nit: I suggest also include "tree_hash(1)" for completeness. Done. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:43: // This is essentially log2(max_field_length) On 2013/10/24 23:14:23, wtc wrote: > > It should be (log2(max_field_length) + 7)/8 because log2(max_field_length) is a > length in bits. > > Also, it may be a good idea to just use constants instead of computing the > prefix lengths every time. All of the kMax***LengthInBytes constants are 2^n - > 1, so their lengths are naturally bounded. More on this in ReadLength(). Note: > you can choose to not make this change. I just wanted you to consider this > alternative. Good idea - switched the constants to the number of bytes needed to store the values, rather than the maximal value. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:56: // true and stores the result in |*out|. On 2013/10/24 23:14:23, wtc wrote: > > Document how |in| is updated. You may want to point out TLS encodes integers in > big-endian order. Done, also added the description of the modification to |in| to other relevant methods. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:58: bool ReadUint(size_t length, base::StringPiece* in, T* out) { On 2013/10/24 23:14:23, wtc wrote: > > It would be nice to assert that length <= sizeof(T). Otherwise |*out| may > receive a truncated value. > > You have such an assertion in WriteUint. Done. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:64: } On 2013/10/24 23:14:23, wtc wrote: > > Nit: omit curly braces. I prefer to keep them in this context, as the body of the for loop is non-trivial. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:72: // success, returns true and stores the result in |*out|. On 2013/10/24 23:14:23, wtc wrote: > > Document how |in| is updated. Done. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:76: if (!ReadUint(prefix_length, in, &length) || length > max_length) On 2013/10/24 23:14:23, wtc wrote: > > Since we only pass constants of the form 2^n - 1 (where n is 8, 16, or 24) as > |max_length| to ReadLength(), |length > max_length| will never be false. > > I suggest we just pass |prefix_length| directly to ReadLength(). Done. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:122: } On 2013/10/24 23:14:23, wtc wrote: > > Nit: omit curly braces. Added a DVLOG statement, so keeping them (to be consistent with the following if). https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:123: if (list_item.empty()) { On 2013/10/24 23:14:23, wtc wrote: > > Some types may be zero-length. But I agree it doesn't make sense for a list item > to be empty. This specifically is used to parse SCT lists, an SCT should definitely not have zero length. Overall I think we'd want to detect (and warn about) every SCT parsing failure. SCTs, when present, should always be verifiable. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:159: bool ConvertSigAlgorithm(int in, DigitallySigned::SignatureAlgorithm* out) { On 2013/10/24 23:14:23, wtc wrote: > > Nit: Sig => Signature Done. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:193: void WriteUint(size_t max_length, T value, std::string* output) { On 2013/10/24 23:14:23, wtc wrote: > > "max_length" is a poor parameter name because it has a different meaning in > other functions. This parameter should be named "length". Done. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:200: >> ((max_length - 1) * 8))); On 2013/10/24 23:14:23, wtc wrote: > > You don't need the left shift of 0xFF. You can do this: > > (value >> ((max_length - 1) * 8))) & 0xFF Done. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:214: bool WriteVariableBytes(size_t max_field_length, On 2013/10/24 23:14:23, wtc wrote: > > Nit: this parameter's name should ideally match the equivalent parameter name in > ReadVariableBytes. Done. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:227: // Writes a LogEntry of type X509 cert to |output|. On 2013/10/24 23:14:23, wtc wrote: > > Nit: X509 => X.509 Done. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:252: // If |input| is less than kMaxSignatureLengthInBytes, encodes the |input| On 2013/10/24 23:14:23, wtc wrote: > > Nit: If |input| is => If |input.signature_data| is Done. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:272: !ReadUint(kSigAlgorithmLengthInBytes, input, &sig_algo) || On 2013/10/24 23:14:23, wtc wrote: > > The third argument of ReadUint (as the function name suggests) should point to > an unsigned type, but hash_algo and sig_algo are signed int. Done - changed to unsigned. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:289: // of |result.signature|. On 2013/10/24 23:14:23, wtc wrote: > > Nit: |result.signature| => |result.signature_data| Done. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:325: WriteFixedBytes(serialized_log_entry, output); On 2013/10/24 23:14:23, wtc wrote: > > Using the function name "WriteFixedBytes" is a little misleading here because > WriteFixedBytes is documented as "Writes a fixed-length array" but > serialized_log_entry is not fixed-length. > > We can use WriteFixedBytes because serialized_log_entry is already encoded. > > Maybe WriteFixedBytes can be renamed WriteBytes or WriteEncodedBytes, and its > comments updated accordingly. Done - renamed to WriteEncodedBytes (and updated the documentation). https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:356: if (!ReadUint(kVersionLengthInBytes, input, &version)) On 2013/10/24 23:14:23, wtc wrote: > > |version| is a signed int, but ReadUint implies we're reading an unsigned > integer. Done. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.cc#... net/cert/ct_serialization.cc:378: base::TimeDelta::FromMilliseconds(static_cast<int64>(timestamp)); On 2013/10/24 23:14:23, wtc wrote: > > Should we validate |timestamp| before casting it to a signed type and adding it > to base::Time::UnixEpoch()? Done - verified that the value does not exceed max_int64. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.h File net/cert/ct_serialization.h (right): https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.h#n... net/cert/ct_serialization.h:24: NET_EXPORT_PRIVATE bool DecodeDigitallySigned(base::StringPiece* input, On 2013/10/24 23:14:23, wtc wrote: > > Since |input| is not const, I guess it is used as an in/out parameter. You > should document how the function updates |input|. > > Similarly for DecodeSCTList and DecodeSignedCertificateTimestamp. Done - moved the documentation from the .cc file. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization.h#n... net/cert/ct_serialization.h:37: std::vector<base::StringPiece>* output); On 2013/10/24 23:14:23, wtc wrote: > > This function should be documented. Ideally, every function should be > documented. You can just move your comments from the .cc file to this header. > > At least, document that this is "SignedCertificateTimestampList" because the > abbreviation "SCTList" is not used in RFC 6962. Done - moved the documentation. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization_uni... File net/cert/ct_serialization_unittest.cc (right): https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization_uni... net/cert/ct_serialization_unittest.cc:27: std::string test_digitally_signed_; On 2013/10/24 23:14:23, wtc wrote: > > Does this member need to be public? No - changed to protected. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization_uni... net/cert/ct_serialization_unittest.cc:44: test_digitally_signed_.substr(4), On 2013/10/24 23:14:23, wtc wrote: > > Nit: explain 4 (1 byte hash algorithm, 1 byte signature algorithm, 2 bytes > length prefix for the signature). Done. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization_uni... net/cert/ct_serialization_unittest.cc:77: EXPECT_EQ((size_t) (718 + 5), encoded.size()); On 2013/10/24 23:14:23, wtc wrote: > > We usually deal with this by adding the U suffix to constants: 718U + 5U. Done, thanks for the tip. https://codereview.chromium.org/37633002/diff/1/net/cert/ct_serialization_uni... net/cert/ct_serialization_unittest.cc:79: // Length is 718 which is 512 + 206, which is 2<<8 + 0xce On 2013/10/24 23:14:23, wtc wrote: > > Nit: we can say "which is 0x2ce". Is there a particular reason to write it as > 2<<8 + 0xce? To point out it will be serialized in big-endian order? Done - originally described it so it's clear why the 0x02 appears in the upper byte, but both are ok. https://codereview.chromium.org/37633002/diff/1/net/cert/signed_certificate_t... File net/cert/signed_certificate_timestamp.cc (right): https://codereview.chromium.org/37633002/diff/1/net/cert/signed_certificate_t... net/cert/signed_certificate_timestamp.cc:27: DigitallySigned::~DigitallySigned() {} On 2013/10/24 23:14:23, wtc wrote: > > IMPORTANT: None of these constructors initialize any struct members. Then you > don't need to declare the constructors and destructors. Tried that, I ged the following error: ../../net/cert/signed_certificate_timestamp.h:69:1: error: [chromium-style] Complex class/struct needs an explicit out-of-line constructor. This is true for all structs defined in this file. https://codereview.chromium.org/37633002/diff/1/net/cert/signed_certificate_t... File net/cert/signed_certificate_timestamp.h (right): https://codereview.chromium.org/37633002/diff/1/net/cert/signed_certificate_t... net/cert/signed_certificate_timestamp.h:20: struct NET_EXPORT LogEntry { On 2013/10/24 23:14:23, wtc wrote: > > General suggestion: to help future maintainers of this code, I suggest you > include the definitions of the original types in comments. For example, for > struct LogEntry, add: > > enum { x509_entry(0), precert_entry(1), (65535) } LogEntryType; > > struct { > LogEntryType entry_type; > select (entry_type) { > case x509_entry: X509ChainEntry; > case precert_entry: PrecertChainEntry; > } entry; > } LogEntry; > > If you think this is too verbose, at least mention the name of the types (for > exmple, "enum LogEntryType and struct LogEntry in RFC 6962, Sec. 3.1") in > comments. Resolved by mentioning the name of RFC & section for each of the relevant structs/enums. https://codereview.chromium.org/37633002/diff/1/net/cert/signed_certificate_t... net/cert/signed_certificate_timestamp.h:31: Type type; On 2013/10/24 23:14:23, wtc wrote: > > You should use a union below. the objects in the suggested union are not plain data types and so are forbidden (unrestricted unions are available with c++0x only). I could use pointers or boost::variant, if you think it's important enough. https://codereview.chromium.org/37633002/diff/1/net/cert/signed_certificate_t... net/cert/signed_certificate_timestamp.h:34: std::string leaf_cert; On 2013/10/24 23:14:23, wtc wrote: > > > Nit: leaf_cert => leaf_certificate > > because you didn't abbreviate "tbs_certificate". Done. https://codereview.chromium.org/37633002/diff/1/net/cert/signed_certificate_t... net/cert/signed_certificate_timestamp.h:66: std::string signature_data; On 2013/10/24 23:14:23, wtc wrote: > > Nit: this field is named "signature" in the RFC. If you deliberately renamed it > "signature_data" for clarity, that's fine by me. I have (found it confusing when actually accessing the field). https://codereview.chromium.org/37633002/diff/1/net/cert/signed_certificate_t... net/cert/signed_certificate_timestamp.h:74: // Source of the SCT - supplamentary, not defined in CT RFC. On 2013/10/24 23:14:23, wtc wrote: > > Typo: supplamentary => supplementary Done. https://codereview.chromium.org/37633002/diff/1/net/cert/signed_certificate_t... net/cert/signed_certificate_timestamp.h:78: FROM_OCSP_RESPONSE = 2, On 2013/10/24 23:14:23, wtc wrote: > > Nit: the naming of the Origin enumerators differs in style from the other > enumerators in this header, which all have a consistent prefix that suggests > which enum type they belong to. Either style is fine by me. I am just pointing > out this inconsistency. Done - changed to conform other enums. https://codereview.chromium.org/37633002/diff/1/net/cert/signed_certificate_t... net/cert/signed_certificate_timestamp.h:90: // As the same SCT can be provided from multiple sources. On 2013/10/24 23:14:23, wtc wrote: > > Nit: As => as Done. https://codereview.chromium.org/37633002/diff/1/net/net.gyp File net/net.gyp (left): https://codereview.chromium.org/37633002/diff/1/net/net.gyp#oldcode291 net/net.gyp:291: 'cert/test_root_certs_android.cc', On 2013/10/24 23:14:23, wtc wrote: > > IMPORTANT: did you remove this file by mistake? Nope - simply changed to alphabetical order. https://codereview.chromium.org/37633002/diff/1/net/test/ct_test_util.cc File net/test/ct_test_util.cc (right): https://codereview.chromium.org/37633002/diff/1/net/test/ct_test_util.cc#newc... net/test/ct_test_util.cc:23: std::string B(const char* hex_data) { On 2013/10/24 23:14:23, wtc wrote: > > Nit: The function name "B" is too short and cryptic. Done - renamed. https://codereview.chromium.org/37633002/diff/1/net/test/ct_test_util.cc#newc... net/test/ct_test_util.cc:31: std::string H(const base::StringPiece& data) { On 2013/10/24 23:14:23, wtc wrote: > > This H function is not used. Done - removed. https://codereview.chromium.org/37633002/diff/1/net/test/ct_test_util.h File net/test/ct_test_util.h (right): https://codereview.chromium.org/37633002/diff/1/net/test/ct_test_util.h#newco... net/test/ct_test_util.h:22: // Fills |entry| with test data for a PreCert entry. On 2013/10/24 23:14:23, wtc wrote: > > Nit: PreCert => Precertificate > > PreCert seems to refer to this struct: > > struct { > opaque issuer_key_hash[32]; > TBSCertificate tbs_certificate; > } PreCert; Done. https://codereview.chromium.org/37633002/diff/1/net/test/ct_test_util.h#newco... net/test/ct_test_util.h:29: std::string GetTestSignedCertificateTimestamp(); On 2013/10/24 23:14:23, wtc wrote: > > Nit: add a blank line after this line. Done.
lgtm https://codereview.chromium.org/37633002/diff/270001/net/cert/ct_serializatio... File net/cert/ct_serialization.cc (right): https://codereview.chromium.org/37633002/diff/270001/net/cert/ct_serializatio... net/cert/ct_serialization.cc:64: // |max_length| indicates the maximum length supported (eg: 2^24 - 1). On update comment https://codereview.chromium.org/37633002/diff/270001/net/cert/ct_serializatio... net/cert/ct_serialization.cc:285: // XXX(rsleevi): Declare a swap specialization to avoid the extra copy Just remove this comment https://codereview.chromium.org/37633002/diff/270001/net/cert/ct_serializatio... net/cert/ct_serialization.cc:367: // XXX(rsleevi): Declare a swap specialization to avoid the extra copy. same here re: comment https://codereview.chromium.org/37633002/diff/270001/net/test/ct_test_util.cc File net/test/ct_test_util.cc (right): https://codereview.chromium.org/37633002/diff/270001/net/test/ct_test_util.cc... net/test/ct_test_util.cc:87: } // namespace nit: linebreak between the var & } // namespace
Addressed Ryan's comments. https://codereview.chromium.org/37633002/diff/270001/net/cert/ct_serializatio... File net/cert/ct_serialization.cc (right): https://codereview.chromium.org/37633002/diff/270001/net/cert/ct_serializatio... net/cert/ct_serialization.cc:64: // |max_length| indicates the maximum length supported (eg: 2^24 - 1). On On 2013/10/30 20:48:27, Ryan Sleevi wrote: > update comment Done, same for all other comments where |max_length| appeared. https://codereview.chromium.org/37633002/diff/270001/net/cert/ct_serializatio... net/cert/ct_serialization.cc:285: // XXX(rsleevi): Declare a swap specialization to avoid the extra copy On 2013/10/30 20:48:27, Ryan Sleevi wrote: > Just remove this comment Done. https://codereview.chromium.org/37633002/diff/270001/net/cert/ct_serializatio... net/cert/ct_serialization.cc:367: // XXX(rsleevi): Declare a swap specialization to avoid the extra copy. On 2013/10/30 20:48:27, Ryan Sleevi wrote: > same here re: comment Done. https://codereview.chromium.org/37633002/diff/270001/net/test/ct_test_util.cc File net/test/ct_test_util.cc (right): https://codereview.chromium.org/37633002/diff/270001/net/test/ct_test_util.cc... net/test/ct_test_util.cc:87: } // namespace On 2013/10/30 20:48:27, Ryan Sleevi wrote: > nit: linebreak between the var & } // namespace Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eranm@google.com/37633002/410002
Message was sent while issue was closed.
Change committed as 232131
Message was sent while issue was closed.
Fixed the test so the ASAN build will not fail (the issue was using a freed string in a StringPiece during the test).
Consider this an LGTM for either this copy approach, or for doing the StringPiece of the whole string, then substr.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eranm@google.com/37633002/770001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eranm@google.com/37633002/770001
Message was sent while issue was closed.
Change committed as 232267 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
