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

You're aware of window.crypto, right? I think adding a separate window.crypt will be pretty confusing to web authors...

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

Has this been discussed with any other browser vendors?

(In reply to comment #3) > Has this been discussed with any other browser vendors? Not yet, but it will be.

(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

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

generateKeypair is working off main thread

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.

That assert doesn't look happy (in the "probably exploitable if you try" sense).

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

Backtrace

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.

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

Next: test suite

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.

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

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

Currently hunting down a leak I have introduced

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.

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

A few more documentation/comment tweaks and a new test file for testing bad input and I will ask for review

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.

Is there a spec for this, preferably one that's on a standards track?

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

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.

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

Can you please email it to whatwg@lists.whatwg.org?

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

(In reply to comment #35) > Can you please email it to whatwg@lists.whatwg.org? Done.

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.

Update: The WHAT-WG discussion is being archived here: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-May/031741.html

Just getting my head around WebIDL, I am sure a lot is missing yet like exceptions and wrong, as in how I handle callbacks

I was thinking something more like this

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?

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

Saving work - this is a bookmarklet demo that will allow OTR style chatting on any chat web site

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.

Saving work, not built or tested

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’

Everything builds and links now, the persistent key gen fails in nsDOMCryptInternalAPI::GenerateKeyPair

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

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

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?

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

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

Inparams would be const nsAString&, out are nsAString&

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

Running into a dead end with some private NSS functions used to get a private key from a keyID.

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

I gave feedback to David on IRC.

Saving my work

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

Ah, ok, making headway with NS_IMPL_ISUPPORTS0

I still cannot figure out how to enable the destructor and nsNSSShutdown bits

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

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

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

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

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)

You can't use NSS directly from the DOM. NSS is built too late for that.

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

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

(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

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.

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

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

This should build

need to schedule a full review on the calendar

Saving my work, still have some runtime issues to fix

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

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 on attachment 614197 [details] [diff] [review] v 5.3 Builds - cleaning up runtime issues Crashing inside PK11_GetInternalSlot - Bad Memory access??

backtrace: http://pastebin.mozilla.org/1565956

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

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

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?

w00t!

Fixed a few things wrong with the pubkey property getters, added more checks to the test

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

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

Working on PKEncrypt

Saving work, on to the scriptable interface with off main thread encrypt

This thing builds, going to test it

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

Segfault inside PK11_GenerateKeyPair when generating a 2048 key. more debug info later

Resolving Invalid as the W3C API is vastly different, see bug 865789