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

(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.

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

(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.

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.

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

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 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.

(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.

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.

(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.

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.

(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.

(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.

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.

(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.

(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.

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.

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’

(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)

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

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.

(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

(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.

(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.

(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.

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.)

(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.

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

(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)

(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

(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.

(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?

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 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):)

(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!

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.

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

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.

(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'?

(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 :)

(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.