Closed
Bug 867473
Opened 11 years ago
Closed 6 years ago
remove nsIX509Cert.getChain (and nsIX509Cert.issuer)
Categories
(Core :: Security: PSM, defect, P1)
Core
Security: PSM
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)
59 bytes,
text/x-review-board-request
|
franziskus
:
review+
jcj
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
franziskus
:
review+
jcj
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
franziskus
:
review+
jcj
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jcj
:
review+
|
Details |
+++ 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.
Assignee | ||
Updated•8 years ago
|
Whiteboard: [Snappy] → [Snappy][psm-backlog]
Assignee | ||
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 2•6 years ago
|
||
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]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Comment 7•6 years ago
|
||
mozreview-review |
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 8•6 years ago
|
||
mozreview-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 9•6 years ago
|
||
mozreview-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 10•6 years ago
|
||
mozreview-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 11•6 years ago
|
||
mozreview-review |
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 12•6 years ago
|
||
mozreview-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 13•6 years ago
|
||
mozreview-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+
Assignee | ||
Comment 14•6 years ago
|
||
(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 15•6 years ago
|
||
mozreview-review |
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+
Comment 16•6 years ago
|
||
> 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)
Assignee | ||
Comment 17•6 years ago
|
||
Thanks for the reviews! try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffcc94714eb57f5596cf3eb7a99cfcf63dbd2ff3
Comment 18•6 years ago
|
||
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
Comment 19•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d419e0c3d798 https://hg.mozilla.org/mozilla-central/rev/864b6ccea302 https://hg.mozilla.org/mozilla-central/rev/2cf15e5e3918 https://hg.mozilla.org/mozilla-central/rev/05d72da7c0a3
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•