369144 - certutil needs option to generate SubjectKeyID extension

Status: RESOLVED FIXED

(NSS :: Tools, enhancement, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

Certutil has an option to generate an AuthorityKeyID extension in certs
and in CSRs, but no similar option to generate a SubjectKeyID extension.

Comment 1

[Julien Pierre]

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 .

Comment 2

[Julien Pierre]

Attachment: 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
            Data:
                31:32:33
                123

            Name: Certificate Authority Key Identifier
            Key ID:
                31:32:33
                123

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

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

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) {
>-	encodeContext.data = (unsigned char *)value;
>-	encodeContext.len = len;
>+    if (srcString != NULL) {
>+	encodeContext.data = (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 
   encodeContext.data, 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 *.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

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

[Julien Pierre]

Attachment: Update

Nelson,

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.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 303429 [details] [diff] [review]
Update

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.

Comment 7

[Julien Pierre]

Attachment: Update with Nelson's feedback

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

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

r=nelson

Comment 9

[Julien Pierre]

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