Adding <keygen> Content Setting (core)

Adding <keygen> Content Setting (core)

Adding the KEYGEN content setting type and core functionality.

---

The purpose of the content setting is to disable <keygen> usage by default, while allowing user and enterprise settings to selectively enable <keygen>. This is early UI gating prior to the deprecation of <keygen>.

From UX discussions, we'd like to use a Page Action presented on the form rendering in order to allow the user to change the key generation settings of the page. The content setting will then determine whether the OnKeygen call actually runs the crypto to generate and store the keys.

blink-dev Thread:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/pX5NbX0Xack/kmHsyMGJZAMJ

---

BUG=514767
Committed: https://crrev.com/4d3f6bf5aeed76f36cf5982718be9e8bc6cc33ee
Cr-Commit-Position: refs/heads/master@{#359950}

[svaldez, 2015-11-03 15:38:20]

svaldez@chromium.org changed reviewers:
+ jochen@chromium.org, raymes@chromium.org, rsleevi@chromium.org

[jochen (gone - plz use gerrit), 2015-11-05 01:16:11]

which parts do you want me to review?

[raymes, 2015-11-05 03:15:47]

raymes@chromium.org changed reviewers:
+ msramek@chromium.org
- jochen@chromium.org

[raymes, 2015-11-05 03:15:47]

https://codereview.chromium.org/1417173010/diff/20001/components/content_sett...
File
components/content_settings/core/browser/content_settings_policy_provider.cc
(right):

https://codereview.chromium.org/1417173010/diff/20001/components/content_sett...
components/content_settings/core/browser/content_settings_policy_provider.cc:45:
CONTENT_SETTINGS_TYPE_KEYGEN, CONTENT_SETTING_ALLOW},
Do we really want to add enterprise management for this setting?

https://codereview.chromium.org/1417173010/diff/20001/components/content_sett...
File components/content_settings/core/common/content_settings_types.h (right):

https://codereview.chromium.org/1417173010/diff/20001/components/content_sett...
components/content_settings/core/common/content_settings_types.h:24:
CONTENT_SETTINGS_TYPE_KEYGEN,
nit: we tend to just add new ones at the end

[Ryan Sleevi, 2015-11-05 03:27:47]

https://codereview.chromium.org/1417173010/diff/20001/components/content_sett...
File
components/content_settings/core/browser/content_settings_policy_provider.cc
(right):

https://codereview.chromium.org/1417173010/diff/20001/components/content_sett...
components/content_settings/core/browser/content_settings_policy_provider.cc:45:
CONTENT_SETTINGS_TYPE_KEYGEN, CONTENT_SETTING_ALLOW},
On 2015/11/05 03:15:47, raymes wrote:
> Do we really want to add enterprise management for this setting?

Forwarded thread. Internal enterprise use is the key use case, so yeah.

Note that <keygen> is a special snowflake. It's not a JS feature - it's
something that goes in the form submission. It's done as a blocking call from
Blink renderer to the browser process, due to WebKit awfulness related to forms
processing that would have been even more painful. So we can't just treat it
like, say, window.alert - with a discrete intent on a current page - since the
actual 'scary' part of <keygen> always happens as part of a navigation (form
submission).

So instead, when a <keygen> tag is encountered, it surfaces the UI that says
"Hey, this is going to break", and if the user doesn't accept it before they
attempt to navigate, then it breaks, just like it indicated.

However, this requiring user interaction breaks the enterprise use case, which
often do a multi-phase navigation where they bounce w/ an autosubmitting form to
a processing page, which then does further stuff. A concrete example would be
some of the stuff that Google has deployed inhouse, and which relies on that
transparency (if you've used a Chromebook at all at Google, you've run in to
this use case).

Hence the Enterprise escape clause until the Enterprise systems (including a
number of whitebox solutions offered by CA vendors and Microsoft) can adapt or
until the full *removal* of the <keygen> tag happens.

[Bernhard Bauer, 2015-11-05 11:20:13]

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

[Bernhard Bauer, 2015-11-05 11:20:14]

https://codereview.chromium.org/1417173010/diff/20001/components/content_sett...
File components/content_settings/core/common/content_settings.h (right):

https://codereview.chromium.org/1417173010/diff/20001/components/content_sett...
components/content_settings/core/common/content_settings.h:59:
ContentSettingsForOneType keygen_rules;
You only need these rules for content settings that we check directly in the
renderer to avoid IPC roundtrips to the browser (image and script falls under
this, because those are checked very often). See also my comments in
https://codereview.chromium.org/1417033010/.

[svaldez, 2015-11-05 16:30:26]

https://codereview.chromium.org/1417173010/diff/20001/components/content_sett...
File components/content_settings/core/common/content_settings.h (right):

https://codereview.chromium.org/1417173010/diff/20001/components/content_sett...
components/content_settings/core/common/content_settings.h:59:
ContentSettingsForOneType keygen_rules;
On 2015/11/05 11:20:14, Bernhard Bauer wrote:
> You only need these rules for content settings that we check directly in the
> renderer to avoid IPC roundtrips to the browser (image and script falls under
> this, because those are checked very often). See also my comments in
> https://codereview.chromium.org/1417033010/.

Done.

https://codereview.chromium.org/1417173010/diff/20001/components/content_sett...
File components/content_settings/core/common/content_settings_types.h (right):

https://codereview.chromium.org/1417173010/diff/20001/components/content_sett...
components/content_settings/core/common/content_settings_types.h:24:
CONTENT_SETTINGS_TYPE_KEYGEN,
On 2015/11/05 03:15:47, raymes wrote:
> nit: we tend to just add new ones at the end

Done.

[svaldez, 2015-11-05 16:40:48]

Description was changed from

==========
Adding <keygen> Content Setting (core)

Adding the KEYGEN content setting type and core functionality.

BUG=514767
==========

to

==========
Adding <keygen> Content Setting (core)

Adding the KEYGEN content setting type and core functionality.

---

The purpose of the content setting is to disable <keygen> usage by default,
while allowing user and enterprise settings to selectively enable <keygen>. This
is early UI gating prior to the deprecation of <keygen>.

From UX discussions, we'd like to use a Page Action presented on the form
rendering in order to allow the user to change the key generation settings of
the page. The content setting will then determine whether the OnKeygen call
actually runs the crypto to generate and store the keys.


blink-dev Thread:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/pX5NbX0Xack/kmHsyMGJ...

---

BUG=514767
==========

[msramek, 2015-11-05 17:07:09]

https://codereview.chromium.org/1417173010/diff/40001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/1417173010/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:59:
EXPECT_EQ(CONTENT_SETTING_BLOCK,
nit: Please keep the same ordering as in ContentSettingsType (here and
elsewhere)

https://codereview.chromium.org/1417173010/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/content_settings/content_settings_apitest.cc
(right):

https://codereview.chromium.org/1417173010/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_apitest.cc:95:
EXPECT_EQ(
It doesn't make much sense to test the effect of extensions, as you did not
expose this content setting to extensions' API.

That is something that can be easily done in a separate CL, so please remove
this file.

https://codereview.chromium.org/1417173010/diff/40001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1417173010/diff/40001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:9373: <owner>Please list the metric's owners.
Add more owner tags as needed.</owner>
Please list the metric's owners! :)

[svaldez, 2015-11-05 17:37:51]

https://codereview.chromium.org/1417173010/diff/40001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/1417173010/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:59:
EXPECT_EQ(CONTENT_SETTING_BLOCK,
On 2015/11/05 17:07:09, msramek wrote:
> nit: Please keep the same ordering as in ContentSettingsType (here and
> elsewhere)

Done.

https://codereview.chromium.org/1417173010/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/content_settings/content_settings_apitest.cc
(right):

https://codereview.chromium.org/1417173010/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_apitest.cc:95:
EXPECT_EQ(
On 2015/11/05 17:07:09, msramek wrote:
> It doesn't make much sense to test the effect of extensions, as you did not
> expose this content setting to extensions' API.
> 
> That is something that can be easily done in a separate CL, so please remove
> this file.

Done.

https://codereview.chromium.org/1417173010/diff/40001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1417173010/diff/40001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:9373: <owner>Please list the metric's owners.
Add more owner tags as needed.</owner>
On 2015/11/05 17:07:09, msramek wrote:
> Please list the metric's owners! :)

Done.

[Bernhard Bauer, 2015-11-06 11:36:53]

LGTM w/ nits:

https://codereview.chromium.org/1417173010/diff/60001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/1417173010/diff/60001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:839: //
Remove managed-default-content-settings-preferences.
Nit: there are way too many dashes here 😉

https://codereview.chromium.org/1417173010/diff/60001/components/content_sett...
File
components/content_settings/core/browser/content_settings_default_provider.cc
(right):

https://codereview.chromium.org/1417173010/diff/60001/components/content_sett...
components/content_settings/core/browser/content_settings_default_provider.cc:198:
"ContentSettings.DefaultKeygenSetting",
Do you need to add this histogram to histograms.xml as well?

[svaldez, 2015-11-06 14:40:45]

https://codereview.chromium.org/1417173010/diff/60001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/1417173010/diff/60001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:839: //
Remove managed-default-content-settings-preferences.
On 2015/11/06 11:36:53, Bernhard Bauer wrote:
> Nit: there are way too many dashes here 😉

Done.

https://codereview.chromium.org/1417173010/diff/60001/components/content_sett...
File
components/content_settings/core/browser/content_settings_default_provider.cc
(right):

https://codereview.chromium.org/1417173010/diff/60001/components/content_sett...
components/content_settings/core/browser/content_settings_default_provider.cc:198:
"ContentSettings.DefaultKeygenSetting",
On 2015/11/06 11:36:53, Bernhard Bauer wrote:
> Do you need to add this histogram to histograms.xml as well?

Done.

[msramek, 2015-11-06 16:56:15]

https://codereview.chromium.org/1417173010/diff/100001/chrome/browser/content...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/1417173010/diff/100001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:183:
EXPECT_EQ(CONTENT_SETTING_BLOCK,
nit: This test would be better if you tested ALLOW instead. BLOCK is the default
value, so even if SetContentSetting failed, this test would still pass.

https://codereview.chromium.org/1417173010/diff/100001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:603:
std::string(), CONTENT_SETTING_BLOCK);
Like above: To test that SetContentSetting works, use a nondefault value (i.e.
ALLOW).

The idea of this test is that the value set for "pattern" is valid for
[host_ending_with_dot, host_ending_with_dot]. Even if this failed, your test
would miss it, as BLOCK is the default value.

https://codereview.chromium.org/1417173010/diff/100001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:834: new
base::FundamentalValue(CONTENT_SETTING_BLOCK));
Ditto as above.

https://codereview.chromium.org/1417173010/diff/100001/components/content_set...
File
components/content_settings/core/browser/content_settings_policy_provider.cc
(right):

https://codereview.chromium.org/1417173010/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_policy_provider.cc:90:
prefs::kManagedDefaultKeygenSetting},
nit: This fits on a line.

https://codereview.chromium.org/1417173010/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1417173010/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:4960: +  <summary>The default setting
for using key generation in forms.</summary>
nit: Please note when this metric is measured (see above and below: "at profile
open.")

[svaldez, 2015-11-06 21:30:07]

https://codereview.chromium.org/1417173010/diff/100001/chrome/browser/content...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/1417173010/diff/100001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:183:
EXPECT_EQ(CONTENT_SETTING_BLOCK,
On 2015/11/06 16:56:15, msramek wrote:
> nit: This test would be better if you tested ALLOW instead. BLOCK is the
default
> value, so even if SetContentSetting failed, this test would still pass.

Done.

https://codereview.chromium.org/1417173010/diff/100001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:603:
std::string(), CONTENT_SETTING_BLOCK);
On 2015/11/06 16:56:15, msramek wrote:
> Like above: To test that SetContentSetting works, use a nondefault value (i.e.
> ALLOW).
> 
> The idea of this test is that the value set for "pattern" is valid for
> [host_ending_with_dot, host_ending_with_dot]. Even if this failed, your test
> would miss it, as BLOCK is the default value.

Done.

https://codereview.chromium.org/1417173010/diff/100001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:834: new
base::FundamentalValue(CONTENT_SETTING_BLOCK));
On 2015/11/06 16:56:15, msramek wrote:
> Ditto as above.

Done.

https://codereview.chromium.org/1417173010/diff/100001/components/content_set...
File
components/content_settings/core/browser/content_settings_policy_provider.cc
(right):

https://codereview.chromium.org/1417173010/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_policy_provider.cc:90:
prefs::kManagedDefaultKeygenSetting},
On 2015/11/06 16:56:15, msramek wrote:
> nit: This fits on a line.

Done.

https://codereview.chromium.org/1417173010/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1417173010/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:4960: +  <summary>The default setting
for using key generation in forms.</summary>
On 2015/11/06 16:56:15, msramek wrote:
> nit: Please note when this metric is measured (see above and below: "at
profile
> open.")

Done.

[msramek, 2015-11-09 10:51:34]

LGTM

[svaldez, 2015-11-11 15:33:05]

svaldez@chromium.org changed reviewers:
+ davidben@chromium.org, jwd@chromium.org

[svaldez, 2015-11-11 15:33:06]

Hi,

This is part of a set of CLs to add a key generation content setting in
preparation for the <keygen>/x-x509-user-cert deprecation. Could you take a look
at the files you have ownership on and comment/LGTM?

davidben: content/
jwd: tools/metrics/

[jwd, 2015-11-11 19:21:15]

https://codereview.chromium.org/1417173010/diff/140001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1417173010/diff/140001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:9412: <action
name="Options_DefaultKeygenSettingChanged">
How/where does this get logged?

[svaldez, 2015-11-11 20:26:27]

https://codereview.chromium.org/1417173010/diff/140001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1417173010/diff/140001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:9412: <action
name="Options_DefaultKeygenSettingChanged">
On 2015/11/11 19:21:15, Jesse Doherty wrote:
> How/where does this get logged?

This currently gets logged as part of another Keygen CL
(https://codereview.chromium.org/1412523003/). Is it reasonable to have it
split, or should this file be moved over to the UI CL?

[jwd, 2015-11-11 22:07:24]

https://codereview.chromium.org/1417173010/diff/140001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1417173010/diff/140001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:9412: <action
name="Options_DefaultKeygenSettingChanged">
On 2015/11/11 20:26:27, svaldez wrote:
> On 2015/11/11 19:21:15, Jesse Doherty wrote:
> > How/where does this get logged?
> 
> This currently gets logged as part of another Keygen CL
> (https://codereview.chromium.org/1412523003/). Is it reasonable to have it
> split, or should this file be moved over to the UI CL?

I'd prefer it be in the same CL.

[svaldez, 2015-11-12 18:02:59]

https://codereview.chromium.org/1417173010/diff/140001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1417173010/diff/140001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:9412: <action
name="Options_DefaultKeygenSettingChanged">
On 2015/11/11 22:07:24, Jesse Doherty wrote:
> On 2015/11/11 20:26:27, svaldez wrote:
> > On 2015/11/11 19:21:15, Jesse Doherty wrote:
> > > How/where does this get logged?
> > 
> > This currently gets logged as part of another Keygen CL
> > (https://codereview.chromium.org/1412523003/). Is it reasonable to have it
> > split, or should this file be moved over to the UI CL?
> 
> I'd prefer it be in the same CL.

Removed from this CL.

[jwd, 2015-11-12 19:24:27]

Metics LGTM

[davidben, 2015-11-16 21:23:59]

content lgtm. (Didn't look closely at the rest. I'm assuming someone else did
the primary review.)

[svaldez, 2015-11-16 21:24:11]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2015-11-16 21:24:11]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
msramek@chromium.org
Link to the patchset: https://codereview.chromium.org/1417173010/#ps160001
(title: "Moving actions.xml.")

[commit-bot: I haz the power, 2015-11-16 21:25:43]

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

[commit-bot: I haz the power, 2015-11-16 21:38:17]

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

[commit-bot: I haz the power, 2015-11-16 21:38:19]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[svaldez, 2015-11-16 21:53:13]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2015-11-16 21:53:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
msramek@chromium.org, bauerb@chromium.org, davidben@chromium.org,
jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/1417173010/#ps180001
(title: "Fixing ContentSetting Register.")

[commit-bot: I haz the power, 2015-11-16 21:54:07]

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

[commit-bot: I haz the power, 2015-11-16 23:14:54]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-11-16 23:15:49]

Patchset 10 (id:??) landed as
https://crrev.com/4d3f6bf5aeed76f36cf5982718be9e8bc6cc33ee
Cr-Commit-Position: refs/heads/master@{#359950}