Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
86503002:
Certificate Transparency: Logging SCTs to the NetLog. (Closed)
|
Created:
7 years ago by Eran M. (Google) Modified:
7 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org, eroman, mmenke Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCertificate Transparency: Logging SCTs to the NetLog.
This includes logging raw SCTList, as well as decoded & checked SCTs (as two separate events).
This should be the *final* CT patch from me to net/.
BUG=309578
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=237943
Patch Set 1 #Patch Set 2 : Preparing for review #
Total comments: 20
Patch Set 3 : Addressing review comments #
Total comments: 59
Patch Set 4 : Address *all* comments #
Total comments: 28
Patch Set 5 : Merging with master #Patch Set 6 : Field name changed in master #Patch Set 7 : Addressinh eroman's review comments #Patch Set 8 : Forgot one file. #
Messages
Total messages: 20 (0 generated)
https://codereview.chromium.org/86503002/diff/20001/net/base/net_log_event_ty... File net/base/net_log_event_type_list.h (right): https://codereview.chromium.org/86503002/diff/20001/net/base/net_log_event_ty... net/base/net_log_event_type_list.h:562: // This event is only present when logging at LOG_ALL. I don't see any evidence that this is logged exclusively in the presence of LOG_ALL... https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... File net/cert/ct_signed_certificate_timestamp_log_param.cc (right): https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:22: void GetSCTFieldsAsStrings( I don't understand this --> why not write directly into a base::Value? The indirection of writing to a std::map and then transferring to a base::DictionaryValue seems unnecessary. https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:39: (*out)["version"] = base::StringPrintf("%d", sct.version); sct.version as I understand is an unsigned value (enum), so technically %d isn't the right choice. Since this is being written into a base::DictionaryValue and will be jsonified you can write it out directly as a number for the interchange. https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:40: base::Base64Encode(sct.log_id, &((*out)["log_id"])); what is the format of the log_id? base::Values can directly receive a UTF-8 string if it is already one. https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:43: base::StringPrintf("%lld", time_since_epoch.InMilliseconds()); things like base::Int64ToString() would be cleaner. https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:46: base::StringPrintf("%d", sct.signature.hash_algorithm); Same nit, i believe this is an unsigned value. Moreover doesn't need to be serialized to a string, can be left as a Number. https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:54: AddToDictionary(base::DictionaryValue* dict) : dict_(dict) {} Explicit. But really I don't believe this class should exist, since can directly populate the base::Value without generating a map<> first. https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:58: dict_->Set(p.first, new base::StringValue(p.second)); dict_->SetString() https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:92: dict->Set(description, new base::StringValue(b64_data)); dict->SetString()
Thanks for the review, eroman@ ! Simplified the code as you suggested, getting rid of the std::map entirely. PTAL. https://codereview.chromium.org/86503002/diff/20001/net/base/net_log_event_ty... File net/base/net_log_event_type_list.h (right): https://codereview.chromium.org/86503002/diff/20001/net/base/net_log_event_ty... net/base/net_log_event_type_list.h:562: // This event is only present when logging at LOG_ALL. On 2013/11/27 00:03:20, eroman wrote: > I don't see any evidence that this is logged exclusively in the presence of > LOG_ALL... Sorry, that's leftover comment from the description of EVENT_TYPE(SSL_CERTIFICATES_RECEIVED). SSL_CERTIFICATES_RECEIVED claims the same thing but the code (in net/cert/x509_certificate_net_log_param.cc) does not do anything with the log_level parameter. Removed the comment. https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... File net/cert/ct_signed_certificate_timestamp_log_param.cc (right): https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:22: void GetSCTFieldsAsStrings( On 2013/11/27 00:03:20, eroman wrote: > I don't understand this --> why not write directly into a base::Value? The > indirection of writing to a std::map and then transferring to a > base::DictionaryValue seems unnecessary. Done - this was leftover from a previous attempt in implementation, not necessary anymore. https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:39: (*out)["version"] = base::StringPrintf("%d", sct.version); On 2013/11/27 00:03:20, eroman wrote: > sct.version as I understand is an unsigned value (enum), so technically %d isn't > the right choice. Since this is being written into a base::DictionaryValue and > will be jsonified you can write it out directly as a number for the interchange. Done - I assume you meant base::DictionaryValue::SetInteger? https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:40: base::Base64Encode(sct.log_id, &((*out)["log_id"])); On 2013/11/27 00:03:20, eroman wrote: > what is the format of the log_id? base::Values can directly receive a UTF-8 > string if it is already one. It's not a human-readable string, but a hash of a public key, so I'm base-64 encoding it. https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:43: base::StringPrintf("%lld", time_since_epoch.InMilliseconds()); On 2013/11/27 00:03:20, eroman wrote: > things like base::Int64ToString() would be cleaner. Done. https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:46: base::StringPrintf("%d", sct.signature.hash_algorithm); On 2013/11/27 00:03:20, eroman wrote: > Same nit, i believe this is an unsigned value. Moreover doesn't need to be > serialized to a string, can be left as a Number. Done and done, for both values. https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:54: AddToDictionary(base::DictionaryValue* dict) : dict_(dict) {} On 2013/11/27 00:03:20, eroman wrote: > Explicit. But really I don't believe this class should exist, since can directly > populate the base::Value without generating a map<> first. Removed this class together with the associated map use. https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:58: dict_->Set(p.first, new base::StringValue(p.second)); On 2013/11/27 00:03:20, eroman wrote: > dict_->SetString() Done - removed class. https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:92: dict->Set(description, new base::StringValue(b64_data)); On 2013/11/27 00:03:20, eroman wrote: > dict->SetString() Done. https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:92: dict->Set(description, new base::StringValue(b64_data)); On 2013/11/27 00:03:20, eroman wrote: > dict->SetString() Done.
Thanks for the review, eroman@ ! Simplified the code as you suggested, getting rid of the std::map entirely. PTAL. https://codereview.chromium.org/86503002/diff/20001/net/base/net_log_event_ty... File net/base/net_log_event_type_list.h (right): https://codereview.chromium.org/86503002/diff/20001/net/base/net_log_event_ty... net/base/net_log_event_type_list.h:562: // This event is only present when logging at LOG_ALL. On 2013/11/27 00:03:20, eroman wrote: > I don't see any evidence that this is logged exclusively in the presence of > LOG_ALL... Sorry, that's leftover comment from the description of EVENT_TYPE(SSL_CERTIFICATES_RECEIVED). SSL_CERTIFICATES_RECEIVED claims the same thing but the code (in net/cert/x509_certificate_net_log_param.cc) does not do anything with the log_level parameter. Removed the comment. https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... File net/cert/ct_signed_certificate_timestamp_log_param.cc (right): https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:22: void GetSCTFieldsAsStrings( On 2013/11/27 00:03:20, eroman wrote: > I don't understand this --> why not write directly into a base::Value? The > indirection of writing to a std::map and then transferring to a > base::DictionaryValue seems unnecessary. Done - this was leftover from a previous attempt in implementation, not necessary anymore. https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:39: (*out)["version"] = base::StringPrintf("%d", sct.version); On 2013/11/27 00:03:20, eroman wrote: > sct.version as I understand is an unsigned value (enum), so technically %d isn't > the right choice. Since this is being written into a base::DictionaryValue and > will be jsonified you can write it out directly as a number for the interchange. Done - I assume you meant base::DictionaryValue::SetInteger? https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:40: base::Base64Encode(sct.log_id, &((*out)["log_id"])); On 2013/11/27 00:03:20, eroman wrote: > what is the format of the log_id? base::Values can directly receive a UTF-8 > string if it is already one. It's not a human-readable string, but a hash of a public key, so I'm base-64 encoding it. https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:43: base::StringPrintf("%lld", time_since_epoch.InMilliseconds()); On 2013/11/27 00:03:20, eroman wrote: > things like base::Int64ToString() would be cleaner. Done. https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:46: base::StringPrintf("%d", sct.signature.hash_algorithm); On 2013/11/27 00:03:20, eroman wrote: > Same nit, i believe this is an unsigned value. Moreover doesn't need to be > serialized to a string, can be left as a Number. Done and done, for both values. https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:54: AddToDictionary(base::DictionaryValue* dict) : dict_(dict) {} On 2013/11/27 00:03:20, eroman wrote: > Explicit. But really I don't believe this class should exist, since can directly > populate the base::Value without generating a map<> first. Removed this class together with the associated map use. https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:58: dict_->Set(p.first, new base::StringValue(p.second)); On 2013/11/27 00:03:20, eroman wrote: > dict_->SetString() Done - removed class. https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:92: dict->Set(description, new base::StringValue(b64_data)); On 2013/11/27 00:03:20, eroman wrote: > dict->SetString() Done. https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:92: dict->Set(description, new base::StringValue(b64_data)); On 2013/11/27 00:03:20, eroman wrote: > dict->SetString() Done.
Patch set 3 LGTM. Please wait for the approval by eroman. High-level comments: 1. ct_signed_certificate_timestamp_log_param.cc has several minor issues. It could also benefit from more comments. 2. I think both NetLog events should be logged in multi_log_ct_verifier.cc. https://codereview.chromium.org/86503002/diff/40001/net/base/net_log_event_ty... File net/base/net_log_event_type_list.h (right): https://codereview.chromium.org/86503002/diff/40001/net/base/net_log_event_ty... net/base/net_log_event_type_list.h:571: EVENT_TYPE(SSL_SIGNED_CERTIFICATE_TIMESTAMPS_RECEIVED) Consider using "CT_" instead of "SSL_" for these event names because these events are really the events of a CTVerifier. https://codereview.chromium.org/86503002/diff/40001/net/base/net_log_event_ty... net/base/net_log_event_type_list.h:586: // "timestamp": <numeric timestamp in milliseconds since epoch>, Nit: epoch => the Unix epoch https://codereview.chromium.org/86503002/diff/40001/net/base/net_log_event_ty... net/base/net_log_event_type_list.h:588: // "signature_algorithm": <numeric indicator of signature algorithm>, It may be worthwhile to convert the algorithm numbers to strings, such as "sha256" and "rsa". You can make this a TODO. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... File net/cert/ct_signed_certificate_timestamp_log_param.cc (right): https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:23: base::DictionaryValue* GetSCTFieldsAsStrings( Nit: it is rare to see a file with no comments :-) I think these three internal functions could benefit from a concise, one-sentence comment that describes what they do. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:41: out->SetInteger("version", sct.version); Nit: the function name says "get SCT fields as strings", but the sct.version field is set as an integer, contradicting the function name. Perhaps the function name should say "PrintableValues" rather than "Strings". https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:42: std::string log_id_b64; Nit: consider reusing the same std::string local variable to receive the output of all base::Base64Encode() calls in this function. Alternatively, this function can call AddBase64EncodedStringToDictionary(). https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:43: base::Base64Encode(sct.log_id, &log_id_b64); Nit: it may be a good idea to check the return value of base::Base64Encode. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:65: Nit: delete this blank line and line 79. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:69: ++it) Nit: I think people usually align these with the first character after "for (", even though it implies five chars indentation. In this case, it seems that everything may fit on the same line. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:91: const ct::CTVerifyResult* ct_result, NetLog::LogLevel log_level) { IMPORTANT: the |log_level| parameter is not used in these two Callback functions. If the |log_level| parameter is not required by NetLog, we probably should remove the parameter. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:95: ct::SCTListToPrintableValues(ct_result->verified_scts)); Nit: it is strange to see these internal functions with the ct:: prefix. Make them look like "important" functions declared in some header. I would suggest not wrapping "namespace ct" around these three internal functions. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:97: dict->Set("failed_to_verify_scts", Nit: just wondering why the name string doesn't match the name of the struct member |unverified_scts|. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:101: ct::SCTListToPrintableValues(ct_result->unknown_logs_scts)); Nit: in these three dict->Set() calls, the second argument should be aligned with the first argument. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... File net/cert/ct_signed_certificate_timestamp_log_param.h (right): https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.h:6: #define NET_BASE_CT_SIGNED_CERTIFICATE_TIMESTAMP_LOG_PARAM_H_ The include guard macro's name needs to be updated to match the new file pathname. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.h:8: #include "net/base/net_log.h" Nit: Add a blank line after this line. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.h:15: base::Value* NetLogSignedCertificateTimestampCallback( Nit: should we document these two functions? Or are they standard functions for NetLog? https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.h:23: } // namespace net Nit: Add a blank line before this line. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.h:25: #endif // NET_BASE_CT_SIGNED_CERTIFICATE_TIMESTAMP_LOG_PARAM_H_ Nit: two spaces before "//", one space after "//". https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_verifier.h File net/cert/ct_verifier.h (right): https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_verifier.h#ne... net/cert/ct_verifier.h:12: class BoundNetLog; Nit: move this to line 18, with the other forward declaration. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_verifier.h#ne... net/cert/ct_verifier.h:33: const BoundNetLog& net_log) = 0; Nit: this may be OK. Please check whether |net_log| is by convention the last parameter. |net_log| is declared as a const reference, which looks like an input, and the Style Guide recommends listing inputs before outputs. https://codereview.chromium.org/86503002/diff/40001/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/86503002/diff/40001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:3499: NetLog::TYPE_SSL_SIGNED_CERTIFICATE_TIMESTAMPS_CHECKED, 1. I think this event should be added inside the cert_transparency_verifier_->Verify() function. It has everything it needs (only |ct_verify_result_| is needed) to create the event. 2. Nit: CHECKED => VERIFIED? This matches the method name (cert_transparency_verifier_->Verify). Are you trying to avoid "VERIFIED" being misinterpreted as "verified valid"? https://codereview.chromium.org/86503002/diff/40001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:3504: Nit: delete this blank line. I would also add a blank line before this if statement.
Almost there! I would like to see the next patchset before approving. https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... File net/cert/ct_signed_certificate_timestamp_log_param.cc (right): https://codereview.chromium.org/86503002/diff/20001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:39: (*out)["version"] = base::StringPrintf("%d", sct.version); On 2013/11/27 12:40:22, eranm wrote: > On 2013/11/27 00:03:20, eroman wrote: > > sct.version as I understand is an unsigned value (enum), so technically %d > isn't > > the right choice. Since this is being written into a base::DictionaryValue and > > will be jsonified you can write it out directly as a number for the > interchange. > > Done - I assume you meant base::DictionaryValue::SetInteger? Correct. https://codereview.chromium.org/86503002/diff/40001/net/base/net_log_event_ty... File net/base/net_log_event_type_list.h (right): https://codereview.chromium.org/86503002/diff/40001/net/base/net_log_event_ty... net/base/net_log_event_type_list.h:571: EVENT_TYPE(SSL_SIGNED_CERTIFICATE_TIMESTAMPS_RECEIVED) On 2013/11/27 16:00:58, wtc wrote: > > Consider using "CT_" instead of "SSL_" for these event names because these > events are really the events of a CTVerifier. I actually like the name SSL_* here, as it is consistent with "SSL_CERTIFICATES_RECEIVED" above. Since the name already contains "CERTIFICATE_TIMESTAMS" a "CT_" prefix would be redundant (these are purely names and don't need to worry about having a consistent prefix). So you can drop the "SSL_" if you wanted. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... File net/cert/ct_signed_certificate_timestamp_log_param.cc (right): https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:23: base::DictionaryValue* GetSCTFieldsAsStrings( Please rename this function; the "AsStrings" part of the name is no longer as relevant. Also the "Fields" part of the name is a bit weak. Some other ideas: CreateDictionaryFromSCT() SCTToDictionary() ToDictionary() https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:27: std::string origin_string; [Optional] Stylistically I recommend splitting this off into a helper OriginToString(). Similar to wtc's comment, I recommend additionally adding a HashAlgorithmToString() and SignatureAlgorithmToString(). https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:43: base::Base64Encode(sct.log_id, &log_id_b64); You already have a helper which abstracts the base64 encoding, use it instead (i.e. there should only be one line in this file which base64 encodes). SetBinaryData("log_id", sct.log_id, out); https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:51: base::Base64Encode(sct.extensions, &extensions_b64); SetBinaryData("extensions", sct.extensions, out); https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:57: base::Base64Encode(sct.signature.signature_data, &signature_data_b64); SetBinaryData("extensions", sct.extensions, out); https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:75: void AddBase64EncodedStringToDictionary( I recommend calling this: SetBinaryData(const char* key, const std::string& value, base::DictionaryValue* dict); https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:91: const ct::CTVerifyResult* ct_result, NetLog::LogLevel log_level) { On 2013/11/27 16:00:58, wtc wrote: > > IMPORTANT: the |log_level| parameter is not used in these two Callback > functions. If the |log_level| parameter is not required by NetLog, we probably > should remove the parameter. The parameter is required, hence this is needed to compile. https://codereview.chromium.org/86503002/diff/40001/net/cert/multi_log_ct_ver... File net/cert/multi_log_ct_verifier.cc (right): https://codereview.chromium.org/86503002/diff/40001/net/cert/multi_log_ct_ver... net/cert/multi_log_ct_verifier.cc:70: base::Bind(&NetLogRawSignedCertificateTimestampCallback, [optional] I suggest inlining this into the AddEvent() call.
Addressed all review comments, hopefully it's now good to go - the main .cc file is much shorter now. https://codereview.chromium.org/86503002/diff/40001/net/base/net_log_event_ty... File net/base/net_log_event_type_list.h (right): https://codereview.chromium.org/86503002/diff/40001/net/base/net_log_event_ty... net/base/net_log_event_type_list.h:571: EVENT_TYPE(SSL_SIGNED_CERTIFICATE_TIMESTAMPS_RECEIVED) On 2013/11/27 20:39:55, eroman wrote: > On 2013/11/27 16:00:58, wtc wrote: > > > > Consider using "CT_" instead of "SSL_" for these event names because these > > events are really the events of a CTVerifier. > > I actually like the name SSL_* here, as it is consistent with > "SSL_CERTIFICATES_RECEIVED" above. > > Since the name already contains "CERTIFICATE_TIMESTAMS" a "CT_" prefix would be > redundant (these are purely names and don't need to worry about having a > consistent prefix). So you can drop the "SSL_" if you wanted. Dropped the SSL_ prefix. This should satisfy both comments :) https://codereview.chromium.org/86503002/diff/40001/net/base/net_log_event_ty... net/base/net_log_event_type_list.h:586: // "timestamp": <numeric timestamp in milliseconds since epoch>, On 2013/11/27 16:00:58, wtc wrote: > > Nit: epoch => the Unix epoch Done. https://codereview.chromium.org/86503002/diff/40001/net/base/net_log_event_ty... net/base/net_log_event_type_list.h:588: // "signature_algorithm": <numeric indicator of signature algorithm>, On 2013/11/27 16:00:58, wtc wrote: > > It may be worthwhile to convert the algorithm numbers to strings, such as > "sha256" and "rsa". You can make this a TODO. Done - outputting the names now. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... File net/cert/ct_signed_certificate_timestamp_log_param.cc (right): https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:23: base::DictionaryValue* GetSCTFieldsAsStrings( On 2013/11/27 16:00:58, wtc wrote: > > Nit: it is rare to see a file with no comments :-) > > I think these three internal functions could benefit from a concise, > one-sentence comment that describes what they do. Done. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:23: base::DictionaryValue* GetSCTFieldsAsStrings( On 2013/11/27 20:39:55, eroman wrote: > Please rename this function; the "AsStrings" part of the name is no longer as > relevant. Also the "Fields" part of the name is a bit weak. > > Some other ideas: > CreateDictionaryFromSCT() > SCTToDictionary() > ToDictionary() Done. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:27: std::string origin_string; On 2013/11/27 20:39:55, eroman wrote: > [Optional] Stylistically I recommend splitting this off into a helper > OriginToString(). Similar to wtc's comment, I recommend additionally adding a > HashAlgorithmToString() and SignatureAlgorithmToString(). Done, done and done. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:41: out->SetInteger("version", sct.version); On 2013/11/27 16:00:58, wtc wrote: > > Nit: the function name says "get SCT fields as strings", but the sct.version > field is set as an integer, contradicting the function name. > > Perhaps the function name should say "PrintableValues" rather than "Strings". Renamed function as suggested. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:42: std::string log_id_b64; On 2013/11/27 16:00:58, wtc wrote: > > Nit: consider reusing the same std::string local variable to receive the output > of all base::Base64Encode() calls in this function. > > Alternatively, this function can call AddBase64EncodedStringToDictionary(). Done - calling AddBase64EncodedStringToDictionary (silly of me not to notice it earlier). https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:43: base::Base64Encode(sct.log_id, &log_id_b64); On 2013/11/27 16:00:58, wtc wrote: > > Nit: it may be a good idea to check the return value of base::Base64Encode. Base64Encode promises not to change the output parameter's value if it fails, so it means we'll add an empty string if it does. I think it's a reasonable thing to do - I'm open to another approach if there's something better to do with base64 encoding failure. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:43: base::Base64Encode(sct.log_id, &log_id_b64); On 2013/11/27 20:39:55, eroman wrote: > You already have a helper which abstracts the base64 encoding, use it instead > (i.e. there should only be one line in this file which base64 encodes). > > SetBinaryData("log_id", sct.log_id, out); Done. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:51: base::Base64Encode(sct.extensions, &extensions_b64); On 2013/11/27 20:39:55, eroman wrote: > SetBinaryData("extensions", sct.extensions, out); Done. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:57: base::Base64Encode(sct.signature.signature_data, &signature_data_b64); On 2013/11/27 20:39:55, eroman wrote: > SetBinaryData("extensions", sct.extensions, out); Done. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:65: On 2013/11/27 16:00:58, wtc wrote: > > Nit: delete this blank line and line 79. Done. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:69: ++it) On 2013/11/27 16:00:58, wtc wrote: > > Nit: I think people usually align these with the first character after "for (", > even though it implies five chars indentation. > > In this case, it seems that everything may fit on the same line. It almost fits... I've indented as you suggested. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:75: void AddBase64EncodedStringToDictionary( On 2013/11/27 20:39:55, eroman wrote: > I recommend calling this: > > SetBinaryData(const char* key, const std::string& value, > base::DictionaryValue* dict); Done. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:95: ct::SCTListToPrintableValues(ct_result->verified_scts)); On 2013/11/27 16:00:58, wtc wrote: > > Nit: it is strange to see these internal functions with the ct:: prefix. Make > them look like "important" functions declared in some header. > > I would suggest not wrapping "namespace ct" around these three internal > functions. Done. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:97: dict->Set("failed_to_verify_scts", On 2013/11/27 16:00:58, wtc wrote: > > Nit: just wondering why the name string doesn't match the name of the struct > member |unverified_scts|. For no good reason, really. Changed. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:101: ct::SCTListToPrintableValues(ct_result->unknown_logs_scts)); On 2013/11/27 16:00:58, wtc wrote: > > Nit: in these three dict->Set() calls, the second argument should be aligned > with the first argument. Done. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... File net/cert/ct_signed_certificate_timestamp_log_param.h (right): https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.h:6: #define NET_BASE_CT_SIGNED_CERTIFICATE_TIMESTAMP_LOG_PARAM_H_ On 2013/11/27 16:00:58, wtc wrote: > > The include guard macro's name needs to be updated to match the new file > pathname. Done. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.h:8: #include "net/base/net_log.h" On 2013/11/27 16:00:58, wtc wrote: > > Nit: Add a blank line after this line. Done. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.h:15: base::Value* NetLogSignedCertificateTimestampCallback( On 2013/11/27 16:00:58, wtc wrote: > > Nit: should we document these two functions? Or are they standard functions for > NetLog? Documented them, also referred to the documentation in net_log_event_type_list.h https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.h:23: } // namespace net On 2013/11/27 16:00:58, wtc wrote: > > Nit: Add a blank line before this line. Done. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.h:25: #endif // NET_BASE_CT_SIGNED_CERTIFICATE_TIMESTAMP_LOG_PARAM_H_ On 2013/11/27 16:00:58, wtc wrote: > > Nit: two spaces before "//", one space after "//". Almost got it right! Done. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_verifier.h File net/cert/ct_verifier.h (right): https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_verifier.h#ne... net/cert/ct_verifier.h:12: class BoundNetLog; On 2013/11/27 16:00:58, wtc wrote: > > Nit: move this to line 18, with the other forward declaration. Done. https://codereview.chromium.org/86503002/diff/40001/net/cert/ct_verifier.h#ne... net/cert/ct_verifier.h:33: const BoundNetLog& net_log) = 0; On 2013/11/27 16:00:58, wtc wrote: > > Nit: this may be OK. Please check whether |net_log| is by convention the last > parameter. > > |net_log| is declared as a const reference, which looks like an input, and the > Style Guide recommends listing inputs before outputs. It is the case for the CertVerifier (see ssl_client_socket_nss.cc line 3381) https://codereview.chromium.org/86503002/diff/40001/net/cert/multi_log_ct_ver... File net/cert/multi_log_ct_verifier.cc (right): https://codereview.chromium.org/86503002/diff/40001/net/cert/multi_log_ct_ver... net/cert/multi_log_ct_verifier.cc:70: base::Bind(&NetLogRawSignedCertificateTimestampCallback, On 2013/11/27 20:39:55, eroman wrote: > [optional] I suggest inlining this into the AddEvent() call. With bound callbacks I find it slightly clearer to define the callback first (makes errors less harrowing as well). Seems to be the convention in this file, also. https://codereview.chromium.org/86503002/diff/40001/net/socket/ssl_client_soc... File net/socket/ssl_client_socket_nss.cc (right): https://codereview.chromium.org/86503002/diff/40001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:3499: NetLog::TYPE_SSL_SIGNED_CERTIFICATE_TIMESTAMPS_CHECKED, On 2013/11/27 16:00:58, wtc wrote: > > 1. I think this event should be added inside the > cert_transparency_verifier_->Verify() function. It has everything it needs (only > |ct_verify_result_| is needed) to create the event. > > 2. Nit: CHECKED => VERIFIED? > > This matches the method name (cert_transparency_verifier_->Verify). > > Are you trying to avoid "VERIFIED" being misinterpreted as "verified valid"? 1. Done. 2. Yes, I want to distinct "verified valid" and "checked". Will be happy to re-word, though. https://codereview.chromium.org/86503002/diff/40001/net/socket/ssl_client_soc... net/socket/ssl_client_socket_nss.cc:3504: On 2013/11/27 16:00:58, wtc wrote: > > Nit: delete this blank line. > > I would also add a blank line before this if statement. Done.
Patchset 4 LGTM https://codereview.chromium.org/86503002/diff/60001/net/base/net_log_event_ty... File net/base/net_log_event_type_list.h (right): https://codereview.chromium.org/86503002/diff/60001/net/base/net_log_event_ty... net/base/net_log_event_type_list.h:577: // "failed_to_verify_scts": <A list of SCTs>, Update this comment to match the code (i.e. unverified_scts). https://codereview.chromium.org/86503002/diff/60001/net/base/net_log_event_ty... net/base/net_log_event_type_list.h:583: // "origin": <one of: embedded_in_certificate, tls_extension, ocsp>, nit: enclose the options in quotemarks to emphasize that they are strings. https://codereview.chromium.org/86503002/diff/60001/net/cert/ct_signed_certif... File net/cert/ct_signed_certificate_timestamp_log_param.cc (right): https://codereview.chromium.org/86503002/diff/60001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:22: std::string OriginToString(ct::SignedCertificateTimestamp::Origin origin) { [optional] Since this only returns string literals, I suggest making the return value a |const char*|. It won't improve performance though, since the call to DictionaryValue::SetString() will implicitly construct a std::string out of it anyway. https://codereview.chromium.org/86503002/diff/60001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:36: std::string HashAlgorithmToString( Same comment as above. https://codereview.chromium.org/86503002/diff/60001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:59: std::string SignatureAlgorithmToString( Same comment as above. https://codereview.chromium.org/86503002/diff/60001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:75: // Base64 encode the given |data| string and put it in |dict| with the s/data/value. https://codereview.chromium.org/86503002/diff/60001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:86: delete blank line. https://codereview.chromium.org/86503002/diff/60001/net/cert/ct_signed_certif... File net/cert/ct_signed_certificate_timestamp_log_param.h (right): https://codereview.chromium.org/86503002/diff/60001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.h:16: // Returns a dictionary of processed Signed Certificate Timestamps to be [optional] I prefer "Creates" both here and below, but will leave it up to you. https://codereview.chromium.org/86503002/diff/60001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.h:23: // Returns a dictionary of raw Signed Certificate Timestamps to be logged same suggestion as above. https://codereview.chromium.org/86503002/diff/60001/net/cert/multi_log_ct_ver... File net/cert/multi_log_ct_verifier.cc (right): https://codereview.chromium.org/86503002/diff/60001/net/cert/multi_log_ct_ver... net/cert/multi_log_ct_verifier.cc:67: // Log to Net Log here, after extracting embedded SCTs but before nit: Remove "here," https://codereview.chromium.org/86503002/diff/60001/net/cert/multi_log_ct_ver... net/cert/multi_log_ct_verifier.cc:70: base::Bind(&NetLogRawSignedCertificateTimestampCallback, style: Indent continued lines by 4. https://codereview.chromium.org/86503002/diff/60001/net/cert/multi_log_ct_ver... net/cert/multi_log_ct_verifier.cc:93: base::Bind(&NetLogSignedCertificateTimestampCallback, result); indent continued lines by 4. https://codereview.chromium.org/86503002/diff/60001/net/cert/multi_log_ct_ver... File net/cert/multi_log_ct_verifier_unittest.cc (right): https://codereview.chromium.org/86503002/diff/60001/net/cert/multi_log_ct_ver... net/cert/multi_log_ct_verifier_unittest.cc:135: verifier_->Verify(chain_, sct_list, "", &result, BoundNetLog())); For extra credit, add a unittest which passes a CapturingBoundNetLog and then verify the logged events.
Patch set 4 LGTM. I only skimmed through the diffs between patch sets 3 and 4.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eranm@google.com/86503002/100001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eranm@google.com/86503002/100001
Retried try job too often on ios_dbg_simulator for step(s) url_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Addressed all of eroman's comments. https://codereview.chromium.org/86503002/diff/60001/net/base/net_log_event_ty... File net/base/net_log_event_type_list.h (right): https://codereview.chromium.org/86503002/diff/60001/net/base/net_log_event_ty... net/base/net_log_event_type_list.h:577: // "failed_to_verify_scts": <A list of SCTs>, On 2013/11/27 22:33:46, eroman wrote: > Update this comment to match the code (i.e. unverified_scts). Done. https://codereview.chromium.org/86503002/diff/60001/net/base/net_log_event_ty... net/base/net_log_event_type_list.h:583: // "origin": <one of: embedded_in_certificate, tls_extension, ocsp>, On 2013/11/27 22:33:46, eroman wrote: > nit: enclose the options in quotemarks to emphasize that they are strings. Done. https://codereview.chromium.org/86503002/diff/60001/net/cert/ct_signed_certif... File net/cert/ct_signed_certificate_timestamp_log_param.cc (right): https://codereview.chromium.org/86503002/diff/60001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:22: std::string OriginToString(ct::SignedCertificateTimestamp::Origin origin) { On 2013/11/27 22:33:46, eroman wrote: > [optional] Since this only returns string literals, I suggest making the return > value a |const char*|. It won't improve performance though, since the call to > DictionaryValue::SetString() will implicitly construct a std::string out of it > anyway. Done. https://codereview.chromium.org/86503002/diff/60001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:36: std::string HashAlgorithmToString( On 2013/11/27 22:33:46, eroman wrote: > Same comment as above. Done. https://codereview.chromium.org/86503002/diff/60001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:59: std::string SignatureAlgorithmToString( On 2013/11/27 22:33:46, eroman wrote: > Same comment as above. Done. https://codereview.chromium.org/86503002/diff/60001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:75: // Base64 encode the given |data| string and put it in |dict| with the On 2013/11/27 22:33:46, eroman wrote: > s/data/value. Done. https://codereview.chromium.org/86503002/diff/60001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:75: // Base64 encode the given |data| string and put it in |dict| with the On 2013/11/27 22:33:46, eroman wrote: > s/data/value. Done. https://codereview.chromium.org/86503002/diff/60001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.cc:86: On 2013/11/27 22:33:46, eroman wrote: > delete blank line. Done. https://codereview.chromium.org/86503002/diff/60001/net/cert/ct_signed_certif... File net/cert/ct_signed_certificate_timestamp_log_param.h (right): https://codereview.chromium.org/86503002/diff/60001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.h:16: // Returns a dictionary of processed Signed Certificate Timestamps to be On 2013/11/27 22:33:46, eroman wrote: > [optional] I prefer "Creates" both here and below, but will leave it up to you. Done. https://codereview.chromium.org/86503002/diff/60001/net/cert/ct_signed_certif... net/cert/ct_signed_certificate_timestamp_log_param.h:23: // Returns a dictionary of raw Signed Certificate Timestamps to be logged On 2013/11/27 22:33:46, eroman wrote: > same suggestion as above. Same fix as above. https://codereview.chromium.org/86503002/diff/60001/net/cert/multi_log_ct_ver... File net/cert/multi_log_ct_verifier.cc (right): https://codereview.chromium.org/86503002/diff/60001/net/cert/multi_log_ct_ver... net/cert/multi_log_ct_verifier.cc:67: // Log to Net Log here, after extracting embedded SCTs but before On 2013/11/27 22:33:46, eroman wrote: > nit: Remove "here," Done. https://codereview.chromium.org/86503002/diff/60001/net/cert/multi_log_ct_ver... net/cert/multi_log_ct_verifier.cc:70: base::Bind(&NetLogRawSignedCertificateTimestampCallback, On 2013/11/27 22:33:46, eroman wrote: > style: Indent continued lines by 4. Done. https://codereview.chromium.org/86503002/diff/60001/net/cert/multi_log_ct_ver... net/cert/multi_log_ct_verifier.cc:70: base::Bind(&NetLogRawSignedCertificateTimestampCallback, On 2013/11/27 22:33:46, eroman wrote: > style: Indent continued lines by 4. Done. https://codereview.chromium.org/86503002/diff/60001/net/cert/multi_log_ct_ver... net/cert/multi_log_ct_verifier.cc:93: base::Bind(&NetLogSignedCertificateTimestampCallback, result); On 2013/11/27 22:33:46, eroman wrote: > indent continued lines by 4. Done. https://codereview.chromium.org/86503002/diff/60001/net/cert/multi_log_ct_ver... File net/cert/multi_log_ct_verifier_unittest.cc (right): https://codereview.chromium.org/86503002/diff/60001/net/cert/multi_log_ct_ver... net/cert/multi_log_ct_verifier_unittest.cc:135: verifier_->Verify(chain_, sct_list, "", &result, BoundNetLog())); On 2013/11/27 22:33:46, eroman wrote: > For extra credit, add a unittest which passes a CapturingBoundNetLog and then > verify the logged events. Done - the CapturedEntry is lacking methods to query the underlying params for dictionary values, so I could only verify one of the logged events, not the other. Will add the necessary methods next week.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eranm@google.com/86503002/140001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eranm@google.com/86503002/140001
Retried try job too often on ios_dbg_simulator for step(s) url_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eranm@google.com/86503002/140001
Retried try job too often on ios_dbg_simulator for step(s) url_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eranm@google.com/86503002/140001
Message was sent while issue was closed.
Change committed as 237943 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
