231881 - export cert functions that decode extensions

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

The CERTCertificate structure contains a pointer to an array of unparsed
extensions.  There are function in nss to parse many (most) of the common
extensions, but most of those functions are not exported, and are declared
in private header files.  

These functions need to be exported and made public.  It is difficult for
applications to make any use of these extensions without the functions to 
parse them.  The Pretty Printing enhancements in bug 132942 will need them.

The issue/question is:  
Do we make some of the existing private header files public?
or do we move the declarations of the affected functions to public headers?

files that contain the relevant declarations include:
certdb/genname.h
certdb/xconst.h 
and perhaps others.

Comment 1

[Wan-Teh Chang]

I think the declaration of these functions should be
moved to cert.h.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1 - see comment 2

This patch is large, bug the changes it makes are conceptually small.
It touches a lot of lines because functions were renamed for exporting.

Reviewers, please review this bug to see that it does all the following,
and only these things:

1. renames functions as follows:

    cert_get_next_general_name	       -> CERT_GetNextGeneralName
    cert_get_prev_general_name	       -> CERT_GetPrevGeneralName
    cert_get_next_name_constraint      -> CERT_GetNextNameConstraint
    cert_get_prev_name_constraint      -> CERT_GetPrevNameConstraint
    cert_DecodeAuthInfoAccessExtension -> CERT_DecodeAuthInfoAccessExtension

2. Adds declarations for the following functions to cert.h (public file)

CERT_DecodeAltNameExtension(
CERT_DecodeNameConstraintsExtension(
CERT_DecodeAuthInfoAccessExtension(
CERT_DecodePrivKeyUsagePeriodExtension(
CERT_GetNextGeneralName(
CERT_GetPrevGeneralName(
CERT_GetNextNameConstraint(
CERT_GetPrevNameConstraint(

3. Renames struct PKUPEncodedContext to CERTPrivKeyUsagePeriod.
   and moves the declaration to cert.h, and a typedef for it.
   and sorts the struct typedefs into alphabetical order.  

   Also renamed struct AltNameEncodedContext to CERTAltNameEncodedContext

4. removes declarations of the followingt functions from private header
genname.h:

cert_get_next_general_name
cert_get_prev_general_name

5. Renames CERT_EncodePublicKeyUsagePeriod to 
	   CERT_EncodePrivateKeyUsagePeriod.
   The function has always encoded Private Key Usage Periods, and was 
   simply misnamed.  

   Adds new function CERT_DecodePrivKeyUsagePeriodExtension to xconst.c.
   This function complements CERT_EncodePrivateKeyUsagePeriod() and uses 
   the same signature as the other CERT_Decode functions.

6. makes xconst.h idempotent, and compilable by C++.

7. exports the following functions:

CERT_DecodeAltNameExtension;
CERT_DecodeAuthInfoAccessExtension;
CERT_DecodeAuthKeyID;
CERT_DecodeCRLDistributionPoints;
CERT_DecodeNameConstraintsExtension;
CERT_DecodePrivKeyUsagePeriodExtension;
CERT_GetNextGeneralName;
CERT_GetNextNameConstraint;
CERT_GetPrevGeneralName;
CERT_GetPrevNameConstraint;

8. Breaks some lines that were longer than 80 columns.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 139952 [details] [diff] [review]
patch v1 - see comment 2

Please review.

Comment 4

[Robert Relyea]

Comment on attachment 139952 [details] [diff] [review]
patch v1 - see comment 2

The patch looks good. My only comment is I would prefer the changes to certt.h
not get checked in.

The only changes in certt.h is to reorder several typedefs. The current
typedefs are orded alphabetically according the newly defined types, the new
order is orded alphabetically according to the base structures of these types.
I believe the original order is more "correct" or "useful" since when humans
look at the file, the are usually know they defined type and what to know what
data structure defines it.

Example:
Original order...
 typedef struct CERTCertificateStr		  CERTCertificate;
 typedef struct CERTCertificateListStr		  CERTCertificateList;
 typedef struct CERTCertificateRequestStr	  CERTCertificateRequest;

New order:
 typedef struct CERTCertificateListStr		  CERTCertificateList;
 typedef struct CERTCertificateRequestStr	  CERTCertificateRequest;
 typedef struct CERTCertificateStr		  CERTCertificate;

r+ because the rest of the patch can go in as is.

Comment 5

[Wan-Teh Chang]

Comment on attachment 139952 [details] [diff] [review]
patch v1 - see comment 2

r=wtc.

>+CERTPrivKeyUsagePeriod *
>+CERT_DecodePrivKeyUsagePeriodExtension(PLArenaPool *arena, SECItem *extnValue)
>+{
...
>+    /* decode the policy info */
>+    rv = SEC_QuickDERDecodeItem(arena, pPeriod, 
>+                                CERTPrivateKeyUsagePeriodTemplate,
>+			        &newExtnValue);

The comment is wrong.

Comment 6

[Wan-Teh Chang]

Comment on attachment 139952 [details] [diff] [review]
patch v1 - see comment 2

Another comment: I think the 'arena' field in the
CERTPrivKeyUsagePeriod structure is useless because
the structure doesn't own that arena and won't need
to allocate memory from that arena after the structure
has been constructed:

>+/* Private Key Usage Period extension struct. */
>+struct CERTPrivKeyUsagePeriodStr {
>+    SECItem notBefore;
>+    SECItem notAfter;
>+    PRArenaPool *arena;
>+};

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Bob write:  

> The only changes in certt.h is to reorder several typedefs.

No, there is a new struct in certt.h, and a new typedef for it.
I metnioned that in item 3 of my summary (I said cert.h, but it was 
actually in certt.h).

I'm willing to sort the names by the typedef'ed name, rather than 
the structure name.  

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Revised patch for certt.h

I resorted the typedefs by the typedef name.  This patch is the result.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 140039 [details] [diff] [review]
Revised patch for certt.h

bob, pls review.

Comment 10

[Robert Relyea]

Comment on attachment 140039 [details] [diff] [review]
Revised patch for certt.h

Looks good.

r=relyea

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

/cvsroot/mozilla/security/nss/lib/certdb/alg1485.c,v  <--  alg1485.c
new revision: 1.20; previous revision: 1.19

/cvsroot/mozilla/security/nss/lib/certdb/cert.h,v  <--  cert.h
new revision: 1.45; previous revision: 1.44

/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.62; previous revision: 1.61

/cvsroot/mozilla/security/nss/lib/certdb/certt.h,v  <--  certt.h
new revision: 1.25; previous revision: 1.24

/cvsroot/mozilla/security/nss/lib/certdb/genname.c,v  <--  genname.c
new revision: 1.26; previous revision: 1.25

/cvsroot/mozilla/security/nss/lib/certdb/genname.h,v  <--  genname.h
new revision: 1.7; previous revision: 1.6

/cvsroot/mozilla/security/nss/lib/certdb/xconst.c,v  <--  xconst.c
new revision: 1.6; previous revision: 1.5

/cvsroot/mozilla/security/nss/lib/certdb/xconst.h,v  <--  xconst.h
new revision: 1.3; previous revision: 1.2

/cvsroot/mozilla/security/nss/lib/certhigh/ocsp.c,v  <--  ocsp.c
new revision: 1.18; previous revision: 1.17

/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.129; previous revision: 1.128

/cvsroot/mozilla/security/nss/cmd/certcgi/certcgi.c,v  <--  certcgi.c
new revision: 1.15; previous revision: 1.14