1333140 - Require [SecureContext] for Web Crypto

Status: RESOLVED FIXED

(Core :: DOM: Web Crypto, defect, P2)

Description

[Ryan Sleevi]

WebCrypto CR requires [SecureContext], as captured on https://github.com/w3c/webcrypto/issues/28 and reflected in both the ED and CR.

Firefox does not currently require this.

Chrome partially does, and is in the process of aligning with the spec ( https://codereview.chromium.org/2639593002/ - WebCrypto was implemented before [SecureContext] support in Chrome, hence spec divergence)

It would be great it Firefox could align with the spec on requiring a SecureContext for WebCrypto.

Comment 1

[Daniel Veditz [:dveditz]]

Though we did this from the start?

Comment 2

[Ryan Sleevi]

We had a report of compat issue in https://groups.google.com/a/chromium.org/d/msg/security-dev/DtOFo51WFMo/Ypy4gakaAgAJ (and linked bug)

I couldn't find where SecureContext was being enforced w/in the FF code, either in the generated bindings or the WebCryptoTask

My understanding was that FF didn't implement the SecureContext requirement because Chrome's was buggy (which we're trying to fix) and because the spec didn't normatively require it (which it now does)

Comment 3

[J.C. Jones [:jcj] (he/him)]

It wasn't feasible to mark things [SecureContext] until Bug 1286312 landed, which it (recently) has, so this is probably unblocked.

Comment 4

[Richard Barnes [:rbarnes]]

Speaking historically, I will fess up to not implementing it the first time around because (1) we didn't have [SecureContext] yet, (2) the whole "powerful features" debate was still ongoing, and (3) as a result of the above, the arguments that there were some valid use cases for it on HTTP won the day.

Now that (1) and (2) have settled down, I think we should absolutely go ahead with this if we can convince ourselves the compat risk is acceptable.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1333140-Require-SecureContext-for-Web-Crypto.patch

Ok, so this is easy. Tests succeed locally and it looks like we don't need any further changes to get this landed.

Richard, how do we determine the compat risk here and what do we define as acceptable?

Comment 6

[Richard Barnes [:rbarnes]]

Tim: How about we add a telemetry probe (say WEBCRYPTO_SECURE_CONTEXT), and let that ride a release or two ahead of the actual change?

It seems like it would be best to measure this in units of what would fail if we added the SecureContext annotation, so probably in dom::Crypto::Subtle.  It looks like you can call JS_GetIsSecureContext on some derivative of mParent to get the flag you want to collect, as here:

http://searchfox.org/mozilla-central/source/dom/workers/WorkerScope.cpp#179

Something like JS_GetIsSecureContext(js::GetObjectCompartment(mParent->GetGlobalJSObject()));

It would also be good to have an overall idea of how often crypto.subtle is accessed in the first place.  I think the WebIDL folks might have some usage counter stuff for this that you can just turn on.  It looks like you might just be able to add "UseCounter", e.g.:

http://searchfox.org/mozilla-central/source/dom/webidl/OfflineResourceList.webidl#28
https://mzl.la/2kU3TsA

As to what risk is acceptable, I would say let's take a look at what the measurements look like.  Obviously, the lower the impact, the better :)

Comment 7

[Richard Barnes [:rbarnes]]

Comment on attachment 8835006 [details] [diff] [review]
0001-Bug-1333140-Require-SecureContext-for-Web-Crypto.patch

Review of attachment 8835006 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me, modulo the below nit.  As noted in my other comments, though, let's hold on this until we get some telemetry.

::: dom/webidl/Crypto.webidl
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/.
>   *
>   * The origin of this IDL file is
> + * https://w3c.github.io/webcrypto/Overview.html#crypto-interface

Probably better to refer to the slightly more official version:

https://www.w3.org/TR/WebCryptoAPI/#crypto-interface

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1333140-Require-SecureContext-for-Web-Crypto.patch, v2

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1333140-Require-SecureContext-for-Web-Crypto.patch

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1333140-Collect-telemetry-about-non-HTTPS-usage-.patch

Should we ask a DOM peer as well for the UseCounter changes?

Comment 11

[Richard Barnes [:rbarnes]]

Comment on attachment 8835725 [details] [diff] [review]
0001-Bug-1333140-Collect-telemetry-about-non-HTTPS-usage-.patch

Review of attachment 8835725 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM modulo one nit below.  Adding r? to bz since he's helped out with WebCrypto stuff in the past.

::: toolkit/components/telemetry/Histograms.json
@@ +8822,5 @@
>      "n_values": 30,
>      "description": "Algorithms used with WebCrypto (see table in WebCryptoTask.cpp)"
>    },
> +  "WEBCRYPTO_SECURE_CONTEXT": {
> +    "expires_in_version": "56",

Nit: I would push this out a little further, say to 60.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8835723 [details] [diff] [review]
0002-Bug-1333140-Require-SecureContext-for-Web-Crypto.patch

You may well need some web platform test changes here too, right?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8835725 [details] [diff] [review]
0001-Bug-1333140-Collect-telemetry-about-non-HTTPS-usage-.patch

I'm not super-happy with that JSAPI stuff there.  Far better would be to have IsSecureContext on nsIGlobalObject.  Please file a followup bug to do that, and reference this code from that bug?

Apart from that, this is likely to run into false positives from code that just walks all properties of default objects, which does exist out there.  It's not clear to me to what extent this will affect your numbers.  I _might_ be better to measure the thing we really care about, which are actual API calls to crypto.subtle.*.

Measuring both and comparing might be informative....

Comment 14

[Richard Barnes [:rbarnes]]

Note that we already have overall counters for WEBCRYPTO_METHOD, so we would just have to split them out by secure / not.

Tim: If you want to do that, you could just set a flag on the SubtleCrypto object so that you don't have to query IsSecureContext() multiple times.  It shouldn't change.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #13)
> I'm not super-happy with that JSAPI stuff there.  Far better would be to
> have IsSecureContext on nsIGlobalObject.  Please file a followup bug to do
> that, and reference this code from that bug?

Sure, will do.

> Apart from that, this is likely to run into false positives from code that
> just walks all properties of default objects, which does exist out there. 
> It's not clear to me to what extent this will affect your numbers.  I
> _might_ be better to measure the thing we really care about, which are
> actual API calls to crypto.subtle.*.
> 
> Measuring both and comparing might be informative....

Indeed, very good point.

(In reply to Richard Barnes [:rbarnes] from comment #14)
> Note that we already have overall counters for WEBCRYPTO_METHOD, so we would
> just have to split them out by secure / not.

Good idea.

> Tim: If you want to do that, you could just set a flag on the SubtleCrypto
> object so that you don't have to query IsSecureContext() multiple times.  It
> shouldn't change.

Did that.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1333140-Collect-telemetry-about-non-HTTPS-usage-.patch, v2

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1333140-Collect-telemetry-about-non-HTTPS-usage-.patch, v3

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1333140-Require-SecureContext-for-WebCrypto-API.patch, v2

Now also including the WPT fixes. We of course will have to wait for the Telemetry data to come back, and if that looks promising write an intent to ship before we can land this.

Comment 19

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8868995 [details] [diff] [review]
0001-Bug-1333140-Collect-telemetry-about-non-HTTPS-usage-.patch, v3

Review of attachment 8868995 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM - just a couple of questions.

::: dom/base/Crypto.cpp
@@ +9,5 @@
>  #include "nsIRandomGenerator.h"
>  #include "MainThreadUtils.h"
>  #include "nsXULAppAPI.h"
>  
> +#include "mozilla/Telemetry.h"

Is this #include necessary here?

::: toolkit/components/telemetry/Histograms.json
@@ +10722,5 @@
>      "n_values": 20,
>      "description": "Methods invoked under window.crypto.subtle (0=encrypt, 1=decrypt, 2=sign, 3=verify, 4=digest, 5=generateKey, 6=deriveKey, 7=deriveBits, 8=importKey, 9=exportKey, 10=wrapKey, 11=unwrapKey)"
>    },
> +  "WEBCRYPTO_METHOD_SECURE": {
> +    "alert_emails": ["[e-mail]"],

Should we just use [e-mail]?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8868995 [details] [diff] [review]
0001-Bug-1333140-Collect-telemetry-about-non-HTTPS-usage-.patch, v3

So this will take how many were secure vs insecure, but not the per-method-type breakdown, right?

Also, this will measure the number of calls, not the number of pages or documents, so can be skewed by one page making ridiculous numbers of calls.  Is that ok?

Apart from that and the "please file a followup to have a way to get this information without JSAPI" bit, r=me

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8869019 [details] [diff] [review]
0002-Bug-1333140-Require-SecureContext-for-WebCrypto-API.patch, v2

In the spec, [SecureContext] is also ont he overall CryptoKey interface and the overall SubtleCrypto interface.

Did we skip adding those on purpose?  If not, we should probably just add them.

Comment 22

[Benjamin Smedberg]

Comment on attachment 8868995 [details] [diff] [review]
0001-Bug-1333140-Collect-telemetry-about-non-HTTPS-usage-.patch, v3

data-r=me

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #19)
> ::: dom/base/Crypto.cpp
> @@ +9,5 @@
> >  #include "nsIRandomGenerator.h"
> >  #include "MainThreadUtils.h"
> >  #include "nsXULAppAPI.h"
> >  
> > +#include "mozilla/Telemetry.h"
> 
> Is this #include necessary here?

Oops, no.

> ::: toolkit/components/telemetry/Histograms.json
> @@ +10722,5 @@
> >      "n_values": 20,
> >      "description": "Methods invoked under window.crypto.subtle (0=encrypt, 1=decrypt, 2=sign, 3=verify, 4=digest, 5=generateKey, 6=deriveKey, 7=deriveBits, 8=importKey, 9=exportKey, 10=wrapKey, 11=unwrapKey)"
> >    },
> > +  "WEBCRYPTO_METHOD_SECURE": {
> > +    "alert_emails": ["[e-mail]"],
> 
> Should we just use [e-mail]?

Yeah, didn't know that existed.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Boris Zbarsky [:bz] (work week until 5/26) from comment #21)
> In the spec, [SecureContext] is also ont he overall CryptoKey interface and
> the overall SubtleCrypto interface.
> 
> Did we skip adding those on purpose?  If not, we should probably just add
> them.

Didn't skip them on purpose, will add.

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Boris Zbarsky [:bz] (work week until 5/26) from comment #20)
> So this will take how many were secure vs insecure, but not the
> per-method-type breakdown, right?

Correct.

> Also, this will measure the number of calls, not the number of pages or
> documents, so can be skewed by one page making ridiculous numbers of calls. 
> Is that ok?

That's a good point, I should have thought about that. Will amend the patch to only report once per SubtleCrypto instance. This could still be skewed by pages with tons of iframes that all use WebCrypto but I don't think it's worth guarding against that.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1333140-Collect-telemetry-about-non-HTTPS-usage-.patch, v4

This now records telemetry only once per SubtleCrypto instance.

Carrying over data-r=bsmedberg

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1333140-Require-SecureContext-for-WebCrypto-API.patch, v3

This adds [SecureContext] to SubtleCrypto and CryptoKey now as well. Boris doesn't accept review requests currently, maybe Olli can help?

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #27)
> This adds [SecureContext] to SubtleCrypto and CryptoKey now as well. Boris
> doesn't accept review requests currently, maybe Olli can help?

OTOH, this won't land for a while anyway. Need some telemetry first.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8872280 [details] [diff] [review]
0002-Bug-1333140-Require-SecureContext-for-WebCrypto-API.patch, v3

>  * The origin of this IDL file is
>- * https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#crypto-interface
>+ * https://www.w3.org/TR/WebCryptoAPI/#crypto-interface

You mean https://w3c.github.io/webcrypto/Overview.html right?
We don't try to implement TR specs if there is some newer available. TR stand for trash for that reason (and not technical report) ;)

Comment 30

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8872279 [details] [diff] [review]
0001-Bug-1333140-Collect-telemetry-about-non-HTTPS-usage-.patch, v4

Review of attachment 8872279 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM - just some nits.

::: dom/base/SubtleCrypto.cpp
@@ +24,5 @@
>  
>  
>  
>  SubtleCrypto::SubtleCrypto(nsIGlobalObject* aParent)
> +  : mParent(aParent), mRecordedTelemetry(false)

nit: let's initialize one member per line

@@ +41,5 @@
> +    return;
> +  }
> +
> +  mRecordedTelemetry = true;
> +  JSObject *global = mParent->GetGlobalJSObject();

nit: * to the left

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #20)
> Apart from that and the "please file a followup to have a way to get this
> information without JSAPI" bit, r=me

Filed bug 1368929 for nsIGlobalObject::IsSecureContext().

Comment 32

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #30)
> >  SubtleCrypto::SubtleCrypto(nsIGlobalObject* aParent)
> > +  : mParent(aParent), mRecordedTelemetry(false)
> 
> nit: let's initialize one member per line

Done.

> > +  mRecordedTelemetry = true;
> > +  JSObject *global = mParent->GetGlobalJSObject();
> 
> nit: * to the left

Done. Thanks!

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #29)
> >  * The origin of this IDL file is
> >- * https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#crypto-interface
> >+ * https://www.w3.org/TR/WebCryptoAPI/#crypto-interface
> 
> You mean https://w3c.github.io/webcrypto/Overview.html right?
> We don't try to implement TR specs if there is some newer available. TR
> stand for trash for that reason (and not technical report) ;)

Yeah... fixed :)

Comment 34

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1333140-Collect-telemetry-about-non-HTTPS-usage-.patch, v5

r=keeler data-r=bsmedberg

Comment 35

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1333140-Require-SecureContext-for-WebCrypto-API-.patch, v4

r=smaug

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/917caf00a8b4
Collect telemetry about (non-)HTTPS usage of crypto.subtle r=keeler data-r=bsmedberg

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/917caf00a8b4

Comment 38

[Tim Taubert [:ttaubert] (inactive)]

Looking at the telemetry data we have 12.83% of calls to crypto.subtle.* methods from a non-secure context on Beta 55, over the last 7 days.

It's maybe worth noting that 12.83% is rather high, we recorded 10.31% non-secure uses since we started measuring. 7% on Nightly.

Benjamin, what's a good way to reason about this data? Can we somehow compare it to the overall number of pings to see what percentage of users would be affected? Do we have experience evaluating similar data before?

Comment 39

[Ryan Sleevi]

Likely related to the discussion here: https://twitter.com/mikewest/status/880767995936747521

Comment 40

[Benjamin Smedberg]

I provided a tiny bit of advice on IRC, but these kinds of questions are best addressed to a product manager. I suggested abovens.

Comment 41

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Ryan Sleevi from comment #39)
> Likely related to the discussion here:
> https://twitter.com/mikewest/status/880767995936747521

That's a good pointer, thanks, I missed that. So Twitter/AMP likely skews those numbers, but it's probably not something we want to break either...

Comment 42

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #41)
> (In reply to Ryan Sleevi from comment #39)
> > Likely related to the discussion here:
> > https://twitter.com/mikewest/status/880767995936747521
> 
> That's a good pointer, thanks, I missed that. So Twitter/AMP likely skews
> those numbers, but it's probably not something we want to break either...

To clarify, AMP loads a polyfill if crypto.subtle isn't available. We wouldn't break anyone's Twitter if we restrict to [SecureContext].

The problem however is that our telemetry numbers currently aren't worth much because we can't tell which sites use the API from a non-secure context. The only comparison we have is the data hovering at around 1-2% (not of all users, only of users using the WebCrypto API) for about two weeks before Google noticed the uptick caused by Twitter/AMP as well.

Jeff, Andreas, what's your opinion on this? Is this enough evidence to move forward with the restriction?

Comment 43

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #42)
> The problem however is that our telemetry numbers currently aren't worth
> much because we can't tell which sites use the API from a non-secure
> context.

That wasn't worded well. I wanted to say that unfortunately we can't tell how much Twitter/AMP skews those numbers.

Comment 44

[Andreas Bovens [:abovens]]

My suggestion is to move forward with the restriction. AMP falling back to a polyfill when the API isn't natively available is likely to cover most instances of non-secure usage of crypto.subtle, and for the rest, there's sitecompat outreach: I'm sure this won't be a problem, given that other vendors are likely to move forward with the restriction as well as per https://groups.google.com/a/chromium.org/d/msg/blink-dev/ZD3NWqkk-bo/tgcy_SJQBQAJ

Comment 45

[Jeff Griffiths (:canuckistani) (:⚡︎)]

what Andreas said

Comment 46

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-1333140-Remove-WEBCRYPTO_METHOD_SECURE-histogram.patch

It doesn't make sense to keep the histogram around when we enforce [SecureContext]. Let's remove it, it has served its purpose.

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d02d52b92ee
Require [SecureContext] for WebCrypto API r=bz,smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc91ce755717
Remove WEBCRYPTO_METHOD_SECURE histogram r=keeler

Comment 48

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d54eedad236
Backed out changeset 3d02d52b92ee

Comment 49

[Tim Taubert [:ttaubert] (inactive)]

Looks like this won't be too easy. WebCrypto seems the first secure context API exposed to and used in JS sandboxes, as well as workers. We have to figure these two out before we can actually ship the API restriction.

Comment 50

[Boris Zbarsky [:bzbarsky]]

Workers shouldn't be an issue, right?  I believe we fully support those, at least in the web context...  workers used in our chrome might be a separate issue.

Bug 1273687 tracks the sandbox situation.

Comment 51

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/fc91ce755717

Comment 52

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #50)
> Workers shouldn't be an issue, right?  I believe we fully support those, at
> least in the web context...  workers used in our chrome might be a separate
> issue.

I saw a bunch of WPT failures that test WebCrypto in workers, that's why I assumed it to be a more general problem with workers:

https://treeherder.mozilla.org/logviewer.html#?job_id=149068416&repo=mozilla-inbound&lineNumber=2295

Comment 53

[Boris Zbarsky [:bzbarsky]]

> https://treeherder.mozilla.org/logviewer.html#?job_id=149068416&repo=mozilla-inbound&lineNumber=2295

That's running based on WebCryptoAPI/derive_bits_keys/ecdh_bits.worker.js

The file isn't named *.https.worker.js, so it's not loaded over https by the test harness.  It should be renamed accordingly.  Same applies to various other tests under WebCryptoAPI/, though there are some (e.g. under WebCryptoAPI/generateKey/) that are named right.

Comment 54

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #53)
> The file isn't named *.https.worker.js, so it's not loaded over https by the
> test harness.  It should be renamed accordingly.  Same applies to various
> other tests under WebCryptoAPI/, though there are some (e.g. under
> WebCryptoAPI/generateKey/) that are named right.

Ah, that makes sense. I just tested this with a single worker test and it runs correctly which means there indeed are no problems with workers and [SecureContext] APIs.

I wonder why Chrome doesn't hit that since they updated their implementation and the WPTs.

https://chromium.googlesource.com/chromium/src.git/+/7d57a8fa0d13db4fcb178bb58bb9389fa7670d74
https://github.com/w3c/web-platform-tests/commit/ea527511d7f742f5ad939339df3bad2619382a00

Comment 55

[Anne (:annevk)]

It seems they forgot to rename many of the worker resources (though did rename some). Not sure how that happened though. I created https://github.com/w3c/web-platform-tests/pull/8995 to rectify the situation. Hope that helps and unblocks us here.

Comment 56

[Tim Taubert [:ttaubert] (inactive)]

Thanks Anne!

Comment 57

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:ckerschb, maybe it's time to close this bug?

Comment 58

[Anne (:annevk)]

I don't think this should be closed, without HTTPS

console.log(self.SubtleCrypto)
console.log(self.crypto)

do not log undefined as they should.

Comment 59

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Release mgmt bot [:sylvestre / :calixte] from comment #57)

The leave-open keyword is there and there is no activity for 6 months.
:ckerschb, maybe it's time to close this bug?

I am not sure - JC Jones is the webcrypto expert - 302 to him.

Comment 60

[J.C. Jones [:jcj] (he/him)]

I'm going to try and pick this up here at the end of the quarter. It's added to my backlog, so removing the ni.

Comment 61

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jcj, maybe it's time to close this bug?

Comment 63

[Jonathan Kingston [:jkt] he/him]

It seems Chrome exposes the interface still from https://bugzilla.mozilla.org/show_bug.cgi?id=1333140#c58 however it seems like we should at least hide self.crypto.subtle though.

Maybe that explains the test failures.

Throwing at try to see what happens:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c32cc3b5944d692e0e6e871a0a0b59e9ff30d379

Comment 64

[Anne (:annevk)]

I filed https://bugs.chromium.org/p/chromium/issues/detail?id=1038704 on Chrome.

Comment 65

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1333140 - Require SecureContext for WebCrypto

Comment 66

[Jonathan Kingston [:jkt] he/him]

I think the safe plan here is to match the current Chrome/Webkit implementation which hides self.crypto.subtle and not require telemetry due to minimal breakage. Then follow up with Bug 1606754 in removing the other interfaces once it's covered with web platform tests etc.

The current patch will land after the code freeze.

Comment 68

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0149bf22789
Require SecureContext for WebCrypto r=bholley,keeler,baku

Comment 69

[Ryan Sleevi]

JC: Is my understanding that Mozilla plans to just ship without any compat testing? Just confirming, since that might help make it more tractable to Just Fix the Chrome side :) (where SubtleCrypto's ctor is exposed, but window.crypto.subtle isn't)

Comment 70

[J.C. Jones [:jcj] (he/him)]

Release Note Request (optional, but appreciated)
[Why is this notable]: It's a potential compatibility issue, though telemetry has shown it's not a large usage in the telemetry cohort and Chrome moved first.
[Affects Firefox for Android]: Yes
[Suggested wording]: In Firefox 75, developers will require Secure Contexts to use the Web Crypto API.
[Links (documentation, blog post, etc)]: https://groups.google.com/d/topic/mozilla.dev.platform/55t-Uyx1TxI/discussion

Comment 71

[J.C. Jones [:jcj] (he/him)]

(In reply to Ryan Sleevi from comment #69)

JC: Is my understanding that Mozilla plans to just ship without any compat testing? Just confirming, since that might help make it more tractable to Just Fix the Chrome side :) (where SubtleCrypto's ctor is exposed, but window.crypto.subtle isn't)

We'll have a whole cycle of nightly testing for 75, and this brings this small part of WebCrypto in-line with the Web Platform Tests that mandate [SecureContext] for those interfaces.

If something warrants further action in Nightly or Beta 75 we'll discuss it, but at this point it's long overdue, and the intention is to ship to release in 75.

Comment 72

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/b0149bf22789

Comment 73

[Kohei Yoshino [:kohei]]

Posted site compatibility note for web developers.

Comment 74

[Eric Shepherd [:sheppy]]

Documentation updates:

• Added Web Crypto to Features restricted to secure contexts
• Improvements to the Crypto interface page; noted secure requirement for window.crypto and Crypto.getRandomValues() and added a bit about the entropy of random numbers
• Updated Window.crypto
• Added the needed bits to CryptoKey to indicate it requires a secure context
• Updated SubtleCrypto
• Updated all subpages of SubtleCrypto to include the SecureContext_header banner
• Updated Crypto.getRandomValues() not only with the fact that it's available even on insecure contexts, but also other improvements
• Submitted KumaScript PR 1349 to update GroupData.json with corrected info about the interfaces and dictionaries on the API
• Submitted BCD PR 5884
• Mentioned on Firefox 75 for developers

Comment 75

[Julien Cristau [:jcristau]]

Probably enough to mention this on the MDN page for 75, so I'll leave it off the main user-facing release notes.