Send certificates to devtools when it's open instead of using certId

Send certificates to devtools when it's open instead of using serialized SSLStatus and sending certificate IDs.

This is along the earlier patchsets of stopping to send SSLStatus to the renderer. This will help network service (since currently it's what sends this data, and it shouldn't know about UI state) and PlzNavigate (since this is a precursor for removing cert_ids which depend on having a renderer process during navigate).

Since this patch has gotten large, I'll remove sending sending security_info to the renderer in a followup patch.

BUG=598073, 504347
Committed: https://crrev.com/8b3813b7f3ac9e197305cc440b78c81773cd07af
Cr-Commit-Position: refs/heads/master@{#416700}

[jam, 2016-08-31 18:28:30]

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

[commit-bot: I haz the power, 2016-08-31 18:29:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 18:51:01]

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

[commit-bot: I haz the power, 2016-08-31 18:51:02]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[jam, 2016-08-31 19:23:19]

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

[jam, 2016-08-31 19:23:22]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-08-31 19:23:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 19:35:37]

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

[commit-bot: I haz the power, 2016-08-31 19:35:38]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2016-08-31 19:42:14]

Patchset #2 (id:40001) has been deleted

[jam, 2016-08-31 19:42:22]

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

[commit-bot: I haz the power, 2016-08-31 19:42:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 20:06:07]

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

[commit-bot: I haz the power, 2016-08-31 20:06:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2016-08-31 22:02:39]

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

[commit-bot: I haz the power, 2016-08-31 22:03:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2016-08-31 22:03:58]

Description was changed from

==========
Send certificates to devtools when it's open instead of using certId

BUG=598073, 504347
==========

to

==========
Send certificates to devtools when it's open instead of using serialized
SSLStatus and sending certificate IDs.

This is along the earlier patchsets of stopping to send SSLStatus to the
renderer. This will help network service (since currently it's what sends this
data, and it shouldn't know about UI state) and PlzNavigate (since this is a
precursor for removing cert_ids which depend on having a renderer process during
navigate).

Since this patch has gotten large, I'll remove sending sending security_info to
the renderer in a followup patch.

BUG=598073, 504347
==========

[jam, 2016-08-31 22:03:58]

jam@chromium.org changed reviewers:
+ estark@chromium.org, pfeldman@chromium.org

[jam, 2016-08-31 22:13:28]

https://codereview.chromium.org/2296953004/diff/80001/content/browser/devtool...
File content/browser/devtools/protocol/security_handler.cc (right):

https://codereview.chromium.org/2296953004/diff/80001/content/browser/devtool...
content/browser/devtools/protocol/security_handler.cc:150: Response
SecurityHandler::ShowCertificateViewer(int certificate_id) {
this is temporary since it's sent as part of the SecurityStateExplanation
devtools messages from the browser. I'll update that code in a followup (didn't
want to make this even bigger)

https://codereview.chromium.org/2296953004/diff/80001/content/browser/devtool...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/2296953004/diff/80001/content/browser/devtool...
content/browser/devtools/render_frame_devtools_agent_host.cc:1031: // the WCD
method signature (will be done in followups).
ditto for not making this patch even larger, i'll change the method in a
followup to take an X509Certificate object instead

https://codereview.chromium.org/2296953004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right):

https://codereview.chromium.org/2296953004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:453:
.setCertificateId(0) // Keep this in protocol for compatability.
Pavel: I tried to remove this from the protocol, but got the following errors

D:\src\chrome1\src>ninja -C out\Debug_gn chrome
ninja: Entering directory `out\Debug_gn'
[3/1995] ACTION
//third_party/WebKit/Source/core/inspector:protocol_compatibility_check(//build/toolchain/win:x86)
FAILED: gen/blink/core/inspector/browser_protocol.stamp
C:/python_27_amd64/files/python.exe
../../third_party/WebKit/Source/platform/inspector_protocol/CheckProtocolCompatibility.py
--stamp gen/bl
ink/core/inspector/browser_protocol.stamp
../../third_party/WebKit/Source/core/inspector/browser_protocol.json
../../third_party/WebKit/Sour
ce/platform/v8_inspector/js_protocol.json
  Compatibility checks FAILED
    Network.responseReceived.response
parameter->Network.Response.securityDetails
property->Network.SecurityDetails.certificateId: required
property has been removed
    Network.requestWillBeSent.redirectResponse
parameter->Network.Response.securityDetails
property->Network.SecurityDetails.certificateId:
required property has been removed
ninja: build stopped: subcommand failed.

This is even with tagging SecurityDetails as experimental. so i left it for now.
any better option?

https://codereview.chromium.org/2296953004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h (right):

https://codereview.chromium.org/2296953004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h:200: //
Optimization so that we don't send the full cerfificate data to the frontend.
this hash map isn't for saving memory (since I'm using AtomicString), but so
that we have a small id (the string's hash) that we can send to the devtools
front end instead of the entire certificate

https://codereview.chromium.org/2296953004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/2296953004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:356:
WebInspector.SecurityPanel.createCertificateViewerButton2 = function (text,
certificateHash)
I'll remove createCertificateViewerButton in a followup and then drop the "2"
from this new method

https://codereview.chromium.org/2296953004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:870: var
sanDiv = this._createSanDiv(originState.securityDetails.sanList);
this code isn't changed, just indentation was modified

[pfeldman, 2016-08-31 23:08:45]

https://codereview.chromium.org/2296953004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right):

https://codereview.chromium.org/2296953004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:453:
.setCertificateId(0) // Keep this in protocol for compatability.
I'll take a look at it.

https://codereview.chromium.org/2296953004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h (right):

https://codereview.chromium.org/2296953004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h:202:
CertificateHashMap m_certificateHashMap;
We should clean it up at least from InspectorNetworkAgent::disable.

In fact, it should be cleaned up in InspectorNetworkAgent::didCommitLoad (except
for that one is after main resource cert has been added into the map, so simple
code won't work there).

Could you instead add certificate data into the NetworkResourcesData
per-resource and then use request id to refer to the certificate? You would not
need a map then and the cert lifetime would be auto-managed.

[jam, 2016-08-31 23:15:09]

https://codereview.chromium.org/2296953004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h (right):

https://codereview.chromium.org/2296953004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h:202:
CertificateHashMap m_certificateHashMap;
On 2016/08/31 23:08:45, pfeldman wrote:
> We should clean it up at least from InspectorNetworkAgent::disable.

Done

> 
> In fact, it should be cleaned up in InspectorNetworkAgent::didCommitLoad
(except
> for that one is after main resource cert has been added into the map, so
simple
> code won't work there).

I'm not sure I understand this comment. Why would we want to delete the
certificate information after a resource commits? We'd need the certificate
after it commits in case the user chooses to view the certificate for that
origin?

> 
> Could you instead add certificate data into the NetworkResourcesData
> per-resource and then use request id to refer to the certificate? You would
not
> need a map then and the cert lifetime would be auto-managed.

That was my initial attempt. However it was awkward because SecurityOrigin
stores this data in a map (_origins) which is keyed by origin and the values are
securitydetails. it seemed odd to put the request id in the origin's value?

[jam, 2016-08-31 23:18:15]

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

[commit-bot: I haz the power, 2016-08-31 23:18:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-09-01 00:20:03]

Looks reasonable to me! Since I'm leaving on vacation tomorrow I don't think you
should block this on me. If you should want any more eyeballs besides pfeldman,
I'd suggest lgarron for the security panel js stuff, and davidben or nasko for
the content/browser stuff.

https://codereview.chromium.org/2296953004/diff/100001/content/browser/devtoo...
File content/browser/devtools/protocol/network_handler.h (left):

https://codereview.chromium.org/2296953004/diff/100001/content/browser/devtoo...
content/browser/devtools/protocol/network_handler.h:58: Response
ShowCertificateViewer(int certificate_id);
Is this method still needed?

https://codereview.chromium.org/2296953004/diff/100001/content/browser/devtoo...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/2296953004/diff/100001/content/browser/devtoo...
content/browser/devtools/render_frame_devtools_agent_host.cc:1028: DCHECK(cert);
Instead of DCHECK, should this kill the renderer if CreateFromPickle() is false?
A compromised renderer could send a cert that fails to deserialize.

https://codereview.chromium.org/2296953004/diff/100001/content/child/web_url_...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2296953004/diff/100001/content/child/web_url_...
content/child/web_url_loader_impl.cc:261: SecurityStyle security_style =
GetSecurityStyleForResource(
Could you instead attach a security style to ResourceResponseInfo, alongside
|ssl_connection_status| and |cert_status|? Then you could just compute
GetSecurityStyleForResource() once in ResourceLoader and not have to pull it out
into a separate utils file.

[commit-bot: I haz the power, 2016-09-01 00:46:03]

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

[commit-bot: I haz the power, 2016-09-01 00:46:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2016-09-01 01:14:03]

https://codereview.chromium.org/2296953004/diff/100001/content/browser/devtoo...
File content/browser/devtools/protocol/network_handler.h (left):

https://codereview.chromium.org/2296953004/diff/100001/content/browser/devtoo...
content/browser/devtools/protocol/network_handler.h:58: Response
ShowCertificateViewer(int certificate_id);
On 2016/09/01 00:20:03, estark (OOO through Sept 12) wrote:
> Is this method still needed?

Not anymore, removed.

https://codereview.chromium.org/2296953004/diff/100001/content/browser/devtoo...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/2296953004/diff/100001/content/browser/devtoo...
content/browser/devtools/render_frame_devtools_agent_host.cc:1028: DCHECK(cert);
On 2016/09/01 00:20:03, estark (OOO through Sept 12) wrote:
> Instead of DCHECK, should this kill the renderer if CreateFromPickle() is
false?
> A compromised renderer could send a cert that fails to deserialize.

That would be a null pointer dereference, which afaik is ok (i.e. we don't try
to protect against a bad renderer killing the browser since there are too many
ways to do that).

https://codereview.chromium.org/2296953004/diff/100001/content/child/web_url_...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2296953004/diff/100001/content/child/web_url_...
content/child/web_url_loader_impl.cc:261: SecurityStyle security_style =
GetSecurityStyleForResource(
On 2016/09/01 00:20:03, estark (OOO through Sept 12) wrote:
> Could you instead attach a security style to ResourceResponseInfo, alongside
> |ssl_connection_status| and |cert_status|? Then you could just compute
> GetSecurityStyleForResource() once in ResourceLoader and not have to pull it
out
> into a separate utils file.

The motivation for the network-service part of this cl is to remove
SecurityStyle knowledge from the networking code in content/browser/loader.

[jam, 2016-09-01 05:57:22]

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

[commit-bot: I haz the power, 2016-09-01 05:57:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 08:01:32]

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

[commit-bot: I haz the power, 2016-09-01 08:01:33]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jam, 2016-09-02 01:08:10]

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

[commit-bot: I haz the power, 2016-09-02 01:08:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2016-09-02 01:08:57]

jam@chromium.org changed reviewers:
+ nasko@chromium.org

[jam, 2016-09-02 01:10:47]

+nasko for security review of IPC (and feel free to look at anything else)

I had to change this a bit to make it work in the (mac & linux) sandbox. instead
of using net::X509Certificate, which uses the OS, I parsed the certificate using
the bare helpers per advice from davidben.

pfeldman ptal as well

[commit-bot: I haz the power, 2016-09-02 01:11:03]

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

[commit-bot: I haz the power, 2016-09-02 01:11:04]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[pfeldman, 2016-09-02 01:18:59]

https://codereview.chromium.org/2296953004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right):

https://codereview.chromium.org/2296953004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:955:
m_certificates.clear();
We should clean up on didCommitLoad or reuse resource data. Otherwise enabling
network agent and for example running page cycler will render us out of memory.

https://codereview.chromium.org/2296953004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2296953004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:895: "name":
"showCertificateViewer",
I'm confused, why do we have this new method here? I thought the point was to go
through the network with it.

[jam, 2016-09-02 01:25:01]

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

[commit-bot: I haz the power, 2016-09-02 01:25:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2016-09-02 01:30:40]

https://codereview.chromium.org/2296953004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right):

https://codereview.chromium.org/2296953004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:955:
m_certificates.clear();
On 2016/09/02 01:18:59, pfeldman wrote:
> We should clean up on didCommitLoad or reuse resource data. Otherwise enabling
> network agent and for example running page cycler will render us out of
memory.

I'm not sure I understand the part about cleaning this up in didCommitLoad (like
your previous comment). If that's done, then after the load commits the
certificate viewer button wouldn't work because the certificate strings would be
gone right? Am I misunderstanding?

Regarding running out of memory: I think this will use more memory clearly, but
it shouldn't cause OOM for two reasons:
-the certificate data is shared in the renderer through the use of AtomicString
(helps just one page load)
-if navigating within the same origin and the renderer is reused, again memory
won't grow since the previous strings will be reused
-if navigating across origins, then the process will be killed and a new one
will be created which will clean up the memory

https://codereview.chromium.org/2296953004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2296953004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/browser_protocol.json:895: "name":
"showCertificateViewer",
On 2016/09/02 01:18:59, pfeldman wrote:
> I'm confused, why do we have this new method here? I thought the point was to
go
> through the network with it.

this is temporary, since for the main page certificate viewer button right now
the browser sends the cert id to the renderer. I didn't want to change this in
this cl as well since it's already pretty big.

(see
https://codereview.chromium.org/2296953004/diff/80001/content/browser/devtool...)

[jam, 2016-09-02 01:33:48]

jam@chromium.org changed reviewers:
+ creis@chromium.org
- nasko@chromium.org

[jam, 2016-09-02 01:33:48]

nasko->creis since nasko is ooo

[commit-bot: I haz the power, 2016-09-02 01:38:26]

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

[commit-bot: I haz the power, 2016-09-02 01:38:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2016-09-02 01:48:08]

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

[commit-bot: I haz the power, 2016-09-02 01:48:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 02:08:13]

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

[commit-bot: I haz the power, 2016-09-02 02:08:15]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[jam, 2016-09-02 03:46:16]

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

[jam, 2016-09-02 03:46:28]

Patchset #8 (id:180001) has been deleted

[commit-bot: I haz the power, 2016-09-02 03:46:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 04:00:03]

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

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

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2016-09-02 05:02:45]

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

[jam, 2016-09-02 05:02:54]

Patchset #9 (id:220001) has been deleted

[jam, 2016-09-02 05:03:04]

Patchset #8 (id:200001) has been deleted

[commit-bot: I haz the power, 2016-09-02 05:03:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 09:21:46]

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

[commit-bot: I haz the power, 2016-09-02 09:21:47]

Dry run: This issue passed the CQ dry run.

[jam, 2016-09-02 16:40:44]

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

[jam, 2016-09-02 16:40:47]

https://codereview.chromium.org/2296953004/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right):

https://codereview.chromium.org/2296953004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:955:
m_certificates.clear();
ok I think I understand your point better now.

The latest patchset clears the certificate list when WebDevToolsAgentImpl gets
the didStartProvisionalLoad.

[commit-bot: I haz the power, 2016-09-02 16:41:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pfeldman, 2016-09-02 17:15:01]

There is one more thing (see below). Let me help you with it real quick.

https://codereview.chromium.org/2296953004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h (right):

https://codereview.chromium.org/2296953004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h:105: void
didStartProvisionalLoad();
As discussed offline, you provisional load may fail, we should store data in the
NetworkResourcesData.

https://codereview.chromium.org/2296953004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.h:159: void
showCertificateViewer(ErrorString*, int certificateIndex) override;
I don't think this fits remote debugging scenario well. Imagine that you are
connected to the Chrome for Android and are showing the certificate. You want to
show certificate on the client machine (where front-end is served), not on the
Android device. So what you really want to do is to introduce

getCertificate(String origin) into the protocol, fetch the string to the
front-end and then use web_ui host interface to show the certificate on the
client side. Let me draft that part for you quickly so that you did not have to
go over all of that new code.

https://codereview.chromium.org/2296953004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/2296953004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:337:
WebInspector.SecurityPanel.createCertificateViewerButton = function (text,
certificateId, panel)
We don't do space after function.

[Charlie Reis, 2016-09-02 17:26:30]

creis@chromium.org changed reviewers:
+ mbarbella@chromium.org

[Charlie Reis, 2016-09-02 17:27:59]

On 2016/09/02 01:33:48, jam wrote:
> nasko->creis since nasko is ooo

Was this for IPC owners?  I'm not in that list, so maybe mbarbella@ can take a
look.  (Nasko, Daniel, and Ken are all out today.)

(Let me know if you'd like me to review anything else, though.)

[pfeldman, 2016-09-02 18:03:51]

With this: https://codereview.chromium.org/2302873005 we don't need that IPS and
devtools plumbing.

[commit-bot: I haz the power, 2016-09-02 18:18:51]

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

[commit-bot: I haz the power, 2016-09-02 18:18:52]

Dry run: This issue passed the CQ dry run.

[jam, 2016-09-02 21:46:16]

jam@chromium.org changed reviewers:
- creis@chromium.org, mbarbella@chromium.org

[jam, 2016-09-02 23:55:17]

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

[commit-bot: I haz the power, 2016-09-02 23:55:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2016-09-03 00:00:23]

jam@chromium.org changed reviewers:
+ tsepez@chromium.org

[jam, 2016-09-03 00:00:24]

+Tom for IPC review. Notes on new fields sent to the renderer:
-cert_status and ssl_connection_status are already sent (in security_info, which
will be removed in a followup)
-certificate is used populate the devtools security panel, and also to show the
dialog when the button in devtools security panel is clicked. in both these
cases, malicious renderers can lie today since certificate IDs are used, and can
be basically be already chosen by the renderer since it can cause new certs to
be added in the cert store and the IDs are sequential

Pavel: I've updated the cl to show certificate using the webui method per your
suggestion.

https://codereview.chromium.org/2296953004/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js
(right):

https://codereview.chromium.org/2296953004/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/security/SecurityPanel.js:337:
WebInspector.SecurityPanel.createCertificateViewerButton = function (text,
certificateId, panel)
On 2016/09/02 17:15:01, pfeldman wrote:
> We don't do space after function.

oops, editor reforamtted. fixed

[Ryan Sleevi, 2016-09-03 00:02:49]

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

[Ryan Sleevi, 2016-09-03 00:02:51]

https://codereview.chromium.org/2296953004/diff/260001/content/child/web_url_...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2296953004/diff/260001/content/child/web_url_...
content/child/web_url_loader_impl.cc:316: DCHECK(rv);
DESIGN: You've removed the code that sanity checked the input (233-240), and
this brings parsing into the renderer. Is it safe to assume this information is
trustworthy? I would think DevTools shouldn't be, but I'm curious your security
model.

On an implementation side, exposing //net/cert/internal is definitely not
something we want to do (hence the name internal). My understanding from
discussing with David is that you'd be perfectly happy with
net::X509Certificate, safe for the fact it hinges on/depends on OSCertHandle at
present, which is not sandbox friendly, and this will be sandboxed.

In trying to examine both your current and future needs, would they be met if
there was a hypothetical net::SandboxedX509Certificate that didn't have the
OSCertHandle connotations (something David and I have already been discussing
how to move away from for the general X509Certificate anyways), which returned
the fields of X509Certificate that were relevant?

I want to make sure I understand the needs correctly (and that seems to be
consistent with what this code is doing), so we can see about offering you a
better alternative than reaching into //net/cert/internal.

https://codereview.chromium.org/2296953004/diff/260001/content/renderer/devto...
File content/renderer/devtools/devtools_agent.cc (right):

https://codereview.chromium.org/2296953004/diff/260001/content/renderer/devto...
content/renderer/devtools/devtools_agent.cc:205: for (auto cert : certificate)
STYLE nit: const auto& ? Do you really intend to be making a copy each time?

(Per the guidance on https://chromium-cpp.appspot.com/ regarding range-based for
loops)

[commit-bot: I haz the power, 2016-09-03 01:31:59]

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

[commit-bot: I haz the power, 2016-09-03 01:32:00]

Dry run: This issue passed the CQ dry run.

[jam, 2016-09-03 01:42:39]

https://codereview.chromium.org/2296953004/diff/260001/content/child/web_url_...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/2296953004/diff/260001/content/child/web_url_...
content/child/web_url_loader_impl.cc:316: DCHECK(rv);
On 2016/09/03 00:02:51, Ryan Sleevi (slow) wrote:
> DESIGN: You've removed the code that sanity checked the input (233-240),

This wasn't sanity check as much as deserialization. The other end is the
browser, so there's no security checking needed for data from it.

> and
> this brings parsing into the renderer. Is it safe to assume this information
is
> trustworthy? I would think DevTools shouldn't be, but I'm curious your
security
> model.

The certificate is sent to the renderer only to be displayed in devtools (i.e.
no security decisions are taken based upon it).

A malicious renderer can cause incorrect certificate information to be shown in
devtools' security panel. Note that this is still possible today, since a
renderer can prime the CertStore with whatever certificates it wants and then
give their IDs (which are just sequential numbers). So from that respect, things
shouldn't be worse than today.

As an aside, note that these serious of patches (culminating in removal of
CertStore) have stopped sending the certificate ID and security style (i.e. lock
icon) through the renderer to the browser which was what was used for the UI.

> 
> On an implementation side, exposing //net/cert/internal is definitely not
> something we want to do (hence the name internal). My understanding from
> discussing with David is that you'd be perfectly happy with
> net::X509Certificate, safe for the fact it hinges on/depends on OSCertHandle
at
> present, which is not sandbox friendly, and this will be sandboxed.
> 
> In trying to examine both your current and future needs, would they be met if
> there was a hypothetical net::SandboxedX509Certificate that didn't have the
> OSCertHandle connotations (something David and I have already been discussing
> how to move away from for the general X509Certificate anyways), which returned
> the fields of X509Certificate that were relevant?
> 
> I want to make sure I understand the needs correctly (and that seems to be
> consistent with what this code is doing), so we can see about offering you a
> better alternative than reaching into //net/cert/internal.

Yes definitely that would be preferable. I originally used X509Certificate in
earlier patchsets but then ran into it not working inside Linux & Mac sandbox.
The former was easy to fix by bringing code that David previously removed, but I
couldn't figure out Mac. I also didn't want future consumers of the network
service to have to change their sandbox just to get whatever data they want from
the certificate.

https://codereview.chromium.org/2296953004/diff/260001/content/renderer/devto...
File content/renderer/devtools/devtools_agent.cc (right):

https://codereview.chromium.org/2296953004/diff/260001/content/renderer/devto...
content/renderer/devtools/devtools_agent.cc:205: for (auto cert : certificate)
On 2016/09/03 00:02:51, Ryan Sleevi (slow) wrote:
> STYLE nit: const auto& ? Do you really intend to be making a copy each time?
> 
> (Per the guidance on https://chromium-cpp.appspot.com/ regarding range-based
for
> loops)

That was an oversight, but now that code is gone after Pavel's change

[Ryan Sleevi, 2016-09-03 02:15:54]

On 2016/09/03 01:42:39, jam wrote:
> Yes definitely that would be preferable. I originally used X509Certificate in
> earlier patchsets but then ran into it not working inside Linux & Mac sandbox.
> The former was easy to fix by bringing code that David previously removed, but
I
> couldn't figure out Mac. I also didn't want future consumers of the network
> service to have to change their sandbox just to get whatever data they want
from
> the certificate.

OK, that's encouraging to hear that it was just the sandbox issue. David and I
met today to discuss a plan to resolve that (and general issues related to
X509Certificate), although the 'clean' solution is unfortunately gated on a few
other changes first. One of these changes is that we're actually not 100%
confident that roundtripping through a string (which removing the CertStore
does) will actually result in the right UI for users on Mac and Windows, due to
additional state that can be added by extensibility components (which we know
exist and are used on Windows, and we've guiltily played with abusing in the
past). I don't think that should *block* this CL, but explains the little
hesitance.

To keep this CL moving, to minimize the work you have to do, and since we don't
yet have our "ideal world" APIs (which would move a lot of this up from Chromium
into BoringSSL), my concrete suggestion would be:
- Let's introduce bool net::ParseCertificateSandboxed(base::StringPiece cert,
CertPrincipal* subject, CertPrincipal* issuer, base::Time* not_before,
base::Time* not_after, std::vector<std::string>* dns_names,
std::vector<std::string>* ip_addresses); 
  - If you're not happy with that, we could just do "struct SandboxedCertificate
{ /* those fields */ }" + "bool ParseSandboxedCertificate(base::StringPiece,
SandboxedCertificate*)"
  - If you're not comfortable/confident doing that, then eroman@, nharper@, or
mattm@ would likely be able to spit it out quickly (barring interruptions). Were
it not for a security incident, I'd offer to do it next Wednesday (OOO Mon/Tues)

This would just be "temporary" until we are able to get OSCertHandle more
thoroughly excised from //net, at which point, you'd be able to move back to
net::X509Certificate regardless of which process you're in.

You're not the first to bump into this re: net::X509Certificate (Chromoting had
the issue for a long time, and we couldn't solve it like the above way I
proposed, but now Chromoting no longer has to worry about that part), so this
might give you an easy path for more mojofication of things that might depend on
net::X509Certificate, until we're able to get it fully sandboxed friendly by
excising the OS dependency.

WDYT?

[Ryan Sleevi, 2016-09-03 02:25:35]

On 2016/09/03 02:15:54, Ryan Sleevi (slow) wrote:
> WDYT?

Ah, I should have clarified, I was imagining we could put this in
//net/cert/x509_util for the time being, just to avoid non-net code depending on
//net/cert/internal (Yes, the Chromecast case is also temporary as more stuff is
being uplifted into //net)

[jam, 2016-09-03 03:05:29]

On 2016/09/03 02:15:54, Ryan Sleevi (slow) wrote:
> On 2016/09/03 01:42:39, jam wrote:
> > Yes definitely that would be preferable. I originally used X509Certificate
in
> > earlier patchsets but then ran into it not working inside Linux & Mac
sandbox.
> > The former was easy to fix by bringing code that David previously removed,
but
> I
> > couldn't figure out Mac. I also didn't want future consumers of the network
> > service to have to change their sandbox just to get whatever data they want
> from
> > the certificate.
> 
> OK, that's encouraging to hear that it was just the sandbox issue. David and I
> met today to discuss a plan to resolve that (and general issues related to
> X509Certificate), although the 'clean' solution is unfortunately gated on a
few
> other changes first. One of these changes is that we're actually not 100%
> confident that roundtripping through a string (which removing the CertStore
> does) will actually result in the right UI for users on Mac and Windows, due
to
> additional state that can be added by extensibility components (which we know
> exist and are used on Windows, and we've guiltily played with abusing in the
> past). I don't think that should *block* this CL, but explains the little
> hesitance.
> 
> To keep this CL moving, to minimize the work you have to do, and since we
don't
> yet have our "ideal world" APIs (which would move a lot of this up from
Chromium
> into BoringSSL), my concrete suggestion would be:
> - Let's introduce bool net::ParseCertificateSandboxed(base::StringPiece cert,
> CertPrincipal* subject, CertPrincipal* issuer, base::Time* not_before,
> base::Time* not_after, std::vector<std::string>* dns_names,
> std::vector<std::string>* ip_addresses); 
>   - If you're not happy with that, we could just do "struct
SandboxedCertificate
> { /* those fields */ }" + "bool ParseSandboxedCertificate(base::StringPiece,
> SandboxedCertificate*)"
>   - If you're not comfortable/confident doing that, then eroman@, nharper@, or
> mattm@ would likely be able to spit it out quickly (barring interruptions).
Were
> it not for a security incident, I'd offer to do it next Wednesday (OOO
Mon/Tues)
> 
> This would just be "temporary" until we are able to get OSCertHandle more
> thoroughly excised from //net, at which point, you'd be able to move back to
> net::X509Certificate regardless of which process you're in.
> 
> You're not the first to bump into this re: net::X509Certificate (Chromoting
had
> the issue for a long time, and we couldn't solve it like the above way I
> proposed, but now Chromoting no longer has to worry about that part), so this
> might give you an easy path for more mojofication of things that might depend
on
> net::X509Certificate, until we're able to get it fully sandboxed friendly by
> excising the OS dependency.
> 
> WDYT?

Regarding the API of what ParseCertificateSandboxed looks like, that's up to you
(I have no preference).

I don't mind moving the code that I wrote to the location you suggest. Given
that this change is ready and is already large, I prefer to do it in a followup
though. It seems harmless to have it for a few days outside of net, and I prefer
not to delay this change any further.

[jam, 2016-09-03 03:06:49]

On 2016/09/03 03:05:29, jam wrote:
> On 2016/09/03 02:15:54, Ryan Sleevi (slow) wrote:
> > On 2016/09/03 01:42:39, jam wrote:
> > > Yes definitely that would be preferable. I originally used X509Certificate
> in
> > > earlier patchsets but then ran into it not working inside Linux & Mac
> sandbox.
> > > The former was easy to fix by bringing code that David previously removed,
> but
> > I
> > > couldn't figure out Mac. I also didn't want future consumers of the
network
> > > service to have to change their sandbox just to get whatever data they
want
> > from
> > > the certificate.
> > 
> > OK, that's encouraging to hear that it was just the sandbox issue. David and
I
> > met today to discuss a plan to resolve that (and general issues related to
> > X509Certificate), although the 'clean' solution is unfortunately gated on a
> few
> > other changes first. One of these changes is that we're actually not 100%
> > confident that roundtripping through a string (which removing the CertStore
> > does) will actually result in the right UI for users on Mac and Windows, due
> to
> > additional state that can be added by extensibility components (which we
know
> > exist and are used on Windows, and we've guiltily played with abusing in the
> > past). I don't think that should *block* this CL, but explains the little
> > hesitance.
> > 
> > To keep this CL moving, to minimize the work you have to do, and since we
> don't
> > yet have our "ideal world" APIs (which would move a lot of this up from
> Chromium
> > into BoringSSL), my concrete suggestion would be:
> > - Let's introduce bool net::ParseCertificateSandboxed(base::StringPiece
cert,
> > CertPrincipal* subject, CertPrincipal* issuer, base::Time* not_before,
> > base::Time* not_after, std::vector<std::string>* dns_names,
> > std::vector<std::string>* ip_addresses); 
> >   - If you're not happy with that, we could just do "struct
> SandboxedCertificate
> > { /* those fields */ }" + "bool ParseSandboxedCertificate(base::StringPiece,
> > SandboxedCertificate*)"
> >   - If you're not comfortable/confident doing that, then eroman@, nharper@,
or
> > mattm@ would likely be able to spit it out quickly (barring interruptions).
> Were
> > it not for a security incident, I'd offer to do it next Wednesday (OOO
> Mon/Tues)
> > 
> > This would just be "temporary" until we are able to get OSCertHandle more
> > thoroughly excised from //net, at which point, you'd be able to move back to
> > net::X509Certificate regardless of which process you're in.
> > 
> > You're not the first to bump into this re: net::X509Certificate (Chromoting
> had
> > the issue for a long time, and we couldn't solve it like the above way I
> > proposed, but now Chromoting no longer has to worry about that part), so
this
> > might give you an easy path for more mojofication of things that might
depend
> on
> > net::X509Certificate, until we're able to get it fully sandboxed friendly by
> > excising the OS dependency.
> > 
> > WDYT?
> 
> Regarding the API of what ParseCertificateSandboxed looks like, that's up to
you
> (I have no preference).
> 
> I don't mind moving the code that I wrote to the location you suggest. Given
> that this change is ready and is already large, I prefer to do it in a
followup
> though. It seems harmless to have it for a few days outside of net, and I
prefer
> not to delay this change any further.

(but also, I can do it in this cl as long as no review needed (i.e. if it's just
a straight move of the existing code. that way it doesn't delay this cl waiting
for reviews).

[jam, 2016-09-03 03:14:42]

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

[commit-bot: I haz the power, 2016-09-03 03:14:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-03 05:08:59]

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

[commit-bot: I haz the power, 2016-09-03 05:09:01]

Dry run: This issue passed the CQ dry run.

[jam, 2016-09-03 06:18:19]

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

[commit-bot: I haz the power, 2016-09-03 06:18:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2016-09-03 07:30:49]

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

[commit-bot: I haz the power, 2016-09-03 07:30:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-03 07:41:25]

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

[commit-bot: I haz the power, 2016-09-03 07:41:26]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[jam, 2016-09-03 17:09:05]

Patchset #13 (id:340001) has been deleted

[jam, 2016-09-03 17:09:06]

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

[commit-bot: I haz the power, 2016-09-03 17:09:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-03 18:12:17]

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

[commit-bot: I haz the power, 2016-09-03 18:12:18]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jam, 2016-09-04 04:02:22]

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

[commit-bot: I haz the power, 2016-09-04 04:02:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2016-09-04 04:05:39]

Patchset #13 (id:360001) has been deleted

[commit-bot: I haz the power, 2016-09-04 05:34:17]

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

[commit-bot: I haz the power, 2016-09-04 05:34:18]

Dry run: This issue passed the CQ dry run.

[jam, 2016-09-06 15:04:32]

jam@chromium.org changed reviewers:
+ davidben@chromium.org

[jam, 2016-09-06 15:04:33]

+davidben for net/ changes
David: Ryan asked for the certificate parsing code which uses net/cert/internal
to be moved to net/, can you ptal since he's ooo today? thanks

[jam, 2016-09-06 15:05:16]

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

[commit-bot: I haz the power, 2016-09-06 15:05:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[davidben, 2016-09-06 17:21:46]

net lgtm modulo small comments.

https://codereview.chromium.org/2296953004/diff/380001/net/cert/x509_util.cc
File net/cert/x509_util.cc (right):

https://codereview.chromium.org/2296953004/diff/380001/net/cert/x509_util.cc#...
net/cert/x509_util.cc:23: bool GetCommonName(const net::der::Input& tlv,
std::string* common_name) {
No need for net:: prefix since we're already in net. Ditto thorough the file.

https://codereview.chromium.org/2296953004/diff/380001/net/cert/x509_util.cc#...
net/cert/x509_util.cc:28: for (const net::RelativeDistinguishedName& rdn :
rdn_sequence) {
Any reason to use auto some of the time and the type name others? (I don't have
much sense of which is the prevailing style.

https://codereview.chromium.org/2296953004/diff/380001/net/cert/x509_util.cc#...
net/cert/x509_util.cc:48: }
Nit: newline

https://codereview.chromium.org/2296953004/diff/380001/net/cert/x509_util.cc#...
net/cert/x509_util.cc:49: }
Nit: // namespace

https://codereview.chromium.org/2296953004/diff/380001/net/cert/x509_util.cc#...
net/cert/x509_util.cc:124: net::der::Input cert_data(certificate);
#include "net/der/input.h"

https://codereview.chromium.org/2296953004/diff/380001/net/cert/x509_util.cc#...
net/cert/x509_util.cc:126: net::der::BitString signature_value;
#include "net/der/parse_values.h"

https://codereview.chromium.org/2296953004/diff/380001/net/cert/x509_util.cc#...
net/cert/x509_util.cc:129: if (!rv)
Nit: Rather than bool rv, could just write

if (!ParseCertifificate(...))
  return false;

Ditto below.

https://codereview.chromium.org/2296953004/diff/380001/net/cert/x509_util.cc#...
net/cert/x509_util.cc:158: if (extensions.find(net::SubjectAltNameOid()) !=
extensions.end()) {
Not that it matters, but may as well save a lookup and write:

auto iter = extensions.find(SubjectAltNameOid());
if (iter != extensions.end()) {
  ...CreateFromDer(iter->second.value)...

https://codereview.chromium.org/2296953004/diff/380001/net/cert/x509_util.h
File net/cert/x509_util.h (right):

https://codereview.chromium.org/2296953004/diff/380001/net/cert/x509_util.h#n...
net/cert/x509_util.h:78: // Provides a method to parse a DER-encoded X509
certificate without calling any
Nit: X509 -> X.509, also I think we usually write these like "Parses a
DER-encoded X.509 certificate [...]".

[commit-bot: I haz the power, 2016-09-06 17:28:50]

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

[commit-bot: I haz the power, 2016-09-06 17:28:51]

Dry run: This issue passed the CQ dry run.

[jam, 2016-09-06 17:43:24]

https://codereview.chromium.org/2296953004/diff/380001/net/cert/x509_util.cc
File net/cert/x509_util.cc (right):

https://codereview.chromium.org/2296953004/diff/380001/net/cert/x509_util.cc#...
net/cert/x509_util.cc:23: bool GetCommonName(const net::der::Input& tlv,
std::string* common_name) {
On 2016/09/06 17:21:45, davidben wrote:
> No need for net:: prefix since we're already in net. Ditto thorough the file.

Done.

https://codereview.chromium.org/2296953004/diff/380001/net/cert/x509_util.cc#...
net/cert/x509_util.cc:28: for (const net::RelativeDistinguishedName& rdn :
rdn_sequence) {
On 2016/09/06 17:21:45, davidben wrote:
> Any reason to use auto some of the time and the type name others? (I don't
have
> much sense of which is the prevailing style.

no reason, switched to auto

https://codereview.chromium.org/2296953004/diff/380001/net/cert/x509_util.cc#...
net/cert/x509_util.cc:48: }
On 2016/09/06 17:21:45, davidben wrote:
> Nit: newline

Done.

https://codereview.chromium.org/2296953004/diff/380001/net/cert/x509_util.cc#...
net/cert/x509_util.cc:49: }
On 2016/09/06 17:21:45, davidben wrote:
> Nit: // namespace

Done.

https://codereview.chromium.org/2296953004/diff/380001/net/cert/x509_util.cc#...
net/cert/x509_util.cc:124: net::der::Input cert_data(certificate);
On 2016/09/06 17:21:46, davidben wrote:
> #include "net/der/input.h"

Done.

https://codereview.chromium.org/2296953004/diff/380001/net/cert/x509_util.cc#...
net/cert/x509_util.cc:126: net::der::BitString signature_value;
On 2016/09/06 17:21:46, davidben wrote:
> #include "net/der/parse_values.h"

Done.

https://codereview.chromium.org/2296953004/diff/380001/net/cert/x509_util.cc#...
net/cert/x509_util.cc:129: if (!rv)
On 2016/09/06 17:21:45, davidben wrote:
> Nit: Rather than bool rv, could just write
> 
> if (!ParseCertifificate(...))
>   return false;
> 
> Ditto below.

Done.

https://codereview.chromium.org/2296953004/diff/380001/net/cert/x509_util.cc#...
net/cert/x509_util.cc:158: if (extensions.find(net::SubjectAltNameOid()) !=
extensions.end()) {
On 2016/09/06 17:21:45, davidben wrote:
> Not that it matters, but may as well save a lookup and write:
> 
> auto iter = extensions.find(SubjectAltNameOid());
> if (iter != extensions.end()) {
>   ...CreateFromDer(iter->second.value)...

Done.

[jam, 2016-09-06 17:45:47]

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

[commit-bot: I haz the power, 2016-09-06 17:46:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pfeldman, 2016-09-06 17:50:11]

lgtm

https://codereview.chromium.org/2296953004/diff/380001/content/browser/devtoo...
File content/browser/devtools/protocol/network_handler.cc (left):

https://codereview.chromium.org/2296953004/diff/380001/content/browser/devtoo...
content/browser/devtools/protocol/network_handler.cc:25: #include
"net/cert/x509_cert_types.h"
I guess those are no longer needed.

https://codereview.chromium.org/2296953004/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/security/SecurityModel.js
(right):

https://codereview.chromium.org/2296953004/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/security/SecurityModel.js:27:
showCertificateViewer: function(certificateId)
/** annotation missing.

[Tom Sepez, 2016-09-06 17:56:11]

messages LGTM

[jam, 2016-09-06 18:00:34]

https://codereview.chromium.org/2296953004/diff/380001/content/browser/devtoo...
File content/browser/devtools/protocol/network_handler.cc (left):

https://codereview.chromium.org/2296953004/diff/380001/content/browser/devtoo...
content/browser/devtools/protocol/network_handler.cc:25: #include
"net/cert/x509_cert_types.h"
On 2016/09/06 17:50:11, pfeldman wrote:
> I guess those are no longer needed.

Done.

https://codereview.chromium.org/2296953004/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/security/SecurityModel.js
(right):

https://codereview.chromium.org/2296953004/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/security/SecurityModel.js:27:
showCertificateViewer: function(certificateId)
On 2016/09/06 17:50:11, pfeldman wrote:
> /** annotation missing.

Done.

[jam, 2016-09-06 18:01:53]

The CQ bit was checked by jam@chromium.org

[jam, 2016-09-06 18:01:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org, tsepez@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2296953004/#ps420001
(title: "review comments")

[commit-bot: I haz the power, 2016-09-06 18:02:58]

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

[jam, 2016-09-06 18:17:25]

The CQ bit was checked by jam@chromium.org

[jam, 2016-09-06 18:17:25]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
davidben@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2296953004/#ps440001
(title: "self review")

[commit-bot: I haz the power, 2016-09-06 18:18:10]

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

[commit-bot: I haz the power, 2016-09-06 20:26:03]

Description was changed from

==========
Send certificates to devtools when it's open instead of using serialized
SSLStatus and sending certificate IDs.

This is along the earlier patchsets of stopping to send SSLStatus to the
renderer. This will help network service (since currently it's what sends this
data, and it shouldn't know about UI state) and PlzNavigate (since this is a
precursor for removing cert_ids which depend on having a renderer process during
navigate).

Since this patch has gotten large, I'll remove sending sending security_info to
the renderer in a followup patch.

BUG=598073, 504347
==========

to

==========
Send certificates to devtools when it's open instead of using serialized
SSLStatus and sending certificate IDs.

This is along the earlier patchsets of stopping to send SSLStatus to the
renderer. This will help network service (since currently it's what sends this
data, and it shouldn't know about UI state) and PlzNavigate (since this is a
precursor for removing cert_ids which depend on having a renderer process during
navigate).

Since this patch has gotten large, I'll remove sending sending security_info to
the renderer in a followup patch.

BUG=598073, 504347
==========

[commit-bot: I haz the power, 2016-09-06 20:26:06]

Committed patchset #16 (id:440001)

[commit-bot: I haz the power, 2016-09-06 20:29:05]

Description was changed from

==========
Send certificates to devtools when it's open instead of using serialized
SSLStatus and sending certificate IDs.

This is along the earlier patchsets of stopping to send SSLStatus to the
renderer. This will help network service (since currently it's what sends this
data, and it shouldn't know about UI state) and PlzNavigate (since this is a
precursor for removing cert_ids which depend on having a renderer process during
navigate).

Since this patch has gotten large, I'll remove sending sending security_info to
the renderer in a followup patch.

BUG=598073, 504347
==========

to

==========
Send certificates to devtools when it's open instead of using serialized
SSLStatus and sending certificate IDs.

This is along the earlier patchsets of stopping to send SSLStatus to the
renderer. This will help network service (since currently it's what sends this
data, and it shouldn't know about UI state) and PlzNavigate (since this is a
precursor for removing cert_ids which depend on having a renderer process during
navigate).

Since this patch has gotten large, I'll remove sending sending security_info to
the renderer in a followup patch.

BUG=598073, 504347

Committed: https://crrev.com/8b3813b7f3ac9e197305cc440b78c81773cd07af
Cr-Commit-Position: refs/heads/master@{#416700}
==========

[commit-bot: I haz the power, 2016-09-06 20:29:08]

Patchset 16 (id:??) landed as
https://crrev.com/8b3813b7f3ac9e197305cc440b78c81773cd07af
Cr-Commit-Position: refs/heads/master@{#416700}