Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1772603002:
Addition of Certificate Transparency details to Security panel of DevTools (Closed)
|
Created:
4 years, 9 months ago by dwaxweiler Modified:
4 years, 5 months ago CC:
apavlov+blink_chromium.org, blink-reviews, blink-reviews-api_chromium.org, caseq+blink_chromium.org, cbentzel+watch_chromium.org, certificate-transparency-chrome_googlegroups.com, chromium-reviews, darin-cc_chromium.org, devtools-reviews_chromium.org, dglazkov+blink, jam, kinuko+watch, kozyatinskiy+blink_chromium.org, loading-reviews_chromium.org, lushnikov+blink_chromium.org, palmer, pfeldman+blink_chromium.org, pfeldman, Randy Smith (Not in Mondays), sergeyv+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAddition of Certificate Transparency details to Security panel of DevTools
The Signed Certificate Timestamps (SCTs) are listed in detail in the Security panel of the DevTools.
BUG=591848
TEST=Visit a website whose certificate has Certificate Transparency information like mozilla.org, open the Security panel in DevTools, refresh and click on "https://www.mozilla.org" under "Main Origin". You should see three SCT sections underneath the certificate section.
Committed: https://crrev.com/0e8fdc2e0d2d4d88a5552f5ba7d283d4c1fa7588
Cr-Commit-Position: refs/heads/master@{#402577}
Patch Set 1 #
Total comments: 17
Patch Set 2 : reacted to comments by Eran #Patch Set 3 : added missing newly created files #Patch Set 4 : a few layout changes to make sure nothing is cut off #Patch Set 5 : removed SignedCertificateTimestampStore and SignedCertificateTimestampIDStatus(List) #
Total comments: 8
Patch Set 6 : fixed IPC bug & improved layout in DevTools #Patch Set 7 : removed a few unnecessary lines #Patch Set 8 : made a few small layout improvements #Patch Set 9 : hide CT section in DevTools if there are no SCTs #Patch Set 10 : rebase #Patch Set 11 : removed some unnecessary includes and fixed order in net.gypi #
Total comments: 16
Patch Set 12 : reacted to comments of Eran and palmer #Patch Set 13 : rebased & auto-formatted #Patch Set 14 : fixed merging issue #
Total comments: 7
Patch Set 15 : renamed a variable #
Total comments: 18
Patch Set 16 : rebase #Patch Set 17 : removed version, renamed Blink variables, simplified some comments, handled a few MAX values #
Total comments: 21
Patch Set 18 : reacted to comments of dgozman #
Total comments: 6
Patch Set 19 : rebase #Patch Set 20 : reacted to further comments of dgozman #Patch Set 21 : fixed NetLog token #
Total comments: 9
Patch Set 22 : reacted to comments of davidben #Patch Set 23 : changed a dictionary key #
Total comments: 18
Patch Set 24 : reacted to comments of Charlie and Mike #
Total comments: 3
Patch Set 25 : removed interpret_casts #Patch Set 26 : rebase #Messages
Total messages: 123 (42 generated)
Description was changed from ========== Addition of Certificate Transparency details to Security panel of DevTools The Signed Certificate Timestamps (SCTs) are listed in detail in the Security panel of the DevTools. BUG=591848 TEST=Visit a website whose certificate has Certificate Transparency information like mozilla.org, open the Security panel in DevTools, refresh and click on click "https://www.mozilla.org" under "Main Origin". You should see three SCT sections underneath the certificate section. ========== to ========== Addition of Certificate Transparency details to Security panel of DevTools The Signed Certificate Timestamps (SCTs) are listed in detail in the Security panel of the DevTools. BUG=591848 TEST=Visit a website whose certificate has Certificate Transparency information like mozilla.org, open the Security panel in DevTools, refresh and click on click "https://www.mozilla.org" under "Main Origin". You should see three SCT sections underneath the certificate section. ==========
daniel.waxweiler@gmail.com changed reviewers: + lgarron@google.com
Description was changed from ========== Addition of Certificate Transparency details to Security panel of DevTools The Signed Certificate Timestamps (SCTs) are listed in detail in the Security panel of the DevTools. BUG=591848 TEST=Visit a website whose certificate has Certificate Transparency information like mozilla.org, open the Security panel in DevTools, refresh and click on click "https://www.mozilla.org" under "Main Origin". You should see three SCT sections underneath the certificate section. ========== to ========== Addition of Certificate Transparency details to Security panel of DevTools The Signed Certificate Timestamps (SCTs) are listed in detail in the Security panel of the DevTools. BUG=591848 TEST=Visit a website whose certificate has Certificate Transparency information like mozilla.org, open the Security panel in DevTools, refresh and click on "https://www.mozilla.org" under "Main Origin". You should see three SCT sections underneath the certificate section. ==========
eranm@chromium.org changed reviewers: + eranm@chromium.org
Thanks for doing this! Left a few comments - I reviewed everything outside of third_party. It seems to me you can save a lot of code by simply retrieving the SCT from the SignedCertificateTimestampStore when you need it. https://codereview.chromium.org/1772603002/diff/1/content/child/web_url_loade... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1772603002/diff/1/content/child/web_url_loade... content/child/web_url_loader_impl.cc:10: #include <iostream> Including iostream will introduce additional static initializers which will break things - I encountered this exact issue in https://codereview.chromium.org/1758823002/ https://codereview.chromium.org/1772603002/diff/1/content/child/web_url_loade... content/child/web_url_loader_impl.cc:181: const WebString HashAlgorithmToString( Perhaps we can extract the function https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/ct_signed... to a more accessible namespace? https://codereview.chromium.org/1772603002/diff/1/content/child/web_url_loade... content/child/web_url_loader_impl.cc:202: const WebString SignatureAlgorithmToString( Same for https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/ct_signed... https://codereview.chromium.org/1772603002/diff/1/content/child/web_url_loade... content/child/web_url_loader_impl.cc:217: const WebString OriginToString( Same for https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/ct_signed... https://codereview.chromium.org/1772603002/diff/1/content/child/web_url_loade... content/child/web_url_loader_impl.cc:253: std::string ByteToHex(const unsigned char *data, int length) { How about using HexEncode in https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin... ? https://codereview.chromium.org/1772603002/diff/1/content/common/ssl_status_s... File content/common/ssl_status_serialization.h (right): https://codereview.chromium.org/1772603002/diff/1/content/common/ssl_status_s... content/common/ssl_status_serialization.h:13: #include "net/cert/signed_certificate_timestamp.h" Not sure ,why the include if you're not using it? https://codereview.chromium.org/1772603002/diff/1/content/public/common/signe... File content/public/common/signed_certificate_timestamp_id_and_status.h (right): https://codereview.chromium.org/1772603002/diff/1/content/public/common/signe... content/public/common/signed_certificate_timestamp_id_and_status.h:21: int id, I think you're missing the point of the SCT ID - you should not need to serialize the rest of the SCT as the SCTs are allocated IDs after being stored in the SignedCertificateTimestampStore (https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...). You should be able to save a lot of code by not serializing the entire SCTs ,but retrieving according to ID when you need it.
https://codereview.chromium.org/1772603002/diff/1/content/child/web_url_loade... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1772603002/diff/1/content/child/web_url_loade... content/child/web_url_loader_impl.cc:344: // Extract SCT's details. I think this is where you'd use the SignedCertificateTimestampStore's Retrieve function (https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...) to get a net::ct::SignedCertificateTimestamp instance (particularly, a scoped_refptr to an existing one) which you'll use to fill in the blink::WebURLResponse::SignedCertificateTimestamp. https://codereview.chromium.org/1772603002/diff/1/content/common/ssl_status_s... File content/common/ssl_status_serialization.cc (right): https://codereview.chromium.org/1772603002/diff/1/content/common/ssl_status_s... content/common/ssl_status_serialization.cc:47: pickle.WriteUInt16(iter->version); If that's saved to disk (I think it is, not entirely sure) then you would have to deal with de-serialization in clients that don't have these fields (but I don't think you need to because the SCT ID should be all you need).
The CQ bit was checked by daniel.waxweiler@gmail.com
The CQ bit was unchecked by daniel.waxweiler@gmail.com
The CQ bit was checked by daniel.waxweiler@gmail.com
The CQ bit was unchecked by daniel.waxweiler@gmail.com
daniel.waxweiler@gmail.com changed reviewers: + pfeldman@chromium.org
I have updated the CL. https://codereview.chromium.org/1772603002/diff/1/content/child/web_url_loade... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1772603002/diff/1/content/child/web_url_loade... content/child/web_url_loader_impl.cc:10: #include <iostream> On 2016/03/09 20:58:47, Eran Messeri wrote: > Including iostream will introduce additional static initializers which will > break things - I encountered this exact issue in > https://codereview.chromium.org/1758823002/ Acknowledged. https://codereview.chromium.org/1772603002/diff/1/content/child/web_url_loade... content/child/web_url_loader_impl.cc:181: const WebString HashAlgorithmToString( On 2016/03/09 20:58:46, Eran Messeri wrote: > Perhaps we can extract the function > https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/ct_signed... > to a more accessible namespace? Acknowledged. https://codereview.chromium.org/1772603002/diff/1/content/child/web_url_loade... content/child/web_url_loader_impl.cc:202: const WebString SignatureAlgorithmToString( On 2016/03/09 20:58:47, Eran Messeri wrote: > Same for > https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/ct_signed... Acknowledged. https://codereview.chromium.org/1772603002/diff/1/content/child/web_url_loade... content/child/web_url_loader_impl.cc:217: const WebString OriginToString( On 2016/03/09 20:58:47, Eran Messeri wrote: > Same for > https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/ct_signed... Acknowledged. https://codereview.chromium.org/1772603002/diff/1/content/child/web_url_loade... content/child/web_url_loader_impl.cc:253: std::string ByteToHex(const unsigned char *data, int length) { On 2016/03/09 20:58:47, Eran Messeri wrote: > How about using HexEncode in > https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin... > ? Acknowledged. https://codereview.chromium.org/1772603002/diff/1/content/child/web_url_loade... content/child/web_url_loader_impl.cc:344: // Extract SCT's details. On 2016/03/09 21:04:35, Eran Messeri wrote: > I think this is where you'd use the SignedCertificateTimestampStore's Retrieve > function > (https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...) > to get a net::ct::SignedCertificateTimestamp instance (particularly, a > scoped_refptr to an existing one) which you'll use to fill in the > blink::WebURLResponse::SignedCertificateTimestamp. I have thought of the SCT store too, but Retrieve() always returns false, i.e. the SCTs could not be found and the scoped_refptr was not changed. In the class description of SignedCertificateTimestampStore is mentioned "When stored, SignedCertificateTimestamp objects are associated with a RenderProcessHost. If all the RenderProcessHosts associated with the SCT have exited, the SCT is removed from the store." Therefore, I suspect that the SCTs are not available anymore. Related: https://bugs.chromium.org/p/chromium/issues/detail?id=592561 https://codereview.chromium.org/1772603002/diff/1/content/common/ssl_status_s... File content/common/ssl_status_serialization.h (right): https://codereview.chromium.org/1772603002/diff/1/content/common/ssl_status_s... content/common/ssl_status_serialization.h:13: #include "net/cert/signed_certificate_timestamp.h" On 2016/03/09 20:58:47, Eran Messeri wrote: > Not sure ,why the include if you're not using it? Acknowledged.
Apologies for the delay - seeking lgarron's feedback. https://codereview.chromium.org/1772603002/diff/1/content/child/web_url_loade... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1772603002/diff/1/content/child/web_url_loade... content/child/web_url_loader_impl.cc:344: // Extract SCT's details. On 2016/03/10 11:02:27, dwaxweiler wrote: > On 2016/03/09 21:04:35, Eran Messeri wrote: > > I think this is where you'd use the SignedCertificateTimestampStore's Retrieve > > function > > > (https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...) > > to get a net::ct::SignedCertificateTimestamp instance (particularly, a > > scoped_refptr to an existing one) which you'll use to fill in the > > blink::WebURLResponse::SignedCertificateTimestamp. > > I have thought of the SCT store too, but Retrieve() always returns false, i.e. > the SCTs could not be found and the scoped_refptr was not changed. In the class > description of SignedCertificateTimestampStore is mentioned "When stored, > SignedCertificateTimestamp objects are associated with a RenderProcessHost. If > all the RenderProcessHosts associated with the SCT have exited, the SCT is > removed from the store." Therefore, I suspect that the SCTs are not available > anymore. Related: https://bugs.chromium.org/p/chromium/issues/detail?id=592561 +lgarron - is that the reason pages have to be re-loaded after the Security panel is opened in DevTools to provide connection-specific information?
daniel.waxweiler@gmail.com changed reviewers: + paulirish@chromium.com
daniel.waxweiler@gmail.com changed reviewers: + paulirish@chromium.org - paulirish@chromium.com
On 2016/03/14 at 18:27:27, eranm wrote: > Apologies for the delay - seeking lgarron's feedback. > > https://codereview.chromium.org/1772603002/diff/1/content/child/web_url_loade... > File content/child/web_url_loader_impl.cc (right): > > https://codereview.chromium.org/1772603002/diff/1/content/child/web_url_loade... > content/child/web_url_loader_impl.cc:344: // Extract SCT's details. > On 2016/03/10 11:02:27, dwaxweiler wrote: > > On 2016/03/09 21:04:35, Eran Messeri wrote: > > > I think this is where you'd use the SignedCertificateTimestampStore's Retrieve > > > function > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...) > > > to get a net::ct::SignedCertificateTimestamp instance (particularly, a > > > scoped_refptr to an existing one) which you'll use to fill in the > > > blink::WebURLResponse::SignedCertificateTimestamp. > > > > I have thought of the SCT store too, but Retrieve() always returns false, i.e. > > the SCTs could not be found and the scoped_refptr was not changed. In the class > > description of SignedCertificateTimestampStore is mentioned "When stored, > > SignedCertificateTimestamp objects are associated with a RenderProcessHost. If > > all the RenderProcessHosts associated with the SCT have exited, the SCT is > > removed from the store." Therefore, I suspect that the SCTs are not available > > anymore. Related: https://bugs.chromium.org/p/chromium/issues/detail?id=592561 > > +lgarron - is that the reason pages have to be re-loaded after the Security panel is opened in DevTools to provide connection-specific information? Hi Daniel, Sorry for the delay; I'll make sure to check up on this CL at least once every day or two from now on. The current approach works, but it has the major problem that it attaches a lot of data to each request that is only rarely needed. I believe that we definitely care about efficiency for these code paths. In particular, we should avoid constructing strings until the point where we know DevTools is asking for them. Your code currently does a good job of this by waiting until SetSecurityStyleAndDetails() where we know the value of report_security_info, but we already have two strings before then. I would suggest trying Eran's recommendation of using the SignedCertificateTimestampStore. It would go roughly like this: - Only send a list of SCT IDs to DevTools. - Introduce a `getSignedCertificateTimestampDetails` DevTools command to query for details, similar to `getCertificateDetails`. - Implement GetSignedCertificateTimestampDetails() in content/browser/devtools/protocol/network_handler.h using something like: content::SignedCertificateTimestampStore* sct_store( content::SignedCertificateTimestampStore::GetInstance()) eranm@: Yes, the page has to be reloaded because request-specific details are not kept/sent to DevTools unless DevTools is open.
estark@chromium.org changed reviewers: + estark@chromium.org
I've only skimmed this CL, so feel free to ignore me if I'm saying nonsense. But, it seems to me that it would actually be preferable to get rid of the SCTStore, and just send the relevant SCT details on responses to the renderer, and then the renderer can decide whether or not to keep them around (based on whether devtools is enabled). If we were designing this feature from scratch, I don't think (?) we would decide that the browser should keep in memory every SCT used on every connection that any renderer made until the renderer dies, and that the renderer would ask the browser for SCT details on the rare occasion that it needs them. That seems more wasteful than simply sticking a pointer to the SCT on the ResourceResponse and sending necessary details from that SCT to the renderer. When devtools is not enabled, the renderer will be able to discard the SCTs that it doesn't need to display, which is much better than storing them in the browser process only to never be used.
On 2016/03/18 00:33:53, estark wrote: > I've only skimmed this CL, so feel free to ignore me if I'm saying nonsense. > But, it seems to me that it would actually be preferable to get rid of the > SCTStore, and just send the relevant SCT details on responses to the renderer, > and then the renderer can decide whether or not to keep them around (based on > whether devtools is enabled). > > If we were designing this feature from scratch, I don't think (?) we would > decide that the browser should keep in memory every SCT used on every connection > that any renderer made until the renderer dies, and that the renderer would ask > the browser for SCT details on the rare occasion that it needs them. That seems > more wasteful than simply sticking a pointer to the SCT on the ResourceResponse > and sending necessary details from that SCT to the renderer. When devtools is > not enabled, the renderer will be able to discard the SCTs that it doesn't need > to display, which is much better than storing them in the browser process only > to never be used. Oh, one more thing, if we are going to use the SCTStore for this purpose, we need to fix the SCTStore version of crbug.com/561754 first. (That is, during a navigation that gets transferred from one process to another, we need to update the SCTStore to associate the request's SCTs with the new process.) It would be a pretty easy fix, I think, just want to make a note of it so we don't forget.
On 2016/03/18 at 00:33:53, estark wrote: > I've only skimmed this CL, so feel free to ignore me if I'm saying nonsense. But, it seems to me that it would actually be preferable to get rid of the SCTStore, and just send the relevant SCT details on responses to the renderer, and then the renderer can decide whether or not to keep them around (based on whether devtools is enabled). > > If we were designing this feature from scratch, I don't think (?) we would decide that the browser should keep in memory every SCT used on every connection that any renderer made until the renderer dies, and that the renderer would ask the browser for SCT details on the rare occasion that it needs them. That seems more wasteful than simply sticking a pointer to the SCT on the ResourceResponse and sending necessary details from that SCT to the renderer. When devtools is not enabled, the renderer will be able to discard the SCTs that it doesn't need to display, which is much better than storing them in the browser process only to never be used. That sounds reasonable. The SCTs are currently passed around in SSLInfo as a SignedCertificateTimestampIDStatusList, and Daniel is adding a bunch of fields to each entry. These also get serialized and deserialized. So, I'm not sure if it would even be possible use a pointer. Do you think the added fields are okay?
On 2016/03/18 00:45:39, lgarron wrote: > On 2016/03/18 at 00:33:53, estark wrote: > > I've only skimmed this CL, so feel free to ignore me if I'm saying nonsense. > But, it seems to me that it would actually be preferable to get rid of the > SCTStore, and just send the relevant SCT details on responses to the renderer, > and then the renderer can decide whether or not to keep them around (based on > whether devtools is enabled). > > > > If we were designing this feature from scratch, I don't think (?) we would > decide that the browser should keep in memory every SCT used on every connection > that any renderer made until the renderer dies, and that the renderer would ask > the browser for SCT details on the rare occasion that it needs them. That seems > more wasteful than simply sticking a pointer to the SCT on the ResourceResponse > and sending necessary details from that SCT to the renderer. When devtools is > not enabled, the renderer will be able to discard the SCTs that it doesn't need > to display, which is much better than storing them in the browser process only > to never be used. > > That sounds reasonable. > > The SCTs are currently passed around in SSLInfo as a > SignedCertificateTimestampIDStatusList, and Daniel is adding a bunch of fields > to each entry. These also get serialized and deserialized. > > So, I'm not sure if it would even be possible use a pointer. Do you think the > added fields are okay? No, adding a bunch of strings to each ResourceResponse is too wasteful, I think. I was imagining we'd rip out the SCTStore entirely, remove SCTs from SSLStatus (or only keep what we need, which I think might just be the verify statuses), and just add a SignedCertificateTimestampAndStatusList (a vector of pointers and statuses) to ResourceResponseInfo. (Let me know if I'm not being clear or if I misunderstood your question; I can write out in more detail when I get a moment. Unfortunately, this is also all a little entangled in my mind with a larger refactor I want to do with how content manages and passes around TLS connection state.)
That sounds reasonable. I have removed SignedCertificateTimestampStore, SignedCertificateTimestampIDStatusList and SignedCertificateTimestampIDStatus. Numbers of invalid, valid and unknown SCTs were added to SSLStatus, and a SignedCertificateTimestampAndStatusList attribute was added to the ResourceResponseInfo. However, the latter stays empty (see comments for more details). Feel free to comment my approach! https://codereview.chromium.org/1772603002/diff/80001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1772603002/diff/80001/content/browser/loader/... content/browser/loader/resource_loader.cc:106: request->ssl_info().signed_certificate_timestamps; I set the head.signed_certificate_timestamps here, but in web_url_loader_impl.cc it is empty again. https://codereview.chromium.org/1772603002/diff/80001/content/child/web_url_l... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1772603002/diff/80001/content/child/web_url_l... content/child/web_url_loader_impl.cc:263: // TODO: info.signed_certificate_timestamps is empty info.signed_certificate_timestamps is empty although I set it in resource_loader.cc. What could be the problem?
On 2016/03/21 at 23:03:44, daniel.waxweiler wrote: > That sounds reasonable. I have removed SignedCertificateTimestampStore, SignedCertificateTimestampIDStatusList and SignedCertificateTimestampIDStatus. Numbers of invalid, valid and unknown SCTs were added to SSLStatus, and a SignedCertificateTimestampAndStatusList attribute was added to the ResourceResponseInfo. However, the latter stays empty (see comments for more details). > > Feel free to comment my approach! > > https://codereview.chromium.org/1772603002/diff/80001/content/browser/loader/... > File content/browser/loader/resource_loader.cc (right): > > https://codereview.chromium.org/1772603002/diff/80001/content/browser/loader/... > content/browser/loader/resource_loader.cc:106: request->ssl_info().signed_certificate_timestamps; > I set the head.signed_certificate_timestamps here, but in web_url_loader_impl.cc it is empty again. > > https://codereview.chromium.org/1772603002/diff/80001/content/child/web_url_l... > File content/child/web_url_loader_impl.cc (right): > > https://codereview.chromium.org/1772603002/diff/80001/content/child/web_url_l... > content/child/web_url_loader_impl.cc:263: // TODO: info.signed_certificate_timestamps is empty > info.signed_certificate_timestamps is empty although I set it in resource_loader.cc. What could be the problem? I'm okay with the current approach at a high level; I'll see about getting feedback from Max on the UI. We should probably start asking owners to review part of this, starting with pfeldman@. First, is it easy to separate the SignedCertificateTimestampStore removal into a separate CL? (It can be a dependent CL that relies on this one.) It's not strictly necessary, but it would help reviewing, and would allow us to revert independently if the SCT store removal causes a problem.
I haven't looked at this carefully -- not sure if it's going to be split up into multiple CLs -- but just glanced at resource_loader.cc and left a couple comments. https://codereview.chromium.org/1772603002/diff/80001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1772603002/diff/80001/content/browser/loader/... content/browser/loader/resource_loader.cc:102: response->head.has_major_certificate_errors = true; This change doesn't look right. https://codereview.chromium.org/1772603002/diff/80001/content/browser/loader/... content/browser/loader/resource_loader.cc:106: request->ssl_info().signed_certificate_timestamps; On 2016/03/21 23:03:44, dwaxweiler wrote: > I set the head.signed_certificate_timestamps here, but in web_url_loader_impl.cc > it is empty again. You probably need to add the field to content::ResourceResponseInfo in content/common/resource_messages.h
@lgarron: As I am not familiar with moving changes from one git branch to another, I would prefer not to move the SignedCertificateTimestampStore removal into a separate CL. But if you know a good tutorial about this procedure, I am willing to learn and try it. @estark: Please have a look at my comments. https://codereview.chromium.org/1772603002/diff/80001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1772603002/diff/80001/content/browser/loader/... content/browser/loader/resource_loader.cc:102: response->head.has_major_certificate_errors = true; On 2016/03/22 15:51:12, estark wrote: > This change doesn't look right. Acknowledged. https://codereview.chromium.org/1772603002/diff/80001/content/browser/loader/... content/browser/loader/resource_loader.cc:106: request->ssl_info().signed_certificate_timestamps; On 2016/03/22 15:51:12, estark wrote: > On 2016/03/21 23:03:44, dwaxweiler wrote: > > I set the head.signed_certificate_timestamps here, but in > web_url_loader_impl.cc > > it is empty again. > > You probably need to add the field to content::ResourceResponseInfo in > content/common/resource_messages.h Yes, you are right. Thanks! However, the compiler is complaining now because IPC does not seem to support scoped_refpt. Do you have an idea about how to proceed?
On 2016/03/22 20:47:43, dwaxweiler wrote: > @lgarron: As I am not familiar with moving changes from one git branch to > another, I would prefer not to move the SignedCertificateTimestampStore removal > into a separate CL. But if you know a good tutorial about this procedure, I am > willing to learn and try it. > > @estark: Please have a look at my comments. > > https://codereview.chromium.org/1772603002/diff/80001/content/browser/loader/... > File content/browser/loader/resource_loader.cc (right): > > https://codereview.chromium.org/1772603002/diff/80001/content/browser/loader/... > content/browser/loader/resource_loader.cc:102: > response->head.has_major_certificate_errors = true; > On 2016/03/22 15:51:12, estark wrote: > > This change doesn't look right. > > Acknowledged. > > https://codereview.chromium.org/1772603002/diff/80001/content/browser/loader/... > content/browser/loader/resource_loader.cc:106: > request->ssl_info().signed_certificate_timestamps; > On 2016/03/22 15:51:12, estark wrote: > > On 2016/03/21 23:03:44, dwaxweiler wrote: > > > I set the head.signed_certificate_timestamps here, but in > > web_url_loader_impl.cc > > > it is empty again. > > > > You probably need to add the field to content::ResourceResponseInfo in > > content/common/resource_messages.h > > Yes, you are right. Thanks! However, the compiler is complaining now because IPC > does not seem to support scoped_refpt. Do you have an idea about how to proceed? Is that a scoped_refptr for the SignedCertificateTimestamp you're having difficulty using?
On 2016/03/22 21:22:23, Eran Messeri wrote: > On 2016/03/22 20:47:43, dwaxweiler wrote: > > @lgarron: As I am not familiar with moving changes from one git branch to > > another, I would prefer not to move the SignedCertificateTimestampStore > removal > > into a separate CL. But if you know a good tutorial about this procedure, I am > > willing to learn and try it. > > > > @estark: Please have a look at my comments. > > > > > https://codereview.chromium.org/1772603002/diff/80001/content/browser/loader/... > > File content/browser/loader/resource_loader.cc (right): > > > > > https://codereview.chromium.org/1772603002/diff/80001/content/browser/loader/... > > content/browser/loader/resource_loader.cc:102: > > response->head.has_major_certificate_errors = true; > > On 2016/03/22 15:51:12, estark wrote: > > > This change doesn't look right. > > > > Acknowledged. > > > > > https://codereview.chromium.org/1772603002/diff/80001/content/browser/loader/... > > content/browser/loader/resource_loader.cc:106: > > request->ssl_info().signed_certificate_timestamps; > > On 2016/03/22 15:51:12, estark wrote: > > > On 2016/03/21 23:03:44, dwaxweiler wrote: > > > > I set the head.signed_certificate_timestamps here, but in > > > web_url_loader_impl.cc > > > > it is empty again. > > > > > > You probably need to add the field to content::ResourceResponseInfo in > > > content/common/resource_messages.h > > > > Yes, you are right. Thanks! However, the compiler is complaining now because > IPC > > does not seem to support scoped_refpt. Do you have an idea about how to > proceed? > > Is that a scoped_refptr for the SignedCertificateTimestamp you're having > difficulty using? Yes, exactly, a scoped_refptr is used in SignedCertificateTimestampAndStatus and points to a SignedCertificateTimestamp.
Daniel, thanks for working on this! Lucas/Daniel: do you want me to do a full review for this CL? I don't want to delay things unnecessarily but I'm a little concerned that we should get a design and settle on it first before getting in too deep. We don't want to send information to the renderer that doesn't end up getting used. https://codereview.chromium.org/1772603002/diff/80001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1772603002/diff/80001/content/browser/loader/... content/browser/loader/resource_loader.cc:106: request->ssl_info().signed_certificate_timestamps; On 2016/03/22 20:47:43, dwaxweiler wrote: > On 2016/03/22 15:51:12, estark wrote: > > On 2016/03/21 23:03:44, dwaxweiler wrote: > > > I set the head.signed_certificate_timestamps here, but in > > web_url_loader_impl.cc > > > it is empty again. > > > > You probably need to add the field to content::ResourceResponseInfo in > > content/common/resource_messages.h > > Yes, you are right. Thanks! However, the compiler is complaining now because IPC > does not seem to support scoped_refpt. Do you have an idea about how to proceed? The scoped_refptr<net::HttpResponseHeaders> in that same message struct might be a good example; take a look at https://code.google.com/p/chromium/codesearch#chromium/src/content/common/res... I think you should be able to use the existing SignedCertificateTimestamp::Persist() and CreateFromPickle() methods to send the object across the IPC boundary?
With the new patch, everything works again. The layout has been adapted to the latest comments of @lgarron in the bug tracker. We have not discussed about each piece of information in detail, but nobody was opposed to show the details we have included in the mockups. The only thing that is still missing is the check whether the DevTools are enabled or not to pass only needed information around. Where should it happen? https://codereview.chromium.org/1772603002/diff/80001/content/browser/loader/... File content/browser/loader/resource_loader.cc (right): https://codereview.chromium.org/1772603002/diff/80001/content/browser/loader/... content/browser/loader/resource_loader.cc:106: request->ssl_info().signed_certificate_timestamps; On 2016/03/23 01:08:52, estark wrote: > On 2016/03/22 20:47:43, dwaxweiler wrote: > > On 2016/03/22 15:51:12, estark wrote: > > > On 2016/03/21 23:03:44, dwaxweiler wrote: > > > > I set the head.signed_certificate_timestamps here, but in > > > web_url_loader_impl.cc > > > > it is empty again. > > > > > > You probably need to add the field to content::ResourceResponseInfo in > > > content/common/resource_messages.h > > > > Yes, you are right. Thanks! However, the compiler is complaining now because > IPC > > does not seem to support scoped_refpt. Do you have an idea about how to > proceed? > > The scoped_refptr<net::HttpResponseHeaders> in that same message struct might be > a good example; take a look at > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/res... > I think you should be able to use the existing > SignedCertificateTimestamp::Persist() and CreateFromPickle() methods to send the > object across the IPC boundary? Done.
On 2016/03/23 15:50:52, dwaxweiler wrote: > With the new patch, everything works again. The layout has been adapted to the > latest comments of @lgarron in the bug tracker. We have not discussed about each > piece of information in detail, but nobody was opposed to show the details we > have included in the mockups. I don't know if any designers have seen the mockups. Lucas, do you want to follow up with Max or do you want me to? > > The only thing that is still missing is the check whether the DevTools are > enabled or not to pass only needed information around. Where should it happen? That's probably already the case, as setSecurityDetails() only gets called when devtools is enabled, I think. So I don't think you need to do anything special. > > https://codereview.chromium.org/1772603002/diff/80001/content/browser/loader/... > File content/browser/loader/resource_loader.cc (right): > > https://codereview.chromium.org/1772603002/diff/80001/content/browser/loader/... > content/browser/loader/resource_loader.cc:106: > request->ssl_info().signed_certificate_timestamps; > On 2016/03/23 01:08:52, estark wrote: > > On 2016/03/22 20:47:43, dwaxweiler wrote: > > > On 2016/03/22 15:51:12, estark wrote: > > > > On 2016/03/21 23:03:44, dwaxweiler wrote: > > > > > I set the head.signed_certificate_timestamps here, but in > > > > web_url_loader_impl.cc > > > > > it is empty again. > > > > > > > > You probably need to add the field to content::ResourceResponseInfo in > > > > content/common/resource_messages.h > > > > > > Yes, you are right. Thanks! However, the compiler is complaining now because > > IPC > > > does not seem to support scoped_refpt. Do you have an idea about how to > > proceed? > > > > The scoped_refptr<net::HttpResponseHeaders> in that same message struct might > be > > a good example; take a look at > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/res... > > I think you should be able to use the existing > > SignedCertificateTimestamp::Persist() and CreateFromPickle() methods to send > the > > object across the IPC boundary? > > Done.
estark@chromium.org changed reviewers: + lgarron@chromium.org - lgarron@google.com
Ping. What's happening with this CL? I'd like to go ahead and remove the SCTStore one way or another, but if this CL is going to be landed I'll just wait until that happens. (Also, lgarron, I added your @chromium.org address and removed @google.com which I assume is preferable -- sorry if not.)
daniel.waxweiler@gmail.com changed reviewers: + davidben@chromium.org, palmer@chromium.org
I have included the latest commits in this CL.
drive-by review of code outside of third_party. I strongly suggest breaking this patch up to a few smaller ones - for example, one removing the SCTStore and one extracting the SignatureAlgorithmToString method and its friends to a common file. Each of these changes would be much easier to review so the review of the added functionality will focus only on that. https://codereview.chromium.org/1772603002/diff/200001/chrome/browser/ssl/chr... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/1772603002/diff/200001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client.cc:113: int i; nit: Inline definition of i in the for loop(s). Even better, std::vector has a variant of insert that allows you to insert multiple elements, so you could do something like: state->sct_verify_statuses.insert(state->sct_verify_statuses.begin(), ssl.num_unknown_scts, net::ct::SCT_STATUS_LOG_UNKNOWN). https://codereview.chromium.org/1772603002/diff/200001/content/public/common/... File content/public/common/ssl_status.cc (right): https://codereview.chromium.org/1772603002/diff/200001/content/public/common/... content/public/common/ssl_status.cc:38: case net::ct::SCT_STATUS_LOG_UNKNOWN: Nit: would using count_if here (http://en.cppreference.com/w/cpp/algorithm/count) be simpler? https://codereview.chromium.org/1772603002/diff/200001/net/cert/ct_sct_to_str... File net/cert/ct_sct_to_string.h (right): https://codereview.chromium.org/1772603002/diff/200001/net/cert/ct_sct_to_str... net/cert/ct_sct_to_string.h:27: NET_EXPORT const std::string SignatureAlgorithmToString( As these methods seem very similar to what's in net/cert/ct_signed_certificate_timestamp_log_param.cc, I would suggest changing that file to use these functions. https://codereview.chromium.org/1772603002/diff/200001/net/ssl/signed_certifi... File net/ssl/signed_certificate_timestamp_and_status.h (right): https://codereview.chromium.org/1772603002/diff/200001/net/ssl/signed_certifi... net/ssl/signed_certificate_timestamp_and_status.h:18: SignedCertificateTimestampAndStatus(); Why is this necessary?
https://codereview.chromium.org/1772603002/diff/200001/content/common/resourc... File content/common/resource_messages.cc (right): https://codereview.chromium.org/1772603002/diff/200001/content/common/resourc... content/common/resource_messages.cc:336: // Do not disclose Set-Cookie headers over IPC. Which behavior do you intend? https://codereview.chromium.org/1772603002/diff/200001/content/common/ssl_sta... File content/common/ssl_status_serialization.cc (right): https://codereview.chromium.org/1772603002/diff/200001/content/common/ssl_sta... content/common/ssl_status_serialization.cc:55: base::Pickle pickle(state.data(), static_cast<int>(state.size())); This should maybe be a checked_cast (from base/numerics). I see this pattern is pervasive in the codebase... https://codereview.chromium.org/1772603002/diff/200001/content/public/common/... File content/public/common/ssl_status.h (right): https://codereview.chromium.org/1772603002/diff/200001/content/public/common/... content/public/common/ssl_status.h:63: // Signed Certificate Timestamps (SCTs) of Certificat Transparency (CT). Typo: "Certificate" https://codereview.chromium.org/1772603002/diff/200001/content/public/common/... content/public/common/ssl_status.h:64: int num_unknown_scts; In content/child/web_url_loader_impl.cc you used size_t for these. It's probably best to be consistent, either way, unless there is some reason I don't know about.
I have responded with replies and a new patch to the comments made by Eran and palmer. @Eran: I will try to extract the removal of the SCT store into a seperatre CL in the next days. https://codereview.chromium.org/1772603002/diff/200001/chrome/browser/ssl/chr... File chrome/browser/ssl/chrome_security_state_model_client.cc (right): https://codereview.chromium.org/1772603002/diff/200001/chrome/browser/ssl/chr... chrome/browser/ssl/chrome_security_state_model_client.cc:113: int i; On 2016/04/26 09:29:51, Eran Messeri wrote: > nit: Inline definition of i in the for loop(s). > Even better, std::vector has a variant of insert that allows you to insert > multiple elements, so you could do something like: > state->sct_verify_statuses.insert(state->sct_verify_statuses.begin(), > ssl.num_unknown_scts, net::ct::SCT_STATUS_LOG_UNKNOWN). Acknowledged. https://codereview.chromium.org/1772603002/diff/200001/content/common/resourc... File content/common/resource_messages.cc (right): https://codereview.chromium.org/1772603002/diff/200001/content/common/resourc... content/common/resource_messages.cc:336: // Do not disclose Set-Cookie headers over IPC. On 2016/04/26 23:12:01, palmer wrote: > Which behavior do you intend? Sorry, these comments were leftovers from copy-pasting. https://codereview.chromium.org/1772603002/diff/200001/content/common/ssl_sta... File content/common/ssl_status_serialization.cc (right): https://codereview.chromium.org/1772603002/diff/200001/content/common/ssl_sta... content/common/ssl_status_serialization.cc:55: base::Pickle pickle(state.data(), static_cast<int>(state.size())); On 2016/04/26 23:12:01, palmer wrote: > This should maybe be a checked_cast (from base/numerics). I see this pattern is > pervasive in the codebase... Acknowledged. https://codereview.chromium.org/1772603002/diff/200001/content/public/common/... File content/public/common/ssl_status.cc (right): https://codereview.chromium.org/1772603002/diff/200001/content/public/common/... content/public/common/ssl_status.cc:38: case net::ct::SCT_STATUS_LOG_UNKNOWN: On 2016/04/26 09:29:51, Eran Messeri wrote: > Nit: would using count_if here > (http://en.cppreference.com/w/cpp/algorithm/count) be simpler? Yes, it would be easier to read. So, is it okay to replace one loop with three loops? https://codereview.chromium.org/1772603002/diff/200001/content/public/common/... File content/public/common/ssl_status.h (right): https://codereview.chromium.org/1772603002/diff/200001/content/public/common/... content/public/common/ssl_status.h:63: // Signed Certificate Timestamps (SCTs) of Certificat Transparency (CT). On 2016/04/26 23:12:01, palmer wrote: > Typo: "Certificate" Acknowledged. https://codereview.chromium.org/1772603002/diff/200001/content/public/common/... content/public/common/ssl_status.h:64: int num_unknown_scts; On 2016/04/26 23:12:01, palmer wrote: > In content/child/web_url_loader_impl.cc you used size_t for these. It's probably > best to be consistent, either way, unless there is some reason I don't know > about. Acknowledged. https://codereview.chromium.org/1772603002/diff/200001/net/cert/ct_sct_to_str... File net/cert/ct_sct_to_string.h (right): https://codereview.chromium.org/1772603002/diff/200001/net/cert/ct_sct_to_str... net/cert/ct_sct_to_string.h:27: NET_EXPORT const std::string SignatureAlgorithmToString( On 2016/04/26 09:29:51, Eran Messeri wrote: > As these methods seem very similar to what's in > net/cert/ct_signed_certificate_timestamp_log_param.cc, I would suggest changing > that file to use these functions. Acknowledged. https://codereview.chromium.org/1772603002/diff/200001/net/ssl/signed_certifi... File net/ssl/signed_certificate_timestamp_and_status.h (right): https://codereview.chromium.org/1772603002/diff/200001/net/ssl/signed_certifi... net/ssl/signed_certificate_timestamp_and_status.h:18: SignedCertificateTimestampAndStatus(); On 2016/04/26 09:29:51, Eran Messeri wrote: > Why is this necessary? IPC requires an empty constructor.
paulirish@chromium.org changed reviewers: - paulirish@chromium.org
I have extracted the removal of the SignedCertificateTimestampStore into an own CL: https://codereview.chromium.org/1957483003/ As soon as that CL is accepted, I will update this one.
I have updated this CL since the depended one got accepted. Now, it should be easier to review.
I did a first pass over the plumbing parts of this (WebURLLoader, WebURLResponse, etc.). I think someone else should look at the devtools half of it though. (Lucas?) Eran: of all the SCT fields being displayed here, do any of them stick out to you as particularly likely to be useful to developers debugging things? I wonder if we should just show those and do a dropdown or something to reveal the rest of the fields. https://codereview.chromium.org/1772603002/diff/260001/content/child/web_url_... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1772603002/diff/260001/content/child/web_url_... content/child/web_url_loader_impl.cc:264: blink::WebURLResponse::SignedCertificateTimestampList sctList; should be |sct_list| https://codereview.chromium.org/1772603002/diff/260001/net/cert/ct_sct_to_str... File net/cert/ct_sct_to_string.h (right): https://codereview.chromium.org/1772603002/diff/260001/net/cert/ct_sct_to_str... net/cert/ct_sct_to_string.h:13: // Functions for converting non-string attributes of I'm not sure about this file. It might make more sense to just duplicate some of the code in web_url_loader_impl.cc rather than have one set of functions producing the same strings for very different purposes (net log entries vs devtools). I'd let a net OWNER decide though. https://codereview.chromium.org/1772603002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/ResourceResponse.h (right): https://codereview.chromium.org/1772603002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/ResourceResponse.h:44: #include <vector> This should be above the other includes. https://codereview.chromium.org/1772603002/diff/260001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebURLResponse.h (right): https://codereview.chromium.org/1772603002/diff/260001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebURLResponse.h:40: #include <vector> should be above the other includes
@estark: Concerning your question to Eran, there is already a selection of information that is shown at first, and the rest is expandable. Have a look at the screenshots attached to the related bug. Two things have although changed since I took these screenshots: 1. "Sign Algorithm" was corrected by "Signature Algorithm". 2. "Embedded" was replaced by "Embedded in certificate" to make clearer where the SCT had been embedded in. https://codereview.chromium.org/1772603002/diff/260001/content/child/web_url_... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1772603002/diff/260001/content/child/web_url_... content/child/web_url_loader_impl.cc:264: blink::WebURLResponse::SignedCertificateTimestampList sctList; On 2016/05/20 22:36:42, estark wrote: > should be |sct_list| Acknowledged. https://codereview.chromium.org/1772603002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/ResourceResponse.h (right): https://codereview.chromium.org/1772603002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/ResourceResponse.h:44: #include <vector> On 2016/05/20 22:36:42, estark wrote: > This should be above the other includes. That yields a presubmit error "check-webkit-style failed" with the explanation "Alphabetical sorting problem. [build/include_order] [4]" https://codereview.chromium.org/1772603002/diff/260001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebURLResponse.h (right): https://codereview.chromium.org/1772603002/diff/260001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebURLResponse.h:40: #include <vector> On 2016/05/20 22:36:42, estark wrote: > should be above the other includes That yields a presubmit error "check-webkit-style failed" with the explanation "Alphabetical sorting problem. [build/include_order] [4]"
https://codereview.chromium.org/1772603002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/protocol.json (right): https://codereview.chromium.org/1772603002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:1332: "id": "SecurityDetails", This seems like a lot to send per request. What is this used for? Is this information specific to individual request?
https://codereview.chromium.org/1772603002/diff/280001/net/cert/ct_sct_to_str... File net/cert/ct_sct_to_string.h (right): https://codereview.chromium.org/1772603002/diff/280001/net/cert/ct_sct_to_str... net/cert/ct_sct_to_string.h:18: // Converts a numeric |hash_algorithm| to its textual representation. Extremely Minor Nit: You could slightly simplify these to: // Returns a textual representation of |hash_algorithm|. Another possibility is the names and types of these functions explain themselves perfectly, and you don't need a comment. But, do whatever seems best to you, including ignoring this comment. :) https://codereview.chromium.org/1772603002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorResourceAgent.cpp (right): https://codereview.chromium.org/1772603002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorResourceAgent.cpp:417: OwnPtr<protocol::Array<protocol::Network::SignedCertificateTimestamp>> signedCertificateTimestampList = protocol::Array<protocol::Network::SignedCertificateTimestamp>::create(); A typedef might help make this more readable. https://codereview.chromium.org/1772603002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/ResourceResponse.cpp (right): https://codereview.chromium.org/1772603002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/ResourceResponse.cpp:38: , version(sct.version) I think you might want to adhere to the m_fooBar convention in Blink.
net/cert/ct_* lgtm (but you'd need a proper net reviewer to approve those changes). Emily, re data that can be left out: - The Log ID can be hidden if the log name is shown. - The entire signature details (signature & hash algorithms, signature data) could also be hidden. Signature data is only useful is somebody wants to manually check the signature over the assembled data, at which point the NetLog is a much better source of information. All other data (SCT origin, validation status, issuance date, issuing log) seem to me like useful data I wouldn't hide behind a drop-down, but I'm no expert on how to present these things :) https://codereview.chromium.org/1772603002/diff/280001/content/child/web_url_... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1772603002/diff/280001/content/child/web_url_... content/child/web_url_loader_impl.cc:273: net::ct::VersionToString(sct_and_status.sct->version)), Nit: The version can be omitted because there's only V1 now so there won't be a different value here other than V1 until we finalize and implement RFC6962-bis, which will take a while. https://codereview.chromium.org/1772603002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js (right): https://codereview.chromium.org/1772603002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:837: sctTable.addRow(WebInspector.UIString("Log ID"), originState.securityDetails.signedCertificateTimestampList[i].logId.replace(/(.{2})/g,"$1 ")); Nit: The Log ID is mostly superfluous when the Log Name is displayed. I'll leave it to the UI experts to suggest how it can be done, but if it can be shown only on-demand that'd be great.
A few compilation errors I've pointed out, FYI. https://codereview.chromium.org/1772603002/diff/280001/net/cert/ct_sct_to_str... File net/cert/ct_sct_to_string.cc (right): https://codereview.chromium.org/1772603002/diff/280001/net/cert/ct_sct_to_str... net/cert/ct_sct_to_string.cc:57: return "OCSP"; You have to handle net::ct::SignedCertificateTimestamp::SCT_ORIGIN_MAX here. https://codereview.chromium.org/1772603002/diff/280001/net/cert/ct_sct_to_str... net/cert/ct_sct_to_string.cc:71: return "None"; You have to handle net::ct::SCT_STATUS_MAX here. https://codereview.chromium.org/1772603002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorResourceAgent.cpp (right): https://codereview.chromium.org/1772603002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorResourceAgent.cpp:417: OwnPtr<protocol::Array<protocol::Network::SignedCertificateTimestamp>> signedCertificateTimestampList = protocol::Array<protocol::Network::SignedCertificateTimestamp>::create(); You should rebase - OwnPtr seems to be gone in favour of std::unique_ptr, here and in line 419. https://codereview.chromium.org/1772603002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorResourceAgent.cpp:429: signedCertificateTimestampList->addItem(signedCertificateTimestamp.release()); Since you should be using unique_ptrs, this would now be: signedCertificateTimestampList->addItem(std::move(signedCertificateTimestamp));
Having finally compiled and tried this patch, I'm ok with how things are displayed at the moment (log name, origin and verification status shown in summary, all details in the "detailed information" view). My only suggestion is to drop the version - it'll be a while before there would be different values there.
https://codereview.chromium.org/1772603002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/protocol.json (right): https://codereview.chromium.org/1772603002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/protocol.json:1332: "id": "SecurityDetails", On 2016/05/23 16:53:43, pfeldman wrote: > This seems like a lot to send per request. What is this used for? Is this > information specific to individual request? This information is specific to each request, though generally all requests to the same origin would have the same SignedCertificateTimestamps. We want to display it in the per-request view of the Security panel, similar to how you can see the certificate information for each request today. What do you think of doing this instead: InspectorResourceAgent could hold on to the SignedCertificateTimestamps for each request id, and when the security panel wants to display the SCTs for a request, it sends a command asking for them. Kind of similar to how getResponseBody() works... I wonder if we should even store the SCTs on NetworkResourcesData::ResourceData?
> What do you think of doing this instead: InspectorResourceAgent could hold on to > the SignedCertificateTimestamps for each request id, and when the security panel > wants to display the SCTs for a request, it sends a command asking for them. > Kind of similar to how getResponseBody() works... I wonder if we should even > store the SCTs on NetworkResourcesData::ResourceData? I think that would work. It still adds this payload to each of the requests. Any chance we could get those from browser instead?
I have only fixed smaller things and removed the SCT version number. https://codereview.chromium.org/1772603002/diff/280001/content/child/web_url_... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1772603002/diff/280001/content/child/web_url_... content/child/web_url_loader_impl.cc:273: net::ct::VersionToString(sct_and_status.sct->version)), On 2016/05/25 10:47:04, Eran Messeri wrote: > Nit: The version can be omitted because there's only V1 now so there won't be a > different value here other than V1 until we finalize and implement RFC6962-bis, > which will take a while. Acknowledged. https://codereview.chromium.org/1772603002/diff/280001/net/cert/ct_sct_to_str... File net/cert/ct_sct_to_string.cc (right): https://codereview.chromium.org/1772603002/diff/280001/net/cert/ct_sct_to_str... net/cert/ct_sct_to_string.cc:57: return "OCSP"; On 2016/05/25 12:42:21, Eran Messeri wrote: > You have to handle net::ct::SignedCertificateTimestamp::SCT_ORIGIN_MAX here. Acknowledged. https://codereview.chromium.org/1772603002/diff/280001/net/cert/ct_sct_to_str... net/cert/ct_sct_to_string.cc:71: return "None"; On 2016/05/25 12:42:21, Eran Messeri wrote: > You have to handle net::ct::SCT_STATUS_MAX here. Acknowledged. https://codereview.chromium.org/1772603002/diff/280001/net/cert/ct_sct_to_str... File net/cert/ct_sct_to_string.h (right): https://codereview.chromium.org/1772603002/diff/280001/net/cert/ct_sct_to_str... net/cert/ct_sct_to_string.h:18: // Converts a numeric |hash_algorithm| to its textual representation. On 2016/05/23 19:54:25, palmer wrote: > Extremely Minor Nit: You could slightly simplify these to: > > // Returns a textual representation of |hash_algorithm|. > > Another possibility is the names and types of these functions explain themselves > perfectly, and you don't need a comment. > > But, do whatever seems best to you, including ignoring this comment. :) Acknowledged. https://codereview.chromium.org/1772603002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorResourceAgent.cpp (right): https://codereview.chromium.org/1772603002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorResourceAgent.cpp:417: OwnPtr<protocol::Array<protocol::Network::SignedCertificateTimestamp>> signedCertificateTimestampList = protocol::Array<protocol::Network::SignedCertificateTimestamp>::create(); On 2016/05/25 12:42:21, Eran Messeri wrote: > You should rebase - OwnPtr seems to be gone in favour of std::unique_ptr, here > and in line 419. @Eran: Acknowledged. @palmer: Where should I place such a typedef? https://codereview.chromium.org/1772603002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorResourceAgent.cpp:429: signedCertificateTimestampList->addItem(signedCertificateTimestamp.release()); On 2016/05/25 12:42:21, Eran Messeri wrote: > Since you should be using unique_ptrs, this would now be: > signedCertificateTimestampList->addItem(std::move(signedCertificateTimestamp)); Acknowledged. https://codereview.chromium.org/1772603002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/ResourceResponse.cpp (right): https://codereview.chromium.org/1772603002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/ResourceResponse.cpp:38: , version(sct.version) On 2016/05/23 19:54:25, palmer wrote: > I think you might want to adhere to the m_fooBar convention in Blink. Acknowledged.
On 2016/05/27 21:36:56, pfeldman wrote: > > What do you think of doing this instead: InspectorResourceAgent could hold on > to > > the SignedCertificateTimestamps for each request id, and when the security > panel > > wants to display the SCTs for a request, it sends a command asking for them. > > Kind of similar to how getResponseBody() works... I wonder if we should even > > store the SCTs on NetworkResourcesData::ResourceData? > > I think that would work. It still adds this payload to each of the requests. Any > chance we could get those from browser instead? Yes, I think we could request the SCTs from the browser instead, similar to how we discussed delivering certificate chains from the browser. However, I feel bad asking Daniel to do that, since he already added the SCTs to the requests in a previous CL where dgozman@ suggested that adding this information shouldn't be a problem. (https://codereview.chromium.org/1957483003/) Plus I suspect it might be a somewhat bigger refactor than he intended to sign up for. How would you feel about managing the SCTs in the InspectorResourceAgent for now and filing a refactoring bug to manage them in the browser process instead? Since I'm planning to do very similar work for certificate chains at some point, I could do the SCTs at the same time.
My hesitation is that when you reimplement it the right way, not a single line of this change will stand. I understand the investment, but we should do the right thing after all. Let me defer this to dgozman@ since he suggested that it was not a problem. If he stamps it, I would not object.
estark@chromium.org changed reviewers: + dgozman@chromium.org
+dgozman: please see the last few replies on this thread discussing sending SCTs on requests on IPCs
On 2016/06/01 23:40:54, estark wrote: > +dgozman: please see the last few replies on this thread discussing sending SCTs > on requests on IPCs Sorry, I misunderstood what was happening. I didn't get an impression that overhead would be as large as 12-20kb when replying in https://codereview.chromium.org/1957483003/. I thought it was about something much smaller, like extra raw headers we send when DevTools is open, which is totally fine. Sorry for confusion, but I think it's better to do this right. I can help with devtools part in browser - fetching SCTs using id. Another way forward would be to guard current approach of sending SCTs with each response by hidden DevTools experiment. This way we can iterate on frontend part, then do mentioned refactoring and put this out of experiment. WDYT?
On 2016/06/02 23:14:23, dgozman wrote: > On 2016/06/01 23:40:54, estark wrote: > > +dgozman: please see the last few replies on this thread discussing sending > SCTs > > on requests on IPCs > > Sorry, I misunderstood what was happening. I didn't get an impression that > overhead would be as large as 12-20kb when replying in > https://codereview.chromium.org/1957483003/. I thought it was about something > much smaller, like extra raw headers we send when DevTools is open, which is > totally fine. > > Sorry for confusion, but I think it's better to do this right. I can help with > devtools part in browser - fetching SCTs using id. > > Another way forward would be to guard current approach of sending SCTs with each > response by hidden DevTools experiment. This way we can iterate on frontend > part, then do mentioned refactoring and put this out of experiment. WDYT? Not sure where the 12-20kb number is coming from but I have only heard that as an estimate of certificate chain size. I would think SCTs would be much much smaller than that... Eran, do you know an approximate average size of a ct::SignedCertificateTimestamp object?
On 2016/06/02 23:46:37, estark wrote: > On 2016/06/02 23:14:23, dgozman wrote: > > On 2016/06/01 23:40:54, estark wrote: > > > +dgozman: please see the last few replies on this thread discussing sending > > SCTs > > > on requests on IPCs > > > > Sorry, I misunderstood what was happening. I didn't get an impression that > > overhead would be as large as 12-20kb when replying in > > https://codereview.chromium.org/1957483003/. I thought it was about something > > much smaller, like extra raw headers we send when DevTools is open, which is > > totally fine. > > > > Sorry for confusion, but I think it's better to do this right. I can help with > > devtools part in browser - fetching SCTs using id. > > > > Another way forward would be to guard current approach of sending SCTs with > each > > response by hidden DevTools experiment. This way we can iterate on frontend > > part, then do mentioned refactoring and put this out of experiment. WDYT? > > Not sure where the 12-20kb number is coming from but I have only heard that as > an estimate of certificate chain size. I would think SCTs would be much much > smaller than that... > Eran, do you know an approximate average size of a > ct::SignedCertificateTimestamp object? SCTs are 100 bytes each and we see 2 or 3 when we do receive them.
200-300 bytes is not that bad. But I definitely saw 20k in some design doc. Was it about something else? Anyway, this is almost there from devtools standpoint. A couple of comments below. https://codereview.chromium.org/1772603002/diff/320001/content/child/web_url_... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1772603002/diff/320001/content/child/web_url_... content/child/web_url_loader_impl.cc:266: for (const auto& sct_and_status : info.signed_certificate_timestamps) { I'm probably missing something, but I don't see where we guard sending this information by "devtools are attached" check. https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js (right): https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:789: if (originState.securityDetails.signedCertificateTimestampList.length > 0) { nit: drop "> 0" https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:815: if (originState.securityDetails.signedCertificateTimestampList.length <= 0) if (!....length) https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:824: sctSummaryTable.addRow(WebInspector.UIString("SCT"), originState.securityDetails.signedCertificateTimestampList[i].logDescription + " (" + Is SCT a well-known term for web developers? I think full name will not hurt. https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:834: var sctTable = new WebInspector.SecurityDetailsTable(); nit: let's extract a |var sct = originState.securityDetails.signedCertificateTimestampList[i]| for readability. https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/ResourceResponse.h (right): https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/ResourceResponse.h:44: #include <vector> Are we allowed to use vector in platform/ ? Could you please check with platform architecture team? https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/ResourceResponse.h:66: class SignedCertificateTimestamp { I'm surprised why we don't reuse public/platform version. https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/ResourceResponse.h:87: SignedCertificateTimestamp( explicit?
Answering most of the questions of @dgozman, I have to pass on the remaining ones found in third_party/WebKit/Source/platform/network/ResourceResponse.h to @pfeldman. https://codereview.chromium.org/1772603002/diff/320001/content/child/web_url_... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1772603002/diff/320001/content/child/web_url_... content/child/web_url_loader_impl.cc:266: for (const auto& sct_and_status : info.signed_certificate_timestamps) { On 2016/06/04 00:27:37, dgozman wrote: > I'm probably missing something, but I don't see where we guard sending this > information by "devtools are attached" check. Some time ago, I asked the same question, which @estark answered: "That's probably already the case, as setSecurityDetails() only gets called when devtools is enabled, I think. So I don't think you need to do anything special." https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js (right): https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:789: if (originState.securityDetails.signedCertificateTimestampList.length > 0) { On 2016/06/04 00:27:37, dgozman wrote: > nit: drop "> 0" Acknowledged. https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:815: if (originState.securityDetails.signedCertificateTimestampList.length <= 0) On 2016/06/04 00:27:37, dgozman wrote: > if (!....length) Acknowledged. https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:824: sctSummaryTable.addRow(WebInspector.UIString("SCT"), originState.securityDetails.signedCertificateTimestampList[i].logDescription + " (" + On 2016/06/04 00:27:37, dgozman wrote: > Is SCT a well-known term for web developers? I think full name will not hurt. I have also been thinking about including the full name somewhere, but I do not like to increase the width of the left column of the table even more. Moreover, the full name would often appear three times one below the other, which might look a bit crowded. What do you tink of writing out the second occurence of "SCT" in the line "SCTs: 3 valid SCTs" in the previous paragraph? I was thinking about creating another CL for that change. https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:834: var sctTable = new WebInspector.SecurityDetailsTable(); On 2016/06/04 00:27:37, dgozman wrote: > nit: let's extract a |var sct = > originState.securityDetails.signedCertificateTimestampList[i]| for readability. Acknowledged.
On 2016/06/04 00:27:37, dgozman_slow wrote: > 200-300 bytes is not that bad. But I definitely saw 20k in some design doc. Was > it about something else? > Yes, it was about full certificate chains.
The CQ bit was checked by daniel.waxweiler@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org Link to the patchset: https://codereview.chromium.org/1772603002/#ps320001 (title: "removed version, renamed Blink variables, simplified some comments, handled a few MAX values ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772603002/320001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
It looks like some of dgozman's comments aren't resolved yet. Daniel, are there open questions that you're blocked on right now? https://codereview.chromium.org/1772603002/diff/320001/content/child/web_url_... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1772603002/diff/320001/content/child/web_url_... content/child/web_url_loader_impl.cc:266: for (const auto& sct_and_status : info.signed_certificate_timestamps) { On 2016/06/04 08:38:02, dwaxweiler wrote: > On 2016/06/04 00:27:37, dgozman wrote: > > I'm probably missing something, but I don't see where we guard sending this > > information by "devtools are attached" check. > > Some time ago, I asked the same question, which @estark answered: "That's > probably already the case, as setSecurityDetails() only gets called when > devtools is enabled, I think. So I don't think you need to do anything special." |report_security_info| is only true if reportRawHeaders() is set, and if false we return early at the top of this function. https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/ResourceResponse.h (right): https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/ResourceResponse.h:44: #include <vector> On 2016/06/04 00:27:37, dgozman_slow wrote: > Are we allowed to use vector in platform/ ? Could you please check with platform > architecture team? Daniel, this should probably be wtf/Vector.h, similar to e.g. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/netwo...
I have reacted to the latest comments of dgozman and estark. @estark: I have been waiting to get some replies to the comments of dgozman in third_party/WebKit/Source/platform/network/ResourceResponse.h, especially to his proposal to reuse third_party/WebKit/public/platform/WebURLResponse.h code, as I do not whether I should do that. https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/ResourceResponse.h (right): https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/ResourceResponse.h:44: #include <vector> On 2016/06/08 16:51:55, estark wrote: > On 2016/06/04 00:27:37, dgozman_slow wrote: > > Are we allowed to use vector in platform/ ? Could you please check with > platform > > architecture team? > > Daniel, this should probably be wtf/Vector.h, similar to e.g. > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/netwo... Acknowledged. https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/ResourceResponse.h:66: class SignedCertificateTimestamp { On 2016/06/04 00:27:37, dgozman_slow wrote: > I'm surprised why we don't reuse public/platform version. I don't know whether a reuse is permitted. https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/ResourceResponse.h:87: SignedCertificateTimestamp( On 2016/06/04 00:27:37, dgozman_slow wrote: > explicit? Acknowledged.
Sorry for delay, I was traveling this week. lgtm with a couple of minor comments https://codereview.chromium.org/1772603002/diff/320001/content/child/web_url_... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1772603002/diff/320001/content/child/web_url_... content/child/web_url_loader_impl.cc:266: for (const auto& sct_and_status : info.signed_certificate_timestamps) { On 2016/06/08 16:51:55, estark wrote: > On 2016/06/04 08:38:02, dwaxweiler wrote: > > On 2016/06/04 00:27:37, dgozman wrote: > > > I'm probably missing something, but I don't see where we guard sending this > > > information by "devtools are attached" check. > > > > Some time ago, I asked the same question, which @estark answered: "That's > > probably already the case, as setSecurityDetails() only gets called when > > devtools is enabled, I think. So I don't think you need to do anything > special." > > |report_security_info| is only true if reportRawHeaders() is set, and if false > we return early at the top of this function. Thanks for clarifying. https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js (right): https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:824: sctSummaryTable.addRow(WebInspector.UIString("SCT"), originState.securityDetails.signedCertificateTimestampList[i].logDescription + " (" + On 2016/06/04 08:38:02, dwaxweiler wrote: > On 2016/06/04 00:27:37, dgozman wrote: > > Is SCT a well-known term for web developers? I think full name will not hurt. > > I have also been thinking about including the full name somewhere, but I do not > like to increase the width of the left column of the table even more. Moreover, > the full name would often appear three times one below the other, which might > look a bit crowded. What do you tink of writing out the second occurence of > "SCT" in the line "SCTs: 3 valid SCTs" in the previous paragraph? I was thinking > about creating another CL for that change. Let's go with what you suggest. Let's also add {string=} title parameter to addRow function and pass "Signed Certificate Timestamps". https://codereview.chromium.org/1772603002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorResourceAgent.cpp (right): https://codereview.chromium.org/1772603002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorResourceAgent.cpp:408: // Add all SCT(s) to an array. nit: this comment doesn't add anything. https://codereview.chromium.org/1772603002/diff/340001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebURLResponse.h (right): https://codereview.chromium.org/1772603002/diff/340001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebURLResponse.h:41: #include <vector> Remove this one. https://codereview.chromium.org/1772603002/diff/340001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebURLResponse.h:97: using SignedCertificateTimestampList = std::vector<SignedCertificateTimestamp>; This should be WebVector.
I have rebased and reacted to a few further comments. https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js (right): https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:824: sctSummaryTable.addRow(WebInspector.UIString("SCT"), originState.securityDetails.signedCertificateTimestampList[i].logDescription + " (" + On 2016/06/09 08:34:12, dgozman_slow wrote: > On 2016/06/04 08:38:02, dwaxweiler wrote: > > On 2016/06/04 00:27:37, dgozman wrote: > > > Is SCT a well-known term for web developers? I think full name will not > hurt. > > > > I have also been thinking about including the full name somewhere, but I do > not > > like to increase the width of the left column of the table even more. > Moreover, > > the full name would often appear three times one below the other, which might > > look a bit crowded. What do you tink of writing out the second occurence of > > "SCT" in the line "SCTs: 3 valid SCTs" in the previous paragraph? I was > thinking > > about creating another CL for that change. > > Let's go with what you suggest. Let's also add > {string=} title > parameter to addRow function and pass "Signed Certificate Timestamps". Okay. I do not completely get your suggestion. What would the addRow function do with an additional parameter called title? Do you suggest that for this CL or for another one? https://codereview.chromium.org/1772603002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/InspectorResourceAgent.cpp (right): https://codereview.chromium.org/1772603002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/InspectorResourceAgent.cpp:408: // Add all SCT(s) to an array. On 2016/06/09 08:34:12, dgozman_slow wrote: > nit: this comment doesn't add anything. Acknowledged. https://codereview.chromium.org/1772603002/diff/340001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebURLResponse.h (right): https://codereview.chromium.org/1772603002/diff/340001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebURLResponse.h:41: #include <vector> On 2016/06/09 08:34:12, dgozman_slow wrote: > Remove this one. Acknowledged. https://codereview.chromium.org/1772603002/diff/340001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebURLResponse.h:97: using SignedCertificateTimestampList = std::vector<SignedCertificateTimestamp>; On 2016/06/09 08:34:12, dgozman_slow wrote: > This should be WebVector. Acknowledged.
The CQ bit was checked by daniel.waxweiler@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org, dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/1772603002/#ps380001 (title: "reacted to further comments of dgozman")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772603002/380001
On 2016/06/10 07:42:37, dwaxweiler wrote: > I have rebased and reacted to a few further comments. > > https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... > File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js > (right): > > https://codereview.chromium.org/1772603002/diff/320001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:824: > sctSummaryTable.addRow(WebInspector.UIString("SCT"), > originState.securityDetails.signedCertificateTimestampList[i].logDescription + " > (" + > On 2016/06/09 08:34:12, dgozman_slow wrote: > > On 2016/06/04 08:38:02, dwaxweiler wrote: > > > On 2016/06/04 00:27:37, dgozman wrote: > > > > Is SCT a well-known term for web developers? I think full name will not > > hurt. > > > > > > I have also been thinking about including the full name somewhere, but I do > > not > > > like to increase the width of the left column of the table even more. > > Moreover, > > > the full name would often appear three times one below the other, which > might > > > look a bit crowded. What do you tink of writing out the second occurence of > > > "SCT" in the line "SCTs: 3 valid SCTs" in the previous paragraph? I was > > thinking > > > about creating another CL for that change. > > > > Let's go with what you suggest. Let's also add > > {string=} title > > parameter to addRow function and pass "Signed Certificate Timestamps". > > Okay. I do not completely get your suggestion. What would the addRow function do > with an additional parameter called title? Do you suggest that for this CL or > for another one? We can do this in a follow up. Setting title property on an element will show the tooltip for it. So, in addition to setting textContent, let's set the title on the same element.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by daniel.waxweiler@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772603002/380001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by daniel.waxweiler@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772603002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
palmer@chromium.org changed reviewers: - palmer@chromium.org
Sorry, I didn't realize this was waiting on me. net lgtm with comments. https://codereview.chromium.org/1772603002/diff/400001/net/cert/ct_sct_to_str... File net/cert/ct_sct_to_string.cc (right): https://codereview.chromium.org/1772603002/diff/400001/net/cert/ct_sct_to_str... net/cert/ct_sct_to_string.cc:14: net::ct::DigitallySigned::HashAlgorithm hashAlgorithm) { Ditto on net::ct:: prefix https://codereview.chromium.org/1772603002/diff/400001/net/cert/ct_sct_to_str... File net/cert/ct_sct_to_string.h (right): https://codereview.chromium.org/1772603002/diff/400001/net/cert/ct_sct_to_str... net/cert/ct_sct_to_string.h:20: net::ct::DigitallySigned::HashAlgorithm hashAlgorithm); Remove net::ct:: prefix throughout this file. (You're already in the namespace.) https://codereview.chromium.org/1772603002/diff/400001/net/cert/ct_sct_to_str... net/cert/ct_sct_to_string.h:23: NET_EXPORT const std::string OriginToString( #include <string> https://codereview.chromium.org/1772603002/diff/400001/net/cert/ct_signed_cer... File net/cert/ct_signed_certificate_timestamp_log_param.cc (right): https://codereview.chromium.org/1772603002/diff/400001/net/cert/ct_signed_cer... net/cert/ct_signed_certificate_timestamp_log_param.cc:49: out->SetString("origin", origin); I suppose this is fine, but you also may as well just change the comment in net_log_event_type_list.h. This seems kind of silly. :-) The log format doesn't need to stay the same.
I have reacted to the comments of davidben. https://codereview.chromium.org/1772603002/diff/400001/net/cert/ct_sct_to_str... File net/cert/ct_sct_to_string.cc (right): https://codereview.chromium.org/1772603002/diff/400001/net/cert/ct_sct_to_str... net/cert/ct_sct_to_string.cc:14: net::ct::DigitallySigned::HashAlgorithm hashAlgorithm) { On 2016/06/14 15:47:37, davidben wrote: > Ditto on net::ct:: prefix Acknowledged. https://codereview.chromium.org/1772603002/diff/400001/net/cert/ct_sct_to_str... File net/cert/ct_sct_to_string.h (right): https://codereview.chromium.org/1772603002/diff/400001/net/cert/ct_sct_to_str... net/cert/ct_sct_to_string.h:20: net::ct::DigitallySigned::HashAlgorithm hashAlgorithm); On 2016/06/14 15:47:37, davidben wrote: > Remove net::ct:: prefix throughout this file. (You're already in the namespace.) Acknowledged. https://codereview.chromium.org/1772603002/diff/400001/net/cert/ct_sct_to_str... net/cert/ct_sct_to_string.h:23: NET_EXPORT const std::string OriginToString( On 2016/06/14 15:47:37, davidben wrote: > #include <string> Acknowledged. https://codereview.chromium.org/1772603002/diff/400001/net/cert/ct_signed_cer... File net/cert/ct_signed_certificate_timestamp_log_param.cc (right): https://codereview.chromium.org/1772603002/diff/400001/net/cert/ct_signed_cer... net/cert/ct_signed_certificate_timestamp_log_param.cc:49: out->SetString("origin", origin); On 2016/06/14 15:47:37, davidben wrote: > I suppose this is fine, but you also may as well just change the comment in > net_log_event_type_list.h. This seems kind of silly. :-) The log format doesn't > need to stay the same. At least one check of the tests in net/cert/multi_log_ct_verifier_unittest.cc would then also need to be adapted. In the concerned method CheckForEmbeddedSCTInNetLog(), other tokens use the same format, e.g. "verified_scts".
https://codereview.chromium.org/1772603002/diff/400001/net/cert/ct_signed_cer... File net/cert/ct_signed_certificate_timestamp_log_param.cc (right): https://codereview.chromium.org/1772603002/diff/400001/net/cert/ct_signed_cer... net/cert/ct_signed_certificate_timestamp_log_param.cc:49: out->SetString("origin", origin); On 2016/06/14 16:57:02, dwaxweiler wrote: > On 2016/06/14 15:47:37, davidben wrote: > > I suppose this is fine, but you also may as well just change the comment in > > net_log_event_type_list.h. This seems kind of silly. :-) The log format > doesn't > > need to stay the same. > > At least one check of the tests in net/cert/multi_log_ct_verifier_unittest.cc > would then also need to be adapted. In the concerned method > CheckForEmbeddedSCTInNetLog(), other tokens use the same format, e.g. > "verified_scts". *shrug* I have a minor preference for updating the comment and test expectation if it's not too much, but it's only a minor preference. (The dictionary keys all look_like_this, but I think dictionary values tend to be all over the place. Actually, for the really common ones we don't even put strings in them and use numbers that get cross-referenced in some constants table. But that's probably overkill right onw.)
The CQ bit was checked by daniel.waxweiler@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772603002/440001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by daniel.waxweiler@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org, dgozman@chromium.org, davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1772603002/#ps440001 (title: "changed a dictionary key")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772603002/440001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
I have added a few reviewers since an okay is still missing for these files: content/child/web_url_loader_impl.cc third_party/WebKit/Source/platform/exported/WebURLResponse.cpp third_party/WebKit/Source/platform/network/ResourceResponse.cpp third_party/WebKit/Source/platform/network/ResourceResponse.h third_party/WebKit/public/platform/WebURLResponse.h
I'm trimming the reviewers list back down. Daniel, typically you would only add a single OWNER for each file that you need to get reviewed, rather than many owners. Since pfeldman is already a reviewer on this CL, has some context, and is an OWNER of the remaining files that need to be stamped, maybe he can take another look. (pfeldman, note that a few replies upthread we cleared up with dgozman that the SCTs are virtually always <300-400 bytes per request, which is why he was okay with adding them. WDYT?) (Daniel: I like this extension for helping you pick OWNERs: https://chrome.google.com/webstore/detail/chromite-butler/bhcnanendmgjjeghama...)
estark@chromium.org changed reviewers: + creis@chromium.org, mkwst@chromium.org - pfeldman@chromium.org
I guess pfeldman is OOO, sorry about that. In his stead, I'd suggest creis@ for content/child/web_url_loader_impl.cc and mkwst@ for third_party/WebKit. Adding them now.
https://codereview.chromium.org/1772603002/diff/440001/content/child/web_url_... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1772603002/diff/440001/content/child/web_url_... content/child/web_url_loader_impl.cc:271: blink::WebURLResponse::SignedCertificateTimestamp sct( This seems too complicated to be embedded inside this already complex function. I'm guessing we can't do it inside SignedCertificateTimestamp because Blink shouldn't depend on net::SignedCertificateTimestampAndStatus? At the least, we should have a conversion helper function. https://codereview.chromium.org/1772603002/diff/440001/content/child/web_url_... content/child/web_url_loader_impl.cc:277: base::HexEncode(reinterpret_cast<const unsigned char*>( We generally don't like using reinterpret_casts. Is there a way to avoid it? https://codereview.chromium.org/1772603002/diff/440001/content/child/web_url_... content/child/web_url_loader_impl.cc:288: sct_and_status.sct->signature.signature_data.length()))); I'm not qualified to review all these conversions and whether they're safe or correct. Has someone taken a close look at them already?
mkwst@chromium.org changed reviewers: + dgozman@chromium.org
The third_party/WebKit bits look reasonable, but I'm a little worried about the amount of data we'll be pushing down. We're already pushing a lot, and this seems like it might be adding quite a bit more. https://codereview.chromium.org/1772603002/diff/440001/content/child/web_url_... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1772603002/diff/440001/content/child/web_url_... content/child/web_url_loader_impl.cc:267: for (size_t i = 0; i < sct_list.size(); ++i) { Can you not iterate through `info.signed_certificate_timestamps` with a fancy new C++11 for loop? https://codereview.chromium.org/1772603002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/1772603002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:1002: { Nit: Please match the indentation from the rest of the file. https://codereview.chromium.org/1772603002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/exported/WebURLResponse.cpp (right): https://codereview.chromium.org/1772603002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/exported/WebURLResponse.cpp:327: for (auto const& iter : webSecurityDetails.sctList) Nit: `const auto` rather than `auto const`? https://codereview.chromium.org/1772603002/diff/440001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebURLResponse.h (right): https://codereview.chromium.org/1772603002/diff/440001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebURLResponse.h:130: SignedCertificateTimestampList sctList; This seems like a lot of data to add for something that only a very few people will ever see. Will an SCT list be appended to every response, or only the document? The bug discusses "Main Origin", but it's not clear if that's the extent of the feature, or whether other origins in that section would have similar data. Do we keep a copy of this data in the browser process as well? If so, could we request it when needed (or only append it if devtools is open), as opposed to pushing it down proactively?
I have reacted to most comments of Charlie and Mike. https://codereview.chromium.org/1772603002/diff/440001/content/child/web_url_... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1772603002/diff/440001/content/child/web_url_... content/child/web_url_loader_impl.cc:267: for (size_t i = 0; i < sct_list.size(); ++i) { On 2016/06/23 06:48:01, Mike West wrote: > Can you not iterate through `info.signed_certificate_timestamps` with a fancy > new C++11 for loop? This is possible, but I would still need a counter for sct_list. https://codereview.chromium.org/1772603002/diff/440001/content/child/web_url_... content/child/web_url_loader_impl.cc:271: blink::WebURLResponse::SignedCertificateTimestamp sct( On 2016/06/21 23:44:03, Charlie Reis wrote: > This seems too complicated to be embedded inside this already complex function. > I'm guessing we can't do it inside SignedCertificateTimestamp because Blink > shouldn't depend on net::SignedCertificateTimestampAndStatus? > > At the least, we should have a conversion helper function. Acknowledged. https://codereview.chromium.org/1772603002/diff/440001/content/child/web_url_... content/child/web_url_loader_impl.cc:277: base::HexEncode(reinterpret_cast<const unsigned char*>( On 2016/06/21 23:44:03, Charlie Reis wrote: > We generally don't like using reinterpret_casts. Is there a way to avoid it? I am not aware of an alternative, but I am willing to learn. https://codereview.chromium.org/1772603002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/browser_protocol.json (right): https://codereview.chromium.org/1772603002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/browser_protocol.json:1002: { On 2016/06/23 06:48:01, Mike West wrote: > Nit: Please match the indentation from the rest of the file. Acknowledged. https://codereview.chromium.org/1772603002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/exported/WebURLResponse.cpp (right): https://codereview.chromium.org/1772603002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/exported/WebURLResponse.cpp:327: for (auto const& iter : webSecurityDetails.sctList) On 2016/06/23 06:48:01, Mike West wrote: > Nit: `const auto` rather than `auto const`? Acknowledged. https://codereview.chromium.org/1772603002/diff/440001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebURLResponse.h (right): https://codereview.chromium.org/1772603002/diff/440001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebURLResponse.h:130: SignedCertificateTimestampList sctList; On 2016/06/23 06:48:01, Mike West wrote: > This seems like a lot of data to add for something that only a very few people > will ever see. Will an SCT list be appended to every response, or only the > document? The bug discusses "Main Origin", but it's not clear if that's the > extent of the feature, or whether other origins in that section would have > similar data. > > Do we keep a copy of this data in the browser process as well? If so, could we > request it when needed (or only append it if devtools is open), as opposed to > pushing it down proactively? A list of SCTs could be appended to every response authenticated with a certificate with SCTs, so not only the "Main Origin" but every origin is affected. However, the method ResourceResponse::setSecurityDetails() appending the data is only called when the DevTools are open. I do not think that a copy of this data is kept in the browser process. Correct me if I am wrong!
third_party/WebKit LGTM. I think I'm happy with web_url_loader_impl.cc too (with the exception of the reinterpret_cast bits that Charlie noted above). https://codereview.chromium.org/1772603002/diff/440001/content/child/web_url_... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1772603002/diff/440001/content/child/web_url_... content/child/web_url_loader_impl.cc:267: for (size_t i = 0; i < sct_list.size(); ++i) { On 2016/06/23 at 11:53:02, dwaxweiler wrote: > On 2016/06/23 06:48:01, Mike West wrote: > > Can you not iterate through `info.signed_certificate_timestamps` with a fancy > > new C++11 for loop? > > This is possible, but I would still need a counter for sct_list. `sct_list.append(sct)` seems like it would work. This is a nit; it's not terribly important, but dropping the assignment below, and not thinking about sizes anymore seems like a (small) win. https://codereview.chromium.org/1772603002/diff/440001/content/child/web_url_... content/child/web_url_loader_impl.cc:277: base::HexEncode(reinterpret_cast<const unsigned char*>( On 2016/06/23 at 11:53:02, dwaxweiler wrote: > On 2016/06/21 23:44:03, Charlie Reis wrote: > > We generally don't like using reinterpret_casts. Is there a way to avoid it? > > I am not aware of an alternative, but I am willing to learn. Wouldn't `base::HexEncode(sct_and_status.sct->log_id.c_str(), sct_and_status.sct->log_id.size())` work? I don't understand what the cast is doing for you here. https://codereview.chromium.org/1772603002/diff/440001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebURLResponse.h (right): https://codereview.chromium.org/1772603002/diff/440001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebURLResponse.h:130: SignedCertificateTimestampList sctList; On 2016/06/23 at 11:53:02, dwaxweiler wrote: > A list of SCTs could be appended to every response authenticated with a certificate with SCTs, so not only the "Main Origin" but every origin is affected. However, the method ResourceResponse::setSecurityDetails() appending the data is only called when the DevTools are open. I do not think that a copy of this data is kept in the browser process. Correct me if I am wrong! Ok, I think you're right: `WebURLLoaderImpl::PopulateURLResponse()` takes a `report_security_info` boolean, which boils down to the `ResourceRequest::reportRawHeaders()` value set in `InspectorNetworkAgent`. Thanks for the pointer. I'm much less concerned about the memory overhead if we're only incurring it when devtools tells us to. https://codereview.chromium.org/1772603002/diff/460001/content/child/web_url_... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1772603002/diff/460001/content/child/web_url_... content/child/web_url_loader_impl.cc:291: sct_list[i] = NetSCTToBlinkSCT(info.signed_certificate_timestamps[i]); Note: Now that you've extracted everything here, the C++11 loop is even more of a nit than it was before. I still think it's ~marginally cleaner, but, you know. Whatever. :) https://codereview.chromium.org/1772603002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/exported/WebURLResponse.cpp (right): https://codereview.chromium.org/1772603002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/exported/WebURLResponse.cpp:328: sctList.append(static_cast<blink::ResourceResponse::SignedCertificateTimestamp>(iter)); It surprises me a little bit that this works. You're static_casting a `blink::WebURLRequest::SignedCertificateTimestamp` to `blink::ResourceResponse::SignedCertificateTimestamp` whose attributes have different types. Why doesn't that explode? https://codereview.chromium.org/1772603002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/network/ResourceResponse.h (right): https://codereview.chromium.org/1772603002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/network/ResourceResponse.h:87: const struct blink::WebURLResponse::SignedCertificateTimestamp&); Ah. It doesn't explode because you defined a conversion mechanism. Ignore me.
Daniel, is this ready for Charlie to take another look? It does look like you should be able to omit the reinterpret_casts as Mike suggested (https://codereview.chromium.org/1772603002/diff/440001/content/child/web_url_...).
Sorry for the delay! I got rid of the reinterpret_casts. Thanks, Mike! Charlie, you can have another look. https://codereview.chromium.org/1772603002/diff/440001/content/child/web_url_... File content/child/web_url_loader_impl.cc (right): https://codereview.chromium.org/1772603002/diff/440001/content/child/web_url_... content/child/web_url_loader_impl.cc:267: for (size_t i = 0; i < sct_list.size(); ++i) { On 2016/06/23 16:32:30, Mike West wrote: > On 2016/06/23 at 11:53:02, dwaxweiler wrote: > > On 2016/06/23 06:48:01, Mike West wrote: > > > Can you not iterate through `info.signed_certificate_timestamps` with a > fancy > > > new C++11 for loop? > > > > This is possible, but I would still need a counter for sct_list. > > `sct_list.append(sct)` seems like it would work. This is a nit; it's not > terribly important, but dropping the assignment below, and not thinking about > sizes anymore seems like a (small) win. Unfortunately, blink::WebVector does not provide such a handy method. https://codereview.chromium.org/1772603002/diff/440001/content/child/web_url_... content/child/web_url_loader_impl.cc:277: base::HexEncode(reinterpret_cast<const unsigned char*>( On 2016/06/23 16:32:30, Mike West wrote: > On 2016/06/23 at 11:53:02, dwaxweiler wrote: > > On 2016/06/21 23:44:03, Charlie Reis wrote: > > > We generally don't like using reinterpret_casts. Is there a way to avoid > it? > > > > I am not aware of an alternative, but I am willing to learn. > > Wouldn't `base::HexEncode(sct_and_status.sct->log_id.c_str(), > sct_and_status.sct->log_id.size())` work? I don't understand what the cast is > doing for you here. You are totally right!
The CQ bit was checked by daniel.waxweiler@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/06/28 at 09:51:06, daniel.waxweiler wrote: > Sorry for the delay! I got rid of the reinterpret_casts. Thanks, Mike! Charlie, you can have another look. Non-owner's LGTM for your changes to //content/child/web_url_loader_impl.cc, thanks for taking another pass.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks. content/ LGTM.
The CQ bit was checked by daniel.waxweiler@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org, davidben@chromium.org, dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/1772603002/#ps500001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Addition of Certificate Transparency details to Security panel of DevTools The Signed Certificate Timestamps (SCTs) are listed in detail in the Security panel of the DevTools. BUG=591848 TEST=Visit a website whose certificate has Certificate Transparency information like mozilla.org, open the Security panel in DevTools, refresh and click on "https://www.mozilla.org" under "Main Origin". You should see three SCT sections underneath the certificate section. ========== to ========== Addition of Certificate Transparency details to Security panel of DevTools The Signed Certificate Timestamps (SCTs) are listed in detail in the Security panel of the DevTools. BUG=591848 TEST=Visit a website whose certificate has Certificate Transparency information like mozilla.org, open the Security panel in DevTools, refresh and click on "https://www.mozilla.org" under "Main Origin". You should see three SCT sections underneath the certificate section. ==========
Message was sent while issue was closed.
Committed patchset #26 (id:500001)
Message was sent while issue was closed.
Description was changed from ========== Addition of Certificate Transparency details to Security panel of DevTools The Signed Certificate Timestamps (SCTs) are listed in detail in the Security panel of the DevTools. BUG=591848 TEST=Visit a website whose certificate has Certificate Transparency information like mozilla.org, open the Security panel in DevTools, refresh and click on "https://www.mozilla.org" under "Main Origin". You should see three SCT sections underneath the certificate section. ========== to ========== Addition of Certificate Transparency details to Security panel of DevTools The Signed Certificate Timestamps (SCTs) are listed in detail in the Security panel of the DevTools. BUG=591848 TEST=Visit a website whose certificate has Certificate Transparency information like mozilla.org, open the Security panel in DevTools, refresh and click on "https://www.mozilla.org" under "Main Origin". You should see three SCT sections underneath the certificate section. Committed: https://crrev.com/0e8fdc2e0d2d4d88a5552f5ba7d283d4c1fa7588 Cr-Commit-Position: refs/heads/master@{#402577} ==========
Message was sent while issue was closed.
Patchset 26 (id:??) landed as https://crrev.com/0e8fdc2e0d2d4d88a5552f5ba7d283d4c1fa7588 Cr-Commit-Position: refs/heads/master@{#402577} |
