If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Exporting/Sharing keypairs

RESOLVED FIXED in M2

Status

Other Applications
McCoy
--
enhancement
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: nmaier, Assigned: mossop)

Tracking

unspecified

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

10 years ago
McCoy must allow to import/export keypairs (certs).

Otherwise is is impossible to share such keys in small teams, transfer ownership or perform backups of them (assuming sharing the whole profile isn't a good option).

I didn't dig into the code enough to guess what would be the best solution, but the import/export capabilities for Client Certificates (PKCS12) reensemble such a feature quite well IMO.
(Assignee)

Updated

10 years ago
Severity: critical → enhancement
(Reporter)

Comment 1

10 years ago
> Dave Townsend (Mossop) <dtownsend@mozilla.com> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>            Severity|critical                    |enhancement

Actually, this isn't just a random enhancement to me, but a damn critical feature that is still missing.
To be more clear: Without some import/export functionality that enables sharing/backup McCoy/update-signing isn't an option for me. And for some of my projects https isn't an option either, which eventually leaves me effectively option-less for said projects.
(Assignee)

Comment 2

10 years ago
What exactly do you need that passing around the McCoy profile data doesn't provide? Why do you even need to share the keys around, do you really have add-ons where it varies who posts the update.rdf?
(Reporter)

Comment 3

10 years ago
(In reply to comment #2)
> What exactly do you need that passing around the McCoy profile data doesn't
> provide?
I can only pass around the whole profile, but I don't want to share ALL my keys when only ONE of them must be shared, obviously.
I cannot import the update keys from somebody else in my already filled McCoy profile when I e.g. take over a project from somebody else or become the "release-guy".
Sure, using multiple-profiles would be possible (I didn't check if it actually possible at the moment), but it is inconvenient at best, and the data isn't in an accessible format (effectively meaning a lock-in to McCoy). 

> Why do you even need to share the keys around, do you really have
> add-ons where it varies who posts the update.rdf?
Yes, I did (and likely will do) extensions for different people/companies.
Some of them are not maintained by me any longer, the clients do their updates themselves. In this case I'd have to share my profile with them, or somehow copy it, delete all unrelated keys and hope that they were actually wiped from the key database in an irretrievable fashion.

Clients could go https instead, but acquiring https certs from a CA for intranet-only servers seems overkill to me (and I'm sure for them too).
Deploying their own CA doesn't work with extensions (you intensionally disabled it).
(Assignee)

Comment 4

10 years ago
So the majority here seems to be ablout passing the development of an extension from one person to another, certainly something we want to support.

FWIW it's probably far easier to go https in the intranet situation since you can just self sign and administrators install the CA into Firefox.
(Reporter)

Comment 5

10 years ago
(In reply to comment #4)
> So the majority here seems to be ablout passing the development of an extension
> from one person to another, certainly something we want to support.
> 
I value the capability to share a single keypair between team members even higher.
Assume you're involved in more than one extension development and therefore have multiple keypairs. How do you pass just a single key for project A without sharing the keypair for project B as well?
You join another project, then how do you receive and "import" the keypair from that project.
Having just a single project member having the keypair isn't something I'd desire. What happens if this member "disappears" for whatever reason? Right, no more updates for your users.

(In reply to comment #4)
> FWIW it's probably far easier to go https in the intranet situation since you
> can just self sign and administrators install the CA into Firefox.
> 
This is getting Off-Topic, but anyway.
Am I missing something? Your CheckCert function will not allow any CA that doesn't come from Builtin-Object-Token. And Installing a local CA cert doesn't put it there.
http://mxr.mozilla.org/seamonkey/source/xpinstall/src/nsXPInstallManager.cpp#1011

I actually tested it myself some time ago. Created a CA, signed a new server cert with that CA. Installed the CA cert into a nightly and tried to access a localhost server. Browsing worked fine as expected (no warning), but updates from there failed in CheckCert as expected (s.a.).
(Assignee)

Comment 6

10 years ago
(In reply to comment #5)
> (In reply to comment #4)
> > So the majority here seems to be ablout passing the development of an extension
> > from one person to another, certainly something we want to support.
> > 
> I value the capability to share a single keypair between team members even
> higher.
> Assume you're involved in more than one extension development and therefore
> have multiple keypairs. How do you pass just a single key for project A without
> sharing the keypair for project B as well?
> You join another project, then how do you receive and "import" the keypair from
> that project.
> Having just a single project member having the keypair isn't something I'd
> desire. What happens if this member "disappears" for whatever reason? Right, no
> more updates for your users.

And I disagree that sharing keypairs amongst teams is a massive requirement. But it doesn't really matter, this is a valid feature request and will be implemented.
(Assignee)

Updated

10 years ago
Target Milestone: --- → M2
(Assignee)

Updated

10 years ago
Blocks: 410265

Comment 7

9 years ago
It's a set of PGP keys is all, you can literally SEND them to each other. You want to know how? Well I suggest email, but you could use IM, or snail mail, SMS, or voice. If you want to encrypt it so it can't be stolen just install enigmail in thunderbird and use it to encrypt it, it happens to use PGP too.
(Assignee)

Comment 8

9 years ago
Created attachment 323059 [details] [diff] [review]
binary patch rev 1

This is the binary part of the patch adding support to the component for exporting and importing private keys as a PEM encoded EncryptedPrivateKeyInfo.

Kai, any chance you have a moment to give this a glance over, it's mostly from the sample code Elio provided and I'm pretty sure it is correct since it can export and import keys that openssl happily reads. Just wanted a check on the allocations and just to check I'm not doing anything silly if possible.
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Attachment #323059 - Flags: review?(kaie)
(Assignee)

Comment 9

9 years ago
Created attachment 323061 [details] [diff] [review]
UI patch

This simply hooks up the UI for import/export and adds a set of mochitests verifying the import functionality. Reworks the keyservice mochitest a little into separate functions which is why it looks worse than it actually is.
Attachment #323061 - Flags: review?(mark.finkle)
Comment on attachment 323061 [details] [diff] [review]
UI patch

>+++ b/chrome/content/keymanager.js

>+        case "cmd_importkey":
>+          var picker = Cc["@mozilla.org/filepicker;1"].
>+                       createInstance(Ci.nsIFilePicker);
>+          picker.init(window, gStrings.getString("importfile.title"), Ci.nsIFilePicker.modeOpen);
>+          if (picker.show() != Ci.nsIFilePicker.returnCancel) {
>+            var istream = Cc["@mozilla.org/network/file-input-stream;1"].
>+                          createInstance(Ci.nsIFileInputStream);
>+            istream.init(picker.file, 0x01, 0444, 0);
>+            istream.QueryInterface(Ci.nsILineInputStream);
>+            var data = "";
>+            var line = {};
>+            do {
>+              var hasmore = istream.readLine(line);
>+              data += line.value + "\n";
>+            } while (hasmore);
>+            istream.close();

You convert to utf8 when exporting. Should you be worried about that when importing?

>+          var pass1 = { value: "" };
>+          var pass2 = { value: "" };
>+          do {
>+            if (!promptSvc.promptPassword(window,
>+                                          gStrings.getString("exportpass1.title"),
>+                                          gStrings.getString("exportpass1.text"),
>+                                          pass1, null, {}))
>+              return;
>+            if (!promptSvc.promptPassword(window,
>+                                          gStrings.getString("exportpass2.title"),
>+                                          gStrings.getString("exportpass2.text"),
>+                                          pass2, null, {}))
>+              return;
>+            if (pass1.value != pass2.value) {
>+              promptSvc.alert(window, gStrings.getString("exportmismatch.title"),
>+                                      gStrings.getString("exportmismatch.text"));
>+            }
>+          } while (pass1.value != pass2.value);

too bad we don't have a better password/confirm password prompt


>+++ b/chrome/content/keymanager.xul
 
>   <menupopup id="menu-keycontext">
>     <menuitem id="context-copypublic" command="cmd_copypublic"
>               label="&copypublic.label;" class="menuitem-iconic"/>
>+    <menuitem id="context-exportkey" command="cmd_exportkey"
>+              label="&exportkey.label;" class="menuitem-iconic"/>
>     <menuitem id="context-renamekey" command="cmd_renamekey"
>               label="&renamekey.label;" class="menuitem-iconic"/>
>     <menuitem id="context-deletekey" command="cmd_deletekey"
>               label="&deletekey.tooltip;" class="menuitem-iconic"/>
>   </menupopup>
> 
>   <menubar>
>-    <menu label="&keymenu.label;" accesskey="&keymenu.accesskey;">
>+    <menu label="&keystoremenu.label;" accesskey="&keystoremenu.accesskey;">
>       <menupopup>
>         <menuitem id="menu-createkey" command="cmd_createkey"
>                   label="&createkey.tooltip;" class="menuitem-iconic"
>                   accesskey="&createkey.accesskey;"/>
>+        <menuitem id="menu-importkey" command="cmd_importkey"
>+                  label="&importkey.label;" class="menuitem-iconic"
>+                  accesskey="&importkey.accesskey;"/>
>         <menuitem id="menu-changepassword" command="cmd_changepassword"
>                   label="&changepassword.label;" class="menuitem-iconic"
>                   accesskey="&changepassword.accesskey;"/>
>-        <menuseparator/>
>+      </menupopup>
>+    </menu>
>+    <menu label="&keymenu.label;" accesskey="&keymenu.accesskey;">
>+      <menupopup>
>         <menuitem id="menu-copypublic" command="cmd_copypublic"
>                   label="&copypublic.label;" class="menuitem-iconic"
>                   accesskey="&copypublic.accesskey;"/>
>+        <menuitem id="menu-exportkey" command="cmd_exportkey"
>+                  label="&exportkey.label;" class="menuitem-iconic"/>
>         <menuitem id="menu-renamekey" command="cmd_renamekey"
>                   label="&renamekey.label;" class="menuitem-iconic"
>                   accesskey="&renamekey.accesskey;"/>
>         <menuitem id="menu-deletekey" command="cmd_deletekey"
>                   label="&deletekey.tooltip;" class="menuitem-iconic"
>                   accesskey="&deletekey.accesskey;"/>
>       </menupopup>
>     </menu>

You like the 2 menu approach? Two of three menus on the keystore menu are "key" related. Any chance of user confusion here?
Attachment #323061 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 11

9 years ago
Comment on attachment 323059 [details] [diff] [review]
binary patch rev 1

This isn't compiling on windows due to some linking problems
Attachment #323059 - Flags: review?(kaie)
(Assignee)

Comment 12

9 years ago
Created attachment 323214 [details] [diff] [review]
binray patch rev 2

Always seem to hit linking issues with the ASN1 templates on Windows. In this case the only way I could resolve was by copying SECKEY_EncryptedPrivateKeyInfoTemplate into my code rather than trying to reference the same structure in NSS.
Attachment #323059 - Attachment is obsolete: true
Attachment #323214 - Flags: review?(kaie)

Comment 13

9 years ago
(In reply to comment #8)
> Kai, any chance you have a moment to give this a glance over, it's mostly from
> the sample code Elio provided and I'm pretty sure it is correct since it can
> export and import keys that openssl happily reads. Just wanted a check on the
> allocations and just to check I'm not doing anything silly if possible.

I glanced over it, and nothing bad jumped into my face.

Comment 14

9 years ago
But let me say that I haven't used many of these NSS APIs myself before, the best reviewer would be Bob Relyea, but I guess that Elio has already consulted with him when writing the sample code you got.

Comment 15

9 years ago
Comment on attachment 323214 [details] [diff] [review]
binray patch rev 2

I'm removing the request asking me for review, I think I'm not the right one to do a full review of this patch.
Attachment #323214 - Flags: review?(kaie)

Comment 16

9 years ago
I didn't do a full review, but I did look the key import and export code.

The export code is perfect.

I would have rather seen something else in the import code: the use of PK11_ImportEncryptedPrivateKeyInfo() instead... I'm not sure the code you have will work for keys other than RSA, and may not work on some tokens.

The issue is you need the public key to do this. (actually a component of the public key). This is used as an index to find the private key again later. RSA keys have this component built into the private key (the modulus), but other key types don't have this.

Anyway, I don't disapprove of the patch, but the issues should certainly be documented (preferably with a comment in the code).

bob

Comment 17

9 years ago
So when I look at the code, I realized that it is a function called export and import key pair, but it really only export and imports the private key.

If you include the public key, then it's possible to use PK11_ImportEncryptedPrivateKeyInfo(). I'm not sure if the intent is to use the key pair, or if the function is misnamed, but if it's the latter, we could probably save some heartache by passing in either a public key blob or the actual public key.

bob
(Assignee)

Comment 18

9 years ago
Created attachment 325405 [details] [diff] [review]
full patch with comments addressed

This is the full patch, taken on the comments from the binary patch and renamed the methods to exportPrivateKey and importPrivateKey and added comments to say that only RSA keys are supported.
Attachment #323061 - Attachment is obsolete: true
Attachment #323214 - Attachment is obsolete: true
Attachment #325405 - Flags: review+
(Assignee)

Comment 19

9 years ago
Committed in r15866
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.