372241 - Need more versatile form of CERT_NameToAscii

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 1

[Neil Williams]

(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.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v0.1

This is my first attempt at a solution.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Maybe we want a forth choice: OpenSSL compatibility mode

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 275941 [details] [diff] [review]
patch v0.1

Neil, please review

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 275941 [details] [diff] [review]
patch v0.1

Bob, Steve, Please review.

Comment 7

[Robert Relyea]

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 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 275941 [details] [diff] [review]
patch v0.1

Requesting second review for branch.

Comment 9

[Julien Pierre]

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.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Attachment: 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 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 303808 [details] [diff] [review]
patch v2 - rewrite

seeking timely review

Comment 12

[Julien Pierre]

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 .

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Attachment: 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 14

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

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