Closed
Bug 67890
Opened 24 years ago
Closed 16 years ago
create self-signed cert with existing key that signed CSR
Categories
(NSS :: Tools, enhancement, P3)
Tracking
(Not tracked)
RESOLVED
FIXED
3.12.1
People
(Reporter: bugz, Assigned: alvolkov.bgs)
Details
Attachments
(2 files, 3 obsolete files)
3.99 KB,
patch
|
rrelyea
:
review+
wtc
:
review+
julien.pierre
:
review+
|
Details | Diff | Splinter Review |
2.29 KB,
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•24 years ago
|
Priority: -- → P3
Target Milestone: --- → Future
Comment 1•23 years ago
|
||
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•22 years ago
|
||
Changed the QA contact to Bishakha.
QA Contact: sonja.mirtitsch → bishakhabanerjee
Comment 3•22 years ago
|
||
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.
Reporter | ||
Comment 4•22 years ago
|
||
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.
Reporter | ||
Comment 5•22 years ago
|
||
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.
Updated•19 years ago
|
QA Contact: bishakhabanerjee → jason.m.reid
Comment 6•18 years ago
|
||
Alexei, Please evaluate this bug, and the patch attached to it. It's only P3.
Assignee: bugz → alexei.volkov.bugs
Target Milestone: Future → ---
Comment 7•18 years ago
|
||
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.
Attachment #96177 -
Flags: review?(alexei.volkov.bugs)
Updated•18 years ago
|
QA Contact: jason.m.reid → tools
Assignee | ||
Comment 8•18 years ago
|
||
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
Attachment #96177 -
Attachment is obsolete: true
Attachment #218482 -
Flags: review?(nelson)
Attachment #96177 -
Flags: review?(alexei.volkov.bugs)
Comment 9•18 years ago
|
||
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•18 years ago
|
||
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.
Attachment #218482 -
Flags: review?(rrelyea)
Comment 11•18 years ago
|
||
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
Attachment #218482 -
Flags: review?(rrelyea) → review-
Comment 12•18 years ago
|
||
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•18 years ago
|
||
Comment on attachment 218482 [details] [diff] [review] integrate fix for -C -x and bug fix Please note my review remarks in comment 10 above
Attachment #218482 -
Flags: review?(nelson) → review-
Assignee | ||
Updated•18 years ago
|
Target Milestone: --- → 3.12
Assignee | ||
Comment 14•17 years ago
|
||
this patch also has the following fixes: * reusing db located in "client" directory for the test * rename function to cert_certu_misc
Attachment #218482 -
Attachment is obsolete: true
Attachment #256714 -
Flags: review?(nelson)
Comment 15•17 years ago
|
||
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.
Attachment #256714 -
Flags: review?(nelson) → review+
Comment 16•16 years ago
|
||
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.
Summary: self-signing cert requests with certutil → create self-signed cert with existing key that signed CSR
Target Milestone: 3.12 → 3.12.1
Assignee | ||
Comment 17•16 years ago
|
||
Patch to 341371 made certutil to be able to generate CR with exiting key. Closing this bug as dup
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → DUPLICATE
Comment 18•16 years ago
|
||
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.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Comment 19•16 years ago
|
||
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.
Attachment #256714 -
Attachment is obsolete: true
Attachment #317126 -
Flags: review?(alexei.volkov.bugs)
Comment 20•16 years ago
|
||
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•16 years ago
|
||
Comment on attachment 317126 [details] [diff] [review] Alexei's previous patch, brought forward to tip (checked in on trunk) seeking additional reviewers.
Attachment #317126 -
Flags: review?(wtc)
Comment 22•16 years ago
|
||
Comment on attachment 317126 [details] [diff] [review] Alexei's previous patch, brought forward to tip (checked in on trunk) looking for timely review.
Attachment #317126 -
Flags: review?(rrelyea)
Attachment #317126 -
Flags: review?(julien.pierre.boogz)
Attachment #317126 -
Flags: review?(alexei.volkov.bugs)
Updated•16 years ago
|
Attachment #317126 -
Flags: review?(julien.pierre.boogz) → review+
Comment 23•16 years ago
|
||
Comment on attachment 317126 [details] [diff] [review] Alexei's previous patch, brought forward to tip (checked in on trunk) r+ hopefully timely enough. bob
Attachment #317126 -
Flags: review?(rrelyea) → review+
Comment 24•16 years ago
|
||
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
Attachment #317126 -
Attachment description: Alexei's previous patch, brought forward to tip → Alexei's previous patch, brought forward to tip (checked in on trunk)
Comment 25•16 years ago
|
||
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.
Attachment #317126 -
Flags: review?(wtc) → review+
Comment 26•16 years ago
|
||
Comment 27•16 years ago
|
||
I like the idea that there is always one place that has responsibility for destroying the priv key.
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•