New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove <keygen> #2221

Merged
merged 5 commits into from Jan 13, 2017

Conversation

5 participants
@domenic
Member

domenic commented Dec 28, 2016

This removes the <keygen> element entirely from the specification, with
the exception of retaining the parser behavior. Closes #2079.

/cc @sleevi, especially to check if I got the suggested replacements right:

For enterprise device management use cases, use native on-device management capabilities.

For certificate enrollment use cases, use the Web Cryptography API to generate the certificate and then allow the user to install the generated certificate manually. [WEBCRYPTO]

Web platform tests: web-platform-tests/wpt#4402

Remove <keygen>
This removes the <keygen> element entirely from the specification, with
the exception of retaining the parser behavior. Closes #2079.
source Outdated
<dd>
<p>For enterprise device management use cases, use native on-device management capabilities.</p>
<p>For certificate enrollment use cases, use the Web Cryptography API to generate the certificate and then allow the

This comment has been minimized.

@sleevi

sleevi Dec 28, 2016

This may be conflating handling of application/x-x509-user-cert (installing certs) and the behaviour (generating keys)

For certificate enrollment cases, use the Web Cryptography API to generate a keypair for the certificate, and then export the certificate and key to allow the user to manually install them into the desired application or operating system store.

That feels very clunkily worded, and perhaps the semantic game is overkill; your original text is arguably fine, just the distinction between Web Crypto API doesn't give you any certificate capabilities (but you can build them using it - e.g. as PKIjs demonstrates), but it does give you the necessary key primitives (generateKey, sign) for provisioning or installation (exportKey)

@sleevi

This comment has been minimized.

sleevi commented Dec 28, 2016

So to further capture what's implemented (now) in Blink, and what I haven't yet tested in Edge (no Edge device handy):

  • We treat keygen as forbidding innerHTML, outerHTML, and createContextualFragment (due to IE forbidding it). This results in a NoModificationAllowedException if you try to setInnerText & setOuterText,
  • We have some special logic in the parser (which I'm totally game for cleaning up), which
    • If you're in the context of a <select> tag, it closes that scope and begins the <keygen> scope (input and textarea explicitly share this behaviour. It also registers as a parse error, FWIW. select elements will end the current tag and generate a parse error, but won't generate a new select scope)
    • <keygen> is treated as a self-closing element (along with area, br, img, wbr, and embed)

I don't believe any of those behaviours are desirable, but because they fundamentally affected the parsing algorithm, I deferred those changes until we could see other vendor consensus here.

@domenic

This comment has been minimized.

Member

domenic commented Dec 28, 2016

In general we've kept the parser logic for previous changes like this (including Chrome's applet removal).

The forbidding of innerHTML/outerHTML/etc. is very interesting and doesn't seem to match any spec I know of. I can't reproduce this behavior in either Chrome Canary or in Edge: http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4762

I guess I can reproduce it in Chrome Canary for innerText: http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4763 but not in Edge.

@sleevi

This comment has been minimized.

sleevi commented Dec 28, 2016

I only checked code, not behaviour, so thanks @domenic ;)

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTMLElement.cpp?rcl=0&l=138 was the first claim, and while it may have originally forbid those, now it just seems to be innerText/outerText

WebKit does not share this behaviour, although it shares the same implementation.

It seems Chrome introduced it in https://chromium.googlesource.com/chromium/src/+/fa742b7351d8d751e9c4fd2602ec0b1cf2913086 for and , due to https://html.spec.whatwg.org/multipage/syntax.html#serialising-html-fragments

Since your change touches on this behaviour (at least, if I'm reading the diff correctly), by removing it from the list in the HTML serializer (which, AFAICT, WebKit didn't implement for <keygen>), then I can likely safely remove this behaviour from Chrome and implementations can be aligned. But at least worth noting on this PR as an area of spec deviance (either WebKit under spec-today or Chrome under spec-tomorrow, AFAICT)

domenic added a commit to web-platform-tests/wpt that referenced this pull request Dec 28, 2016

@domenic

This comment has been minimized.

Member

domenic commented Dec 28, 2016

Hmm, I'm still kind of confused. I think that section of the spec is just saying that keygen serializes as a void element, right? And I don't believe I removed that in this diff. I also don't understand what that would have to do with the innerText/outerText setters, which should be related to parsing, not serialization...

In any case, that ieForbidsInsertHTML function is fascinating... probably it should just be removed, as Edge at least doesn't seem to follow it anymore.

@sleevi

This comment has been minimized.

sleevi commented Dec 28, 2016

@domenic From at least trying to read the code and track its archaeology (for the removal... Chesterton's fences and all), from what I can tell, the original function was introduced to indicate that tags were self-closing & lacked an end-tag. This then ended up being used by the serializer to indicate when to serialize it sans closing tag (I believe you refer to that as void element). By virtue of having no closing tag, it also doesn't allow serialization of children.

Since it doesn't allow serialization of children, it seems the function was also assumed (as the comment notes, no tests) to also prevent innerHTML/outerHTML, so that no children could be added - essentially, to allow serialization to round-trip. (If you could set innerHTML, those child nodes wouldn't be serialized, would they?)

I thought your diff was touching on that list, and removing <keygen> from those that serialize void, but I guess I misread it (ahh the joy of giant diffs). But that's, at least, the logic behind the Blink and Chrome implementations. I suppose http://w3c-test.org/html/syntax/serializing-html-fragments/outerHTML.html is the useful/relevant test here.

@domenic

This comment has been minimized.

Member

domenic commented Dec 28, 2016

Ah OK, makes sense. I guess it shouldn't impact this removal PR, but I can do some interop testing and file a separate Chrome issue if it happens to be the only browser left with this innerText/outerText blocklist.

domenic added a commit to web-platform-tests/wpt that referenced this pull request Dec 28, 2016

Test that innerText can be set on a variety of elements
As discovered and discussed in whatwg/html#2221 (comment) , Chrome has a blocklist here that seems historical and not supported by any spec.
source Outdated
<code>mtext</code></span>, and <span>MathML <code>annotation-xml</code></span>; and <span>SVG
<code>foreignObject</code></span>, <span>SVG <code>desc</code></span>, and <span>SVG
<code>title</code></span>.</p> <!-- we could actually put all non-HTML elements in this list, I
think -->

This comment has been minimized.

@annevk

annevk Dec 29, 2016

Member

This change seems like a bugfix. Should we land that separately?

This comment has been minimized.

@domenic

domenic Dec 29, 2016

Member

Yeah probably

This comment has been minimized.

@annevk

annevk Dec 29, 2016

Member

I guess it's okay as long as you note it in the commit comment.

This comment has been minimized.

@domenic

domenic Dec 29, 2016

Member

Separated into #2224 ; might benefit from more careful review anyway since this special elements list has been tricky in the past

domenic added some commits Dec 29, 2016

Remove keygen from void elements list
Per #1624 this only deals with conforming elements
@annevk

This comment has been minimized.

Member

annevk commented Dec 29, 2016

Is the reasoning for landing this now that two browsers no longer support it? Do we know if Safari is planning on removing it? I'm happy to see this go, but I'd also like to make sure everyone is on board.

@domenic

This comment has been minimized.

Member

domenic commented Dec 29, 2016

Is the reasoning for landing this now that two browsers no longer support it? Do we know if Safari is planning on removing it? I'm happy to see this go, but I'd also like to make sure everyone is on board.

Yeah, that's the idea, plus per #2079 (comment) Mozilla is interested. I tried to solicit Apple opinions in that thread but no response; maybe after the holidays.

domenic added a commit to web-platform-tests/wpt that referenced this pull request Jan 10, 2017

Test that innerText can be set on a variety of elements
As discovered and discussed in whatwg/html#2221 (comment) , Chrome has a blocklist here that seems historical and not supported by any spec.

domenic added a commit to web-platform-tests/wpt that referenced this pull request Jan 10, 2017

Test that createContextualFragment works on a variety of contexts
As discovered and discussed in whatwg/html#2221 (comment) , some browsers have a blocklist here that seems historical and not supported by any spec.

The same blocklist is shared by innerText (tested in #3491) and outerText (not tested as it's not yet standard).
@domenic

This comment has been minimized.

Member

domenic commented Jan 11, 2017

@cdumez any thoughts from WebKit on removing keygen? It's now no longer supported in both Edge and Blink, with Gecko interested in removing as well.

@cdumez

This comment has been minimized.

Contributor

cdumez commented Jan 12, 2017

@domenic I am not familiar with keygen. I forwarded the query to the right people internally. I'll let you know when they come back to me.

@cdumez

This comment has been minimized.

Contributor

cdumez commented Jan 12, 2017

@domenic Sounds like we're willing to try dropping it in WebKit as well.

@annevk

annevk approved these changes Jan 12, 2017

Maybe at some point we should define "legacy elements" as a term to avoid duplication and have a URL for them, but on the other hand they only occur in two places at the moment.

@domenic domenic merged commit 5baa387 into master Jan 13, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@domenic domenic deleted the remove-keygen branch Jan 13, 2017

domenic added a commit to web-platform-tests/wpt that referenced this pull request Jan 13, 2017

@domenic

This comment has been minimized.

@Darkspirit

This comment has been minimized.

Darkspirit commented Jan 13, 2017

The BMO link depends on https://bugzilla.mozilla.org/show_bug.cgi?id=1088063 "Create a WebCrypto based API for managing TLS client certificates"
As business deployments haven't enabled telemetry, it's not possible to just remove keygen without an easy way to import tls client certs with WebCrypto. Maybe just ask for permission like for notifications/webcam/audio, but without storing a permanent decision.
I would be delighted if you took this into account. Dropping keygen seems to be the right decision.

@sleevi

This comment has been minimized.

sleevi commented Jan 13, 2017

@TerraX-net That should only be specific to Mozilla, I believe, and not necessarily a blocker.

On the simplest level, never provided the functionality described by that bug, for any UA that implemented it, because it did not afford management capabilities. The closest you got was import capabilities - would typically create keys in the OS store (for !Mozilla-provided distributions of Firefox) or in the application store (for Mozilla Firefox). The rest of the behaviour was related to application/x-x509-user-cert mime handling.

For at least Edge and Safari, any integration with the OS store also arguably applies to the file & mime type - that is, PKCS#12 files are appropriately associated with their importing tool - either CredUI/CertUI on the Edge/Windows case and Chrome/Windows case, or Keychain for the Safari/macOS or Chrome/macOS case.

Firefox has an integrated keystore specific to the application, but that doesn't mean the keystore is part of the Web Platform per se (indeed, arguably it's not - the Firefox-exposed keystore is part of the LSB on Linux, just that Mozilla-provided distributions of Firefox disable that integration while distro-provided distributions of Firefox enable it). I suppose simply requesting to Mozilla that it recognize and associate PKCS#12 files and allow importing them, but that is, as I mentioned, somewhat and slightly orthogonal to the issue you're linking, and to the issue being discussed here on the bug.

sideshowbarker pushed a commit to validator/tests that referenced this pull request Jan 15, 2017

@js-choi js-choi referenced this pull request Dec 20, 2017

Open

Keygen is depreciated #134

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment