Last Comment Bug 408260 - certutil usage doesn't give enough information about trust arguments
: certutil usage doesn't give enough information about trust arguments
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Tools (show other bugs)
: 3.11.8
: All All
: P3 normal (vote)
: 3.12.2
Assigned To: Julien Pierre
:
:
Mentors:
http://bonsai.mozilla.org/cvsblame.cg...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-12-13 13:58 PST by Julien Pierre
Modified: 2008-08-28 14:38 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Improve usage message (695 bytes, patch)
2008-01-31 20:30 PST, Julien Pierre
nelson: review+
Details | Diff | Splinter Review
Also add back header line in list certs (964 bytes, patch)
2008-02-05 14:27 PST, Julien Pierre
no flags Details | Diff | Splinter Review
Don't print header if base64 or binary output was requested (1.06 KB, patch)
2008-02-05 17:08 PST, Julien Pierre
nelson: review-
Details | Diff | Splinter Review
Fix indentation by adding one more line (1.10 KB, patch)
2008-02-06 21:24 PST, Julien Pierre
nelson: review-
Details | Diff | Splinter Review
As discussed over IM (1.08 KB, patch)
2008-02-13 15:05 PST, Julien Pierre
nelson: review+
Details | Diff | Splinter Review
don't print headers when listing cert details (593 bytes, patch)
2008-08-27 18:22 PDT, Julien Pierre
nelson: review+
Details | Diff | Splinter Review

Description Julien Pierre 2007-12-13 13:58:04 PST
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 2008-01-31 20:30:50 PST
Created attachment 300821 [details] [diff] [review]
Improve usage message
Comment 2 Nelson Bolyard (seldom reads bugmail) 2008-02-01 18:56:33 PST
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) 2008-02-02 10:43:10 PST
Comment on attachment 300821 [details] [diff] [review]
Improve usage message

r=nelson
Please consider the question in comment 2.
Comment 4 Julien Pierre 2008-02-05 14:27:49 PST
Created attachment 301582 [details] [diff] [review]
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 2008-02-05 17:03:50 PST
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 2008-02-05 17:08:38 PST
Created attachment 301605 [details] [diff] [review]
Don't print header if base64 or binary output was requested
Comment 7 Nelson Bolyard (seldom reads bugmail) 2008-02-05 19:56:26 PST
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 2008-02-06 21:24:08 PST
Created attachment 301821 [details] [diff] [review]
Fix indentation by adding one more line
Comment 9 Nelson Bolyard (seldom reads bugmail) 2008-02-06 22:11:23 PST
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 2008-02-06 22:30:34 PST
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) 2008-02-13 14:34:23 PST
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 2008-02-13 15:05:37 PST
Created attachment 303138 [details] [diff] [review]
As discussed over IM
Comment 13 Nelson Bolyard (seldom reads bugmail) 2008-02-13 16:43:03 PST
Comment on attachment 303138 [details] [diff] [review]
As discussed over IM

I think this is a good compromise.
Comment 14 Julien Pierre 2008-02-13 16:52:13 PST
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) 2008-08-19 17:58:24 PDT
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 2008-08-27 18:22:17 PDT
Created attachment 335820 [details] [diff] [review]
don't print headers when listing cert details
Comment 17 Nelson Bolyard (seldom reads bugmail) 2008-08-27 20:20:26 PDT
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 2008-08-28 14:38:45 PDT
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

Note You need to log in before you can comment on or make changes to this bug.