324744 - add generation of policy extensions to certutil

Status: RESOLVED FIXED

(NSS :: Tools, enhancement, P1)

Description

[Julien Pierre]

certutil currently does not know how to add Policy, Policy Constraints, or Inhibit-any-policy extensions when generating certificates .

For the purpose of full coverage testing of libpkix, we should add this ability
to certutil .

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

This will become P1 for 3.12

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

P1, Must have for 3.12

Comment 3

[Alexei Volkov]

Attachment: adding missing extentions generation functionality to certutil

Generation of the following extensions are added: AIA, SIA, Cert Polices, Policy Mappings, Policy Constraint, Inhibit Any Policy.
The patch also fixes bug in "User Notice" decoding.

However, this patch is not for review yet, since it missing an implementation of a parser, that would transform an OID defined by a string to an encoded OID and also this patch depends on 346354 which it not yet fixed.

Comment 4

[Steve Parkinson]

There is a function in libpkix to parse the OID:

pkix_pl_system.h:

PKIX_Error * PKIX_PL_OID_Create( char *stringRep, PKIX_PL_OID **pOID, void *plContext);

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

PKIX_PL_OID_Create creates a "PKIX_PL_OID" which is a structure pointing to
an array of ints, each one representing one of the components of an OID.
So, the output is not in the format needed (Not a DER OID).  Also, it 
isn't capable of allocating from an arenapool.  

There is a string-to-DER-OID converter attached to bug 161326, awaiting 
review (IIRC).  It takes as input the syntax allowed for OIDs in RFC 1443 
and its successors, in which OIDs may be prefixed by "OID." 

Comment 6

[Neil Williams]

Part of bug 324740 is included in the patch for this bug. When this is closed that one should be too.

Comment 7

[Neil Williams]

Attachment: Alexei's patch with some cleanup

This make some minor changes to Alexei's previous patch, including changing the code to call SEC_StringToOID() for policy OIDs. This does not add all the code necessary for certutil to generate the new extensions. That will be the subject of the next patch. (This one is already pretty big.)

Comment 8

[Neil Williams]

Attachment: same as above but erroneous chunks removed

Glancing through the previous patch I noticed it contained some chunks that do not pertain to cert extensions. They are omitted here.

Comment 9

[Neil Williams]

Attachment: second patch to certutil, secutil to generate new extensions

This patch needs to be applied after the previous one. It may be hard to review until that one is comitted. Please let me know if you decide to postpone your review until then.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 275204 [details] [diff] [review]
second patch to certutil, secutil to generate new extensions

This is a partial review.  I have not completed the review of this 
patch, but want to write now about several changes that are needed.

1) This patch modifies nss/lib/ssl/derive.c, adding a #include to a 
file from nss/cmd/lib.  Files in nss/lib must not include files 
from nss/cmd.  

2) The large number of PRBool arguments to functions AddExtensions
and CertReq need to be eliminated, and converted into a single 
argument, which is a pointer to an array of PRBools.  
There should be a enumerated type that defines symbolic names for
the indexes into that array, similar to the opt_* and cmd_* 
enumerated names for commands and options.  

More review comments will follow.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 275204 [details] [diff] [review]
second patch to certutil, secutil to generate new extensions

Here's the review for the patch to SECU_ParseCommandLine.

I like that this patch doesn't add another new array of longopts to 
the input args for SECU_ParseCommandLine.   And, I like the fact that 
it avoids the problem with uninitialized string pointers, which it 
does by using a previously unused value of "flag" to signify the
presence of a long option string.  

But because long option names require a zero flag value, it doesn't 
allow long option names to be synonyms for short option names, and it 
uses an additional array, called revlookup, because the flag value
cannot be used to correlate the long option with the command/option
entry.  

I think it would be best to allow long option names with non-zero 
flag values, and to expand "flag" to be wider than char.  This will
permit long options to be synonyms of one-character options, will 
remove the low limit on the number of unique flag values, and will 
obviate the revlookup array (which is always leaked in this patch).
It will necessitate another solution to the uninitialized string 
pointer problem, but that seems feasible.  

Also, this patch uses names from an old version of the patch for bug 
346354, not from the patch that was ultimately reviewed and committed.  
So, it needs to be updated to use the new symbol names.

>     optstring = (char *)PORT_Alloc(cmd->numCommands + 2*cmd->numOptions);
>+    if (optstring == NULL) {
>+        if (optstring)            <- cannot be true
>+	    PORT_Free(optstring);   <- so this is dead.
>+        return SECFailure;
>+    }	

Has anything else been allocated that must be freed in this path?

>+    if (lcmd + lopt > 0) {
>+	longopts = PORT_NewArray(PLLongOpt, lcmd+lopt+1);
>+	revlookup = PORT_NewArray(secuCommandFlag *, lcmd+lopt);
>+	if (!longopts || !revlookup) {
>+	    PORT_Free(optstring);
>+	    if (longopts)
>+		PORT_Free(longopts);
>+	    return SECFailure;
>+	}

Suppose that, of longopts aned revlookup, one allocation succeeded
and one failed.  The above code would leak one or the other.
In fact, I see no code that ever frees revlookup.

Comment 12

[Neil Williams]

(In reply to comment #10)
> (From update of attachment 275204 [details] [diff] [review])
> This is a partial review.  I have not completed the review of this 
> patch, but want to write now about several changes that are needed.
> 
> 1) This patch modifies nss/lib/ssl/derive.c, adding a #include to a 
> file from nss/cmd/lib.  Files in nss/lib must not include files 
> from nss/cmd.  

When I diff this patch it *removes* that #include.

> 
> 2) The large number of PRBool arguments to functions AddExtensions
> and CertReq need to be eliminated, and converted into a single 
> argument, which is a pointer to an array of PRBools.  
> There should be a enumerated type that defines symbolic names for
> the indexes into that array, similar to the opt_* and cmd_* 
> enumerated names for commands and options.  
> 
> More review comments will follow.

I agree. I'll change it.

(In reply to comment 11)
I caught the revlookup free problem after submitting the patch.

Nelson, Bug 390973 also modifies SECU_ParseCommandLine. I propose making the changes to that function in 390973 and leave this about adding new options to certutil. Do you want me to do both?



 

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Neil,  about the patch to ssl/derive.c: you're right.  
I guess my mind wouldn't accept that that #include was already in the tree!
Let's discuss whether you should take bug 390973 or not by phone.

Comment 14

[Julien Pierre]

Comment on attachment 274226 [details] [diff] [review]
same as above but erroneous chunks removed

Neil,

I'm sorry my review took this long. There are a few issues with this patch.

1) Nit: you should replace policyConstr = PORT_ArenaZAlloc(arena, sizeof(CERTCertificatePolicyConstraints));

with

policyConstr = PORT_ArenaZNew(arena, CERTCertificatePolicyConstraints);

There are a total of 12 occurrences of PORT_ArenaZAlloc that should be replaced in your patch. Those that use the variable name for the size argument also need to be replaced with PORT_ArenaZNew and take the type argument instead of size.

2) In AddPolicyMappings, there are two consecutive assignments of current , current = NULL and current = PORT_ArenaZAlloc ... The first one can be deleted.
Same issue in RequestPolicyQualifiers and AddCertPolicies.

3) Nit: in AddPolicyMappings, I think it would be better to prompt for the number of mappings upfront . The use of PORT_ArenaGrow is inefficient, especially since you are allocating other things in between calls.
You could then use PORT_ArenaZNewArray for the allocation. Same issue in RequestPolicyQualifiers for the number of policy qualifiers and the number of user notices. Same issue in AddCertPolicies for the number of policies.
Same issue in AddInfoAccess for the number of CERTInfoAccess .
I know this is a lot of changes, and you don't have to do it, but it would reduce and simplify the code a lot and make it more readable if you don't need to deal with array growth or allocating and setting the final NULL pointer marker (it would be set in the first n+1 zero-allocation).

4) In AddPolicyMappings, your indentation is off for the final PORT_FreeArena call . Same issue in RequestPolicyQualifiers and AddCertPolicies

5) In RequestPolicyQualifiers, you need to check the return value from SEC_ASN1EncodeItem

6) In RequestPolicyQualifiers, many allocations are not checked for failure :

                input.data = (void*)PORT_ArenaStrdup(arena, buffer);

                notice->noticeReference.organization.data =
                    (void*)PORT_ArenaStrdup(arena, buffer);

                noticeNumArr = PORT_ArenaZAlloc(arena, sizeof(SECItem*));
                
                noticeNum = PORT_ArenaZAlloc(arena, sizeof(SECItem));

                    noticeNumArr = PORT_ArenaGrow (arena, noticeNumArr,
                                                   sizeof (noticeNum) * inCount,
                                                   sizeof (noticeNum) *(inCount + 1));
                notice->displayText.data = (void*)PORT_ArenaStrdup(arena, buffer);
                
                SECITEM_CopyItem(arena, &current->qualifierID, &oid->oid);

7) In AddCertPolicies :

If you aren't going to make the changes for 3), you should change the following :

        if (certPoliciesArr == NULL) {
            certPoliciesArr = PORT_ArenaZAlloc (arena, sizeof (*certPoliciesArr));
            if (certPoliciesArr == NULL) {
                GEN_BREAK (SECFailure);
            }
        }

        certPoliciesArr = PORT_ArenaGrow (arena, certPoliciesArr,
                                         sizeof (current) * count,
                                         sizeof (current) *(count + 1));

        if (certPoliciesArr == NULL) {
            GEN_BREAK (SECFailure);
        }

The PORT_ArenaGrow should be done as an else case to the first if statement, ie. grow only if certPoliciesArr is non-NULL .
And you only need one check for certPoliciesArr == NULL, after the end of both clauses of the if statement .

8) In AddInfoAccess, the following allocations need to be checked :

SECITEM_CopyItem(arena, &current->method, &oid->oid);

9) In AddInfoAccess, same issue as 7) for infoAccArr

10) In AddInfoAccess, some comments would be welcome to indicate that the function handles both AIA and SIA in particular

11) in certutil.c , in the forward declaration for AddExtensions, it would be nice to have the name of each argument and not just their type, given the large number of args, unless this is going to be changed
Using an array would be OK, but a bit flag would be even better I think.

Comment 15

[Kai Engert (:KaiE:)]

Do you have a patch that applies to the tip of NSS and compiles? I tried to use the patches attached to this bug, I tried patches attached to the related/dependent bugs, I tried to merge the patches to the current trunk, but I always ended with compiler errors.

Being able to generate certs with policy extensions might help me around EV testing.

Comment 16

[Neil Williams]

(In reply to comment #15)
> Do you have a patch that applies to the tip of NSS and compiles? I tried to use
> the patches attached to this bug, I tried patches attached to the
> related/dependent bugs, I tried to merge the patches to the current trunk, but
> I always ended with compiler errors.

What's your build platform? It was compiling for me when I created the patch but there has been quite a bit of patching to certutil recently.

> 
> Being able to generate certs with policy extensions might help me around EV
> testing.

Definitely. This bug and a couple of related ones are high priority for me right now.

Comment 17

[Kai Engert (:KaiE:)]

(In reply to comment #16)
> What's your build platform? It was compiling for me when I created the patch
> but there has been quite a bit of patching to certutil recently.

I'm on Linux. It didn't look like a platform specific thing, because it complained about missing symbols.

Comment 18

[Neil Williams]

Attachment: addresses Julien's review comments

Julien, thanks for the detailed review especially the comments about the memory allocation. Regarding your comments: I made all the changes you suggested in #s 1,2,4-10, I didn't think it important to do #3. Your #11 and Nelson's comment #10 and I are all in agreement. This patch makes the extension list an array. I also added a certutil.h to contain the handfull of declarations including the new ones.

You'll notice that even with this patch you can't yet request the new extensions from the command line. That's because attachment #275204 [details] [diff] [review] (or it's successor) needs to be applied first.

Comment 19

[Neil Williams]

Attachment: removes extraneous changes from previous patch

Interdiff revealed some changes that were not supposed to be in the previous patch. This should fix it and make it somewhat easier to review the interdiffs.

Comment 20

[Neil Williams]

Attachment: last patch still wrong

Try again.

Comment 21

[Julien Pierre]

Comment on attachment 278513 [details] [diff] [review]
last patch still wrong

Neil,

This is almost right.

From comment 14:

- task 4) is incomplete - PORT_FreeArena indentation in AddCertPolicies is still wrong.

- tasks 6) and 8) are incomplete. SECITEM_CopyItem in RequestPolicyQualifiers and AddInfoAccess are not checked

- I found an issue I previously did not notice. SEC_ASN1EncodeInteger is not checked in RequestPolicyQualifiers

Comment 22

[Julien Pierre]

I misread the interdiff. Tasks 6 and 8 about SECITEM_CopyItem are complete.  The others still need to be fixed.

Comment 23

[Neil Williams]

Attachment: last review comments incorporated

Julien, thanks for your review. This should fix the last two items. And interdiff should show only two small chunks changed.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 279183 [details] [diff] [review]
last review comments incorporated

I'm performing an uninvited SR (:-) and I have some SR questions
and one comment (something needs to be fixed).

Q's: CERT_DecodeUserNotice has some BIG changes.  It appears to 
me that the DER syntax it accepts has changed.  Do we have any
existing certs, made by real world CAs, that have this extension?
Do we know that they successfully decoded before this patch?
Do we know that they successfully decode with this patch?

comment:  This patch adds new symbols to nss.def, but they're
not in alphabetical order.  Please fix.

> CERT_CheckNameSpace;
>+CERT_EncodeInfoAccessExtension;
>+CERT_EncodeCertPoliciesExtension;
>+CERT_EncodeInhibitAnyExtension;
>+CERT_EncodeNoticeReference;
>+CERT_EncodePolicyMappingExtension;
>+CERT_EncodePolicyConstraintsExtension;
>+CERT_EncodeUserNotice;
> CERT_FindCRLEntryReasonExten;

Comment 25

[Neil Williams]

(In reply to comment #24)
> (From update of attachment 279183 [details] [diff] [review])
> I'm performing an uninvited SR (:-) and I have some SR questions
> and one comment (something needs to be fixed).
> 
> Q's: CERT_DecodeUserNotice has some BIG changes.  It appears to 
> me that the DER syntax it accepts has changed.  Do we have any
> existing certs, made by real world CAs, that have this extension?
> Do we know that they successfully decoded before this patch?
> Do we know that they successfully decode with this patch?

Alexei, can you comment?
> 
> comment:  This patch adds new symbols to nss.def, but they're
> not in alphabetical order.  Please fix...
> 

Will do. I'll wait for a round of comments before committing.

Comment 26

[Alexei Volkov]

> Q's: CERT_DecodeUserNotice has some BIG changes.  It appears to 
> me that the DER syntax it accepts has changed.  Do we have any
> existing certs, made by real world CAs, that have this extension?
> Do we know that they successfully decoded before this patch?
> Do we know that they successfully decode with this patch?
> 
The code was written before quick der decoder supported OPTIONAL & INLINE.
Also, there are couple of problems with the old code/template.
I did minimal testing on the new code by creating a cert that could be decoded
by openssl and nss. No real world certs have been used for testing.

Comment 27

[Julien Pierre]

Actually QuickDER always supported optional inline. But the legacy decoder did not. When I wrote QuickDER back in 2002, I replaced SEC_ASN1DecodeItem with SEC_QuickDERDecodeItem wherever possible. This was one of those cases I changed. But I did not go the extra step and notice the mess that this code was.

Comment 28

[Neil Williams]

Checking in attachment 279183 [details] [diff] [review]. One more patch after this to close this bug.

Checking in cmd/certutil/certext.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certext.c,v  <--  certext.c
new revision: 1.2; previous revision: 1.1
done
Checking in cmd/certutil/certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.119; previous revision: 1.118
done
Checking in lib/certdb/cert.h;
/cvsroot/mozilla/security/nss/lib/certdb/cert.h,v  <--  cert.h
new revision: 1.60; previous revision: 1.59
done
Checking in lib/certdb/polcyxtn.c;
/cvsroot/mozilla/security/nss/lib/certdb/polcyxtn.c,v  <--  polcyxtn.c
new revision: 1.8; previous revision: 1.7
done
Checking in lib/certdb/xconst.c;
/cvsroot/mozilla/security/nss/lib/certdb/xconst.c,v  <--  xconst.c
new revision: 1.10; previous revision: 1.9
done
Checking in lib/nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.177; previous revision: 1.176
done
Checking in lib/ssl/derive.c;
/cvsroot/mozilla/security/nss/lib/ssl/derive.c,v  <--  derive.c
new revision: 1.6; previous revision: 1.5
done
Checking in lib/util/seccomon.h;
/cvsroot/mozilla/security/nss/lib/util/seccomon.h,v  <--  seccomon.h
new revision: 1.6; previous revision: 1.5
done
Checking in lib/util/secoid.c;
/cvsroot/mozilla/security/nss/lib/util/secoid.c,v  <--  secoid.c
new revision: 1.39; previous revision: 1.38
done
Checking in lib/util/secoidt.h;
/cvsroot/mozilla/security/nss/lib/util/secoidt.h,v  <--  secoidt.h
new revision: 1.25; previous revision: 1.24
done

Comment 29

[Neil Williams]

This new file didn't go in with the rest of the changes.

RCS file: /cvsroot/mozilla/security/nss/cmd/certutil/certutil.h,v
done
Checking in cmd/certutil/certutil.h;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.h,v  <--  certutil.h
initial revision: 1.1
done

Comment 30

[Neil Williams]

Warnings from Studio were errors on gcc. This corrects.

/cvsroot/mozilla/security/nss/lib/ssl/derive.c,v  <--  derive.c
new revision: 1.7; previous revision: 1.6

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

Fixed Windows build breakage from previous checkin for this bug.
Checking in certext.c; new revision: 1.3; previous revision: 1.2

Looking at AIX breakage next

Comment 32

[Nelson Bolyard (seldom reads bugmail)]

Fixed build bustage on AIX. Can't have trailing commas in enums on AIX.
Checking in certext.c; new revision: 1.4; previous revision: 1.3

Comment 33

[Neil Williams]

Attachment: description of the interactive option selection for new extensions

The option data for the new extensions added for this bug is gathered interactively. Some of the options are fairly complicated. This attachment is a cursory description of the dialog for each extension. The syntax for these is descriptions is meant to be simple: Each query is enclosed in double quotes; braces ({}) enclose a group of questions that may be repeated--usually the last quesry in the group conditionally terminates it; angle brackets (<>) enclose indicate a subroutine call; if the answer to a query conditionally results in dependant queries, those are indented; the CertPolicies extension has an or-case for policy qualifier indicated by ||.

Comment 34

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 280400 [details]
description of the interactive option selection for new extensions

Neil, thanks for attaching this attachment.  
Clearly the UI being added was not reviewed from a UI perspective. 
Here are some comments on the interactive UI

The propmpts must tell the user what kind of thing to type in.
certutil users will surely not know.  We must lead them by the hand.

> 	"Enter explicit policy value or
>          Enter to omit" ?

What is a policy value?  An OID? 

> 	"Enter inhibit mapping value or
>          Enter to omit" ?

What is an inhibit mapping value?  An OID?  A boolean?  

> "Enter more set to Policy Mappings extension [y/N]" ?

What does that mean: "Enter more set"?  
Maybe it should be "Do you want to enter another policy mapping?"

> "Enter user notice reference?"

What kind of answer does that want?  Y/N?  What is default?  Other?

> "Enter user organization:"

What kind of input is wanted here?  A string? (how long?)  an OID?  

> "Enter notice number of any key to continue:

??  What is the syntax of a notice number?  A decimal number?  an OID? 
What is the range?  

What is a "number of any key"?  
Did it mean "OR any key"?
If "any key" continues, then what is the alternative?  

> Enter more location to the Authority Information Access extension [y/N]

What does that mean?  Does it mean "Do you want to enter another AIA?"


>GetGeneralName:
>	{	"Select one of the following general name type:
>                1 - instance of other name
>		 2 - rfc822Name:
>		 3 - dnsName
>                4 - x400Address
>		 5 - directoryName
>		 6 - ediPartyName
>                7 - uniformResourceidentifier
>		 8 - ipAddress
>		 9 - registerID
>                Any other number to finish
>		 Choice:" ?
>
>		"Enter data:" ?
>	}

As reported in bug 292285, we cannot encode names of these three types:
   OtherName,
   X400Address,
   EDIPartyName,
in part because these things are ASN.1 SEQUENECES, and not merely string
types.  

So, we must not offer to encode those types.  Those choices must come out.

This sort of UI review should have been done before the patch was reviewed
and/or committed.  This bug cannot be resolved fixed until these issues are 
addressed.

Comment 35

[Neil Williams]

Attachment: add options to certutil for generating new cert extensions

This is a proposed set of command line options for generating the extensions added in attachment 279183 [details] [diff] [review]. This should be the last patch  for this bug.

Comment 36

[Neil Williams]

Comment on attachment 280412 [details] [diff] [review]
add options to certutil for generating new cert extensions

There was an omission in this patch. I will make that correction and address comment #34 in the next patch.

Comment 37

[Neil Williams]

Attachment: add options and try to get the dialogue right

Nelson, please check the diff to see if the interactive messages are more acceptable. I haven't tested this enough yet to ask for a formal review.

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 280422 [details] [diff] [review]
add options and try to get the dialogue right

r+ for the part of this patch for certutil.c, which adds the new command line options.  
The part in certext.c needs more review, which I am still doing.

Comment 39

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 280422 [details] [diff] [review]
add options and try to get the dialogue right

This patch improves the text for the prompts, to be sure, but more 
improvements are possible and desired.

I re-read RFC 3280 to see what some of these mysterious values are.

> "Enter skip cert count for require explicit policy or null to omit",

How does one enter NULL?

In this context, the "skip count" is the number of certs BELOW 
(subordinate to) this one that may exist before all certs in the entire 
cert path must have a policy extension.  This extension says, in effect, 
"if the cert chain is longer than this many certs below this one, then 
ALL the certs in the chain MUST have an acceptable policy identifier in 
the certificate policies extension."

Somehow the words "skip count" don't convey that very well.  I might call
it "maximum subordinate path length before policies extensions are required
for the WHOLE chain".  But that's kinda long.  :-/

Negative skip counts must not be permitted.

> "Enter skip cert count for Inihibit Policy Mapping or null to omit"

Again, how does one enter NULL?

In this context, the "skip count" is the number of certs BELOW 
(subordinate to) this one that may have policy mapping extensions. 
If the cert path is longer than the "skip count" in the downward direction,
then the certs beyong this number CANNOT have policy mapping extensions 
in them.  It says, in effect, "prohibit policy mapping extensions in certs
more than this number down in the chain", or "allow policy mapping 
extensions in subordiante certs down to this depth."  Our prompt should 
convey that, as best it can.  Negative skip counts must not be permitted.

Notice that if the user enables the policy constraints extension, he MUST
provide either the RequireExplicitPolicy skipcount or the 
InhibitPolicyMapping skipcount, or both.  He does not have the option of 
omitting them both.  The code should ensure that it does not let him 
omit both.

> "Enter the number of certs in the path permitted to use anyPolicy."

In this context, skipcount is the number of certs below (subordinate to)
this one in which an "any-policy" policy OID is ALLOWED to match the 
caller's specified policy OID.  If the cert path is longer than this 
skipcount, then an "any-policy" policy OID that appears in any certs 
farther down the chain than this count will NOT match the caller's 
requested policy.  Negative skip counts must not be permitted. 

In all three of the above skipcount prompts, the message should convey
that the count affects the number of subordinate certs in the chain,
and that the count is an exception; that is, it is the number of certs
for which policy mapping is NOT inhibited, or in which matching with 
anyPolicy is NOT inhibited.  

> "Enter more Policy Mappings [y/N]"   
Should have a question mark in it.

After choosing a policy OID, or "any", the user MUST choose at least
ONE CPS Pointer qualifier or User notice qualifier. More than one is 
OK, but zero is not OK.  

URI: must be capitalized in "Enter CPS Pointer uri: "

Explicit Text must not exceed 200 characters (not bytes).

In all the places where the question asks "Enter more ...", instead of 
the question "Enter more", I suggest "Enter antoher"

I suggest you collapse the two prompts:
"Enter User Notice reference number: "  and
"Enter one more User Notice number[y/N]"
as follows:
The first prompt should be 
"Enter User Notice reference number: "
The second and subsequent prompts should be 
"Enter User Notice reference number, or negative number to quit".
This cuts the number of prompts for subsequent reference numbers in half.
There must be at least one reference number when that option is chosen.

Notice that the user may specify a notice reference, or an explicit
text, or both, but cannot omit both.  That is, if the user chooses
"2 - User Notice", then he must provide a non-zero number of user notices.

Comment 40

[Neil Williams]

Attachment: more tweeks to cert extension dialog

This patch should address all the latest dialog problems. For extension field names I have mostly used the formal names from RFC3280, i.e., "userNotice" instead of "user notice". I also fixed a bug in CertificatePolicies extension processing when the user wants AnyPolicy. The fix works fine but when the extension is listed by certutil it is identified by the policy OID string instead of "anyPolicy". Have to change lib/util/secoid.c to fix that. I'd like to commit this first though.

Comment 41

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 280679 [details] [diff] [review]
more tweeks to cert extension dialog

This is definitely getting closer.

>+        if (GetYesNo ("Enter another value for the CRLDistributionPoint "
>+                      "extension [y/N]") == 0) {

Please put a question mark at the end of all prompt questions, especially
the ones that are followed with [y/N].

>+    if (PrintChoicesAndGetAnswer("for requireExplicitPolicy enter "
>+               "the number of certs in path before explicit policy required\n"

That string is over 90 characters long.  Please wrap it in some way to less than
80 columns.

>+    if (PrintChoicesAndGetAnswer("for inihibitPolicyMapping enter"
>+               "the number of certs in path with no policy mapping allowed\n"
>+               "(press Enter to omit)", buffer, sizeof(buffer)) == SECFailure) {

Actually, for this value, the skipCerts number is the maximum cert path depth
for which policy mapping extensions ARE allowed.  They're inhibited beyond that 
max depth.  
    
>         if (PrintChoicesAndGetAnswer("Enter policy ID object identifier"
>-                                     " or any for AnyPolicy:",
>+                                     " or \"any\" for AnyPolicy:",
>                                      buffer, sizeof(buffer)) == SECFailure) {
>             GEN_BREAK (SECFailure);
>         }
>+	
>+	if (strncmp(buffer, "any", 3) == 0) {
>+	    /* use string version of X509_CERTIFICATE_POLICIES.anyPolicy */
>+	    strcpy(buffer, "OID.2.5.29.32.0");
>+	}
>         rv = SEC_StringToOID(arena, &current->policyID, buffer, 0);

Hmm.  That function expects OIDs to be entered in dotted decimal notation,
with optional preceeding "OID." (case insensitive).  I wonder if the prompt 
should say "Enter policy OID as dotted decimal" or something like that. 
Alternatively, should we handle the space-separated decimal OID format? 
(e.g. "2 5 29 32 0")

If a user enters an unacceptable value, do we want to force failure?
or do we want to prompt again for a better answer?

>-        "\t\t [-8 dns-names]\n",
>+        "\t\t [-8 DNS-names]\n"
>+        "\t\t [-extAIA] [-extSIA] [-extCP] [-extPM] [-extPC] [-extIA]\n",

These long opts require a double-dash, e.g. --extAIA and the usage should
show that.

>+    FPS "%-20s Create an Authority Information Access extension\n",
>+	"   -extAIA ");

Likewise here, we need to show the double-dashes.

Comment 42

[Neil Williams]

Attachment: more dialog polishing, fix usage message for new ext options

This patch seems to break the QA tests by rendering a created cert with CRLdp ext unreadable. I won't commit this until that's fixed.

Comment 43

[Neil Williams]

(In reply to comment #42)
> Created an attachment (id=280947) [details]
> more dialog polishing, fix usage message for new ext options
> 
> This patch seems to break the QA tests by rendering a created cert with CRLdp
> ext unreadable. I won't commit this until that's fixed.
> 

Turns out that if the temporary cert file that certutil -S uses exists in the current directory and is longer than the cert created by certutil the extra bytes are not truncated from the file. When the temp file is read back in the length is checked against the DER encoded length which fails.

All.sh passes if a new test directory is used.

Comment 44

[Neil Williams]

(In reply to comment #43)
> (In reply to comment #42)
> ...
> Turns out that if the temporary cert file that certutil -S uses exists in the
> current directory and is longer than the cert created by certutil the extra
> bytes are not truncated from the file. When the temp file is read back in the
> length is checked against the DER encoded length which fails.
> 
> All.sh passes if a new test directory is used.
> 

Created Bug 396484 to track this.

Comment 45

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 280947 [details] [diff] [review]
more dialog polishing, fix usage message for new ext options

r+=nelson, with some changes to certext.txt as shown below

>@@ -65,30 +65,26 @@ test.com
> y
> = 3 Name:_Certificate_Authority_Key_Identifier|Critical:_True|Issuer:|DNS_name:_"test.com"|Serial_Number:|214123
> # ################################################################
> ! TEST_7 CRL Distribution Points Extension
> 1
>-1
>-InstanceOfOtherName
>+2
>+InstanceOfDNSName
> 2
> rfc822Name

Since 2 is rfc822 name and not a DNS name, get rid of "InstanceOfDNSName"
and change rfc822name to [e-mail]

> 3
> test.com
>-4
>[e-mail]
>-6
>-ediPArtyName
> 8
> ipAddress

change that to a real IP address, e.g. 1.2.3.4

> 9
> 123451235

Change that to a real OID

> 10
> 0
> 10
> n
> n
>-= 4 Name:_CRL_Distribution_Points|InstanceOfOtherName|rfc822Name|test.com|[e-mail]|ediPArtyName
>+= 4 Name:_CRL_Distribution_Points|InstanceOfDNSName|rfc822Name|test.com|ipAddress

You may also need to check the = 3 and = 4 lines.

Comment 46

[Neil Williams]

Attachment: patch as checked in

Comment 47

[Neil Williams]

Checking in cmd/certutil/certext.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certext.c,v  <--  certext.c
new revision: 1.5; previous revision: 1.4
done
Checking in cmd/certutil/certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.120; previous revision: 1.119
done
Checking in tests/cert/certext.txt;
/cvsroot/mozilla/security/nss/tests/cert/certext.txt,v  <--  certext.txt
new revision: 1.2; previous revision: 1.1
done

Comment 48

[Neil Williams]

Closing this and bug 324740.

Comment 49

[Wan-Teh Chang]

Attachment: Remove "secutil.h" from lib/ssl/derive.c on NSS_3_11_BRANCH

See comment 10 and comment 13.  Neil checked in this patch on the
NSS trunk (derive.c, rev. 1.6) but didn't check it in on the
NSS_3_11_BRANCH.

Comment 50

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 319238 [details] [diff] [review]
Remove "secutil.h" from lib/ssl/derive.c on NSS_3_11_BRANCH

Wan-Teh, what problem does this patch solve for 3.11.x?

Comment 51

[Wan-Teh Chang]

Comment on attachment 319238 [details] [diff] [review]
Remove "secutil.h" from lib/ssl/derive.c on NSS_3_11_BRANCH

Kai ran into this problem in bug 419030, so his
patch has a workaround you asked about, which he
explained at the end of bug 419030 comment 27.

Comment 52

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 319238 [details] [diff] [review]
Remove "secutil.h" from lib/ssl/derive.c on NSS_3_11_BRANCH

This patch causes numerous new warnings about calls to undefined symbols to 
appear on Windows.  On other platforms, it might cause build failures.

derive.c(636) SECKEY_GetPrivateKeyType undefined; assuming extern returning int
derive.c(681) SECKEY_PublicKeyStrength undefined; assuming extern returning int
derive.c(758) SECKEY_CreateECPrivateKey undefined; assuming extern returning int
derive.c(758) = 'SECKEYPrivateKey *' differs in levels of indirection from 'int'
derive.c(761) SECKEY_DestroyPrivateKey undefined; assuming extern returning int
derive.c(763) SECKEY_DestroyPublicKey undefined; assuming extern returning int
derive.c(777) = 'SECKEYPrivateKey *' differs in levels of indirection from 'int'

Comment 53

[Wan-Teh Chang]

Attachment: Replace "secutil.h" with "keyhi.h" in lib/ssl/derive.c on NSS_3_11_BRANCH

Sorry.  We need to backport both rev. 1.6 and rev. 1.7 from the trunk.
Here is the correct patch.

Comment 54

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, since this bug is resolved/fixed, 
and review requests on resolved bugs don't show up on my radar,
let's take your new patch to bug 431204.  I think it belongs there anyway, 

Comment 55

[Wan-Teh Chang]

Comment on attachment 319405 [details] [diff] [review]
Replace "secutil.h" with "keyhi.h" in lib/ssl/derive.c on NSS_3_11_BRANCH

I moved this patch to the SSL_CanBypass bug (bug 325672),
which introduced the problem this patch solves.  The problem
is minor, so I don't mind if it's not fixed on the NSS_3_11_BRANCH.