408260 - certutil usage doesn't give enough information about trust arguments

Status: RESOLVED FIXED

(NSS :: Tools, defect, P3)

Description

[Julien Pierre]

The only information that certutil -H gives about trust arguments is :

   -t trustargs      Set the certificate trust attributes:
                          p      valid peer
                          P      trusted peer (implies p)
                          c      valid CA
                          T      trusted CA to issue client certs (implies c)
                          C      trusted CA to issue server certs (implies c)
                          u      user cert
                          w      send warning
                          g      make step-up cert

Nowhere is it specified that there are 3 sets of trust bits - one for SSL, another for S/MIME and one more for code signing.

Comment 1

[Julien Pierre]

Attachment: Improve usage message

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

See 
<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/cmd/lib/secutil.c&rev=1.19#2218>
Prior to revision 1.20 of nss/cmd/lib/secutil.c, certutil -L printed out 
a header line that read:

Certificate Name                                         Trust Attributes

followed by the list of nicknames and trust flags, followed by this legend:


p    Valid peer
P    Trusted peer (implies p)
c    Valid CA
T    Trusted CA to issue client certs (implies c)
C    Trusted CA to certs(only server certs for ssl) (implies c)
u    User cert
w    Send warning

Do we want to put a header and legend back into the output of certutil -L?

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 300821 [details] [diff] [review]
Improve usage message

r=nelson
Please consider the question in comment 2.

Comment 4

[Julien Pierre]

Attachment: Also add back header line in list certs

Nelson,

Thanks for the review.
Re: comment 2, I don't think we need to add the description of the trust flags back in the list certs output, since it is listed in the usage for certutil (certutil -H). However, the header line however could be helpful. I'm not sure why it was removed.

Comment 5

[Julien Pierre]

Comment on attachment 301582 [details] [diff] [review]
Also add back header line in list certs

I figured out why the headers/footers were removed. It screws up the binary output (certutil -L -r).

Comment 6

[Julien Pierre]

Attachment: Don't print header if base64 or binary output was requested

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 301605 [details] [diff] [review]
Don't print header if base64 or binary output was requested

The good news is that this patch applies cleanly, even after the patch for bug 
291384 was checked in.  The bad news is that with this patch, the line that
contains the string "(SSL, S/MIME, code signing)" is 88 columns wide (not counting
the trailing \r\n) in the output.  Please shorten that line by 10 characters.

>+        PR_fprintf(outfile, "\n%-60s %-5s\n%-60s %-5s\n\n", "Certificate Nickname",
>+               "Trust Attributes", "", "(SSL, S/MIME, code signing)");

Comment 8

[Julien Pierre]

Attachment: Fix indentation by adding one more line

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 301821 [details] [diff] [review]
Fix indentation by adding one more line

Sorry, but I think that breaking the names of the 3 trust flag categories onto two lines obscures the fact that this is a triplet of three values on a single line.  
Please just shorten that
header line by 10 columns.

Comment 10

[Julien Pierre]

Nelson,

Attachment 301821 [details] [diff] produces the following output :

Certificate Nickname                                         Trust Attributes
                                                             (SSL, S/MIME,
                                                              code signing)


I don't think this is any more obscure in 3 lines than it would be in 2 lines.
I can't express the whole header in 2 lines of text that will fit 80 columns, without either having it look ugly and unaligned, or changing the alignment for every line output of -L, something that would also be requried if I wanted to make it fit one line - but at the cost of reducing the nickname space. And the alignment is not only in certutil.c but also in  SECU_PrintCertNickname which is in lib. I thought it might have been used by other tools in nss/cmd, though I see that it's not right now. I think changing the alignment would be fine if we were changing the number of columns, but this is only a header. It doesn't warrant reducing the length of the nickname in the output. Please reconsider my last patch.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Here's a thought.  Shorten "code signing" to either "JAR" or "XPI".
In practice, those are the only two forms of objects for which NSS code 
signing is used.

Comment 12

[Julien Pierre]

Attachment: As discussed over IM

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 303138 [details] [diff] [review]
As discussed over IM

I think this is a good compromise.

Comment 14

[Julien Pierre]

Thanks, Nelson.

I checked this in to the trunk.

Checking in certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.131; previous revision: 1.130
done

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

There is one problem with the checkin that was revision 1.131.

That patch should have caused those additional header lines to appear in
the output of certutil -L ONLY when it is not using the -n option.
That is, it should only affect listings of cert nicknames, not the 
commands that actually output a cert in some form.  But now, those header
files are appearing in the output of commands such as:

   certutil -d DB -L -n "Builtin Object Token:Taiwan GRCA"

Comment 16

[Julien Pierre]

Attachment: don't print headers when listing cert details

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 335820 [details] [diff] [review]
don't print headers when listing cert details

looks like that will do it.

Comment 18

[Julien Pierre]

Thanks for the review, Nelson. I checked in this fix to the trunk.

Checking in certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.142; previous revision: 1.141
done