Note: There are a few cases of duplicates in user autocompletion which are being worked on.

certutil needs option to generate SubjectKeyID extension



11 years ago
10 years ago


(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Julien Pierre)


Firefox Tracking Flags

(Not tracked)



(2 attachments, 1 obsolete attachment)

11.59 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review-
Details | Diff | Splinter Review
11.40 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
Certutil has an option to generate an AuthorityKeyID extension in certs
and in CSRs, but no similar option to generate a SubjectKeyID extension.


10 years ago
Target Milestone: --- → 3.11.8


10 years ago
Priority: -- → P2


10 years ago
Assignee: neil.williams → nobody
Target Milestone: 3.11.8 → ---

Comment 1

10 years ago
We'll need support for this extension if we want to use certutil to produce a useful set of certs to test PKIX. It makes little sense to be able to generate AKID but not SKID. I'm setting the target to 3.12 .
Target Milestone: --- → 3.12


10 years ago
Assignee: nobody → julien.pierre.boogz


10 years ago

Comment 2

10 years ago
Created attachment 303182 [details] [diff] [review]
Implement support for encoding SubjectKeyID

This patch does a few things :

1) It modifies the prototype for CERT_EncodeSubjectKeyID to match with what SECU_EncodeAndAddExtensionValue expects. This is OK because that function was dead code.

2) It eliminates the double declaration for CERT_EncodeSubjectKeyID in cert.h

3) It exports CERT_EncodeSubjectKeyID from nss.def

4) It adds the proper glue in certutil.c, certutil.h and certext.h to support the --extSKID argument

I have tested this successfully and generated a self-signed cert with both AKID and SKID . Here is the relevant portion from certutil -L :

        Signed Extensions:
            Name: Certificate Subject Key ID

            Name: Certificate Authority Key Identifier
            Key ID:
Attachment #303182 - Flags: review?(nelson)

Comment 3

10 years ago
Comment on attachment 303182 [details] [diff] [review]
Implement support for encoding SubjectKeyID

This is a good first draft, but there are a number of small issues.

1. Let's not ask the user if he wants to do something twice.
When Neil recently added a bunch of new extension generation options,
we worked to minimize the number of questions the user must answer to 
do the job.  Let's continue that minimization here.  
In the new function AddSubjKeyID, please just remove these new lines:

>+        if (GetYesNo ("Enter value for the subjKeyID extension [y/N]?") == 0)
>+            break;

If the user doesn't want to enter a subject key ID, he can just enter an
empty string for the key ID, and that will avoid it.

Yes, I know that will make the behavior of AddSubjKeyID different from 
the existing behavior of AddAuthKeyID, from which this code was copied.
But I still want to minimize the number of user interactions.  If possible
I'd like to go back and remove that unnecessary question from AddAuthKeyID

2. (optional) AddSubjKeyID takes the user's input as a literal string, and 
encodes it into the SKID exactly as the user enters it.  Do we want to offer 
a way for the user to enter a hex string, for entering a binary SKID?  

3. The change to the usage message makes it output a line that is much 
longer than 80 characters.

>-        "\t\t [--extAIA] [--extSIA] [--extCP] [--extPM] [--extPC] [--extIA]\n",
>-	progName);
>+        "\t\t [--extAIA] [--extSIA] [--extCP] [--extPM] [--extPC] [--extIA]"
>+        " [--extSKID]\n", progName);

That first line is already 78 characters long in the display output, 
so don't add more to it.  Just add a new line.

4. a few issues with the code below.

> SECStatus 
>-CERT_EncodeSubjectKeyID(PRArenaPool *arena, char *value, int len, SECItem *encodedValue)
>+CERT_EncodeSubjectKeyID(PRArenaPool *arena, SECItem* srcString,
>+                        SECItem *encodedValue)
> {
>     SECItem encodeContext;
>     SECStatus rv = SECSuccess;
>@@ -107,9 +108,11 @@ CERT_EncodeSubjectKeyID(PRArenaPool *are
>     PORT_Memset (&encodeContext, 0, sizeof (encodeContext));
>-    if (value != NULL) {
>- = (unsigned char *)value;
>-	encodeContext.len = len;
>+    if (srcString != NULL) {
>+ = (unsigned char *)srcString->data;
>+	encodeContext.len = srcString->len;
>+    } else {
>+        return SECFailure;
>     }
>     if (SEC_ASN1EncodeItem (arena, encodedValue, &encodeContext,
> 			    CERTSubjectKeyIDTemplate) == NULL) {

4a) There's no need to cast strString->data before assigning it to, since both are of the same type.
4b) You could just copy the whole srcString secitem into encodeContext,
   instead of copying some individual components.  e.g. 
       encodeContext = *srcString;
4c) You could just eliminate encodeContext, and pass strString instead 
   of &encodeContext to SEC_ASN1EncodeItem.  
4d) if you decide not to eliminate encodeContext, and to continue to 
   copy srcString to encodeContext, then change the declaration of 
   srcString to be const SECItem *.
Attachment #303182 - Flags: review?(nelson) → review-

Comment 4

10 years ago
Comment on attachment 303182 [details] [diff] [review]
Implement support for encoding SubjectKeyID

One additional comment about this patch.  
An error code must be set before returning SECFailure.

>+    } else {
>+        return SECFailure;
>     }

Comment 5

10 years ago
Created attachment 303429 [details] [diff] [review]


This patch takes into account all your feedback.

There is also a new change in certcgi.c , since I hadn't noticed that it was previously calling CERT_EncodeSubjectKeyID .

I added a new function to do hex conversion in place - SECU_HexToSECItem. We only had the reverse (from binary to hex string). The program now excepts either ASCII, or hex if the string starts with 0x and only contains an even number of hex characters. I did this for both SKID and AKID.

I removed the prompt for adding the SKID and AKID as they were unnecessary. They were causing the whole extension to be skipped, not the value. But if that's what was desired, the user could simply not specify -3 or --extSKID on the command-line.
Attachment #303182 - Attachment is obsolete: true
Attachment #303429 - Flags: review?(nelson)

Comment 6

10 years ago
Comment on attachment 303429 [details] [diff] [review]

Three problems with this patch.
1. The removal of the GetYesNo call from function AddAuthKeyID will break
test scripts, and perhaps break products that invoke certutil to make certs.

2. The new function SECU_HexToSECItem needs to set error codes before 
returning SECFailure, and 

3. Please rename SECU_HexToSECItem to better describe what it does.
Attachment #303429 - Flags: review?(nelson) → review-

Comment 7

10 years ago
Created attachment 303637 [details] [diff] [review]
Update with Nelson's feedback
Attachment #303637 - Flags: review?(nelson)

Comment 8

10 years ago
Comment on attachment 303637 [details] [diff] [review]
Update with Nelson's feedback

Attachment #303637 - Flags: review?(nelson) → review+

Comment 9

10 years ago
Thanks, Nelson.
I checked this in to the trunk.

Checking in cmd/certcgi/certcgi.c;
/cvsroot/mozilla/security/nss/cmd/certcgi/certcgi.c,v  <--  certcgi.c
new revision: 1.19; previous revision: 1.18
Checking in cmd/certutil/certext.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certext.c,v  <--  certext.c
new revision: 1.6; previous revision: 1.5
Checking in cmd/certutil/certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.134; previous revision: 1.133
Checking in cmd/certutil/certutil.h;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.h,v  <--  certutil.h
new revision: 1.2; previous revision: 1.1
Checking in cmd/lib/secutil.c;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v  <--  secutil.c
new revision: 1.85; previous revision: 1.84
Checking in cmd/lib/secutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.h,v  <--  secutil.h
new revision: 1.27; previous revision: 1.26
Checking in lib/certdb/cert.h;
/cvsroot/mozilla/security/nss/lib/certdb/cert.h,v  <--  cert.h
new revision: 1.64; previous revision: 1.63
Checking in lib/certdb/xconst.c;
/cvsroot/mozilla/security/nss/lib/certdb/xconst.c,v  <--  xconst.c
new revision: 1.12; previous revision: 1.11
Checking in lib/certdb/xconst.h;
/cvsroot/mozilla/security/nss/lib/certdb/xconst.h,v  <--  xconst.h
new revision: 1.5; previous revision: 1.4
Checking in lib/nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.186; previous revision: 1.185
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.