157942 - pk12util documentation is out of date

Status: RESOLVED FIXED

(NSS :: Documentation, defect, P4)

Description

[Wan-Teh Chang]

The pk12util documentation at
http://www.mozilla.org/projects/security/pki/nss/tools/pk12util.html
is out of date.  It has the following problems.

1. It may give readers the wrong impression that pk12util can only
be used with *servers*.

2. It documents the obsolete -a option.

3. It does not document the new options.  The current options are:

    { /* opt_CertDir           */ 'd', PR_TRUE,  0, PR_FALSE },
    { /* opt_TokenName         */ 'h', PR_TRUE,  0, PR_FALSE },
    { /* opt_Import            */ 'i', PR_TRUE,  0, PR_FALSE },
    { /* opt_SlotPWFile        */ 'k', PR_TRUE,  0, PR_FALSE },
    { /* opt_SlotPW            */ 'K', PR_TRUE,  0, PR_FALSE },
    { /* opt_Mode              */ 'm', PR_TRUE,  0, PR_FALSE },
    { /* opt_Nickname          */ 'n', PR_TRUE,  0, PR_FALSE },
    { /* opt_Export            */ 'o', PR_TRUE,  0, PR_FALSE },
    { /* opt_P12FilePWFile     */ 'w', PR_TRUE,  0, PR_FALSE },
    { /* opt_P12FilePW         */ 'W', PR_TRUE,  0, PR_FALSE },
    { /* opt_DBPrefix          */ 'P', PR_TRUE,  0, PR_FALSE },
    { /* opt_Debug             */ 'v', PR_FALSE, 0, PR_FALSE }

Comment 1

[Wan-Teh Chang]

I found that the -m option (opt_Mode) was unused, so I
deleted it (bug 157946).

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

"Future" target implies P4.

Comment 3

[Elio Maldonado]

Also    
{ /* opt_Raw   	       */ 'r', PR_FALSE, 0, PR_FALSE },

Comment 4

[Elio Maldonado]

Attachment: diif with partial fix

This is work in progress so the attachment is only a partial fix to to get some early feedback.

Comment 5

[Elio Maldonado]

Attachment: previous attachment only wasn't good.

Could I get some get early feedback on progress so far? I still need to document three options, namely; -c, -C, and -k.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Attachment: web page with Elio's patch applied

I find it much easier to review the web page with the patch applied than to 
review the patch itself, especially since the patch wasn't a unified diff.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 332440 [details] [diff] [review]
previous attachment only wasn't good.

Since this patch isn't really being considered for checkin in its present form, I am removing the review request.  However, I will write review comments about the page.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 332440 [details] [diff] [review]
previous attachment only wasn't good.

Here are lots of comments about this page, in no particular order.

1. Anywhere in the page that it shows syntax, or sample input or sample 
output, that part should use the <code> tag.  e.g. 
    <code>-d CertAndKeyDirectory</code> 
It may also use other tags, e.g. 
    <code>-d <I>CertAndKeyDirectory</I></code> 
but the part that shows actual input or output (or syntax) should be in code
format, to distinguish it from the surrounding explanatory prose.

2. Please remove the "Availability" section.  The availability is: on all
fully supported NSS platforms, just as with all other NSS utilities.  

3. I suggest you use a table for the descriptions of the various options.
The table should have two columns, the left column for the syntax and the
right column for the explanation.  That will solve all the ugly alignment 
problems.

4. The names used for option arguments in the description section should 
match the names used in the synopsis section.  Presently they do not.
For example, (this is not a complete list of the differences)

        in synopsis             in description
    ---------------------   ---------------------------
    -d CertAndKeyDir        -d CertAndKeyDirectory
    -i pkcs12ImportFile     -i Pkcs12FileNameForImport 
    -o pkcs12ExportFile     -o Pkcs12FileNameForExport
    -l pkcs12FileToList     -l Pkcs12FileNameToList
    -K slotpw               -K slotPassword
    -k slotpwfile           -k slotPwFile

5. I would suggest that you shorten the longer names of the option arguments.
For example, CertAndKeyDir could be just "dir", pkcs12ImportFile could be
just p12file.

6. The synopsis shows some things as being optional, by enclosing them in 
square brackets, when they are actually required.  Example:
pk12util -o requires the -n "option".

7. Many of the options appear to be common to all the different commands.
I would suggest that you not list those separately in the synopsis of each
command, but just collapse them into one meta-option, so that the only 
options that are specified for each command are those that are not common
to all commands, e.g. 

pk12util -i pkcs12ImportFile [-h tokenname] [common-options]
or
pk12util -o pkcs12ExportFile -n certname [-c keyCipher] [-C certCipher] \
[-x keyLen] [common-options]
or
pk12util -l pkcs12FileToList [common-options]

and then define those common options somewhere.

8. The directory that is presently described as the CertAndKeyDirectory is
actually the directory where the secmod.db file is found.  It's best to 
just call it the DB directory, I think.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

One more correction for this page.  For the -d option, it presently says:

> This is optional. If not specified, the default directory is the current 
> directory.

That is incorrect.  For all NSS utilities (except signtool, IINM), the 
default directory is either
   $HOME/.netscape     (when $HOME exists in the environment), or 
   ./.netscape         (when $HOME does not exist in the environment)

Comment 10

[Robert Relyea]

I am trying to export a cert and private key with a command like

> pk12util -o ~/work/openssl-certs/p12/exported.ca-cert.p12 -n "CA Cert"
> -d ~/work/test/nssdb1 -K foobar -W foobar -c {SOME_CIPHER_STRING}
>  --key_len {SOME_LENGTH} -C {SOME_CIPHER_STRING} --cert_key_len 
> {SOME_LENGTH}

> Where are the strings to specify the ciphers (which I assume are the PBE 
> ones) and which key strengths can I choose? 

The strings match the strings in the oid table in util/secoid.c
You can either specifiy a PBE based cipher or a symetric key cipher. In the latter case pk12util will use the PBE5v2 based encryptions.

> Are these the symmetric ones?

In the PBE5v2 case, yes. If you want PKCS12v2/PBE5v1 then you need to specify the pbe oids (which have cipher embedded in them).

> Is -c the PBE cipher for encrypting the private key? 

Yes.

> What is the other one for?  
 -C is for encrypting the whole package (or maybe it's the cert bag). pkcs12 packages have traditionally been wrapped completely with 40 bit encryption, except in FIPS mode where they aren't encrypted at all (in both cases the key is always strongly encrypted).

I only know this because I've looked at the code recently, though the foggy cobwebbs of old arguments sort of come out as to why this is the case:

1) It was desirable to encrypt the whole package.
2) Under export rules, it was OK to strongly encrypt the private key, but it was not OK to strongly encrypt the whole package, so by default the package was lightly encrypted (40-bit only).
3) In FIPS mode 40-bit crypto was not allowed, so we solve this problem by not encrypting the contents at all.

The fundamental reasons for lightly encrypting have gone away 7 or 8 years ago, so it may make sense to strongly encrypt the whole package.

Comment 11

[Elio Maldonado]

Attachment: patch addresses review comments

Comment 12

[Elio Maldonado]

Attachment: web page with patch applied

Comment 13

[Elio Maldonado]

Comment on attachment 337347 [details] [diff] [review]
patch addresses review comments

Using a table, following the style in other tools. Added discussion on key and PFX encryption, available algorithms and defaults.
Included both the patch and the resulting web page.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 337349 [details]
web page with patch applied

Thanks for attaching a copy of the page with the patch applied.  
I'll add my review to the patch.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 337347 [details] [diff] [review]
patch addresses review comments

This latest version is a HUGE improvement over what we had before.
There are a few remaining issues that should be corrected, listed here in 
no particular order.

1. Old "Usage" section should be deleted.  It has been completely superseded 
by the new Synopsis section.  It should just go away.

2. The synopsis for pk12util -o still shows that the -n parameter is optional.  
It shows "[-n certname]".  Please remove the square brackets, which denote 
an optional parameter.  -n is not optional for pk12util -o.

3. problems with the terms "option" "argument" and "command".

In some places, the new page calls -i -o and -l "options", but in other 
places, it calls them commands.  The term commands is also used in the 
documentation of other NSS utilities, such as certutil, so I think it is 
best to be consistent with those other docs, and call them commands here.

Perhaps we want to call the components of a pk12util command the 
"command option" and the "parameter options", or simply 
the command and parameters.

In common usage, we say that options (the things that start with dashes) 
may have arguments. In the example used above, -n certname, -n is the option 
and certname is the argument.   So, I would prefer not to call the options
that follow the command "arguments".

4. Sometimes, in the act of producing more accurate documentation, one 
uncovers (documents) a fact that is so shocking as to be considered a bug.  
In such cases, it may be best to fix the bug, rather than document the bug.  
I think there is an example of that in this new document.  

The revelation that, in FIPS mode, pk12util produces UNENCRYPTED PFX files 
by default is SHOCKING, and seems to violate a fundamental FIPS tenet.  
I think we really should not document that fact (even if it is accurate), 
but rather we should fix pk12util to NEVER allow an unencrypted PFX file to 
be generated in FIPS mode.

I look forward to the next revision.

Comment 16

[Elio Maldonado]

Attachment: patch addresses review comments #15

web page with patch applied coming next.

Comment 17

[Elio Maldonado]

Attachment: web page with patch 337470 applied

Comment 18

[Elio Maldonado]

1. Old "Usage" section has been deleted.
2. Fixed the synopsis, the -n parameter is required.  
3. Problems with the terms "option" "argument" and "command". Using option and argument for consistency with other NSS tools documentation but added a clarification that options -i, -o, and -l should be considered commands within the pk12util invocation.
4. Indeed, in FIPS mode pk12util produces UNENCRYPTED PFX files by default though SHOCKING, and though it seems to violate a fundamental FIPS tenet this was intentional.  The problem is that FIPS doesn't not allow weak encryption (40-bit) so the outer package is not encrypted. This is my interpretation from a conversation with Bob Relyea a while back. We should double check with him again that this is indeed the intent.
5. Other minor changes to fix uncaught typos or poor grammar.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 337470 [details] [diff] [review]
patch addresses review comments #15

The names of the option arguments in the synopsis still do not 
match the names in the table.  For example:  -W p12filepw appears in 
the synopsis, but -W p12filePassword appears in the table.  

Please make them match.  Then check it in.
Thanks.

Comment 20

[Elio Maldonado]

Attachment: new web page, fixed inconsistent naming of arguments.

Can't check it in yet as I have to get some problems with my CVS account resolved. Let me save it here for the time being.

Comment 21

[Elio Maldonado]

I should also wait a bit until the patch Bug 449334 is approved and checked in as this documents the new long options. Same for 452076 most likely.

Comment 22

[Elio Maldonado]

Attachment: synch up with changed checked in for Bug 449334 fix

Use the new -m and -n short options in synopsis and table as well as the long options. Describe the default key encryption with the long algorithm name as this is what shows up on a p12 listing. Move some explanations after the list of ciphers for clarity sake. Will attach the actual page next.

Comment 23

[Elio Maldonado]

Attachment: page with the patch applied

Nelson, please review. I will ask Bob Relyea to review as well, particularly the password based encryption notes.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 340619 [details]
page with the patch applied

r=nelson.
It might be a further improvement to put the "options" in the table in alphabetical order.  But That's not required.  This is a huge improvement over the previous documentation!

Comment 25

[Elio Maldonado]

Comment on attachment 340619 [details]
page with the patch applied

Let's get Bob's review as well.

Comment 26

[Robert Relyea]

Comment on attachment 340619 [details]
page with the patch applied

r+ 

Though I would suggest some wording changes for the pfx encryption. PFX is a microsoft term for the pkcs12 standard. Better to use something the user understands, like "overal encryption of the package. PKCS #12 provides for not only the encryption of the private keys also the cert and meta-data associated with the keys (nicknames, etc)."

It should be made clear the causual that the key is still encrypted in FIPS mode.:)

bob