Last Comment Bug 649154 - (domcrypt) Implement DOMCrypt (Internal) API
(domcrypt)
: Implement DOMCrypt (Internal) API
Status: RESOLVED WONTFIX
[sec-assigned:curtisk:744938]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal with 7 votes (vote)
: ---
Assigned To: David Dahl :ddahl
:
: Andrew Overholt [:overholt]
Mentors:
https://wiki.mozilla.org/Privacy/Feat...
: 250792 542879 686249 741677 (view as bug list)
Depends on: 744938
Blocks: webapi 721193 721199 721200 657432 669927 746467 746469 746471 746472 746473 746476 820058
  Show dependency treegraph
 
Reported: 2011-04-11 15:33 PDT by David Dahl :ddahl
Modified: 2014-01-30 10:35 PST (History)
56 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v 0.1 WIP saving work (20.96 KB, patch)
2011-04-12 22:40 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.2 WIP - NSS is running in a ChromeWorker now (84.05 KB, patch)
2011-04-13 21:13 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
Worker Assert 'NS_IsMainThread()' (10.16 KB, text/plain)
2011-04-14 12:31 PDT, David Dahl :ddahl
no flags Details
v 0.3 WIP Filling in the rest of the API (95.78 KB, patch)
2011-04-14 22:04 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.4 All Crypto API methods now work (105.34 KB, patch)
2011-04-15 22:39 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.5 Test suite coming along - most methods are tested (108.55 KB, patch)
2011-04-18 22:12 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.6 API is fully tested now - need to test 3 additional methods (115.74 KB, patch)
2011-04-19 21:38 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
Occasional crash when showing a modal dialog (20.25 KB, text/plain)
2011-04-21 19:17 PDT, David Dahl :ddahl
no flags Details
v 0.7 Added prompt tests (116.92 KB, patch)
2011-04-21 21:45 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.8 refactored callback storage (120.42 KB, patch)
2011-04-23 09:23 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.8.1 Crushed the leaks, yay (118.49 KB, patch)
2011-04-25 21:32 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.8.2 async file issues (120.50 KB, patch)
2011-04-27 20:41 PDT, David Dahl :ddahl
sdwilsh: feedback-
Details | Diff | Splinter Review
v 0.9 Most of sdwilsh's feedback is resolved (127.89 KB, patch)
2011-05-06 22:26 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.9.1 most comments addressed (131.55 KB, patch)
2011-05-11 15:12 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
WIP WebIDL of current proposed spec (3.23 KB, text/plain)
2011-06-04 21:55 PDT, David Dahl :ddahl
no flags Details
WebIDL (2.83 KB, text/plain)
2011-06-05 01:25 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details
v 0.1 Chatter Bookmarklet - WIP (11.03 KB, text/plain)
2011-07-28 08:25 PDT, David Dahl :ddahl
no flags Details
v 1 Psuedocode (3.31 KB, text/plain)
2011-12-01 17:20 PST, David Dahl :ddahl
no flags Details
domcrypt-platform-generate-keypair v 1 (6.33 KB, patch)
2011-12-16 16:30 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
domcrypt-platform-generate-keypair v 1.1 (7.29 KB, patch)
2012-01-11 09:27 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v. 2 - it builds! (12.67 KB, patch)
2012-01-12 15:07 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v. 2.1 XPCOM Errors (18.29 KB, patch)
2012-01-13 09:30 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 2.2 persistent key generation fails (25.71 KB, patch)
2012-01-19 22:20 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
backtrace (4.34 KB, text/plain)
2012-01-20 15:12 PST, David Dahl :ddahl
no flags Details
v 3 Adding rough draft of PKEncrypt and PKDecrypt, still has keypair issues (25.51 KB, patch)
2012-01-23 22:04 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 4 Linking problem (26.82 KB, patch)
2012-01-26 21:03 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 4.1 Cannot use private NSS functions, etc... (27.10 KB, patch)
2012-01-26 22:19 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 4.2 Naive Patch with copied functions, it'll never work (33.41 KB, patch)
2012-01-30 16:14 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
irc discussion with kaie (12.37 KB, text/plain)
2012-02-01 06:24 PST, David Dahl :ddahl
no flags Details
v. 4.3 breaking out PSM from XPCOM/DOM untested (45.28 KB, patch)
2012-02-01 17:30 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
4.4 Attempting to isolate all XPCOM bits to dom/ (41.42 KB, patch)
2012-02-14 10:58 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
4.5 Builds now, added PKDecrypt (41.80 KB, patch)
2012-02-14 21:58 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 4.6 Build errors, header export problem (47.23 KB, patch)
2012-02-15 15:58 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 4.7 Saving Latest Work, more build errors (27.84 KB, patch)
2012-02-17 15:16 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 4.8 include errors, trouble with exports and nss headers (28.09 KB, patch)
2012-02-24 16:31 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 4.9 It builds and Links! (23.65 KB, patch)
2012-02-29 14:33 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 5 It Builds, but the component is not available in Components.classes (26.57 KB, patch)
2012-02-29 22:09 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 5.1 build problems (26.87 KB, patch)
2012-03-01 17:16 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 5.2 (27.08 KB, patch)
2012-03-02 04:13 PST, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
v 5.3 Builds - cleaning up runtime issues (29.75 KB, patch)
2012-04-11 15:28 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
new attempt at nsNSSShutdownObject inheritance (29.68 KB, patch)
2012-04-13 14:10 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
nss loading issues (30.02 KB, patch)
2012-04-13 22:59 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
5.4 GenerateKeyPair is working, test is passing (31.73 KB, patch)
2012-04-17 16:31 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 5.5 Patch - PKGenerateKeyPair (31.71 KB, patch)
2012-04-17 21:58 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 5.5 Moved keygen to a thread, needs a test (36.39 KB, patch)
2012-04-20 16:57 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
Patch 5.6 - Keygen on thread tests passing (36.81 KB, patch)
2012-04-30 16:22 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
Patch 5.7 build fails, just saving work (42.92 KB, patch)
2012-05-03 19:40 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
Patch 5.8: PKEncrypt builds (43.52 KB, patch)
2012-05-04 21:37 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
Added PKEncryptResult interface, fixed some crashes (58.53 KB, patch)
2012-07-11 13:26 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
Latest Patch, builds! (63.26 KB, patch)
2012-11-30 12:57 PST, David Dahl :ddahl
no flags Details | Diff | Splinter Review

Description David Dahl :ddahl 2011-04-11 15:33:30 PDT
Port DOMCrypt Firefox extension into mozilla-central as a DOM property. see https://github.com/daviddahl/domcrypt/ and http://domcrypt.org This will initially be preffed off as we get feedback from web developers, endusers and work out the possible use of crypto standards or look into standardization.

Firefox Project page is here: https://wiki.mozilla.org/DOMCryptAPI

DOMCrypt currently has a simple, 'webby' API:

crypt.generateKeyPair()
crypt.encrypt()
crypt.decrypt()
crypt.prompDecrypt()
crypt.makeHash()
crypt.getPubKey()
crypt.sign()
crypt.verify()

All of this API wraps a js-ctypes wrapping of NSS. (A former incarnation of WeaveCrypto.js)

There is also an "AddressbookManager" API that allows endusers to exchange public keys (or 'addressbook entries') via a simple meta-tag / notification prompt

crypt.getAddressbook()

Internally, DOMCrypt sniffs out addressbook-entry meta tags and prompts the user to save the contents of this tag to the addressbook.

==================================
Improvements that need to be made:

1. Move all crypto API work to a ChromeWorker with a callback that is executed in a document sandbox

2. Namespace this API so additional APIs can be added and future standardization issues can be dealt with. I think the current API should reside under crypt.pk.*

3. decide on a better name for the property. crypt is too close to crypto, which is already a property of the window.

4. a comprehensive browser test suite
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-04-11 15:42:12 PDT
You're aware of window.crypto, right?  I think adding a separate window.crypt will be pretty confusing to web authors...
Comment 2 David Dahl :ddahl 2011-04-11 15:54:45 PDT
(In reply to comment #1)
> You're aware of window.crypto, right?  I think adding a separate window.crypt
> will be pretty confusing to web authors...

Yes. in my extension I called it crypt, it was just a placeholder name. A new name is a top priority.
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2011-04-12 04:01:51 PDT
Has this been discussed with any other browser vendors?
Comment 4 David Dahl :ddahl 2011-04-12 06:37:32 PDT
(In reply to comment #3)
> Has this been discussed with any other browser vendors?

Not yet, but it will be.
Comment 5 David Dahl :ddahl 2011-04-12 10:15:33 PDT
(In reply to comment #0)
> Firefox Project page is here: https://wiki.mozilla.org/DOMCryptAPI

Moved this page to https://wiki.mozilla.org/Privacy/Features/DOMCryptAPI

and on the roadmap here: https://wiki.mozilla.org/Privacy/Roadmap_2011#Enhance_User_Controlled_Disclosure
Comment 6 David Dahl :ddahl 2011-04-12 22:40:33 PDT
Created attachment 525624 [details] [diff] [review]
v 0.1 WIP saving work

Getting things working so that the property is available to content and the jsm is available to addons and we are using a worker for heavy lifting
Comment 7 David Dahl :ddahl 2011-04-13 21:13:25 PDT
Created attachment 525922 [details] [diff] [review]
v 0.2 WIP - NSS is running in a ChromeWorker now

generateKeypair is working off main thread
Comment 8 David Dahl :ddahl 2011-04-14 11:51:55 PDT
I have noticed a single ASSERT when calling the generateKeyPair method in the worker:

###!!! ASSERTION: Wrong thread!: 'NS_IsMainThread()', file /home/ddahl/code/moz/mozilla-central/dom/src/threads/nsDOMWorker.cpp, line 929

http://mxr.mozilla.org/mozilla-central/source/dom/src/threads/nsDOMWorker.cpp#929

However, it all works, there is no crash, and other NSS methods wrapped with ctypes do not assert.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2011-04-14 11:54:07 PDT
That assert doesn't look happy (in the "probably exploitable if you try" sense).
Comment 10 David Dahl :ddahl 2011-04-14 12:30:36 PDT
Some more context:

I create the worker in the jsm on import:

var workerFactory = Cc["@mozilla.org/threads/workerfactory;1"].
                      createInstance(Ci.nsIWorkerFactory);

var worker = workerFactory.newChromeWorker("domcrypt_worker.js");

Now that I put in some logging messages, I see that this ASSERT happens before I even call any of the worker's methods:

###!!! ASSERTION: Wrong thread!: 'NS_IsMainThread()', file /home/ddahl/code/moz/mozilla-central/dom/src/threads/nsDOMWorker.cpp, line 929
*** DOMCryptMethods: save generateKeypairCallback
*** DOMCryptMethods: Calling the Worker...
WeaveCrypto: Worker started

It seems like this happens even before I init NSS - I will attach a backtrace
Comment 11 David Dahl :ddahl 2011-04-14 12:31:43 PDT
Created attachment 526080 [details]
Worker Assert 'NS_IsMainThread()'

Backtrace
Comment 12 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-04-14 13:19:56 PDT
It's not clear to me whether or not this is a problem yet. Basically it's fallout from our patch to use the tracing API to 'un-gray' XPConnect objects.
Comment 13 David Dahl :ddahl 2011-04-14 13:35:46 PDT
(In reply to comment #12)
> It's not clear to me whether or not this is a problem yet. Basically it's
> fallout from our patch to use the tracing API to 'un-gray' XPConnect objects.

ok, let me know if you need a test case or anything else to help figure it out.
Comment 14 David Dahl :ddahl 2011-04-14 22:04:24 PDT
Created attachment 526183 [details] [diff] [review]
v 0.3 WIP Filling in the rest of the API
Comment 15 David Dahl :ddahl 2011-04-15 22:39:29 PDT
Created attachment 526459 [details] [diff] [review]
v 0.4 All Crypto API methods now work

Next: test suite
Comment 16 David Dahl :ddahl 2011-04-18 22:11:05 PDT
still working on the test suite - experiencing a crash in a browser chrome test:

http://pastebin.mozilla.org/1207414

Somewhat unclear why I am getting this:

WARNING: NS_ENSURE_SUCCESS(result, result) failed with result 0x80004005: file /home/ddahl/code/moz/mozilla-central/editor/libeditor/base/nsEditor.cpp, line 3927
WARNING: NS_ENSURE_SUCCESS(res, res) failed with result 0x80004005: file /home/ddahl/code/moz/mozilla-central/editor/libeditor/text/nsTextEditRules.cpp, line 471
Assertion failure: wrong tag, at /home/ddahl/code/moz/mozilla-central/js/src/jsgc.cpp:694

// debugging just ensued...

Looks like this may have had to do with my calling Cu.forceGC at an inopportune moment. It is not crashing any longer. The strange thing is that the content script I have written as a manual test did not exhibit this problem. 

The dialog is still not accepted however.
Comment 17 David Dahl :ddahl 2011-04-18 22:12:58 PDT
Created attachment 526933 [details] [diff] [review]
v 0.5 Test suite coming along - most methods are tested
Comment 18 David Dahl :ddahl 2011-04-19 21:38:25 PDT
Created attachment 527188 [details] [diff] [review]
v 0.6 API is fully tested now - need to test 3 additional methods
Comment 19 David Dahl :ddahl 2011-04-21 19:17:07 PDT
Created attachment 527713 [details]
Occasional crash when showing a modal dialog

backtrace: Every 20 times (or so) I run a manual test that quickly runs through the entire API, I see a crash having to do with the modal dialog shown before the keypair is generated
Comment 20 David Dahl :ddahl 2011-04-21 21:45:59 PDT
Created attachment 527720 [details] [diff] [review]
v 0.7 Added prompt tests

Almost ready for review, needs another test. current TODO list:

1. start using passphrase() getter * done *
2. test additional prompts * done *
3. handle error conditions in the worker more gracefully * done *
4. move to NetUtils and Fileutils for all filesystem access
5. decrypt should check the passphrase cache first - test needs to be written for this where we set the TTL to 1 second
6. change API property name to mozCipher from cipher?
7. expose validate passphrase method
Comment 21 David Dahl :ddahl 2011-04-23 09:23:35 PDT
Created attachment 527944 [details] [diff] [review]
v 0.8 refactored callback storage

Currently hunting down a leak I have introduced
Comment 22 David Dahl :ddahl 2011-04-25 21:32:45 PDT
Created attachment 528262 [details] [diff] [review]
v 0.8.1 Crushed the leaks, yay
Comment 23 David Dahl :ddahl 2011-04-27 20:41:42 PDT
Created attachment 528782 [details] [diff] [review]
v 0.8.2 async file issues

In my attempt to make all file reading and writing async, I just cannot figure out the best way to initialize the window property - I also think there is something wrong with my file writing function in DOMCryptMethods.jsm

You can start a manual test of the API by loading this file: obj-dir/_tests/testing/mochitest/browser/dom/tests/browser/test-domcrypt.html
Comment 24 Shawn Wilsher :sdwilsh 2011-04-29 11:30:25 PDT
Comment on attachment 528782 [details] [diff] [review]
v 0.8.2 async file issues

Review of attachment 528782 [details] [diff] [review]:

I didn't look at the tests, but here's your high-level feedback.  I think you are headed in the right direction, but there are some API issues that need to get worked out before this is feedback+able.

::: browser/app/profile/firefox.js
@@ +1009,5 @@
 // Whether the Panorama should animate going in/out of tabs
 pref("browser.panorama.animate_zoom", true);
+
+// DOMCrypt prefs
+pref("dom.domcrypt.enabled", true);

remember that you'll want this to be false in your final patch

@@ +1010,5 @@
 pref("browser.panorama.animate_zoom", true);
+
+// DOMCrypt prefs
+pref("dom.domcrypt.enabled", true);
+pref("dom.domcrypt.passphraseTTL", 3600000);

this needs a comment about what it's for and the units it is in

::: dom/base/DOMCrypt.js
@@ +12,5 @@
+ * License.
+ *
+ * The Original Code is DOMCrypt API code.
+ *
+ * The Initial Developer of the Original Code is Mozilla Foundation

global-nit: per https://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c, "the Mozilla Foundation." should be on a newline.

@@ +113,5 @@
+   * This method sets up the crypto API and returns the object that is
+   * accessible from the DOM
+   *
+   * @param nsIDOMWindow aWindow
+   * @returns object

global-nit: need more documentation on all the @param comments.  I also don't think repeating the method name in the comment right above the method is terribly useful.

@@ +126,5 @@
+                              { sandboxPrototype: this.window, wantXrays: false });
+
+    Services.obs.addObserver(this, "inner-window-destroyed", false);
+
+    // TODO: change the property name to mozCipher?

We absolutely need to prefix it.  We may want to look into extending the current DOM property that bz mentioned in comment 1.

::: dom/base/DOMCryptMethods.jsm
@@ +46,5 @@
+XPCOMUtils.defineLazyServiceGetter(this, "promptSvc",
+                                   "@mozilla.org/embedcomp/prompt-service;1",
+                                   "nsIPromptService");
+
+XPCOMUtils.defineLazyServiceGetter(this, "SDR",

I would prefer a more descriptive variable name here.

@@ +69,5 @@
+ * @returns string
+ */
+function getStr(aName)
+{
+  return stringBundle.GetStringFromName(aName);

You probably don't want to have to grab these each time you want to use them because that may cause disk I/O (if I recall correctly).  You can memoize them as you need them by copying code like this:
https://hg.mozilla.org/mozilla-central/annotate/ec809c159ad2/toolkit/mozapps/downloads/DownloadUtils.jsm#l103

@@ +103,5 @@
+// We can call ChromeWorkers from this JSM via nsIWorkerFactory
+var workerFactory = Cc["@mozilla.org/threads/workerfactory;1"].
+                      createInstance(Ci.nsIWorkerFactory);
+
+var worker = workerFactory.newChromeWorker("domcrypt_worker.js");

I highly suspect you'll want to do this work lazily as there is no reason to create this on the first import of the JSM.  You can make DOMCryptMethods a lazy getter and do this work at the first access.  Or even better, do it the first time you need to use the worker (so make worker a lazy getter).

@@ +107,5 @@
+var worker = workerFactory.newChromeWorker("domcrypt_worker.js");
+
+worker.onmessage = function DCM_worker_onmessage(aEvent) {
+  switch (aEvent.data.action) {
+  case "keypairGenerated":

All of your actions here should be constants instead of magic strings.

@@ +108,5 @@
+
+worker.onmessage = function DCM_worker_onmessage(aEvent) {
+  switch (aEvent.data.action) {
+  case "keypairGenerated":
+    DOMCryptMethods.handleGenerateKeypair(aEvent.data.keypairData);

All of your handle* methods are on DOMCryptMethods, but they really should only be used internally, correct?  In general, any helper methods should not be on the object you export.  This holds true with properties you use to store things (such as the callbacks).  NetUtil.jsm keeps these things in the global scope which prevents them from being exported.

@@ +165,5 @@
+ *
+ * DOMCryptMethods' onmessage chooses which callback to execute in the original
+ * content window's sandbox.
+ */
+var DOMCryptMethods = {

A bunch of methods on this object need comments explaining what they do and what their arguments are.

@@ +190,5 @@
+  },
+
+  callbacks: null,
+
+  registerCallback: function DCM_registerCallback(aLabel, aCallback, aSandbox)

I think it's overly clunky to make consumers of this have to worry about this themselves.  Each method that takes a callback should be able to manage this itself (and this means each of those methods should also grow an aCallback parameter).

@@ +202,5 @@
+  },
+
+  /**
+   * wrap the content-provided script in order to make it easier
+   * to import and run in the sandbox

This does not seem to match what this method is doing at all.

@@ +208,5 @@
+   * @param string aPubKey
+   * @returns function
+   */
+  makeGenerateKeypairCallback:
+  function DA_makeGenerateKeypairCallback(aPubKey)

The prefix on this method does not match the rest.

@@ +215,5 @@
+    let callback = function generateKeypair_callback()
+                   {
+                     self.callbacks.generateKeypair.callback(aPubKey);
+                   };
+    return callback;

FWIW, instead of using self, you can just say |return callback.bind(this);|

@@ +471,5 @@
+    let prompt =
+      promptSvc.promptPassword(this.callbacks.generateKeypair.sandbox.window,
+                               getStr("enterPassphraseTitle"),
+                               getStr("enterPassphraseText"),
+                               passphrase, null, { value: false });

I suspect we don't want to default to using the promptservice for this (although it'd be a good fallback).  Talk to dolske about what to use here.

@@ +503,5 @@
+   */
+  generateKeypair: function DCM_generateKeypair(aPassphrase)
+  {
+    worker.postMessage({ action: "generateKeypair", passphrase: aPassphrase });
+    this.passphraseCache.encryptedPassphrase = SDR.encryptString(aPassphrase);

Do we really want to encrypt something on the main thread?

@@ +527,5 @@
+   * @param string aPlainText
+   * @param string aPublicKey
+   * @returns void
+   */
+  encrypt: function DCM_encrypt(aPlainText, aPublicKey)

You should verify that you are getting what you expect in your arguments and if not throw an error (with the call stack pointing to the caller).  See https://hg.mozilla.org/mozilla-central/annotate/1156b1885571/netwerk/base/src/NetUtil.jsm#l289 for an example on how to do that.  You should also test all the error checking you introduce to make sure you throw errors as expected.

@@ +803,5 @@
+    // get profile directory
+    let file = Cc["@mozilla.org/file/directory_service;1"].
+                 getService(Ci.nsIProperties).
+                 get("ProfD", Ci.nsIFile);
+    file.append(".domcrypt.json");

Use FileUtils.getFile (https://hg.mozilla.org/mozilla-central/annotate/b15803d8aee8/toolkit/mozapps/shared/FileUtils.jsm#l61)

@@ +849,5 @@
+      let converter = Cc["@mozilla.org/intl/converter-output-stream;1"].
+                        createInstance(Ci.nsIConverterOutputStream);
+      converter.init(foStream, "UTF-8", 0, 0);
+      converter.writeString(data);
+      converter.close();

https://developer.mozilla.org/en/Code_snippets/File_I%2f%2fO#Write_a_string but you may not want to use the safe file output stream depending on what it actually being stored here.

@@ +854,5 @@
+    }
+    catch (ex) {
+      log(ex);
+      log(ex.stack);
+    }

What exactly will error here?

@@ +904,5 @@
+                    createInstance(Ci.nsIFileInputStream);
+    let cstream = Cc["@mozilla.org/intl/converter-input-stream;1"].
+                    createInstance(Ci.nsIConverterInputStream);
+    fstream.init(aFile, -1, 0, 0);
+    cstream.init(fstream, "UTF-8", 0, 0);

https://developer.mozilla.org/en/Code_snippets/File_I%2f%2fO#Read_into_a_stream_or_a_string but see the section *right* below it too.

@@ +951,5 @@
+ */
+function initializeDOMCrypt()
+{
+  let fileCreated = false;
+  let file = DOMCryptMethods.configurationFile(fileCreated);

This isn't going to work like you think it is.  In fact, fileCreated is always going to be false.  If you want to pass by reference, you have to pass in an object ({value: false}) and then check the value property after the call.

@@ +984,5 @@
+{
+  let fileCreated = false;
+  let file = DOMCryptMethods.configurationFile(fileCreated);
+
+  let ostream = FileUtils.openSafeFileOutputStream(file);

Depending on what you are saving here and how much you care about it not being corrupted, you may not want to use a safe file output stream.

::: dom/base/domcrypt_worker.js
@@ +46,5 @@
+ * WeaveCrypto's API as it was released in Firefox 4 was reduced in scope due
+ * to Sync's move to J-Pake, hence the need for this more complete version.
+ *
+ * This version has the additional APIs 'sign' and 'verify' and has been
+ * edited for use in a ChromeWorker.

I think you should consider renaming this object now.
Comment 25 David Dahl :ddahl 2011-05-06 07:28:01 PDT
(In reply to comment #24)
> Comment on attachment 528782 [details] [diff] [review] [review]

> @@ +126,5 @@
> +                              { sandboxPrototype: this.window, wantXrays:
> false });
> +
> +    Services.obs.addObserver(this, "inner-window-destroyed", false);
> +
> +    // TODO: change the property name to mozCipher?
> 
> We absolutely need to prefix it.  We may want to look into extending the
> current DOM property that bz mentioned in comment 1.
> 
Was bz suggesting a consolidation, or a new name? I would prefer just calling it "window.mozCipher" for now until more feedback comes in from the community and other browser makers.

> @@ +190,5 @@
> +  },
> +
> +  callbacks: null,
> +
> +  registerCallback: function DCM_registerCallback(aLabel, aCallback,
> aSandbox)
> 
> I think it's overly clunky to make consumers of this have to worry about
> this themselves.  Each method that takes a callback should be able to manage
> this itself (and this means each of those methods should also grow an
> aCallback parameter).
> 
Indeed.

This is a half-measure right now. I am trying to take this pattern and automate it. I also had some weird crashes happen a few times when setting the callback inside the main api method. Not sure why. It was intermittent and may have been unrelated. In any case, this does need to be made simpler.

> @@ +471,5 @@
> +    let prompt =
> +     
> promptSvc.promptPassword(this.callbacks.generateKeypair.sandbox.window,
> +                               getStr("enterPassphraseTitle"),
> +                               getStr("enterPassphraseText"),
> +                               passphrase, null, { value: false });
> 
> I suspect we don't want to default to using the promptservice for this
> (although it'd be a good fallback).  Talk to dolske about what to use here.
> 
The only thing I am concerned about is not allowing any passphrases to be typed into content, that would be bad.
CCing dolske on this.
Comment 26 Boris Zbarsky [:bz] (still a bit busy) 2011-05-06 07:32:49 PDT
> Was bz suggesting a consolidation, or a new name? 

Either is fine offhand, but having both crypto and (unprefixed) crypt would be bad.

Heck, crypto and mozCrypto together would be fine by me.
Comment 27 David Dahl :ddahl 2011-05-06 22:26:34 PDT
Created attachment 530802 [details] [diff] [review]
v 0.9 Most of sdwilsh's feedback is resolved

A few more documentation/comment tweaks and a new test file for testing bad input and I will ask for review
Comment 28 David Dahl :ddahl 2011-05-11 15:12:31 PDT
Created attachment 531768 [details] [diff] [review]
v 0.9.1 most comments addressed

Most of your pre-feedback comments are addressed. I still need to write a couple of tests that prove bad arguments will just cause the API to throw and not introduce garbage into the worker's methods. 

I also have not gotten the write config to disk via async methods working. I think the problem is in the lazy nature of an nsIDOMGlobalPropertyInitializer. Perhaps all of the configuration needs to be taken care of in the DOMCryptAPI.init() method. I'm not sure if that will work either. 

Feel free to toggle this back to feedback instead of review.
Comment 29 Justin Dolske [:Dolske] 2011-05-16 14:34:16 PDT
Is there a spec for this, preferably one that's on a standards track?
Comment 30 David Dahl :ddahl 2011-05-16 14:59:02 PDT
(In reply to comment #29)
> Is there a spec for this, preferably one that's on a standards track?

Not formally. I will write one up ASAP. Should I just publish it on the moz wiki - for now? As far as getting this on a standards track, I really need to sit down and learn the best way to do this. This part of software engineering is all new to me.
Comment 31 Justin Dolske [:Dolske] 2011-05-16 15:52:43 PDT
I think this is a fantastic area to explore, and would be fine as an addon (which it was, originally?), but I think this need additional project work before we talk about landing it Firefox.

A few specific concerns:

* No spec. Need one for other browsers to consider for implementation, and early adopters would need one to refer to.

* Needs a security design review. Since it's pretty fundamentally a security feature, this should happen before landing (and not as a post-landing checkup, as most features do).

* If you haven't already, ought to solicit input from interested parties (community), related stakeholders (NSS/PSM, possibly Sync), and probably relevant standards group (not sure who these would be, but a proposal on WHAGWG / some W3C group would probably be a start).

* Needs a legal review, wrt to potential issues with shipping a crypto API. Shouldn't be a big deal, AFAIK, but needs to happen.

I think people will (fairly) assume that if Mozilla starts shipping DOM APIs, even moz-prefixed, there is a plan to standardize them. And I suspect you'll find there a lot of people with an opinion on that. ;) I'm not trying to throw stop-energy at this, just that if we're going to do this it should be done with due diligence.
Comment 32 Shawn Wilsher :sdwilsh 2011-05-16 15:57:16 PDT
Comment on attachment 531768 [details] [diff] [review]
v 0.9.1 most comments addressed

I'm actually going to hold off on review until comment 31 is addressed.
Comment 33 David Dahl :ddahl 2011-05-16 16:21:40 PDT
(In reply to comment #31)
> A few specific concerns:
> 
> * No spec. Need one for other browsers to consider for implementation, and
> early adopters would need one to refer to.
> 
definitely is something I am beginning to address now. I will publish a spec on our wiki this week. 

> * Needs a security design review. Since it's pretty fundamentally a security
> feature, this should happen before landing (and not as a post-landing
> checkup, as most features do).
Yep. I have already spoken with the security team about this and the fact that this should not just be a normal security code review. 

> 
> * If you haven't already, ought to solicit input from interested parties
> (community), related stakeholders (NSS/PSM, possibly Sync), and probably
> relevant standards group (not sure who these would be, but a proposal on
> WHAGWG / some W3C group would probably be a start).

I have already presented this idea to both members of the Sync team, content team, A member of the Carnegie Mellon web security research group (http://websec.sv.cmu.edu/),  and the main NSS maintainers. I then sat down with Kai Engert and talked at length about the intersection of this code and the various psm APIs. 

People were pretty stoked about this concept, but like your concerns, we have only ever spent time on technical issues. You are correct - a formalized spec will help me get to the next step of presenting this to other browser vendors.

> 
> * Needs a legal review, wrt to potential issues with shipping a crypto API.
> Shouldn't be a big deal, AFAIK, but needs to happen.
>
No doubt.
 
> I think people will (fairly) assume that if Mozilla starts shipping DOM
> APIs, even moz-prefixed, there is a plan to standardize them. And I suspect
> you'll find there a lot of people with an opinion on that. ;) 
Indeed. Opinions vary greatly.

> I'm not trying
> to throw stop-energy at this, just that if we're going to do this it should
> be done with due diligence.
Not at all. I very much appreciate your guidance here, this is going to be the trickiest part of this project. My emphasis on working code is taking a play from the way the audio APIs evolved into such amazing tools. I think we have to really wow people with some working demos before we take that deep dive and wrangle over algorithms and whatnot. 

One thing of note: It looks somewhat likely that the Identity VerifiedEmailProtocol ( https://wiki.mozilla.org/Identity/Verified_Email_Protocol/Latest ) may use the Chrome API in this patch (or variant) for Key Generation and signing of data.  In which case, we can segment this further into chrome-only bits and eventually the content API.
Comment 34 David Dahl :ddahl 2011-05-18 15:02:07 PDT
(In reply to comment #33)
> > * No spec. Need one for other browsers to consider for implementation, and
> > early adopters would need one to refer to.
> > 
> definitely is something I am beginning to address now. I will publish a spec
> on our wiki this week. 

I have created a draft spec: https://wiki.mozilla.org/Privacy/Features/DOMCryptAPISpec/Latest
Comment 35 :Ms2ger (⌚ UTC+1/+2) 2011-05-19 14:46:11 PDT
Can you please email it to whatwg@lists.whatwg.org?
Comment 36 David Dahl :ddahl 2011-05-19 15:22:52 PDT
(In reply to comment #35)
> Can you please email it to whatwg@lists.whatwg.org?

Ah, yes, I will do that as soon as I have ported the newer, async API back over to the DOMCrypt extension. In a day or 2 at the latest.
Comment 37 David Dahl :ddahl 2011-05-20 11:49:57 PDT
(In reply to comment #35)
> Can you please email it to whatwg@lists.whatwg.org?
Done.
Comment 38 Kyle Simpson 2011-05-23 10:29:15 PDT
Just want to throw a minor suggestion (which i've already mentioned to David) into the mix... as far as the vendor-prefixing part of this idea is concerned:

Bug #652140 documents an enhancement suggestion for creating a `window.mozilla` (or `Moz` or `Gecko` or whatever) property, for two specific purposes. Related to *this* thread, the main purpose would be that this object could be a better namespace to hang experimental (not-yet-standardized) APIs off of, so it would be like `mozilla.crypto` or whatever, until such a time as it graduates to non-prefixed and a more standard location, such as `window` or whatever.

I like the idea of this crypto lib making it in, and I think it's a great use-case candidate for the vendor-prefixing/namespacing idea I put forth in that other bug, so perhaps we should consider doing them both at the same time?

I dislike the idea of doing vendor-prefixing (of any JavaScript API) by putting "Moz" into the name of the API, only to later remove it by a full rename, as opposed to using the much more common pattern of object namespacing like `Moz.crypto`. As I mentioned in that other thread, when the feature does graduate to non-prefixed status, the former `Moz.crypto` location could remain as an alias to the new location of the API, as an aid in the migration of code from vendor-prefixed to non-vendor-prefixed.
Comment 39 David Dahl :ddahl 2011-05-23 10:39:11 PDT
(In reply to comment #38)
> I dislike the idea of doing vendor-prefixing (of any JavaScript API) by
> putting "Moz" into the name of the API

Until and if there is a standard, I think this is the way it is done. There is also talk in this bug and on the WHAT-WG list of adding this api to window.crypto or consolidation of some sort - which would cause less confusion.
Comment 40 Kyle Simpson 2011-05-23 10:46:59 PDT
(in reply to comment #39)

>> I dislike the idea of doing vendor-prefixing (of any JavaScript API) by
>> putting "Moz" into the name of the API
>
> Until and if there is a standard, I think this is the way it is done

Firstly, I absolutely agree with the need to do some sort of vendor-prefixing. It's the *manner* in which we go about that which my suggestion is going towards. 

Secondly, "it is done" suggests (unless I read too much into it) that this is just how it's always been done, which is fine, I'm not taking issue with the past. I'm simply suggesting what I think would be a better pattern going forward to accomplish the same goal: vendor-prefixing before standardization.

In the JavaScript world, it's much more common of a pattern to use object property namespacing for this sort of thing, rather than baking in some set of characters into every function/object name. This pattern is also a little bit more friendly for future-refactoring, again from a JavaScript content developer's point of view.
Comment 41 David Dahl :ddahl 2011-05-24 10:32:27 PDT
Update: The WHAT-WG discussion is being archived here: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-May/031741.html
Comment 42 David Dahl :ddahl 2011-06-04 21:55:27 PDT
Created attachment 537418 [details]
WIP WebIDL of current proposed spec

Just getting my head around WebIDL, I am sure a lot is missing yet like exceptions and wrong, as in how I handle callbacks
Comment 43 :Ms2ger (⌚ UTC+1/+2) 2011-06-05 01:25:05 PDT
Created attachment 537428 [details]
WebIDL

I was thinking something more like this
Comment 44 David Dahl :ddahl 2011-06-05 06:14:57 PDT
Comment on attachment 537428 [details]
WebIDL

Ah, cool. Looks like the 'module' bits are unneeded. Do we need to specify how each method should throw?
Comment 45 David Dahl :ddahl 2011-06-05 19:37:40 PDT
Updated the title of this bug as the spec is changing, also, the WebKit bug for this API is here: https://bugs.webkit.org/show_bug.cgi?id=62010
Comment 46 Dave Garrett 2011-07-27 16:08:56 PDT
*** Bug 542879 has been marked as a duplicate of this bug. ***
Comment 47 David Dahl :ddahl 2011-07-28 08:25:29 PDT
Created attachment 549120 [details]
v 0.1 Chatter Bookmarklet - WIP

Saving work - this is a bookmarklet demo that will allow OTR style chatting on any chat web site
Comment 48 David Dahl :ddahl 2011-09-11 22:49:12 PDT
*** Bug 686249 has been marked as a duplicate of this bug. ***
Comment 49 Daniel Veditz [:dveditz] 2011-09-21 13:57:49 PDT
*** Bug 250792 has been marked as a duplicate of this bug. ***
Comment 50 David Dahl :ddahl 2011-12-01 17:20:36 PST
Created attachment 578449 [details]
v 1 Psuedocode

Brian:

This is some pseudo-code I put together while researching all of the APIs I need to use to begin implementing this API. We should meet up (soon) when you have some time to go over it.
Comment 51 David Dahl :ddahl 2011-12-16 16:30:50 PST
Created attachment 582429 [details] [diff] [review]
domcrypt-platform-generate-keypair v 1

Saving work, not built or tested
Comment 52 David Dahl :ddahl 2012-01-11 09:27:36 PST
Created attachment 587729 [details] [diff] [review]
domcrypt-platform-generate-keypair v 1.1
Comment 53 David Dahl :ddahl 2012-01-12 15:07:39 PST
Created attachment 588202 [details] [diff] [review]
v. 2 - it builds!

We now have functions to generate keys - creating an opaque 'nsIDOMCryptAPIKeyPair'. The keys have a 'nickname/keyID' associated with them. There is also a new function that gets the nsIDOMCryptAPIKeyPair via it's nickname/keyID.
Comment 54 David Dahl :ddahl 2012-01-13 09:30:37 PST
Created attachment 588437 [details] [diff] [review]
v. 2.1 XPCOM Errors

Trying to use this new interface in a xpcshell test, and I get: test_domcrypt_pk_generate_keypair.js:17 | TypeError: Cc['@mozilla.org/domcrypt-internal-api;1'] is undefined

I assume I have not added all of the magical XPCOM Gunk. I added NS_IMPL_CLASSINFO(nsDOMCryptInternalAPI, NULL, nsIClassInfo::THREADSAFE, NS_DOMCRYPTINTERNALAPI_CID) but get this error:

/home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsNSSComponent.cpp:2708:41: error: expected identifier before ‘__null’
/home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsNSSComponent.cpp:2708:41: error: expected ‘,’ or ‘...’ before ‘__null’
/home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsNSSComponent.cpp:2714:1: error: expected constructor, destructor, or type conversion before ‘nsrefcnt’
Comment 55 David Dahl :ddahl 2012-01-19 22:20:33 PST
Created attachment 590107 [details] [diff] [review]
v 2.2 persistent key generation fails

Everything builds and links now, the persistent key gen fails in nsDOMCryptInternalAPI::GenerateKeyPair
Comment 56 :Ms2ger (⌚ UTC+1/+2) 2012-01-19 22:33:33 PST
Comment on attachment 590107 [details] [diff] [review]
v 2.2 persistent key generation fails

>--- /dev/null
>+++ b/security/manager/ssl/public/nsIDOMCryptAPIKeyPair.idl
>+interface nsIDOMCryptAPIKeyPair : nsISupports
>+{
>+  readonly attribute wchar keyID;

Is this supposed to be just one character?

>--- a/security/manager/ssl/src/nsNSSComponent.cpp
>+++ b/security/manager/ssl/src/nsNSSComponent.cpp
>+nsDOMCryptAPIKeyPair::GetKeyID(PRUnichar* aOutString)
>+{
>+  aOutString = ToNewUnicode(NS_ConvertUTF8toUTF16(mKeyID));

Does this even work? It seems to me that you're setting the pointer, and not actually filling in *aOutString...
Comment 57 David Dahl :ddahl 2012-01-20 05:42:09 PST
(In reply to Ms2ger from comment #56)
> Comment on attachment 590107 [details] [diff] [review]
> v 2.2 persistent key generation fails
> 
> >--- /dev/null
> >+++ b/security/manager/ssl/public/nsIDOMCryptAPIKeyPair.idl
> >+interface nsIDOMCryptAPIKeyPair : nsISupports
> >+{
> >+  readonly attribute wchar keyID;
> 
> Is this supposed to be just one character?
No, it will be longer.  The resulting data is unsigned char *data - so the IDL type should probably be "string"? Some confusion on my part.

> 
> >--- a/security/manager/ssl/src/nsNSSComponent.cpp
> >+++ b/security/manager/ssl/src/nsNSSComponent.cpp
> >+nsDOMCryptAPIKeyPair::GetKeyID(PRUnichar* aOutString)
> >+{
> >+  aOutString = ToNewUnicode(NS_ConvertUTF8toUTF16(mKeyID));
> 
> Does this even work? It seems to me that you're setting the pointer, and not
> actually filling in *aOutString...
This is untested - I was just getting that part to build for now.
Comment 58 David Dahl :ddahl 2012-01-20 09:29:20 PST
I am wondering if I am using the wrong slot...

  PK11SlotInfo *slot = PK11_GetInternalSlot();

  ...

  aPrivKey = PK11_GenerateKeyPair(slot, 
                                  CKM_RSA_PKCS_KEY_PAIR_GEN, 
                                  &rsaParams, 
                                  &aPubKey, 
                                  PR_TRUE /*isPerm*/, 
                                  PR_TRUE /*isSensitive*/, 
                                  NULL /*&pwdata*/);

... with "isPerm" set to true?
Comment 59 :Ms2ger (⌚ UTC+1/+2) 2012-01-20 09:37:22 PST
(In reply to David Dahl :ddahl from comment #57)
> (In reply to Ms2ger from comment #56)
> > Comment on attachment 590107 [details] [diff] [review]
> > v 2.2 persistent key generation fails
> > 
> > >--- /dev/null
> > >+++ b/security/manager/ssl/public/nsIDOMCryptAPIKeyPair.idl
> > >+interface nsIDOMCryptAPIKeyPair : nsISupports
> > >+{
> > >+  readonly attribute wchar keyID;
> > 
> > Is this supposed to be just one character?
> No, it will be longer.  The resulting data is unsigned char *data - so the
> IDL type should probably be "string"? Some confusion on my part.

You could do that, but DOMString makes for much nicer code.
Comment 60 David Dahl :ddahl 2012-01-20 10:10:38 PST
(In reply to Ms2ger from comment #59)

> You could do that, but DOMString makes for much nicer code.
Ok, will do. I was unsure what type the generated ID would be.
So then this:
NS_IMETHODIMP
nsDOMCryptInternalAPI::GetKeyPairByKeyID(char* aKeyID, 
                                         nsIDOMCryptAPIKeyPair **_retval NS_OUTPARAM)

should be:

NS_IMETHODIMP
nsDOMCryptInternalAPI::GetKeyPairByKeyID(nsAString& aKeyID, 
                                         nsIDOMCryptAPIKeyPair **_retval NS_OUTPARAM)
Comment 61 :Ms2ger (⌚ UTC+1/+2) 2012-01-20 10:32:57 PST
Inparams would be const nsAString&, out are nsAString&
Comment 62 David Dahl :ddahl 2012-01-20 15:12:21 PST
Created attachment 590365 [details]
backtrace
Comment 63 David Dahl :ddahl 2012-01-23 22:04:49 PST
Created attachment 590993 [details] [diff] [review]
v 3 Adding rough draft of PKEncrypt and PKDecrypt, still has keypair issues
Comment 64 David Dahl :ddahl 2012-01-26 21:03:31 PST
Created attachment 592050 [details] [diff] [review]
v 4 Linking problem

Linking error: 
/security/manager/ssl/src/nsNSSComponent.o: In function `nsDOMCryptInternalAPI::GetPublicKeyByKeyID(char*, SECKEYPublicKeyStr*)':
/home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsNSSComponent.cpp:2898: undefined reference to `PK11_FindPrivateKeyFromNickname'
/home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsNSSComponent.cpp:2905: undefined reference to `PK11_ExtractPublicKey'
collect2: ld returned 1 exit status
make[1]: *** [libxul.so] Error 1

Not sure how to include the right headers here, or something...
Comment 65 David Dahl :ddahl 2012-01-26 22:19:51 PST
Created attachment 592057 [details] [diff] [review]
v 4.1 Cannot use private NSS functions, etc...

Running into a dead end with some private NSS functions used to get a private key from a keyID.
Comment 66 David Dahl :ddahl 2012-01-30 16:13:18 PST
Seeing a cascade of missing dependencies and errors trying to use PK11_ExtractPublicKey. This seems like something I am not allowed to use outside of NSS itself. I attempted to re-create it as a private function to no avail: another cascade of errors like:

/home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsNSSComponent.cpp:2763:3: error: invalid use of incomplete type ‘PK11SlotInfo {aka struct PK11SlotInfoStr}’
../../../../dist/include/secmodt.h:59:16: error: forward declaration of ‘PK11SlotInfo {aka struct PK11SlotInfoStr}’
/home/ddahl/code

I think I have already re-created a few wheels here. is there another way to get a keypair from the keydb via a nickname or keyID?

If not, I assume we need to create public versions of these 2 functions:

PK11_ExtractPublicKey
and PK11_FindPrivateKeyFromNickname

I also tried to substitute PK11_FindPrivateKeyFromNickname with PK11_FindKeyByKeyID
Comment 67 David Dahl :ddahl 2012-01-30 16:14:43 PST
Created attachment 592912 [details] [diff] [review]
v 4.2 Naive Patch with copied functions, it'll never work
Comment 68 Kai Engert (:kaie) (on vacation) 2012-02-01 04:25:11 PST
I gave feedback to David on IRC.
Comment 69 David Dahl :ddahl 2012-02-01 06:24:04 PST
Created attachment 593424 [details]
irc discussion with kaie
Comment 70 David Dahl :ddahl 2012-02-01 17:30:55 PST
Created attachment 593668 [details] [diff] [review]
v. 4.3 breaking out PSM from XPCOM/DOM untested

Saving my work
Comment 71 David Dahl :ddahl 2012-02-14 10:58:58 PST
Created attachment 597097 [details] [diff] [review]
4.4 Attempting to isolate all XPCOM bits to dom/

In my attempt to kill off all XPCOM bits of the PSM part of this patch, I cannot get it to build. 
1. The presence of a dtor in the .h or the impl gives me:

/home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsDOMCryptInternalAPI.cpp:99:61: error: definition of implicitly-declared ‘nsDOMCryptInternalAPIKeyPair::~nsDOMCryptInternalAPIKeyPair()’
/home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsDOMCryptInternalAPI.cpp:158:47: error: definition of implicitly-declared ‘nsDOMCryptInternalAPI::~nsDOMCryptInternalAPI()’


So I just removed the dtor for now.

2. I always see: /home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsDOMCryptInternalAPI.cpp:266:3: error: ‘class nsDOMCryptInternalAPIKeyPair’ has no member named ‘AddRef’

It seems like I cannot not have an XPCOM interface and just use a plain old C++ class here.
Comment 72 David Dahl :ddahl 2012-02-14 11:00:34 PST
(In reply to David Dahl :ddahl from comment #71)
> Created attachment 597097 [details] [diff] [review]
> 4.4 Attempting to isolate all XPCOM bits to dom/
> 
> In my attempt to kill off all XPCOM bits of the PSM part of this patch, I
> cannot get it to build.

[clarification] The attempt is to keep all xpcom interfaces in dom/ and have none in PSM to simplify what is landed in PSM
Comment 73 David Dahl :ddahl 2012-02-14 21:56:26 PST
Ah, ok, making headway with NS_IMPL_ISUPPORTS0
Comment 74 David Dahl :ddahl 2012-02-14 21:58:24 PST
Created attachment 597303 [details] [diff] [review]
4.5 Builds now, added PKDecrypt

I still cannot figure out how to enable the destructor and nsNSSShutdown bits
Comment 75 :Ms2ger (⌚ UTC+1/+2) 2012-02-15 05:31:39 PST
(In reply to David Dahl :ddahl from comment #72)
> (In reply to David Dahl :ddahl from comment #71)
> > Created attachment 597097 [details] [diff] [review]
> > 4.4 Attempting to isolate all XPCOM bits to dom/
> > 
> > In my attempt to kill off all XPCOM bits of the PSM part of this patch, I
> > cannot get it to build.
> 
> [clarification] The attempt is to keep all xpcom interfaces in dom/ and have
> none in PSM to simplify what is landed in PSM

Note: that means you can't directly access the code in PSM from script, including xpcshell tests.
Comment 76 David Dahl :ddahl 2012-02-15 06:16:55 PST
(In reply to Ms2ger from comment #75)
> (In reply to David Dahl :ddahl from comment #72)
> > (In reply to David Dahl :ddahl from comment #71)
> > > Created attachment 597097 [details] [diff] [review]
> > > 4.4 Attempting to isolate all XPCOM bits to dom/
> > > 
> > > In my attempt to kill off all XPCOM bits of the PSM part of this patch, I
> > > cannot get it to build.
> > 
> > [clarification] The attempt is to keep all xpcom interfaces in dom/ and have
> > none in PSM to simplify what is landed in PSM
> 
> Note: that means you can't directly access the code in PSM from script,
> including xpcshell tests.

I was thinking I would be able to add an light wrapper to this internalAPI in dom/base or elsewhere that allows script access. The downside of not having direct script access might mean that the testing is not as complete or tightly coupled as it could be. That seems to be a problem now that I think about it.
Comment 77 David Dahl :ddahl 2012-02-15 08:09:56 PST
(In reply to David Dahl :ddahl from comment #76)
> > Note: that means you can't directly access the code in PSM from script,
> > including xpcshell tests.
> 
> I was thinking I would be able to add an light wrapper to this internalAPI
> in dom/base or elsewhere that allows script access. The downside of not
> having direct script access might mean that the testing is not as complete
> or tightly coupled as it could be. That seems to be a problem now that I
> think about it.

Of course, how feasible is creating an IDL where the arguments are low-level raw keys like SECKEYPublicKey, etc? I am unsure about how this could be accomplished.
Comment 78 David Dahl :ddahl 2012-02-15 15:57:11 PST
Build errors starting to cascade:

nsDOMCryptAPI.cpp
In file included from ../../dist/include/nsDOMCryptInternalAPI.h:57:0,
                 from /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPI.cpp:45:
../../dist/system_wrappers/secmodt.h:3:26: fatal error: secmodt.h: No such file or directory
compilation terminated.

It seems like any headers used from inside NSS or PSM are not found inside of nsDOMCryptAPI in dom/base. I added nsDOMCryptInternalAPI.h to the EXPORTS in the security/manager/ssl/src/Makefile.in

I must be doing something wrong here, I cannot imagine I will have to export each header I use since the Makefile in ssl/src had no EXPORTS at all. (Attaching latest patch in a moment.)
Comment 79 David Dahl :ddahl 2012-02-15 15:58:42 PST
Created attachment 597595 [details] [diff] [review]
v 4.6 Build errors, header export problem
Comment 80 :Ms2ger (⌚ UTC+1/+2) 2012-02-16 01:51:44 PST
Comment on attachment 597595 [details] [diff] [review]
v 4.6 Build errors, header export problem

Please start using the new MPL boilerplate: http://www.mozilla.org/MPL/headers/

>--- /dev/null
>+++ b/dom/base/nsDOMCryptAPI.cpp
>+#include "nsCOMPtr.h"
>+#include "jsapi.h"
>+#include "nsStringGlue.h"
>+#include "plbase64.h"
>+
>+#include "nsDOMCryptInternalAPI.h"
>+#include "nsDOMCryptAPI.h"

This include should be first.

>+
>+// Stolen from /toolkit/components/places/Helpers.cpp
>+// TODO: move this to Utils.h as per bz on irc

How about xpcom/io/Base64.h?


>+//---------------------------------------------
>+// Implementing nsDOMCryptAPIKeyPair
>+//---------------------------------------------
>+
>+NS_IMPL_ISUPPORTS1(nsDOMCryptAPIPublicKey, nsIDOMCryptAPIPublicKey)
>+
>+static NS_DEFINE_CID(kDOMCryptAPIPublicKeyCID, 
>+                     NS_DOMCRYPTAPIPUBLICKEY_CID);
>+
>+nsDOMCryptAPIPublicKey::nsDOMCryptAPIPublicKey()
>+{
>+  
>+}
>+
>+nsDOMCryptAPIPublicKey::nsDOMCryptAPIPublicKey()
>+{
>+
>+}

Remove double implementation

>+
>+nsDOMCryptAPIPublicKey::~nsDOMCryptAPIPublicKey(nsACString* aPublicKey,
>+                                                nsACString* aKeyID)
>+{
>+  mBase64EncodedDERPublicKey = aPublicKey;
>+  mKeyID = aKeyID;
>+}

A destructor that takes arguments?

>+nsDOMCryptAPIPublicKey::GetPublicKey(nsACString& aOutString)
>+{
>+  // XXX: format of this key: DEREncoded->base64encoded
>+  // aOutString = ToNewUnicode(NS_ConvertUTF8toUTF16(mKeyID))
>+  aOutString = &mBase64EncodedDERPublicKey;

You need *mBase64EncodedDERPublicKey, no?

>+nsDOMCryptAPIPublicKey::GetKeyID(nsAString& aOutString)
>+{
>+  aOutString = ToNewUnicode(NS_ConvertUTF8toUTF16(mKeyID));

ToNewUnicode() shouldn't be necessary, I think

>+//---------------------------------------------
>+// Implementing nsDOMCryptAPIPKCipherData: 
>+// The result of an Encrypt operation
>+//---------------------------------------------
>+
>+NS_IMPL_ISUPPORTS1(nsDOMCryptAPIPKCipherData, nsIDOMCryptAPIPKCipherData)
>+
>+static NS_DEFINE_CID(kDOMCryptAPIPKCipherDataCID, 
>+                     NS_DOMCRYPTAPIPKCIPHERDATA_CID);
>+
>+nsDOMCryptAPIPKCipherData::nsDOMCryptAPIPKCipherData()
>+{
>+}
>+
>+nsDOMCryptAPIPKCipherData::nsDOMCryptAPIPKCipherData(nsDOMCryptAPIPublicKey* aPublicKey,
>+                                                     char* aEncryptedData)
>+{
>+  mPublicKey = aPublicKey;
>+  mEncryptedData = aEncryptedData;
>+
>+}
>+
>+nsDOMCryptAPIPKCipherData::~nsDOMCryptAPIPKCipherData()
>+{
>+}
>+
>+NS_IMETHODIMP
>+nsDOMCryptAPIPKCipherData::GetPublicKey(nsACString& aOutString)
>+{
>+  mPublicKey->GetPublicKey(aOutString);
>+  
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsDOMCryptAPIPKData::GetKeyID(nsAString& aOutString)
>+{
>+  // aOutString = ToNewUnicode(NS_ConvertUTF8toUTF16(mKeyID));
>+  mPublicKey->GetKeyID(aOutString);
>+  return NS_OK;
>+}
>+
>+//---------------------------------------------
>+// Implementing nsDOMCryptAPI
>+//---------------------------------------------
>+
>+NS_IMPL_ISUPPORTS1(nsDOMCryptAPI, nsIDOMCryptAPI)
>+
>+static NS_DEFINE_CID(kDOMCryptAPICID, NS_DOMCRYPTAPI_CID);
>+
>+nsDOMCryptAPI::nsDOMCryptAPI()
>+{
>+  NS_ADDREF(mInternalAPI = new nsDOMCryptInternalAPI());
>+}
>+
>+nsDOMCryptAPI::~nsDOMCryptAPI()
>+{
>+}
>+
>+NS_IMETHODIMP 
>+nsDOMCryptAPI::PKGenerateKeyPair(PRUint32 aKeySizeInBits,
>+                                 PRUint32 aAlgorithm,
>+                                 nsIDOMCryptAPIPublicKey** _retval NS_OUTPARAM)
>+{
>+  // Wraps nsDOMCryptInternalAPI::PKGenerateKeyPair
>+  char* keyID;
>+  PRUint32 algorithm = 0;
>+  PRUint32 keySizeInBits = 2048;
>+  // TODO: create these variables inside the internal API... 
>+  // perhaps we should not use any NSS types in this API

We probably shouldn't.

>+  SECKEYPrivateKey privk* = NULL;
>+  SECKEYPublicKey pubk* = NULL;
>+  
>+  mInternalAPI->PKGenerateKeyPair(privk, pubk, keySizeInBits, algorithm, keyID);

This returns an nsresult; check it?

>+  nsACString base64DERPubKey;
>+  nsCString tmpStr;
>+
>+  nsresult rv = Base64urlEncode(data, len, tmpStr);
>+
>+  if (NS_FAILED(rv)) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  base64DERPubKey = tmpStr;
>+  
>+  // TODO: get keyID as nsACString
>+  nsACString pubKeyID;
>+  pubKeyID = static_cast<nsACstring>(keyID);

You're doing a lot of weird things here...

How about

nsAutoCString base64DERPubKey;
nsresult rv = Base64urlEncode(data, len, base64DERPubKey);
NS_ENSURE_SUCCESS(rv, rv);

NS_ADDREF(*_retval = new nsDOMCryptAPIPublicKey(base64DERPubKey, nsDependentCString(keyID)));
return NS_OK;


>--- /dev/null
>+++ b/dom/base/nsDOMCryptAPI.h
>+#ifndef _nsDOMCryptAPI_h_
>+#define _nsDOMCryptAPI_h_

No leading or trailing underscores, please.

>+#define NS_DOMCRYPTAPIPUBLICKEY_CONTRACTID "@mozilla.org/domcrypt-api-publickey;1"
>+#define NS_DOMCRYPTAPIPUBLICKEY_COMPONENT_CLASSNAME "DOMCrypt API Public Key";
>+#define NS_DOMCRYPTAPIPUBLICKEY_CID {0x029a82c2, 0x9e7f, 0x49d9, {0x90, 0x7e, 0x37, 0xf4, 0x8d, 0xc2, 0x86, 0xeb}}
>+
>+#define NS_DOMCRYPTAPI_CONTRACTID "@mozilla.org/domcrypt-api;1"
>+#define NS_DOMCRYPTAPI_COMPONENT_CLASSNAME "DOMCrypt API";
>+#define NS_DOMCRYPTAPI_CID {0x42092ff4, 0x23c7, 0x4ed8, {0x9d, 0x57, 0xbf, 0xbc, 0x67, 0xc6, 0x12, 0x78}}
>+
>+#define NS_DOMCRYPTAPIPKCIPHERDATA_CONTRACTID "@mozilla.org/domcrypt-pk-cipher-data;1"
>+#define NS_DOMCRYPTAPIPKCIPHERDATA_COMPONENT_CLASSNAME "DOMCrypt API PK Cipher Data";
>+#define NS_DOMCRYPTAPIPKCIPHERDATA_CID {0xd0a66274, 0xc26a, 0x4b1c, {0x86, 0x85, 0x5a, 0x77, 0x55, 0x5c, 0x17, 0xaf}}
>+
>+#define NS_DOMCRYPTAPIPKCLEARDATA_CONTRACTID "@mozilla.org/domcrypt-pk-clear-data;1"
>+#define NS_DOMCRYPTAPIPKCLEARDATA_COMPONENT_CLASSNAME "DOMCrypt API PK Clear Data";
>+#define NS_DOMCRYPTAPIPKCLEARDATA_CID {0xd7adcacc, 0x3203, 0x4f12, {0x83, 0x7a, 0x16, 0x5b, 0xdf, 0xf3, 0x31, 0xf5}}

I don't really understand why you want all this.


>+class nsDOMCryptAPIPublicKey : public nsIDOMCryptAPIPublicKey
>+{
>+  nsDOMCryptAPIPublicKey(nsACString* aPublicKey, nsAString* aKeyID);

We don't usually use pointers to strings.


>+class nsDOMCryptAPIPKCipherData : public nsIDOMCryptAPIPKCipherData
>+{
>+  nsDOMCryptAPIPublicKey* mPublicKey;
>+  jsval& mEncryptedData; // an ArrayBuffer

privatize those. You probably want to keep mEncryptedData as JSObject*.

>+class nsDOMCryptAPIPKClearData : public nsIDOMCryptAPIPKClearData
>+{
>+  char* mClearData;

Same

>+class nsDOMCryptAPI : public nsIDOMCryptAPI
>+{
>+  nsDOMCryptInternalAPI mInternalAPI;

Pointer?

>--- /dev/null
>+++ b/security/manager/ssl/public/nsIDOMCryptAPIKeyPair.idl
>--- /dev/null
>+++ b/security/manager/ssl/public/nsIDOMCryptInternalAPI.idl

I think we'd best start without interfaces in psm. In any case, these shouldn't be nsIDOM* prefixed; that prefix is for DOM interfaces.

>--- a/security/manager/ssl/src/Makefile.in
>+++ b/security/manager/ssl/src/Makefile.in
>+	nsDOMCryptInternalAPI.cpp \

Two spaces

>+EXPORTS = \
>+	nsDOMCryptInternalAPI.h \
>+	$(NULL)

And here

>--- /dev/null
>+++ b/security/manager/ssl/src/nsDOMCryptInternalAPI.cpp
>+nsDOMCryptInternalAPIKeyPair::GetKeyID(nsAString& aOutString)
>+{
>+  aOutString = ToNewUnicode(NS_ConvertUTF8toUTF16(mKeyID));

No ToNewUnicode

>+nsDOMCryptInternalAPIKeyPair::GetDEREncodedRSAPublicKey(unsigned char* aOutData)
>+{
>+  aOutData = mDERPubKey->data;

This doesn't do anything.

>+nsDOMCryptInternalAPI::GenerateKeyPair(SECKEYPublicKey* aPubKey, 
>+                                       SECKEYPrivateKey* aPrivKey,
>+                                       PRUint32 aKeySizeInBits,
>+                                       PRUint32 aAlgorithm)
>+{

If the goal is to return a new public/private key, those arguments need to be pointers-to-pointers, so you can set |*aPubKey = foo();|

>--- /dev/null
>+++ b/security/manager/ssl/src/nsDOMCryptInternalAPI.h
>+class nsDOMCryptInternalAPIKeyPair : public nsISupports //, public nsNSSShutDownObject
>+class nsDOMCryptInternalAPI : public nsISupports //, public nsNSSShutDownObject

No need to inherit from nsISupports for either of these

>--- a/security/manager/ssl/src/nsNSSComponent.cpp
>+++ b/security/manager/ssl/src/nsNSSComponent.cpp
>--- a/security/manager/ssl/src/nsNSSComponent.h
>+++ b/security/manager/ssl/src/nsNSSComponent.h
>--- a/security/manager/ssl/src/nsNSSModule.cpp
>+++ b/security/manager/ssl/src/nsNSSModule.cpp

Revert changes to these files, I think
Comment 81 :Ms2ger (⌚ UTC+1/+2) 2012-02-16 01:58:07 PST
(In reply to David Dahl :ddahl from comment #78)
> Build errors starting to cascade:
> 
> nsDOMCryptAPI.cpp
> In file included from ../../dist/include/nsDOMCryptInternalAPI.h:57:0,
>                  from
> /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPI.cpp:45:
> ../../dist/system_wrappers/secmodt.h:3:26: fatal error: secmodt.h: No such
> file or directory
> compilation terminated.
> 
> It seems like any headers used from inside NSS or PSM are not found inside
> of nsDOMCryptAPI in dom/base. I added nsDOMCryptInternalAPI.h to the EXPORTS
> in the security/manager/ssl/src/Makefile.in
> 
> I must be doing something wrong here, I cannot imagine I will have to export
> each header I use since the Makefile in ssl/src had no EXPORTS at all.
> (Attaching latest patch in a moment.)

I doubt anything outside PSM uses NSS directly, we probably want to keep it that way.
Comment 82 David Dahl :ddahl 2012-02-16 21:39:06 PST
(In reply to Ms2ger from comment #80)
> Comment on attachment 597595 [details] [diff] [review]
> 
> >+
> >+// Stolen from /toolkit/components/places/Helpers.cpp
> >+// TODO: move this to Utils.h as per bz on irc
> 
> How about xpcom/io/Base64.h?
> 
Ah, cool. I was unaware. I will try to make some helpers that use this as I also need to decode base64url encoding

> >+
> >+nsDOMCryptAPIPublicKey::~nsDOMCryptAPIPublicKey(nsACString* aPublicKey,
> >+                                                nsACString* aKeyID)
> >+{
> >+  mBase64EncodedDERPublicKey = aPublicKey;
> >+  mKeyID = aKeyID;
> >+}
> 
> A destructor that takes arguments?

Oh jeez. that is misplaced.

> You're doing a lot of weird things here...
> 
> How about
> 
> nsAutoCString base64DERPubKey;
> nsresult rv = Base64urlEncode(data, len, base64DERPubKey);
> NS_ENSURE_SUCCESS(rv, rv);
> 
> NS_ADDREF(*_retval = new nsDOMCryptAPIPublicKey(base64DERPubKey,
> nsDependentCString(keyID)));
> return NS_OK;
> 

Thanks - these pointers are invaluable. I know a fair bit of my hacking here is me learning the Mozilla C++ way.  

> >+
> >+#define NS_DOMCRYPTAPIPKCLEARDATA_CONTRACTID "@mozilla.org/domcrypt-pk-clear-data;1"
> >+#define NS_DOMCRYPTAPIPKCLEARDATA_COMPONENT_CLASSNAME "DOMCrypt API PK Clear Data";
> >+#define NS_DOMCRYPTAPIPKCLEARDATA_CID {0xd7adcacc, 0x3203, 0x4f12, {0x83, 0x7a, 0x16, 0x5b, 0xdf, 0xf3, 0x31, 0xf5}}
> 
> I don't really understand why you want all this.
> 
I wanted to explicitly lay out the inputs and outputs from the encrypt and decrypt. The KeyID and publicKey have to travel around with the encrypted data.

Perhaps it is overkill, with some test writing, it should play out as too much or if it is OK.

> 
> >+class nsDOMCryptAPIPublicKey : public nsIDOMCryptAPIPublicKey
> >+{
> >+  nsDOMCryptAPIPublicKey(nsACString* aPublicKey, nsAString* aKeyID);
> 
> We don't usually use pointers to strings.
Ok. Gotcha. 

> >--- /dev/null
> >+++ b/security/manager/ssl/public/nsIDOMCryptAPIKeyPair.idl
> >--- /dev/null
> >+++ b/security/manager/ssl/public/nsIDOMCryptInternalAPI.idl
> 
> I think we'd best start without interfaces in psm. In any case, these
> shouldn't be nsIDOM* prefixed; that prefix is for DOM interfaces.
> 
I forgot to nuke those from the patch, thanks.

> >--- a/security/manager/ssl/src/Makefile.in
> >+++ b/security/manager/ssl/src/Makefile.in
> >+	nsDOMCryptInternalAPI.cpp \
> 
> Two spaces
> 
> >+EXPORTS = \
> >+	nsDOMCryptInternalAPI.h \
> >+	$(NULL)
> 
> And here
> 
> >--- /dev/null
> >+++ b/security/manager/ssl/src/nsDOMCryptInternalAPI.cpp
> >+nsDOMCryptInternalAPIKeyPair::GetKeyID(nsAString& aOutString)
> >+{
> >+  aOutString = ToNewUnicode(NS_ConvertUTF8toUTF16(mKeyID));
> 
> No ToNewUnicode
> 
> >+nsDOMCryptInternalAPIKeyPair::GetDEREncodedRSAPublicKey(unsigned char* aOutData)
> >+{
> >+  aOutData = mDERPubKey->data;
> 
> This doesn't do anything.
> 
> >+nsDOMCryptInternalAPI::GenerateKeyPair(SECKEYPublicKey* aPubKey, 
> >+                                       SECKEYPrivateKey* aPrivKey,
> >+                                       PRUint32 aKeySizeInBits,
> >+                                       PRUint32 aAlgorithm)
> >+{
> 
> If the goal is to return a new public/private key, those arguments need to
> be pointers-to-pointers, so you can set |*aPubKey = foo();|
> 
> >--- /dev/null
> >+++ b/security/manager/ssl/src/nsDOMCryptInternalAPI.h
> >+class nsDOMCryptInternalAPIKeyPair : public nsISupports //, public nsNSSShutDownObject
> >+class nsDOMCryptInternalAPI : public nsISupports //, public nsNSSShutDownObject
> 
> No need to inherit from nsISupports for either of these
> 
> >--- a/security/manager/ssl/src/nsNSSComponent.cpp
> >+++ b/security/manager/ssl/src/nsNSSComponent.cpp
> >--- a/security/manager/ssl/src/nsNSSComponent.h
> >+++ b/security/manager/ssl/src/nsNSSComponent.h
> >--- a/security/manager/ssl/src/nsNSSModule.cpp
> >+++ b/security/manager/ssl/src/nsNSSModule.cpp
> 
> Revert changes to these files, I think

Ah yes. No code should change there anymore. 

A new patch is forthcoming. I am working on getting it to build. 

Thanks again. I hope it is not a burden for you to peek at these ROUGH patches:(
Comment 83 David Dahl :ddahl 2012-02-17 15:16:35 PST
Created attachment 598394 [details] [diff] [review]
v 4.7 Saving Latest Work, more build errors
Comment 84 David Dahl :ddahl 2012-02-24 16:31:04 PST
Created attachment 600564 [details] [diff] [review]
v 4.8 include errors, trouble with exports and nss headers

Mike:

With this patch, all of the nss headers are missing. I tried to export a new header in security/manager/ssl/src/Makefile.in

nsDOMCryptAPI.cpp
In file included from ../../dist/include/nsDOMCryptInternalAPI.h:12:0,
                 from /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPI.cpp:6:
../../dist/system_wrappers/pk11pub.h:3:26: fatal error: pk11pub.h: No such file or directory
compilation terminated.

pk11pub.h is not in dist/include - this is after a full clobber
Comment 85 David Dahl :ddahl 2012-02-24 21:54:57 PST
(In reply to David Dahl :ddahl from comment #84)
> pk11pub.h is not in dist/include - this is after a full clobber

Looks like a full rebuild without the patch queue has fixed this issue.

Now I get:
security/manager/ssl/src/nsDOMCryptInternalAPI.cpp:26:1: error: invalid static_cast from type ‘nsDOMCryptInternalAPIKeyPair*’ to type ‘nsISupports*’

Via this macro: NS_IMPL_ISUPPORTS0(nsDOMCryptInternalAPIKeyPair)
Comment 86 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-02-24 21:57:09 PST
You can't use NSS directly from the DOM.  NSS is built too late for that.
Comment 87 :Ms2ger (⌚ UTC+1/+2) 2012-02-25 01:47:23 PST
Comment on attachment 600564 [details] [diff] [review]
v 4.8 include errors, trouble with exports and nss headers

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

::: dom/base/nsDOMCryptAPI.cpp
@@ +7,5 @@
> +
> +#include "nsCOMPtr.h"
> +#include "jsapi.h"
> +#include "nsStringGlue.h"
> +#include "plbase64.h"

No need to include that anymore

@@ +43,5 @@
> +{
> +  nsresult rv = mozilla::Base64Encode(aPlainData, aB64UrlEncoded);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCString tmpStr = aB64UrlEncoded;

Pass tmpStr to mozilla::Base64Encode instead of copying back and forth.

@@ +60,5 @@
> +  nsCString tmpStr = aB64UrlEncoded;
> +  tmpStr.ReplaceChar('_', '/');
> +  tmpStr.ReplaceChar('-', '+');
> +  
> +  nsACString replacedStr = tmpStr;

No need for this, you can do

return mozilla::Base64Decode(tmpStr, aDecodedData);

@@ +177,5 @@
> +  nsresult rv;
> +
> +  rv = mInternalAPI->PKGenerateKeyPair(keySizeInBits, 
> +                                       algorithm, 
> +                                       internalKeyPair);

nsDOMCryptInternalAPIKeyPair* internalKeyPair;
nsresult rv = mInternalAPI->PKGenerateKeyPair(keySizeInBits, algorithm, &internalKeyPair);

You'll also need to figure out how not to leak it.

@@ +188,5 @@
> +
> +  PRUint32 len;
> +  PRUint8* data;
> +  len = static_cast<PRUint32>(derPubKey->len);
> +  data = static_cast<PRUint8*>(derPubKey->data);

That won't work with unsigned char* derPubKey;

@@ +197,5 @@
> +  if (NS_FAILED(rv)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsACString base64DERPubKey = tmpStr; // convert nsCString to nsACString

Not necessary, an nsCString already is an nsACString.

@@ +199,5 @@
> +  }
> +
> +  nsACString base64DERPubKey = tmpStr; // convert nsCString to nsACString
> +
> +  nsACString pubKeyID;

Need a concrete type; nsCAutoString or nsCString.

::: security/manager/ssl/src/nsDOMCryptInternalAPI.cpp
@@ +82,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +nsDOMCryptInternalAPIKeyPair::GetDEREncodedRSAPublicKey(unsigned char* aOutData)

I think you just want nsACString& aOutData

@@ +91,5 @@
> +  //   return NS_OK;
> +
> +  mDERPubKey = PK11_DEREncodePublicKey(mPublicKey);
> +  // XXXMs2Ger: this does not do anything...
> +  aOutData = mDERPubKey->data;

And then have aOutData = nsDependentCString(mDERPubKey->data, mDERPubKey->len);

::: security/manager/ssl/src/nsDOMCryptInternalAPI.h
@@ +16,5 @@
> +#include "base64.h"
> +#include "secerr.h"
> +#include "pkcs11t.h" 
> +#include "keyhi.h"
> +#include "keythi.h"

You won't be able to include all this from an exported header you need in DOM. Try forward declaring what you need for now.

@@ +21,5 @@
> +
> +class nsDOMCryptInternalAPIKeyPair //: public nsNSSShutDownObject
> +{
> +public:
> +  NS_DECL_ISUPPORTS

Remove this

@@ +24,5 @@
> +public:
> +  NS_DECL_ISUPPORTS
> +  // NS_DECL_NSIDOMCRYPTAPIKEYPAIR
> +
> +  nsDOMCryptInternalAPIKeyPair();

And this

@@ +56,5 @@
> +public:
> +  NS_DECL_ISUPPORTS
> +  // NS_DECL_NSIDOMCRYPTINTERNALAPI
> +
> +  nsDOMCryptInternalAPI();

Same here
Comment 88 David Dahl :ddahl 2012-02-25 06:36:20 PST
(In reply to Ms2ger from comment #87)
> Comment on attachment 600564 [details] [diff] [review]

> You won't be able to include all this from an exported header you need in
> DOM. Try forward declaring what you need for now.

It looks like I need to forward declare SECKEYPublicKey, SECKEYPrivateKey and SECItem - after reading about it, I am unclear how to do it. Is it some kind of #define or typedef?

> 
> @@ +21,5 @@
> > +
> > +class nsDOMCryptInternalAPIKeyPair //: public nsNSSShutDownObject
> > +{
> > +public:
> > +  NS_DECL_ISUPPORTS
> 
> Remove this

When I remove these, I get: /home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsDOMCryptInternalAPI.h:23:3: error: ‘nsresult’ does not name a type
Comment 89 Mike Hommey [:glandium] 2012-02-25 08:37:26 PST
(In reply to David Dahl :ddahl from comment #88)
> (In reply to Ms2ger from comment #87)
> > Comment on attachment 600564 [details] [diff] [review]
> 
> > You won't be able to include all this from an exported header you need in
> > DOM. Try forward declaring what you need for now.
> 
> It looks like I need to forward declare SECKEYPublicKey, SECKEYPrivateKey
> and SECItem - after reading about it, I am unclear how to do it. Is it some
> kind of #define or typedef?

class SECKEYPublicKey;
class SECKEYPrivateKey;
class SECItem;

should be enough.
Comment 90 David Dahl :ddahl 2012-02-25 09:14:11 PST
(In reply to Mike Hommey [:glandium] from comment #89)
> class SECKEYPublicKey;
> class SECKEYPrivateKey;
> class SECItem;
> 
> should be enough.

I was wrong about how many nss types are referenced here - there are many more in the cpp file, and the forward declaration is not working.

see: http://pastebin.mozilla.org/1491898

Is this idea of trying to use this PSM class sans xpcom inside dom/ not feasible?
Comment 91 :Ms2ger (⌚ UTC+1/+2) 2012-02-25 11:25:38 PST
(In reply to David Dahl :ddahl from comment #90)
> (In reply to Mike Hommey [:glandium] from comment #89)
> > class SECKEYPublicKey;
> > class SECKEYPrivateKey;
> > class SECItem;
> > 
> > should be enough.
> 
> I was wrong about how many nss types are referenced here - there are many
> more in the cpp file, and the forward declaration is not working.
> 
> see: http://pastebin.mozilla.org/1491898
> 
> Is this idea of trying to use this PSM class sans xpcom inside dom/ not
> feasible?

The cpp file is in PSM, so you can just move the includes from the .h to that .cpp

(In reply to David Dahl :ddahl from comment #88)
> > @@ +21,5 @@
> > > +
> > > +class nsDOMCryptInternalAPIKeyPair //: public nsNSSShutDownObject
> > > +{
> > > +public:
> > > +  NS_DECL_ISUPPORTS
> > 
> > Remove this
> 
> When I remove these, I get:
> /home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/
> nsDOMCryptInternalAPI.h:23:3: error: ‘nsresult’ does not name a type

Include nscore.h
Comment 92 David Dahl :ddahl 2012-02-29 14:33:01 PST
Created attachment 601771 [details] [diff] [review]
v 4.9 It builds and Links!

Stripped out unneeded interfaces - this patch actually builds and links. Shocking. Provides PKGenerateKeyPair, PKEncrypt, PKDecrypt - not tested. I think the mInternalAPI member variable in nsDOMCryptAPI ought to be lazily init'd or I should be using an nsRefPtr?? not sure. If you have time for feedback I would greatly appreciate it. Also, I think my string usage is still not right in some cases. I re-read the docs again, it helped.
Comment 93 David Dahl :ddahl 2012-02-29 22:09:33 PST
Created attachment 601872 [details] [diff] [review]
v 5 It Builds, but the component is not available in Components.classes

The previous questions are still valid, I added a test and tried to load an instance and I get:

TEST-INFO | (xpcshell/head.js) | test 1 pending
DOMCryptAPI: TypeError: Cc['@mozilla.org/domcrypt-api;1'] is undefined
DOMCryptAPI: run_test()@/home/ddahl/code/moz/domcrypt/obj-x86_64-unknown-linux-gnu-debug/_tests/xpcshell/dom/tests/unit/test_domcryptapi.js:13
_execute_test()@/home/ddahl/code/moz/domcrypt/src/testing/xpcshell/head.js:326
@-e:1


gdb tells me:

(gdb) break nsDOMCryptAPI::nsDOMCryptAPI
Breakpoint 1 at 0x2ad7cdb09edd: file /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPI.cpp, line 96.
(gdb) bt
#0  0x00002ad7d0af3bad in read () at ../sysdeps/unix/syscall-template.S:82
#1  0x00002ad7d0a94d38 in _IO_new_file_underflow (fp=0x2ad7d0db89c0) at fileops.c:606
#2  0x00002ad7d0a95d7e in _IO_default_uflow (fp=0x2ad7d0db89c0) at genops.c:440
#3  0x00002ad7d0a8a08a in _IO_getline_info (fp=0x2ad7d0db89c0, buf=0x7fff0518c370 "", n=255, delim=10, extract_delim=1, eof=0x0) at iogetline.c:74
#4  0x00002ad7d0a88f6b in _IO_fgets (buf=0x7fff0518c370 "", n=<optimized out>, fp=0x2ad7d0db89c0) at iofgets.c:58
#5  0x0000000000406121 in GetLine (cx=0x2ad7d9ff2000, bufp=0x7fff0518c530 "", file=0x2ad7d0db89c0, prompt=0x43000b "js> ")
    at /home/ddahl/code/moz/domcrypt/src/js/xpconnect/shell/xpcshell.cpp:277
#6  0x0000000000408255 in ProcessFile (cx=0x2ad7d9ff2000, obj=0x2ad7dd22f060, filename=0x0, file=0x2ad7d0db89c0, forceTTY=1)
    at /home/ddahl/code/moz/domcrypt/src/js/xpconnect/shell/xpcshell.cpp:1075
#7  0x0000000000408603 in Process (cx=0x2ad7d9ff2000, obj=0x2ad7dd22f060, filename=0x0, forceTTY=1) at /home/ddahl/code/moz/domcrypt/src/js/xpconnect/shell/xpcshell.cpp:1130
#8  0x0000000000408d11 in ProcessArgs (cx=0x2ad7d9ff2000, obj=0x2ad7dd22f060, argv=0x7fff0518da30, argc=20) at /home/ddahl/code/moz/domcrypt/src/js/xpconnect/shell/xpcshell.cpp:1295
#9  0x000000000040a948 in main (argc=20, argv=0x7fff0518da30, envp=0x7fff0518dad8) at /home/ddahl/code/moz/domcrypt/src/js/xpconnect/shell/xpcshell.cpp:2001
(gdb) n
read () at ../sysdeps/unix/syscall-template.S:83
83	in ../sysdeps/unix/syscall-template.S
(gdb) 
_IO_new_file_underflow (fp=0x2ad7d0db89c0) at fileops.c:608
608	fileops.c: No such file or directory.
	in fileops.c
(gdb) 
618	in fileops.c
(gdb) 
615	in fileops.c
(gdb) 
618	in fileops.c
(gdb) 
620	in fileops.c
(gdb) 
621	in fileops.c
(gdb) 
_IO_default_uflow (fp=0x2ad7d0db89c0) at genops.c:441
441	genops.c: No such file or directory.
Comment 94 David Dahl :ddahl 2012-02-29 22:16:40 PST
Comment on attachment 601771 [details] [diff] [review]
v 4.9 It builds and Links!

I also changed the CID to mozilla.org/dom/domcrypt-api;1 and I ran:

$ strings ./dist/bin/components/dom_base.xpt | grep -i crypt
nsIDOMCrypto
nsIDOMCryptAPIPublicKey
nsIDOMCryptAPI
PKEncrypt
PKDecrypt
crypto

So I assume there is one last undocumented thing to do (or two):)
Comment 95 :Ms2ger (⌚ UTC+1/+2) 2012-03-01 01:36:44 PST
Comment on attachment 601872 [details] [diff] [review]
v 5 It Builds, but the component is not available in Components.classes

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

Haven't reviewed it entirely

::: dom/base/nsDOMCryptAPI.cpp
@@ +14,5 @@
> +
> +// Base64urlEncode / Decode helpers
> +static 
> +nsresult 
> +Base64urlEncoder(nsCString aPlainData, nsACString& aB64UrlEncoded)

const nsCString& aPlaindata.

@@ +21,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  // XXXddahl: base64 URL encoded
> +  // nsCString tmpStr = aB64UrlEncoded;
> +  // tmpStr.ReplaceChar('+', '-');
> +  // tmpStr.ReplaceChar('/', '_');

So, since you only call this with a nsCString for aB64UrlEncoded, I think you should just change to signature to take nsCString& instead of nsACString&, and do ReplaceChar directly on aB64UrlEncoded.

@@ +30,5 @@
> +}
> +
> +static 
> +nsresult
> +Base64urlDecoder(nsACString& aB64UrlEncoded, nsACString& aDecodedData)

const nsACString& aB64UrlEncoded

@@ +35,5 @@
> +{
> +  // XXXddahl: this should be base64 URL decoded
> +  // nsCString tmpStr = aB64UrlEncoded;
> +  // tmpStr.ReplaceChar('_', '/');
> +  // tmpStr.ReplaceChar('-', '+');

This should work, I think. Any particular reason to comment it out?

@@ +37,5 @@
> +  // nsCString tmpStr = aB64UrlEncoded;
> +  // tmpStr.ReplaceChar('_', '/');
> +  // tmpStr.ReplaceChar('-', '+');
> +  
> +  // nsACString replacedStr = tmpStr;

Remove this line

@@ +118,5 @@
> +  nsDOMCryptInternalAPIKeyPair* internalKeyPair;
> +
> +  rv = mInternalAPI->PKGenerateKeyPair(keySizeInBits, 
> +                                       algorithm, 
> +                                       internalKeyPair);

&internalKeyPair

@@ +124,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  unsigned char* derPubKey;
> +  internalKeyPair->GetDEREncodedRSAPublicKey(derPubKey);

&derPubKey

@@ +143,5 @@
> +  if (NS_FAILED(rv)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsCString base64DERPubKey = tmpStr; // convert nsCString to nsACString

No need for this, you can just use the nsCString you already have.

@@ +160,5 @@
> +
> +NS_IMETHODIMP
> +nsDOMCryptAPI::PKEncrypt(nsIDOMCryptAPIPublicKey* aPublicKey,
> +                         const jsval& aClearData,
> +                         jsval* _retval NS_OUTPARAM)

You're not returning anything?

@@ +179,5 @@
> +    return NS_ERROR_FAILURE; // TODO: perhaps we need a better error here?
> +  }
> +  char* clearData = NULL;
> +
> +  clearData = static_cast<char*>(JS_GetTypedArrayData(view));

You've got two-step assignment like this in a number of places, they should just be like

char* clearData = static_cast<char*>(JS_GetTypedArrayData(view));

@@ +201,5 @@
> +  mInternalAPI->PKEncrypt(derPubKeyData, 
> +                          clearData, 
> +                          encryptedData);
> +
> +  NS_ENSURE_TRUE(encryptedData != NULL, NS_ERROR_FAILURE);

It will be null, because mInternalAPI->PKEncrypt can't touch your encryptedData

@@ +213,5 @@
> +                         jsval* aClearData NS_OUTPARAM)
> +{
> +  // Convert everything to char* to call the internal API
> +  char* keyID = NULL;
> +  keyID = const_cast<char*>(aKeyID.Data());

Are we sure aKeyID.Data() won't go away too soon? Maybe this needs to copy

@@ +226,5 @@
> +    return NS_ERROR_DOM_TYPE_MISMATCH_ERR;
> +  }
> +
> +  char* encryptedData = NULL;
> +  encryptedData = static_cast<char*>(JS_GetTypedArrayData(encryptedView));

.

@@ +232,5 @@
> +  NS_ENSURE_TRUE(encryptedData != NULL, NS_ERROR_FAILURE);
> +  
> +  // Get the clear buffer data
> +
> +  if (!aClearData->isObject()) {

aClearData is a return value; you need to create a typed array yourself

::: dom/base/nsDOMCryptAPI.h
@@ +49,5 @@
> +  nsDOMCryptAPI();
> +
> +private:
> +  nsDOMCryptInternalAPI* mInternalAPI;
> +  // nsRefPtr<nsDOMCryptInternalAPI> mInternalAPI; // Should use an nsRefPtr??

I don't think so, unless someone else can get its hand on this pointer. I think it should just stay alive as long as nsDOMCryptAPI does, so either |delete| in the dtor, or use nsAutoPtr.

@@ +50,5 @@
> +
> +private:
> +  nsDOMCryptInternalAPI* mInternalAPI;
> +  // nsRefPtr<nsDOMCryptInternalAPI> mInternalAPI; // Should use an nsRefPtr??
> +  // ~nsDOMCryptAPI(); // XXXddahl: trouble declaring dtor

You probably need to make it public

::: security/manager/ssl/src/nsDOMCryptInternalAPI.cpp
@@ +169,5 @@
> +
> +nsresult
> +nsDOMCryptInternalAPI::PKGenerateKeyPair(PRUint32 aKeySizeInBits,
> +                                         PRUint32 aAlgorithm,
> +                                         nsDOMCryptInternalAPIKeyPair *_retval)

This needs to be a pointer-to-a-pointer

@@ +208,5 @@
> +  _retval = new nsDOMCryptInternalAPIKeyPair(privKey, 
> +                                             pubKey, 
> +                                             aKeySizeInBits, 
> +                                             aAlgorithm, 
> +                                             nickname);

This currently only changes the local variable _retval, not the pointer your caller passed in.

@@ +223,5 @@
> +  SECStatus srv;
> +  SECKEYPublicKey* pubk = NULL;
> +  SECItem* derEncodedKey;
> +
> +  derEncodedKey->data = (unsigned char*)aDERPubKey;

Wait, don't you need to initialize your derEncodedKey pointer first?

@@ +231,5 @@
> +  NS_ENSURE_TRUE(pubk != NULL, NS_ERROR_FAILURE);
> +
> +  srv = PK11_PubEncryptPKCS1(pubk, 
> +                             (unsigned char*)aEncryptedData, 
> +                             (unsigned char*)aClearData, 

static_cast

@@ +255,5 @@
> +  PK11SlotInfo *slot = PK11_GetInternalSlot();
> +  SECItem* keyID;
> +  
> +  keyID->data = (unsigned char*)aKeyID;
> +  keyID->len = strlen(aKeyID);

I think you should just pass an XPCOM string here and use its Length() instead of recalculating.

::: security/manager/ssl/tests/unit/test_domcrypt_pk_generate_keypair.js
@@ +13,5 @@
> +  dump("DOMCryptPK test: " + aMsg + "\n");
> +}
> +
> +XPCOMUtils.defineLazyGetter(this, "DOMCryptInternalAPI", function (){
> +  return Cc["@mozilla.org/domcrypt-internal-api;1"].createInstance(Ci.nsIDOMCryptInternalAPI);

You noticed this was missing a dom/, right?
Comment 96 David Dahl :ddahl 2012-03-01 08:44:12 PST
(In reply to Ms2ger from comment #95)
> Comment on attachment 601872 [details] [diff] [review]
> ::: security/manager/ssl/tests/unit/test_domcrypt_pk_generate_keypair.js
> @@ +13,5 @@
> > +  dump("DOMCryptPK test: " + aMsg + "\n");
> > +}
> > +
> > +XPCOMUtils.defineLazyGetter(this, "DOMCryptInternalAPI", function (){
> > +  return Cc["@mozilla.org/domcrypt-internal-api;1"].createInstance(Ci.nsIDOMCryptInternalAPI);
> 
> You noticed this was missing a dom/, right?

I need to remove that test, it us from before I moved the public API out into dom/base and removed the public interface from the internal API.

The test I added should be in dom/tests/unit. I am working through these comments and will post another patch soon. Thanks!
Comment 97 David Dahl :ddahl 2012-03-01 17:16:47 PST
Created attachment 602211 [details] [diff] [review]
v 5.1 build problems

more include problems where nsIDOMCryptInternalAPI.h cannot include nss headers:

nsDOMCryptAPIModule.cpp
In file included from ../../dist/include/nsDOMCryptInternalAPI.h:14:0,
                 from /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPI.h:8,
                 from /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPI.cpp:5:
../../dist/system_wrappers/secerr.h:3:25: fatal error: secerr.h: No such file or directory
compilation terminated.
In file included from ../../dist/include/nsDOMCryptInternalAPI.h:14:0,
                 from /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPI.h:8,
                 from /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPIModule.cpp:8:
../../dist/system_wrappers/secerr.h:3:25: fatal error: secerr.h: No such file or directory
compilation terminated.
Comment 98 :Ms2ger (⌚ UTC+1/+2) 2012-03-02 04:13:28 PST
Created attachment 602308 [details] [diff] [review]
v 5.2

This should build
Comment 99 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-03-21 14:30:12 PDT
need to schedule a full review on the calendar
Comment 100 David Dahl :ddahl 2012-04-11 15:28:42 PDT
Created attachment 614197 [details] [diff] [review]
v 5.3 Builds - cleaning up runtime issues

Saving my work, still have some runtime issues to fix
Comment 101 David Dahl :ddahl 2012-04-11 19:41:26 PDT
Getting a segfault in PK11_GetInternalSlot:

82	../sysdeps/unix/syscall-template.S: No such file or directory.
	in ../sysdeps/unix/syscall-template.S
(gdb) break nsDOMCryptInternalAPI::GenerateKeyPair
Breakpoint 1 at 0x2aece09a35e9: file /home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsDOMCryptInternalAPI.cpp, line 141.
(gdb) c
Continuing.

Breakpoint 1, nsDOMCryptInternalAPI::GenerateKeyPair (this=0x2aeceb807918, aPubKey=0x0, aPrivKey=0x0, aKeySizeInBits=2048, aAlgorithm=1)
    at /home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsDOMCryptInternalAPI.cpp:141
141	  PK11SlotInfo *slot = NULL;
(gdb) n
142	  slot = PK11_GetInternalSlot();
(gdb) 

Program received signal SIGSEGV, Segmentation fault.
0x00002aecdec0e48c in TouchBadMemory () at /home/ddahl/code/moz/domcrypt/src/memory/mozalloc/mozalloc_abort.cpp:68
68	    gDummyCounter += *p;   // TODO annotation saying we know
Comment 102 David Dahl :ddahl 2012-04-11 19:48:33 PDT
More clues...

Breakpoint 1, PK11_GetInternalSlot () at pk11slot.c:1797
1797	    SECMODModule * mod = SECMOD_GetInternalModule();
(gdb) 
1798	    PORT_Assert(mod != NULL);
(gdb) 

Program received signal SIGSEGV, Segmentation fault.
0x00002b027485448c in TouchBadMemory () at /home/ddahl/code/moz/domcrypt/src/memory/mozalloc/mozalloc_abort.cpp:68
68	    gDummyCounter += *p;   // TODO annotation saying we know
Comment 103 David Dahl :ddahl 2012-04-12 09:00:38 PDT
Comment on attachment 614197 [details] [diff] [review]
v 5.3 Builds - cleaning up runtime issues

Crashing inside PK11_GetInternalSlot - Bad Memory access??
Comment 104 David Dahl :ddahl 2012-04-12 09:48:14 PDT
backtrace: http://pastebin.mozilla.org/1565956
Comment 105 David Dahl :ddahl 2012-04-13 14:03:29 PDT
In trying to re-enable nsNSSShutdownObject parent in nsDOMCryptInternalAPI, I cannot build:

c++ -o nsDOMCryptAPI.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include /home/ddahl/code/moz/domcrypt/src/config/gcc_hidden.h -D_IMPL_NS_LAYOUT -DMOZ_JSDEBUGGER -DMOZ_GLUE_IN_PROGRAM -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -DOSTYPE=\"Linux3.0\" -DOSARCH=Linux -DEXCLUDE_SKIA_DEPENDENCIES  -DOS_LINUX=1 -DOS_POSIX=1  -I/home/ddahl/code/moz/domcrypt/src/dom/base -I/home/ddahl/code/moz/domcrypt/src/dom/battery -I/home/ddahl/code/moz/domcrypt/src/dom/power -I/home/ddahl/code/moz/domcrypt/src/dom/network/src -I/home/ddahl/code/moz/domcrypt/src/dom/settings -I/home/ddahl/code/moz/domcrypt/src/dom/sms/src -I/home/ddahl/code/moz/domcrypt/src/dom/contacts -I/home/ddahl/code/moz/domcrypt/src/dom/src/events -I/home/ddahl/code/moz/domcrypt/src/dom/src/storage -I/home/ddahl/code/moz/domcrypt/src/dom/src/offline -I/home/ddahl/code/moz/domcrypt/src/dom/src/geolocation -I/home/ddahl/code/moz/domcrypt/src/dom/src/notification -I/home/ddahl/code/moz/domcrypt/src/dom/workers -I/home/ddahl/code/moz/domcrypt/src/content/xbl/src -I/home/ddahl/code/moz/domcrypt/src/content/xul/document/src -I/home/ddahl/code/moz/domcrypt/src/content/events/src -I/home/ddahl/code/moz/domcrypt/src/content/base/src -I/home/ddahl/code/moz/domcrypt/src/content/html/content/src -I/home/ddahl/code/moz/domcrypt/src/content/html/document/src -I/home/ddahl/code/moz/domcrypt/src/content/svg/content/src -I/home/ddahl/code/moz/domcrypt/src/layout/generic -I/home/ddahl/code/moz/domcrypt/src/layout/style -I/home/ddahl/code/moz/domcrypt/src/layout/xul/base/src -I/home/ddahl/code/moz/domcrypt/src/layout/xul/base/src/tree/src -I/home/ddahl/code/moz/domcrypt/src/ipc/chromium/src -I/home/ddahl/code/moz/domcrypt/src/ipc/glue -I../../ipc/ipdl/_ipdlheaders  -I/home/ddahl/code/moz/domcrypt/src/js/xpconnect/src -I/home/ddahl/code/moz/domcrypt/src/js/xpconnect/wrappers -I/home/ddahl/code/moz/domcrypt/src/xpcom/ds  -I/home/ddahl/code/moz/domcrypt/src/dom/base -I. -I../../dist/include -I../../dist/include/nsprpub  -I/home/ddahl/code/moz/domcrypt/obj-x86_64-unknown-linux-gnu-debug/dist/include/nspr -I/home/ddahl/code/moz/domcrypt/obj-x86_64-unknown-linux-gnu-debug/dist/include/nss      -fPIC -fno-rtti -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wcast-align -Wno-long-long -fno-exceptions -fno-strict-aliasing -std=gnu++0x -ffunction-sections -fdata-sections -pthread -pipe -DDEBUG -D_DEBUG -DTRACING -g -fno-omit-frame-pointer  -pthread -I/usr/include/gtk-2.0 -I/usr/lib/x86_64-linux-gnu/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/pango-1.0 -I/usr/include/gio-unix-2.0/ -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/gtk-unix-print-2.0     -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MF .deps/nsDOMCryptAPI.pp /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPI.cpp
In file included from /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPI.h:8:0,
                 from /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPI.cpp:5:
../../dist/include/nsDOMCryptInternalAPI.h:12:27: fatal error: nsNSSShutDown.h: No such file or directory

I tried EXPORTING nsNSSShutdown to no avail
Comment 106 David Dahl :ddahl 2012-04-13 14:10:24 PDT
Created attachment 614909 [details] [diff] [review]
new attempt at nsNSSShutdownObject inheritance

trying to get the nsNSSShutdownObject inheritance working. This always fails once we build in dom/base/nsDOMCryptAPI.cpp/.h

I thought maybe my previous BadMemory erros might have something to do with this. Also, we cannot use NSS APIs without nsIDOMCryptInternalAPI deriving from nsNSSShutdwonObject
Comment 107 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-04-13 14:57:13 PDT
nsNSSShutDown.h is not exported by PSM so you cannot (currently) extend nsNSSShutdownObject in a class that is outside of PSM. If you need to do so in the interim, add that header to the EXPORTS section of PSM while you hack, until we find a more permanent solution.
Comment 108 David Dahl :ddahl 2012-04-13 15:02:41 PDT
(In reply to Brian Smith (:bsmith) from comment #107)
> in the interim, add that header to the EXPORTS section of PSM while you
> hack, until we find a more permanent solution.

I tried to add the file to the EXPORTS in security/manager/ssl/Makefile.in - is that what you mean by 'the EXPORTS section of PSM'?
Comment 109 David Dahl :ddahl 2012-04-13 22:59:16 PDT
Created attachment 615009 [details] [diff] [review]
nss loading issues
Comment 110 David Dahl :ddahl 2012-04-14 09:49:49 PDT
based on irc conversation it looks like we need to authenticate with the keydb in order to use isPerm = PR_TRUE in PK11_GenerateKeyPair()

Is this the kind of thing we need here:
http://mxr.mozilla.org/mozilla-central/source/security/nss/cmd/signtool/util.c#921

if so, how is the password handled? Is there some other way to authenticate?
Comment 111 David Dahl :ddahl 2012-04-17 16:31:03 PDT
Created attachment 615943 [details] [diff] [review]
5.4 GenerateKeyPair is working, test is passing

w00t!
Comment 112 David Dahl :ddahl 2012-04-17 21:58:35 PDT
Created attachment 616011 [details] [diff] [review]
v 5.5 Patch - PKGenerateKeyPair

Fixed a few things wrong with the pubkey property getters, added more checks to the test
Comment 113 Channy Yun [:channy] 2012-04-17 22:06:58 PDT
(In reply to David Dahl :ddahl from comment #112)
> Created attachment 616011 [details] [diff] [review]
> v 5.5 Patch - PKGenerateKeyPair
> 
> Fixed a few things wrong with the pubkey property getters, added more checks
> to the test

Wow. cool. How about separating bug for each methods? Very long :)
Comment 114 David Dahl :ddahl 2012-04-17 22:15:14 PDT
(In reply to Channy Yun from comment #113)
> (In reply to David Dahl :ddahl from comment #112)
> > Created attachment 616011 [details] [diff] [review]
> > v 5.5 Patch - PKGenerateKeyPair
> > 
> > Fixed a few things wrong with the pubkey property getters, added more checks
> > to the test
> 
> Wow. cool. How about separating bug for each methods? Very long :)

Good idea. This is also only the 'internal' API - there is a separate bug for the DOM bindings: bug 721199. Perhaps this bug will just be for GenerateKeyPair. I will create bugs for encrypt/decrypt, sign/verify, hash, hmac, and symmetric crypto.
Comment 115 David Dahl :ddahl 2012-04-20 16:57:08 PDT
Created attachment 617147 [details] [diff] [review]
v 5.5 Moved keygen to a thread, needs a test
Comment 116 Kyle Hamilton 2012-04-20 17:42:24 PDT
*** Bug 741677 has been marked as a duplicate of this bug. ***
Comment 117 David Dahl :ddahl 2012-04-30 16:22:47 PDT
Created attachment 619745 [details] [diff] [review]
Patch 5.6 - Keygen on thread tests passing

Saving work, seeing WARNING: nsExceptionService ignoring thread destruction after shutdown: file /home/ddahl/code/moz/domcrypt/src/xpcom/base/nsExceptionService.cpp, line 199
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0xC1F30001: file /home/ddahl/code/moz/domcrypt/obj-x86_64-unknown-linux-gnu-debug/xpcom/build/nsThreadUtils.cpp, line 168
Comment 118 David Dahl :ddahl 2012-05-03 19:40:29 PDT
Created attachment 620935 [details] [diff] [review]
Patch 5.7 build fails, just saving work

Working on PKEncrypt
Comment 119 David Dahl :ddahl 2012-05-04 21:37:24 PDT
Created attachment 621242 [details] [diff] [review]
Patch 5.8: PKEncrypt builds

Saving work, on to the scriptable interface with off main thread encrypt
Comment 120 David Dahl :ddahl 2012-07-11 13:26:49 PDT
Created attachment 641179 [details] [diff] [review]
Added PKEncryptResult interface, fixed some crashes
Comment 121 David Dahl :ddahl 2012-11-30 12:57:41 PST
Created attachment 687244 [details] [diff] [review]
Latest Patch, builds!

This thing builds, going to test it
Comment 122 David Dahl :ddahl 2012-11-30 14:19:14 PST
Test is failing like so:

TEST-INFO | (xpcshell/head.js) | test 3 pending
TEST-INFO | /home/ddahl/code/moz/domcrypt/obj-x86_64-unknown-linux-gnu-debug/_tests/xpcshell/dom/tests/unit/test_domcryptapi.js | Starting testPKEncrypt

TEST-PASS | undefined | [undefined : undefined] true == true
WARNING: NS_ENSURE_TRUE(subjectPublicKeyInfo != NULL) failed: file /home/ddahl/code/moz/domcrypt/src/security/manager/ssl/src/nsDOMCryptInternalAPI.cpp, line 481

the code here is:

  CERTSubjectPublicKeyInfo* subjectPublicKeyInfo;
  subjectPublicKeyInfo = SECKEY_DecodeDERSubjectPublicKeyInfo(derEncodedKey);
  NS_ENSURE_TRUE(subjectPublicKeyInfo != NULL, NS_ERROR_FAILURE);

I am wondering if the key that is generated is incorrect kind/size/whatever - I can only generate a 512 bit key otherwise things crash.

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /home/ddahl/code/moz/domcrypt/src/dom/base/nsDOMCryptAPIPKEncryptRunnable.cpp, line 77
Comment 123 David Dahl :ddahl 2012-11-30 14:56:40 PST
Segfault inside PK11_GenerateKeyPair when generating a 2048 key. more debug info later
Comment 124 David Dahl :ddahl 2013-04-25 11:20:43 PDT
Resolving Invalid as the W3C API is vastly different, see bug 865789

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