1023621 - add asynchronous certificate verification api

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

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

In order to stop doing certificate verification that blocks the main thread, we'll have to add an asynchronous api.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to David Keeler (:keeler) [use needinfo?] from comment #0)
> In order to stop doing certificate verification that blocks the main thread,
> we'll have to add an asynchronous api.

Which places are we doing main-thread cert verification that you think we should switch to this new API? In most cases where we're doing main-thread cert verification, the problem is much deeper than main-thread vs. non-main-thread.

Also, we already have two different async cert verification APIs: SSLServerCertVerificationJob::Dispatch and nsIX509Cert3.requestUsagesArrayAsync.

Comment 2

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

I was thinking of things like nsIX509Cert.issuer and any code using getChain to get what it erroneously thinks is a verified chain. The problem with SSLServerCertVerificationJob::Dispatch is that it isn't exposed to JS. nsIX509Cert3.requestUsagesArrayAsync doesn't give a verified chain to a trusted root.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to David Keeler (:keeler) [use needinfo?] from comment #2)
> I was thinking of things like nsIX509Cert.issuer and any code using getChain
> to get what it erroneously thinks is a verified chain. The problem with
> SSLServerCertVerificationJob::Dispatch is that it isn't exposed to JS.
> nsIX509Cert3.requestUsagesArrayAsync doesn't give a verified chain to a
> trusted root.

Inside of Gecko, are there any uses of nsIX509Cert.getChain or nsIX509Cert.issuer that aren't just wrong? Usually we're interested in the chain/issuer of a certificate according to the chain we built in a TLS connection. If we build a new chain, it might not be the same chain, or we may not get the same result, as the chain/result we got during the TLS handshake. That is, all of that stuff is racy, at best. It seems like the other changes you are working on, to thread the certificate chain through the DocShell and whatnot, are part of a better approach.

Anyway, if such an async API is needed, nobody's going to block it from being created. But, it seems like about the same amount of work to do this as it would be to fix the various bits of brokenness that would depend on it.

Comment 4

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

One of my eventual goals for PSM is to ensure that we never block the main thread on fetching OCSP (and, to a lesser extent, on expensive public key verification operations). Eventually we might achieve this by disabling OCSP fetching altogether, but a closer goal would be to restructure things and to fix the cases (like those mentioned in comment 3) that are just wrong. When that happens, all of our tests that verify certificates on the main thread need to be asynchronous, hence this bug.

Comment 5

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

Attachment: bug 1023621 - add asynchronous certificate verification API

This API (nsIX509CertDB.asyncVerifyCertAtTime) will eventually replace
nsIX509Cert.getUsagesArray, nsIX509Cert.requestUsagesArrayAsync, and
nsIX509Cert.getUsagesString because those APIs are architecturally problematic
and don't give very precise information in any case.

Review commit: https://reviewboard.mozilla.org/r/61668/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61668/

Comment 6

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

It turns out that if we do this and then update the certificate viewer, we can remove a whole bunch of code (in particular, it will simplify NSS initialization/shutdown a bit).

Comment 7

[:Cykesiopka]

Comment on attachment 8766967 [details]
bug 1023621 - add asynchronous certificate verification API

https://reviewboard.mozilla.org/r/61668/#review59018

Nice! I look forward to the day when all those old APIs can be ripped out.

::: security/manager/ssl/nsIX509CertDB.idl:48
(Diff revision 1)
>  };
>  
> +[scriptable, function, uuid(49e16fc8-efac-4f57-8361-956ef6b960a4)]
> +interface nsICertVerificationCallback : nsISupports {
> +  void verifyCertFinished(in int32_t aPRErrorCode,
> +                          in nsIX509CertList aVerifiedChain,

It might be useful to note that this could be null.

::: security/manager/ssl/nsNSSCertificateDB.cpp:1634
(Diff revision 1)
> +    : mCert(aCert)
> +    , mUsage(aUsage)
> +    , mFlags(aFlags)
> +    , mHostname(aHostname)
> +    , mTime(aTime)
> +    , mCallback(new nsMainThreadPtrHolder<nsICertVerificationCallback>(aCallback))

I think this technically needs an #include "nsProxyRelease.h".

::: security/manager/ssl/tests/unit/head_psm.js:99
(Diff revision 1)
> +  certificateUsageSSLClient: certificateUsageSSLClient,
> +  certificateUsageSSLServer: certificateUsageSSLServer,
> +  certificateUsageSSLCA: certificateUsageSSLCA,
> +  certificateUsageEmailSigner: certificateUsageEmailSigner,
> +  certificateUsageEmailRecipient: certificateUsageEmailRecipient,
> +  certificateUsageObjectSigner: certificateUsageObjectSigner,
> +  certificateUsageVerifyCA: certificateUsageVerifyCA,
> +  certificateUsageStatusResponder: certificateUsageStatusResponder

These lines can use ES6 shorthand property names if you feel like it - e.g. just using |certificateUsageSSLClient,| etc here will work.

::: security/manager/ssl/tests/unit/head_psm.js:743
(Diff revision 1)
>    token.initPassword("");
>    token.login(/*force*/ false);
>  }
> +
> +// Helper for asyncTestCertificateUsages.
> +function CertVerificationResult(certName, usageString, successExpected) {

|CertVerificationResult| might be nicer as an ES6 class, but no big deal.

::: security/manager/ssl/tests/unit/head_psm.js:772
(Diff revision 1)
> + * verifications have been performed.
> + * Verification happens "now" with no specified flags or hostname.
> + *
> + * @param {nsIX509Cert} cert
> + *   The certificate to be verified.
> + * @param {Array} expectedUsages

Nit: I think {Number[]} is valid JSDoc syntax as well, and is more specific.

::: security/manager/ssl/tests/unit/head_psm.js:780
(Diff revision 1)
> + * @return {Promise}
> + *   A promise that will resolve with no value when all asynchronous operations
> + *   have completed.
> + */
> +function asyncTestCertificateUsages(cert, expectedUsages) {
> +  let deferred = Promise.defer();

I believe defer() is deprecated in favour of |new Promise(...)|, but there are other uses of defer() in PSM anyways, so maybe we can switch in a follow up bug.

::: security/manager/ssl/tests/unit/head_psm.js:786
(Diff revision 1)
> +  let now = (new Date()).getTime() / 1000;
> +  let promises = [];
> +  Object.keys(allCertificateUsages).forEach(usageString => {
> +    let usage = allCertificateUsages[usageString];
> +    let result = new CertVerificationResult(cert.commonName, usageString,
> +                                            expectedUsages.indexOf(usage) > -1);

Nit: |expectedUsages.includes(usage)|.

::: security/manager/ssl/tests/unit/head_psm.js:788
(Diff revision 1)
> +  Object.keys(allCertificateUsages).forEach(usageString => {
> +    let usage = allCertificateUsages[usageString];
> +    let result = new CertVerificationResult(cert.commonName, usageString,
> +                                            expectedUsages.indexOf(usage) > -1);
> +    promises.push(result.deferred);
> +    certdb.asyncVerifyCertAtTime(cert, usage, 0, null, now, result);

|certdb| is undefined here.
I guess this code currently only works because head_psm.js is concatenated with the test files that use it, and both test files using this function define |certdb| at the top level.

::: security/manager/ssl/tests/unit/head_psm.js:790
(Diff revision 1)
> +  Promise.all(promises).then(() => deferred.resolve());
> +  return deferred;

Hmm, can't we just do |return Promise.all(promises)| here?

::: security/manager/ssl/tests/unit/test_cert_keyUsage.js:60
(Diff revision 1)
> +  do_test_pending();
> +  Promise.all(promises).then(() => do_test_finished());

Nit: Looks like if we change run_test() to an add_task() call, this can be simplified to just |yield Promise.all(promises)|.

::: security/manager/ssl/tests/unit/test_intermediate_basic_usage_constraints.js:112
(Diff revision 1)
> +  do_test_pending();
> +  Promise.all(promises).then(() => do_test_finished());

Same add_task() comment as before.

Comment 8

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

https://reviewboard.mozilla.org/r/61668/#review59018

Thanks!

> It might be useful to note that this could be null.

Ok - I added some documentation.

> I think this technically needs an #include "nsProxyRelease.h".

Ok - added.

> These lines can use ES6 shorthand property names if you feel like it - e.g. just using |certificateUsageSSLClient,| etc here will work.

Cool - I didn't know that.

> |CertVerificationResult| might be nicer as an ES6 class, but no big deal.

Sure - changed.

> Nit: I think {Number[]} is valid JSDoc syntax as well, and is more specific.

Sounds good.

> I believe defer() is deprecated in favour of |new Promise(...)|, but there are other uses of defer() in PSM anyways, so maybe we can switch in a follow up bug.

I went ahead and changed it to the standard API for this case.

> Nit: |expectedUsages.includes(usage)|.

Ah, much nicer.

> |certdb| is undefined here.
> I guess this code currently only works because head_psm.js is concatenated with the test files that use it, and both test files using this function define |certdb| at the top level.

Whoops. Fixed by adding it as a paremeter to the function (which is how the other functions in head_psm.js handle this).

> Hmm, can't we just do |return Promise.all(promises)| here?

Heh - yeah.

> Nit: Looks like if we change run_test() to an add_task() call, this can be simplified to just |yield Promise.all(promises)|.

Ah, good idea. In fact, I just changed the list of promises to a series of yields.

Comment 9

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

Comment on attachment 8766967 [details]
bug 1023621 - add asynchronous certificate verification API

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61668/diff/1-2/

Comment 10

[:Cykesiopka]

https://reviewboard.mozilla.org/r/61668/#review59284

::: security/manager/ssl/tests/unit/test_cert_keyUsage.js:59
(Diff revision 2)
> +function run_test() {
> +  run_next_test();
>  }

FWIW run_test() doesn't need to be defined if there is at least one add_task() call now.

Comment 11

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

Comment on attachment 8766967 [details]
bug 1023621 - add asynchronous certificate verification API

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61668/diff/2-3/

Comment 12

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

(In reply to :Cykesiopka from comment #10)
> https://reviewboard.mozilla.org/r/61668/#review59284
> 
> ::: security/manager/ssl/tests/unit/test_cert_keyUsage.js:59
> (Diff revision 2)
> > +function run_test() {
> > +  run_next_test();
> >  }
> 
> FWIW run_test() doesn't need to be defined if there is at least one
> add_task() call now.

Ah, that's nice. I was thinking the boilerplate was a bit of a bummer.

Here's the try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=30ad986fc2c1 (the failing test appears to be intermittent, but the auto-suggester isn't working for me).

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ca87e365282
add asynchronous certificate verification API r=Cykesiopka

Comment 14

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/5ca87e365282