In bug 1030963, we're working on removing the proprietary window.crypto functions/properties. It's been suggested that they be soft-disabled via a pref for at least one release before removing the implementation entirely. This will enable us to give a meaningful heads-up to anyone still using these functions and allow them to transition away before they're completely removed. My intention is to change each implementation to check a pref and basically return "NS_ERROR_NOT_IMPLEMENTED" by default. This would allow anyone who still needs these to temporarily use the pref to re-enable the functionality. Telemetry might also be useful.

What heads up is being requested besides the ones you already gave?

(In reply to David Keeler (:keeler) [use needinfo?] from comment #0) > My intention is to change each implementation to check a pref and basically > return "NS_ERROR_NOT_IMPLEMENTED" by default. This causes its own compatibility problems that, for some sites, are probably worse than removing the functionality. JS code does feature detection using expressions like ("signText" in window.crypto) to see if they should even attempt to use these features. With your proposed implementation, "signedText" in window.crypto will be true, but signText will fail. Instead, I suggest asking the DOM people about how to conditionally expose the properties based on a pref. They have lots of experience with doing this and it might even be less code than what you're planning. I suggest writing a mochitest that tests ("functionName" in window.crypto) for all legacy WebCrypto functions. > This would allow anyone who > still needs these to temporarily use the pref to re-enable the functionality. For how many releases? As long as this functionality is an option, we'll still be on the hook for security fixes to it. Sounds bad. > Telemetry might also be useful. Maybe for political reasons, only.

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #1) > What heads up is being requested besides the ones you already gave? The assumption is there are people using these functions that don't read dev.tech.crypto or dev.platform. The idea is to break anyone's implementation that uses these functions. When they investigate, they'll discover that they can temporarily re-enable the functions, but that they will soon be removed. This gives them time to move to a new implementation. (In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #2) > (In reply to David Keeler (:keeler) [use needinfo?] from comment #0) > > My intention is to change each implementation to check a pref and basically > > return "NS_ERROR_NOT_IMPLEMENTED" by default. > > This causes its own compatibility problems that, for some sites, are > probably worse than removing the functionality. JS code does feature > detection using expressions like ("signText" in window.crypto) to see if > they should even attempt to use these features. With your proposed > implementation, "signedText" in window.crypto will be true, but signText > will fail. Instead, I suggest asking the DOM people about how to > conditionally expose the properties based on a pref. They have lots of > experience with doing this and it might even be less code than what you're > planning. I'll see what they say. But even so, we can't guarantee that implementations do this kind of feature detection, so we could still break them in the same kind of way. > I suggest writing a mochitest that tests ("functionName" in window.crypto) > for all legacy WebCrypto functions. These already exist: dom/tests/mochitest/crypto/test_legacy.html and dom/tests/mochitest/crypto/test_no_legacy.html > > This would allow anyone who > > still needs these to temporarily use the pref to re-enable the functionality. > > For how many releases? Just 1. > > Telemetry might also be useful. > > Maybe for political reasons, only. I think it would be good to have data to back up our arguments if there are any further discussions about this removal.

Kyle, would you mind reviewing this? I was going to ask :jst (since he's been involved with bug 1030963), but it looks like he's on vacation. Here's a try run: https://tbpl.mozilla.org/?tree=Try&rev=1e28feabd17c

Comment on attachment 8453865 [details] [diff] [review] patch Review of attachment 8453865 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/Crypto.webidl @@ +22,5 @@ > > #ifndef MOZ_DISABLE_CRYPTOLEGACY > [NoInterfaceObject] > interface CryptoLegacy { > + [Pref="dom.unsafe_legacy_crypto.enabled"] Brian - looks like you were right. This is way easier than what I was going to do. Unfortunately, putting the pref on the entire interface didn't seem to work. Maybe Kyle knows how to get what we want here.

For reference, this is a diff -w for where I had to change the indentation of one of the tests.

Comment on attachment 8453865 [details] [diff] [review] patch Looks like Kyle's pretty busy. Mounir, would you be able to take a look at this? Thanks.

I might be wrong, but I think it's unlikely that you get a review out of Mounir these days. Try smaug?

Comment on attachment 8453865 [details] [diff] [review] patch (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #8) > I might be wrong, but I think it's unlikely that you get a review out of > Mounir these days. Try smaug? Ok - thanks for the pointer.

I don't quite understand the plan. Web sites using these functions would ask user to use about:config to set a pref? We can use the pref approach if we think removing is regression prone and that we might want to re-enable the functions by default again. Do we have any telemetry data about usage of these functions?

Comment on attachment 8453865 [details] [diff] [review] patch Clearing the review request since I don't understand the plan. (Code looks fine)

We've already announced that we're removing these functions (see bug 1030963 and the linked announcement on various discussion lists). However, it's unlikely that everyone using these functions is actively monitoring those lists. To give a notification that will actually be heeded, the idea is to break any site using these functions in a way that can be temporarily remedied. This gives these sites some time to change what they're doing before they stop working altogether. In terms of telemetry, many of the use cases for these functions only happen once a year (for example, certificate enrollment or e-voting), so telemetry wouldn't give us a good picture of usage anyway. We do know it's very limited because a number of serious bugs went unnoticed for years.

The sites can't fix anything. They just see those functions gone. It is up to the user to change some prefs, and about:config really isn't something for end users. So I don't quite see what the pref helps. Is the idea that a web site detects that certain function isn't there anymore, then checks Firefox version in order to know that the function is under a pref, then shows some notification for the user to go to about:config to add such pref? (Ok, looks like telemetry wouldn't be useful.)

It's intended that the functionality be replaced by more modern mechanisms, as outlined here: https://wiki.mozilla.org/SecurityEngineering/Removing_Proprietary_window.crypto_Functions Using a pref to disable these functions gives sites a period of time to notice the breakage and start changing their implementation to not rely on them. Meanwhile, their users can be told to change the pref (for the most part, people using these prefs would be techincally-savvy users). The idea is that this is preferable to suddenly having a release that does not work at all. Also, it gives us an easily-hotfixable escape valve if removing the functionality turns out to be more problematic than anticipated. If this sounds good, then I'll go ahead with this. If not, then I guess this isn't necessary.

(In reply to David Keeler (:keeler) [use needinfo?] from comment #14) > Using a pref to disable these functions gives sites a period of time to > notice the breakage and start changing their implementation to not rely on > them. Meanwhile, their users can be told to change the pref (for the most > part, people using these prefs would be techincally-savvy users). But about:config isn't something for users. > The idea > is that this is preferable to suddenly having a release that does not work > at all. Also, it gives us an easily-hotfixable escape valve if removing the > functionality turns out to be more problematic than anticipated. This hotfixability I can buy :)

(In reply to Olli Pettay [:smaug] from comment #15) > > Also, it gives us an easily-hotfixable escape valve if removing the > > functionality turns out to be more problematic than anticipated. > This hotfixability I can buy :) Yeah, I guess that's the most compelling argument. So, r+?

Had to rebase this. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3b1cfc38aa4f

Comment on attachment 8468018 [details] [diff] [review] patch rebased Pref="dom.unsafe_legacy_crypto.enabled" in interface CryptoLegacy doesn't work?

I think it should, and if it doesn't, please file a bug. Adding Pref="dom.unsafe_legacy_crypto.enabled" just for the interface would make things simpler.

(and if Pref="dom.unsafe_legacy_crypto.enabled" with interface doesn't work, ok to land the patch as it is.)

Unfortunately, that didn't seem to work. I filed bug 1049788. Thanks for the review! Inbound is closed, so I'll mark this checkin-needed (treeherder try link in comment 17).

Approval Request Comment [Feature/regressing bug #]: removal of proprietary window.crypto properties/functions (see https://groups.google.com/forum/#!msg/mozilla.dev.platform/1zv68u5kWfY/hexfEw9fTEAJ ) [User impact if declined]: the point of this patch is to roll out a change that disables the proprietary window.crypto properties and functions in a way that is hotfixable if we encounter massive compatibility issies. Thus, it needs to be ahead of the patch that completely removes these functions (see bug 1030963). For the safety of our users, it would be best to land that patch as soon as possible, meaning this needs to be uplifted. [Describe test coverage new/current, TBPL]: the presence/absence of these functions is adequately tested [Risks and why]: the only real risk is compatibility. If this causes large-scale incompatibilities, we can issue a hotfix to re-enable these functions. [String/UUID change made/needed]: none

OK. We should get that in the release notes.

Added in the release notes: "Proprietary window.crypto properties/functions removed"