We should enhance the NSS tools to create distrust records, so we don't need to do that manually. I already have a patch, it has the following enhancements: (a) Given a certificate as input, addbuiltin can be used to print out a distrust record for certdata.txt The record will be based on issuer and serial. If desired, in addition, the tool can print out the hashes. addbuiltin -D [-h] -n label [-i certfile] Read a der-encoded cert from certfile or stdin, and output a distrust record (issuer+serial). -D distrust certificate by issuer/serial -h restrict distrust to certificate hash (b) Given a CRL as input, and an index number for a CRL entry, addbuiltin can be used to print out a distrust record for certdata.txt The entry will be based on isser (taken from the CRL) and the serial (the binary encoding of the given entry number). addbuiltin -C -e crl-entry-number -n label [-i crlfile] Read a CRL from crlfile or stdin, and output a distrust record (issuer+serial). (c) In order to assist (b), I've enhanced the crlutil with an additional command. The new -S command doesn't require a database. It will use a CRL as input, print the issuer name, and for each entry it will print the index number and the fingerprint. This can be used to easily search for a desired fingerprint, and locate the entry number that can be used with "addbuiltin -e" crlutil -S -i crl (d) A general enhancement. The entries in certdata.txt are not human readable. I think it would be helpful to have human readable comments that make it easier to identify an entry in certdata.txt This is helpful when checking "is the issue already there", or "which one must be removed", because often the label is not sufficiently helpful. With the new modification, a comment header like the following will be added to cert and trust entries: # # Certificate "test" # # Issuer: "CN=StartCom Class 2 Primary Intermediate Server CA,OU=Secure Digital Certificate Signing,O=StartCom Ltd.,C=IL" # Serial Number: 37545 (0x92a9) # Subject: "E=webmaster@kuix.de,CN=kuix.de,O=Kai Engert,L=Frankfurt,ST=Hessen,C=DE,OID.2.5.4.13=559426-b6PK3edYwS0Fqx17" # Not Valid Before: Mon Nov 07 22:08:11 2011 # Not Valid After : Fri Nov 08 04:42:21 2013 # Fingerprint (MD5): 55:30:2D:10:89:ED:A6:1D:4E:0F:7A:7D:03:32:D5:1A # Fingerprint (SHA1): 2D:C0:7D:73:DD:97:CD:B9:E1:F5:71:10:FE:64:86:3B:76:69:55:A4 or, if all we have is isser and serial # Explicitly Distrust "test" # Issuer: "CN=VeriSign Class 2 CA - Individual Subscriber,OU="www.verisign.com/repository/RPA Incorp. By Ref.,LIAB.LTD(c)98",OU=VeriSign Trust Network,O="VeriSign, Inc."" # Serial Number:52:a6:1f:3e:b9:d5:8d:98:42:7f:f2:34:97:0d:f5:ae (e) I would like to enhance the addbuiltin tool further. When adding a distrust record, I think manually inventing a label might often be difficult. I propose to implement a feature that will automatically create a nickname based on the concatenation of printed issuer DN and printed serial number. Please try these tool enhancements and tell me if you like it.

Updated patch to fix bugs.

Update: No quotes around the DNs that are printed in comments (because they could contain quotes, too, and that looks wrong).

Comment on attachment 598465 [details] [diff] [review] Patch v4 r+ but I have lots of comments: 1) be sure to fix the indents on ConvertCertificate. The patch makes the changes easier to review, but they should be checked in with proper NSS indents. 2) everything under the #ifdef 0 needs to be re-reviewed. Lots of nitty things that the compilier will tell you about;). Also, as they appear to be written, they look more like putname and putinteger rather than getname and getinteger. 3) -t -D and -C are all mutually exclusive. you currently only check if -t and -D are both set or if none of them are set. You can fix this the following ways: a) check all 4 cases (you currently check 2). or b) keep a counter of which are set and fail if it's not exactly 1. The former will more easily give the user a better message, the latter is more scalable if you add any other commands. -------------------------------------------- fixing 1 is sufficent for checkin, thought I'd like to see 1 and 3 fixed as well... Now there is one little matter which may change how you want to do this. Currently -t p,p,p is equivalent to -D -h except the description says 'distrust' rather than 'trust' and it includes the whole cert. Also, there is no way to do -t ,,p without a hash (explicitly distrust for object signing, but potentially allow for others). This is more a completeness thing. but it does bring up a point: 1) it may make sense to separate the trust spec from the 'trust/distrust' label. -D could simply be short hand for -t p,p,p, and the trust/distrust description is based on the trust record itself. Any trust record that had one or more p's and no explicit trusted values (that is distrust if (trust->sslFlags | trust->emailFlags | trust->objectSigningFlags) == CERTDB_TERMINAL_RECORD)) 2) Make whether or not to include the cert an option. The certs are not needed for Trusted certs as well, except currently the PSM UI would break. This is lower priority, I don't think we will be removing the certs themselves anytime soon. 3) Drop the hashes by default if (trust->sslFlags | trust->emailFlags | trust->objectSigningFlags) == CERTDB_TERMINAL_RECORD). This means you want to make the -h flag operation even if -t was specified. If trust is not distrusted, -h is simply a noop. bob

> 3) Drop the hashes by default > if ((trust->sslFlags | trust->emailFlags | trust->objectSigningFlags) > == CERTDB_TERMINAL_RECORD) > This means you want to make the -h flag operation even if -t was > specified. If trust is not distrusted, -h is simply a noop. But if we did that, how would one say "distrust and add a hash" ? Should we rather reverse the meaning of -h and say: -h Include the hash with a distrust record

Actually, ignore comment 5. I was confused. -h is already a noop with positive trust. The patch always includes the hash for positive trust. -h already means what I proposed in comment 5.

Ok, ignore comment 5 and comment 6. My new proposal is: -h means "exclude hash" New parameter description: addbuiltin: you must specify exactly one of -t or -D or -C addbuiltin -t trust -n nickname [-i certfile] [-c] [-h] Read a der-encoded cert from certfile or stdin, and output it to stdout in a format suitable for the builtin root module. Example: addbuiltin -n MyCA -t "C,C,C" -i myca.der >> certdata.txt addbuiltin -D -n label [-i certfile] Read a der-encoded cert from certfile or stdin, and output a distrust record. (-D is equivalent to -t p,p,p -c -h) addbuiltin -C -e crl-entry-number -n label [-i crlfile] Read a CRL from crlfile or stdin, and output a distrust record (issuer+serial). (-C implies -c -h) -t trust trust flags (cCTpPuw). -n nickname nickname to assign to builtin cert, or a label for the distrust record. -c exclude the certificate (only add a trust record) -h exclude hash from trust record (useful to distrust any matching issuer/serial) -e a CRL entry number, as shown by "crlutil -S" -i file input file to read (default stdin) (pipe through atob if the cert is b64-encoded)

Comment on attachment 604010 [details] [diff] [review] Patch v5 Bob, do you want to re-review the differences?

(In reply to Robert Relyea from comment #4) > > 2) everything under the #ifdef 0 needs to be re-reviewed. Lots of nitty > things that the compilier will tell you about;). Also, as they appear to be > written, they look more like putname and putinteger rather than getname and > getinteger. Yes, absolutely. It was just some early pseudo-code and snippets. I've now decided to remove it from this patch. Instead, I filed bug 734047 and attacher it there.

Comment on attachment 604010 [details] [diff] [review] Patch v5 r+ rrelyea Yes, those are good changes. Thanks for making them. Only one nit comment. As coded it is possible now to drop the hash even if the cert is to be trusted (even though that NSS will then reject the trust record). It's not a big deal, but you should either through an error, or silently ignore the exclude (you can to either the else side of your "if ((trust->sslFlags | trust->emailFlags | trust->objectSigningFlags) == CERTDB_TERMINAL_RECORD)" test. bob

addressed Bob's request

Patch v6 checked in. Checking in addbuiltin/addbuiltin.c; /cvsroot/mozilla/security/nss/cmd/addbuiltin/addbuiltin.c,v <-- addbuiltin.c new revision: 1.17; previous revision: 1.16 done Checking in crlutil/crlutil.c; /cvsroot/mozilla/security/nss/cmd/crlutil/crlutil.c,v <-- crlutil.c new revision: 1.36; previous revision: 1.35 done Checking in lib/secutil.c; /cvsroot/mozilla/security/nss/cmd/lib/secutil.c,v <-- secutil.c new revision: 1.113; previous revision: 1.112 done Checking in lib/secutil.h; /cvsroot/mozilla/security/nss/cmd/lib/secutil.h,v <-- secutil.h new revision: 1.40; previous revision: 1.39 done