67890 - create self-signed cert with existing key that signed CSR

Status: RESOLVED FIXED

(NSS :: Tools, enhancement, P3)

Description

[Ian McGreer]

open for consideration:  should certutil be able to self-sign cert requests 
using -C -x?  It is possible to do, by using PKCS#11 to grab the private key.  
For a history, see http://bugzilla.mozilla.org/show_bug.cgi?id=67132

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

In bug 67132, which I originally filed about this problem, I proposed the 
following patch to fix this problem.
Any objections to me checking this in?

diff -u -r1.20 certutil.c
--- certutil.c  2001/01/25 04:14:22     1.20
+++ certutil.c  2001/01/31 05:11:17
@@ -2540,6 +2540,7 @@
     /*  These commands require keygen.  */
     if (certutil.commands[cmd_CertReq].activated ||
         certutil.commands[cmd_CreateAndAddCert].activated ||
+        certutil.commands[cmd_CreateNewCert].activated ||
        certutil.commands[cmd_GenKeyPair].activated) {
        /*  XXX Give it a nickname.  */
        privkey =

Comment 2

[Wan-Teh Chang]

Changed the QA contact to Bishakha.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

After re-reading bug 67132, it's not clear to me whether the command
certutil -C -x   should 
a) generate a new key pair for self-signing the cert, or 
b) find an existing key pair and use that private key to sign the cert.

If it is a, then the small patch above is correct, but a small change
is also needed to nss/tests/cert/cert.sh to make the test scripts work.

RCS file: /cvsroot/mozilla/security/nss/tests/cert/cert.sh,v
retrieving revision 1.16
diff -u -r1.16 cert.sh
--- cert.sh     8 Apr 2002 23:39:22 -0000       1.16
+++ cert.sh     21 Aug 2002 01:08:58 -0000
@@ -250,7 +250,8 @@
 
     CU_ACTION="Sign ${CERTNAME}'s Request"
     certu -C -c "TestCA" -m "$CERTSERIAL" -v 60 -d "${P_R_CADIR}" \
-          -i req -o "${CERTNAME}.cert" -f "${R_PWFILE}" 2>&1
+          -i req -o "${CERTNAME}.cert" -f "${R_PWFILE}" \
+          -z "${R_NOISE_FILE}" 2>&1
     if [ "$RET" -ne 0 ]; then
         return $RET
     fi

If the correct answer is b, then both of these patches are irrelevant.

Comment 4

[Ian McGreer]

I believe the correct answer is (b).  When using -x in combination with -S, the
intention is to create a self-signed cert.  Likewise, I think -C -x should mean,
"create a self-signed cert from this request."

I think some (working) variation of the patch in bug 67132 comment #5 is the
correct solution.

Comment 5

[Ian McGreer]

Attachment: implement -C -x

I started with the patch from bug 67132, and came up with this one, that
actually works.  Note I had to export a function to create the key ID, I
couldn't see any other way to make this work.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Alexei, Please evaluate this bug, and the patch attached to it. It's only P3.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 96177 [details] [diff] [review]
implement -C -x

This patch may need to be brought up to date (that is, to apply cleanly to the trunk) before it can be properly reviewed.

Comment 8

[Alexei Volkov]

Attachment: integrate fix for -C -x and bug fix

The patch integrates Nelson's fix into current source tree with the following changes:
   * fixes leak of selfsignprivkey
   * fixes compiler warning: implicit declaration of function 'PK11_GetPubIndexKeyID'
   * removes unused arena and it's code
   * adds test to cert.sh to check that -C -x works

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 218482 [details] [diff] [review]
integrate fix for -C -x and bug fix

Alexei, This is not a full and complete review.  I suspect the
patch is incomplete, or contains changes that are irrelevant to
this bug.  I'd like you to make the patch complete and/or remove
irrelevant parts.

This patch adds a new declaration of a new function,  	
PK11_GetPubIndexKeyID, to two files: nss.def and pk11pub.h,
yet this patch does not seem to define the new function anywhere.
I'm guessing that either
a) these two new declarations do not belong in this patch, or 
b) these are declaring a function that has already been added to 
NSS, but was not properly declared, or 
c) the definition of this new function belongs in this patch, but 
is missing.  

Please clarify that issue.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 218482 [details] [diff] [review]
integrate fix for -C -x and bug fix

Alexei, please ignore my previous questions about this patch.
I see now that you were merely making the existing private funciton PK11_GetPubIndexKeyID public.

Bob, two questions for you:
1. Do you have any objections to making PK11_GetPubIndexKeyID public?

2. As shown below, this patch asks the user to supply a major hint 
about the slot that contains the private key, so that it can 
authenticate to that slot before calling PK11_FindKeyByKeyID.

>+	    if (PK11_NeedLogin(slot)) {
>+		rv = PK11_Authenticate(slot, PR_TRUE, pwarg);
>+		if (rv != SECSuccess) {
>+		    break;
>+		}
>+	    }
>+	    *selfsignprivkey = PK11_FindKeyByKeyID(slot, keyID, NULL);

Is there a function that will search all the local slots (it necessary) 
and authenticate to them, as necessary, to find the priv key corresponding
to a KeyID?  If so, what is it?

Alexei, I will have more review comments for this bug after Bob answers.
Some observations:
a) the list of new exported functions for NSS 3.11.1 in nss.def needs
to be put in ASCII collating sequence; that is, alphabetized.
b) Let's not add yet another new cert DB just for this test.  Let's 
use some existing cert DB.
c) Let's call the new shell function something more meaningful and 
less redundant than cert_certu_others.  Maybe cert_self_signed or
even cert_misc.  And make the comment for the function have the same
name as the function itself.

Comment 11

[Robert Relyea]

Comment on attachment 218482 [details] [diff] [review]
integrate fix for -C -x and bug fix

re: PK11_GetPubIndexKeyID

In principle, there shouldn't be any reason not to export it. However, there is a function which will find a private key on a slot for a given certificate:

PK11_FindPrivateKeyFromCert

we should use this.

bob

Comment 12

[Robert Relyea]

OK, I was incorrect about PK11_FindPrivateKeyFromCert, that is only for finding a key on a token which has the cert on that same token. In this case we don't yet have a fully formed certificate.

PK11_FindKeybyDERCert() is the correct (though incorrectly named) function. It takes an incomplete CERTCertificate structure (which in fact does not even need the DER cert component). It really only needs the subject public key to be correct.

We clearly need a function in the wrappers that will return a private key based on  it's public key mate. That would simplify this code (as far as reading what is going on) quite a bit.

bob

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 218482 [details] [diff] [review]
integrate fix for -C -x and bug fix

Please note my review remarks in comment 10 above

Comment 14

[Alexei Volkov]

Attachment: use PK11_FindKeybyDERCert to find cert priv key

this patch also has the following fixes:
   * reusing db located in "client" directory for the test
   * rename function to cert_certu_misc

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 256714 [details] [diff] [review]
use PK11_FindKeybyDERCert to find cert priv key

r=nelson.  This patch doesn't do everything I wish it did, but it's a huge
step forward from what we have today so I don't want to stop it from 
being checked in.

The method used in this patch to find the private key requires that the user
tell certutil which slot the private key is in.  (Default is the internal 
key slot, naturally).  I would like it if it could find the private key 
in any slot, without the user having to specify the slot.  I would like
the user to be able to specify slot "all" and have certutil look in all
the slots for the matching private key.

If the user specifies a specific slot, then we should look for the key only 
in that slot,  and only use the key if we find it in that slot.  But 
if the user specifies slot "all", we should search all the slots looking
for the key, and tell the user which slot we found it in, and then ask
the user to login to that slot.  But that further enhancement can come 
later.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Alexei, this bug has a patch that was reviewed (r+) 14 months ago. 
I doubt that it still applies cleanly.  
If it does, and it still works, please commit it for 3.12.1.
If it does not apply cleanly, please generate a new patch for 3.12.1 
and request review, again.

Comment 17

[Alexei Volkov]

Patch to 341371 made certutil to be able to generate CR with exiting key. Closing this bug as dup

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

No, this bug is NOT a duplicate of bug 341371.
bug 341371 is abount generating a CSR.
This bug is about generating a self-signed cert from a CSR.  
That is, the object is this: given a CSR, create a self-signed cert for it.

Here is a test script that can be used to determine if this bug is fixed:

mkdir test
echo "test" > test/pwfile
dd if=/dev/urandom of=test/noise bs=256 count=1
certutil -N -d test -f test/pwfile
certutil -R -d test -f test/pwfile -s "CN=foo,O=bar" -a -o test/cert-req.pem \
  -z test/noise
certutil -C -d test -f test/pwfile -x -a -i test/cert-req.pem -o test/cert.pem
pp -t certificate -a -i test/cert.pem


Today, that script fails.  The patch attached to this bug makes it work
(or did make it work when it was written).  However, that patch no longer 
applies cleanly to the trunk.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Alexei's previous patch, brought forward to tip (checked in on trunk)

This is Alexei's previous patch, after bringing it forward to the tip
of the trunk.  With this patch, the above test script passes.  
I have not seriously reviewed this patch.  I just brought the old one
forward to the tip by hand.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 317126 [details] [diff] [review]
Alexei's previous patch, brought forward to tip (checked in on trunk)

Alexei's previous patch (attachment 256714 [details] [diff] [review])
also included an addition to a test script to test this capability.  
I did not carry that part of the patch forward, so my new patch,
above, is incomplete.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 317126 [details] [diff] [review]
Alexei's previous patch, brought forward to tip (checked in on trunk)

seeking additional reviewers.

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 317126 [details] [diff] [review]
Alexei's previous patch, brought forward to tip (checked in on trunk)

looking for timely review.

Comment 23

[Robert Relyea]

Comment on attachment 317126 [details] [diff] [review]
Alexei's previous patch, brought forward to tip (checked in on trunk)

r+ hopefully timely enough.

bob

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 317126 [details] [diff] [review]
Alexei's previous patch, brought forward to tip (checked in on trunk)

Checking in certutil.c; new revision: 1.137; previous revision: 1.136

Comment 25

[Wan-Teh Chang]

Comment on attachment 317126 [details] [diff] [review]
Alexei's previous patch, brought forward to tip (checked in on trunk)

r=wtc.

Making selfsignprivkey an in/out parameter of CreateCert
makes the code harder to understand.  For example, the
usual convention that output arguments are not modified
when a function fails is not maintained for this function.

The only reason selfsignprivkey needs to be output to the
caller (certutil_main) is to rely on the caller to destroy
selfsignprivkey.  The caller has no other use for selfsignprivkey.
So CreateCert could just create (and destroy) a local private
key if the passed-in private key is NULL.  The next patch
does that.

Comment 26

[Wan-Teh Chang]

Attachment: Avoid in/out parameter (at the cost of duplicating cleanup code)

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

I like the idea that there is always one place that has responsibility for
destroying the priv key.