This bug is the companion to Bug 329067. Bug 329067 is about issues converting LDAP Strings to DNs. This bug is about converting DNs to LDAP strings. NSS has one function for converting CERTNames (DNs) to LDAP strings. It is CERT_NameToAscii. It was originally devised to create humanly readable forms of cert DNs for use in the browser. But later that function was also used by other code (such as XML Dsig code) to translate cert names into XML form to be sent to other computers that would reverse the translation. It was important that these XML programs produced the same DER DN as the one with which they started, after translating from DER to LDAP string and back to DER. And it was important that these programs only use "short names" (attribute type descriptor strings) that were universally recognized by other implementations. It is a problem if one implementation translates an attribute to a short name that is not recognized by another implementation. So, in June 2003, for NSS 3.9, I changed CERT_NameToAscii, greatly reducing the set of short names that it generated in LDAP strings, increasing the number of attribute types that get output using the OID=hexadecimal form. Consequences of that action include: a) significantly reduced human readability of the LDAP strings produced by CERT_NameToAscii, and b) CERT_AsciiToName no longer recognizes the many short names that are no longer produced by CERT_NameToAscii. CERT_AsciiToName really needs to recognize all the "short names" that it can, even if CERT_AsciiToName no longer produces them. The XML Dsig project has never completed, AFAIK. So the changes that were made to benefit XML Dsig, at the expense of human readability, disserved the browser users, and did not really benefit the XML crowd much (if any). It now occurs to me that we need a new form of CERT_NameToAscii, one that allows the caller to specify whether he wants: a) maximum human readability, even the results are slightly non-standard, (such as OIDString=utf8 string format, which is non-standard, but much more humanly readable than OIDString=#hexadecimal), or b) strict RFC compliance, generating the shortname=utf8 string form ONLY for the attributes types required by RFC 4514, and generating the OIDString=#hex form for all other attributes types, or c) maximum invertability of results, using the OIDstring=#hexadecimal form for all attribute types that have more than one permitted encoding. I envision a new function, CERT_NameToUtf8, that takes the same arguments as CERT_NameToAscii, and one additional argument, which is an enumerated type that specifies which of the above choices of output format is preferred. We should try to do this for NSS 3.12, but not for 3.11.x, IMO. When we do that, we should also change CERT_NameToAscii to work the way it did before NSS 3.9, that is, to generate maximum humanly readable form, rather than strict RFC compliant mode.
(In reply to comment #0) >... > > It now occurs to me that we need a new form of CERT_NameToAscii, one that > allows the caller to specify whether he wants: > > a) maximum human readability, even the results are slightly non-standard, > (such as OIDString=utf8 string format, which is non-standard, but much > more humanly readable than OIDString=#hexadecimal), or > > b) strict RFC compliance, generating the shortname=utf8 string form ONLY for > the attributes types required by RFC 4514, and generating the OIDString=#hex > form for all other attributes types, or > > c) maximum invertability of results, using the OIDstring=#hexadecimal form > for all attribute types that have more than one permitted encoding. case c should include the complementary input transformation. Such functionality was considered, but postponed, when Bug 329067 was fixed.
Created attachment 275941 [details] [diff] [review] patch v0.1 This is my first attempt at a solution.
OpenSSL's list of known "short names" and "long names" may be seen at http://cvs.openssl.org/getfile/openssl/crypto/objects/objects.txt?v=1.69 (search for X509, exact case) and in other files in that directory, seen at http://cvs.openssl.org/dir?d=openssl/crypto/objects The format is: OID : short name : long name The list is surprisingly short, but contains some names not (yet) known to NSS.
Maybe we want a forth choice: OpenSSL compatibility mode
Comment on attachment 275941 [details] [diff] [review] patch v0.1 Neil, please review
Comment on attachment 275941 [details] [diff] [review] patch v0.1 Bob, Steve, Please review.
Comment on attachment 275941 [details] [diff] [review] patch v0.1 r+ with the following optional style issues... 1. strict is really an enum, but the code it treating it as 'tri-state' bool. It makes the code harder to read without the backstory. If strict were rather 'mode', and we made direct comparisons with the mode it would limit some of that hardness to read. If we set endKind in a switch as follows: CERT_N2A_READABLE: SEC_OID_UNKNOWN CERT_N2A_STRICT: SEC_OID_AVA_POSTAL_ADDRESS CERT_N2A_MAXIMUM: SEC_OID_AVA_COMMON_NAME Then we can elliminate the (strict < CERT_N2A_MAXIMUM) clause. 2) feel free to expand the readable list with other AVAs that are common in certs in the while (some EV AVAs come to mind). 3. I would not have any heartache if we made SHOULD or MUST the threshold for strict. (I intend my r+ to mean 'code can be checked in as is, or with any of the above modifications"). bob
Comment on attachment 275941 [details] [diff] [review] patch v0.1 Requesting second review for branch.
Comment on attachment 275941 [details] [diff] [review] patch v0.1 This patch looks OK, except for the comment block that starts at "if kind is known". Please rewrite it with proper punctuation so that it is easier to understand. The parentheses are confusing and there is an unmatched number of them. They should probably not be used at all. Also, simplication is not a word.
Created attachment 303808 [details] [diff] [review] patch v2 - rewrite I rewrote the block comment in the patch, in response to Julien's comments, and the new comment was a rather complete specification. This made me rethink the behavior of the code. I ended up rewriting the code. I think it is easier to follow now than before. This patch is larger than the last one and modifies more files, so it probably won't diff against the previous patch very well. I think this should be reviewed as an entirely new patch. I invite comments on the specification and on the code.
Comment on attachment 303808 [details] [diff] [review] patch v2 - rewrite seeking timely review
Comment on attachment 303808 [details] [diff] [review] patch v2 - rewrite This looks mostly OK, except for a couple of things: 1) Below the big block comment, you should remove the definition of the UNLESS macro, since it is not used 2) In AppendAVA, the assignment of endKind is confusing . Why SEC_OID_UNKNOWN or SEC_OID_AVA_POSTAL_ADDRESS ? This needs either a comment to explain that these are array offsets into the table, or define some macros with better names that have the same values. 3) In AppendAVA, add comment on vt == SEC_ASN1_DS test about why we use hex for directory string .
Created attachment 325915 [details] [diff] [review] patch v3 - with more and better (?) comments The only intentional difference between this patch and the previous one is the addition of more comments. I will double check this before requesting review.
Comment on attachment 325915 [details] [diff] [review] patch v3 - with more and better (?) comments Unfortunately, in the 4 months that have elapsed since patch v2 there have been many changes committed to some of the files touched by this patch, so comparing this patch the previous one using Bugzilla's interdiff doesn't work well. However, doing a side by size comparison of the two patches themselves shows that the only differences are: - comments - the nss.def file, where the new symbol is now in section 3.12.1 Julien, please review to see if this patch satisfies your previous review comments.
lib/nss/nss.def; new revision: 1.193; previous revision: 1.192 lib/certdb/alg1485.c; new revision: 1.28; previous revision: 1.27 lib/certdb/cert.h; new revision: 1.72; previous revision: 1.71 lib/certdb/certt.h; new revision: 1.47; previous revision: 1.46 pkix_pl_x500name.c; new revision: 1.7; previous revision: 1.6