Closed Bug 867473 Opened 7 years ago Closed 2 years ago

remove nsIX509Cert.getChain (and nsIX509Cert.issuer)

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: briansmith, Assigned: keeler)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat, main-thread-io, perf, Whiteboard: [Snappy][psm-assigned])

Attachments

(4 files)

+++ This bug was initially created as a clone of Bug #867432 +++
+++ This bug was initially created as a clone of Bug #775698 +++

Certificate validation does disk I/O and/or network I/O so it should never be done on the main thread. Unfortunately, nsIX509Cert.getChain is impossible to implement with its current synchronous signature without doing network I/O or disk I/O on the main thread.

Also, the existence of this function complicates the insanity::pkix integration into Firefox.

In order to remove this function, we need to pass in a certificate chain to the certificate viewer so that it knows which chain to show.
Whiteboard: [Snappy] → [Snappy][psm-backlog]
Duplicate of this bug: 1001513
Priority: -- → P3
I think the only remaining uses of this are in exportToFile in pippki.js in a function that's already async and in the implementation of nsIX509Cert.issuer, which should also be removable after bug 1454504 lands.
Assignee: nobody → dkeeler
Depends on: 1454504
Priority: P3 → P1
Summary: Remove nsIX509Cert.getChain → remove nsIX509Cert.getChain (and nsIX509Cert.issuer)
Whiteboard: [Snappy][psm-backlog] → [Snappy][psm-assigned]
Depends on: 1406856
No longer depends on: 1049110
Comment on attachment 8968693 [details]
bug 867473 - (1/4) refactor certificate chain utility functions in certViewer.js

https://reviewboard.mozilla.org/r/237378/#review243238

lgtm
Attachment #8968693 - Flags: review?(franziskuskiefer) → review+
Comment on attachment 8968694 [details]
bug 867473 - (2/4) move certificate chain utility functions to a shared location

https://reviewboard.mozilla.org/r/237380/#review243240

lgtm
Attachment #8968694 - Flags: review?(franziskuskiefer) → review+
Comment on attachment 8968695 [details]
bug 867473 - (3/4) replace use of nsIX509Cert.getChain() with an asynchronous API

https://reviewboard.mozilla.org/r/237382/#review243252
Attachment #8968695 - Flags: review?(franziskuskiefer) → review+
Comment on attachment 8968696 [details]
bug 867473 - (4/4) remove nsIX509Cert.issuer and getChain

https://reviewboard.mozilla.org/r/237384/#review243246

Patch generally looks good but we either have to keep `issuer` or rework `CertUtils`.

::: security/manager/ssl/nsIX509Cert.idl
(Diff revision 1)
>  
>    /**
> -   *  The certificate used by the issuer to sign this certificate.
> -   */
> -  [must_use]
> -  readonly attribute nsIX509Cert issuer;

`CertUtils.jsm:checkCert` seems to be using this.
Attachment #8968696 - Flags: review?(franziskuskiefer)
Comment on attachment 8968693 [details]
bug 867473 - (1/4) refactor certificate chain utility functions in certViewer.js

https://reviewboard.mozilla.org/r/237378/#review243254
Attachment #8968693 - Flags: review?(jjones) → review+
Comment on attachment 8968694 [details]
bug 867473 - (2/4) move certificate chain utility functions to a shared location

https://reviewboard.mozilla.org/r/237380/#review243256

much move
Attachment #8968694 - Flags: review?(jjones) → review+
Comment on attachment 8968695 [details]
bug 867473 - (3/4) replace use of nsIX509Cert.getChain() with an asynchronous API

https://reviewboard.mozilla.org/r/237382/#review243264

okay good good
Attachment #8968695 - Flags: review?(jjones) → review+
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #10)
> Comment on attachment 8968696 [details]
> bug 867473 - (4/4) remove nsIX509Cert.issuer and getChain
> 
> https://reviewboard.mozilla.org/r/237384/#review243246
> 
> Patch generally looks good but we either have to keep `issuer` or rework
> `CertUtils`.
> 
> ::: security/manager/ssl/nsIX509Cert.idl
> (Diff revision 1)
> >  
> >    /**
> > -   *  The certificate used by the issuer to sign this certificate.
> > -   */
> > -  [must_use]
> > -  readonly attribute nsIX509Cert issuer;
> 
> `CertUtils.jsm:checkCert` seems to be using this.

Right - bug 1454504 was still in flight when I asked for review. Now that it's landed, we should be good to go, unless I'm missing another instance.
Flags: needinfo?(franziskuskiefer)
Comment on attachment 8968696 [details]
bug 867473 - (4/4) remove nsIX509Cert.issuer and getChain

https://reviewboard.mozilla.org/r/237384/#review243442

I don't see anything here that needs changing. r+!
Attachment #8968696 - Flags: review?(jjones) → review+
> Right - bug 1454504 was still in flight when I asked for review. Now that it's landed, we should be good to go, unless I'm missing another instance.

Ah cool. No that's the only use I found. r+
Flags: needinfo?(franziskuskiefer)
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d419e0c3d798
(1/4) refactor certificate chain utility functions in certViewer.js r=fkiefer,jcj
https://hg.mozilla.org/integration/autoland/rev/864b6ccea302
(2/4) move certificate chain utility functions to a shared location r=fkiefer,jcj
https://hg.mozilla.org/integration/autoland/rev/2cf15e5e3918
(3/4) replace use of nsIX509Cert.getChain() with an asynchronous API r=fkiefer,jcj
https://hg.mozilla.org/integration/autoland/rev/05d72da7c0a3
(4/4) remove nsIX509Cert.issuer and getChain r=jcj
You need to log in before you can comment on or make changes to this bug.