Last Comment Bug 867432 - Remove nsIX509Cert.verifyForUsage
: Remove nsIX509Cert.verifyForUsage
Status: RESOLVED FIXED
[Snappy]
: addon-compat, APIchange, main-thread-io, perf
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla24
Assigned To: Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
:
: David Keeler [:keeler] (use needinfo?)
Mentors:
Depends on: 867437 867445
Blocks: 775698
  Show dependency treegraph
 
Reported: 2013-04-30 15:54 PDT by Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
Modified: 2013-06-17 16:39 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove nsIX509Cert.verifyForUsage (7.52 KB, patch)
2013-05-01 17:38 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
cviecco: review+
honzab.moz: superreview+
Details | Diff | Splinter Review

Description Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-04-30 15:54:29 PDT
+++ 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.verifyForUsage 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.

The only user in the tree is Test Pilot. I looked at the code in Test Pilot and it seems unnecessary. I filed bug 867430 about that.
Comment 1 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-04-30 16:09:35 PDT
We may not need to fix bug 867430 for this:

On #developers
<gavin> bsmith: we're going to just remove testpilot
Comment 2 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-05-01 17:38:20 PDT
Created attachment 744359 [details] [diff] [review]
Remove nsIX509Cert.verifyForUsage

Here's the try run: https://tbpl.mozilla.org/?tree=Try&rev=015b6dd0e28b
Comment 3 :Mook 2013-05-01 22:43:33 PDT
Drive-by:

Note that you might want to update the documentation to stop using this method:

https://developer.mozilla.org/en-US/docs/How_to_check_the_security_state_of_an_XMLHTTPRequest_over_SSL

It may also be useful to document (asynchronous) methods for addons to migrate to.
Comment 4 Camilo Viecco (:cviecco) 2013-05-02 09:43:07 PDT
Comment on attachment 744359 [details] [diff] [review]
Remove nsIX509Cert.verifyForUsage

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

r+ with comment addressed.

::: security/manager/ssl/public/nsIX509Cert.idl
@@ +155,5 @@
>    
>    /**
>     *  Constants that describe the certified usages of a certificate.
> +   *
> +   *  Deprecated and unused

File bug to remove these in the future or remove them now.
Comment 5 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-05-07 20:19:08 PDT
(In reply to :Mook from comment #3)
> Drive-by:
> 
> Note that you might want to update the documentation to stop using this
> method:
> 
> https://developer.mozilla.org/en-US/docs/
> How_to_check_the_security_state_of_an_XMLHTTPRequest_over_SSL

I updated the wiki. Please review:

https://developer.mozilla.org/en-US/docs/How_to_check_the_security_state_of_an_XMLHTTPRequest_over_SSL

> It may also be useful to document (asynchronous) methods for addons to
> migrate to.

There isn't a general-purpose async counterpart, currently.
Comment 6 Honza Bambas (:mayhemer) 2013-05-13 13:26:36 PDT
What is the plan for:

http://mxr.mozilla.org/comm-central/search?find=%2Fmailnews%2F&string=VerifyForUsage

We should provide comm-central e.g. a static function duplicating the original method.  Or do you have a different idea?

This patch will effectively break comm-central builds.
Comment 7 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-05-13 14:16:33 PDT
(In reply to Honza Bambas (:mayhemer) from comment #6)
> What is the plan for:
> 
> http://mxr.mozilla.org/comm-central/
> search?find=%2Fmailnews%2F&string=VerifyForUsage
> 
> We should provide comm-central e.g. a static function duplicating the
> original method.  Or do you have a different idea?

See bug 867437. The calls to verifyForUsage in comm-central can/should just be removed.

> This patch will effectively break comm-central builds.

Yes, lots of the patches related to this will break comm-central until comm-central can be updated to undo the breakage.

In general, all the S/MIME-related functionality should be moved from mozilla-central to comm-central so that we don't have to maintain it as part of PSM.
Comment 8 Honza Bambas (:mayhemer) 2013-05-21 07:49:04 PDT
Comment on attachment 744359 [details] [diff] [review]
Remove nsIX509Cert.verifyForUsage

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

Sorry for the delay.

sr=honzab
Comment 9 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-06-16 22:22:20 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d8b09a16c50

Thanks for the reviews.
Comment 10 Ryan VanderMeulen [:RyanVM] 2013-06-17 16:39:35 PDT
https://hg.mozilla.org/mozilla-central/rev/7d8b09a16c50

Note You need to log in before you can comment on or make changes to this bug.