If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

export cert functions that decode extensions

RESOLVED FIXED in 3.10

Status

NSS
Libraries
P2
enhancement
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Nelson Bolyard (seldom reads bugmail))

Tracking

3.10
x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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

14 years ago
I think the declaration of these functions should be
moved to cert.h.
Summary: export cert functions that decode extensions → export cert functions that decode extensions
(Assignee)

Updated

14 years ago
Priority: -- → P2
Target Milestone: --- → 3.10
(Assignee)

Comment 2

14 years ago
Created attachment 139952 [details] [diff] [review]
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.
(Assignee)

Comment 3

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

Please review.
Attachment #139952 - Flags: review?(wchang0222)

Comment 4

14 years ago
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.
Attachment #139952 - Flags: superreview+

Comment 5

14 years ago
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.
Attachment #139952 - Flags: review?(wchang0222) → review+

Comment 6

14 years ago
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;
>+};
(Assignee)

Updated

14 years ago
Blocks: 124923
(Assignee)

Comment 7

14 years ago
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.  
(Assignee)

Comment 8

14 years ago
Created attachment 140039 [details] [diff] [review]
Revised patch for certt.h

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

Comment 9

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

bob, pls review.
Attachment #140039 - Flags: review?(rrelyea0264)

Comment 10

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

Looks good.

r=relyea
Attachment #140039 - Flags: review?(rrelyea0264) → review+
(Assignee)

Updated

14 years ago
No longer blocks: 124923
(Assignee)

Comment 11

14 years ago
/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
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.