PK11_ListCerts missing options

RESOLVED FIXED in 3.9

Status

enhancement
P2
normal
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: julien.pierre, Assigned: julien.pierre)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Currently, PK11_ListCerts offers four options :

typedef enum {
     PK11CertListUnique = 0,
     PK11CertListUser = 1,
     PK11CertListRootUnique = 2,
     PK11CertListCA = 3
} PK11CertListType;

PK11CertListUnique lists all unique certs - whether CA or user.

PK11CertListUser lists all instances of user certs.

PK11CertListRootUnique lists all unique CA certs.

PK11CertListCA lists all instances of CA certs.

There are two missing options :

PK11CertListUserUnique to list unique user certs.

PK11CertList(everything?) to list all instances of all certs, whether CA or user.

We need to find a good name for the last option, but if we follow the existing
naming convention, it would simply be called PK11CertList. This doesn't conflict
with any symbol but is not very intuitive.

This last option would be the right one to use in certutil IMO, instead of the
current PK11CertListUnique, which doesn't show multiple instances of certs. This
is baffling to users for duplicate user certs in multiple tokens (reported by
CMS team).
How about PK11CertListAll?
Yes, PK11CertListAll would work.
Blocks: 215214
Taking bug.
Assignee: wchang0222 → jpierre
Severity: normal → enhancement
Priority: -- → P2
Target Milestone: --- → 3.9
PK11CertListAll is only defined in the header, and never needs to be referenced
in the code, since all the checks are for exclusion, and in this case we want
all certs.
Attachment #130867 - Flags: superreview?(wchang0222)
Attachment #130867 - Flags: review?(relyea)
Wan-Teh, now that the other changes to PK11_ListCerts have been checked in,
could you please review this patch ?

Thanks.
Posted patch update (obsolete) — Splinter Review
also add an alias of PK11CertListCAUnique for PK11CertListRootUnique, since
that wasn't accurate. That option actually returned all CA certs rather than
all roots.
Attachment #130867 - Attachment is obsolete: true
Attachment #131372 - Flags: superreview?(relyea)
Attachment #131372 - Flags: review?(wchang0222)
Comment on attachment 131372 [details] [diff] [review]
update

It seems that the 'aliasing' of PK11CertListRootUnique to PK11CertListCAUnique
should be done with a #define rather than assigning both the same value in the
enum. I'm surprised this makes it through the compilier.

Other than that patch looks fine.

bob
Attachment #131372 - Flags: superreview?(relyea) → superreview-
Posted patch one more update (obsolete) — Splinter Review
- make PK11ListCAUnique a different value from PK11ListRootUnique
- add comment that PK11ListRootUnique is for legacy apps
Attachment #131372 - Attachment is obsolete: true
Attachment #131575 - Flags: superreview?(wchang0222)
Attachment #131575 - Flags: review?(rrelyea0264)
Comment on attachment 131575 [details] [diff] [review]
one more update

1. pk11cert.c

>+    /* if we want root certs, skip the user certs. For legacy apps */

"root certs" should be changed to "CA certs" in the comment.

We should also explain that this check for private keys for
CA certs doesn't make sense (because a cert server will have
a CA cert with a private key) and we are only keeping the
check for legacy apps.	New apps should use PK11CertListCAUnique
instead.

We should also remove the unnecessary code:

    /* if we want Unique certs and we already have it on our list, skip it */
    if ( isUnique && isOnList(certList,c) ) {
	return PR_SUCCESS;
    }

2. secmodt.h

Please add comments explaining every PK11CertListXXX enum constant.

Thanks.
Attachment #131575 - Flags: superreview?(wchang0222)
Attachment #131575 - Flags: superreview-
Attachment #131575 - Flags: review?(rrelyea0264)
- add comments for each enum value in the header file
- add better comment for PK11CertListRootUnique in the code
- remove unnecessary code
Attachment #131575 - Attachment is obsolete: true
Attachment #131634 - Flags: review?(wchang0222)
Comment on attachment 131634 [details] [diff] [review]
incorporate Wan-Teh's latest comments

r=wtc.

Julien, please change to use one of our multi-line
comment block styles:

    /*
     * comment
     */

    /*
    ** comment
    */

before you check this patch in.  Thanks.

Bob, could you review this patch too?
Attachment #131634 - Flags: superreview?(rrelyea0264)
Attachment #131634 - Flags: review?(wchang0222)
Attachment #131634 - Flags: review+
Checked in with the comment change.

cvs commit: Examining .
Checking in pk11cert.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.121; previous revision: 1.120
done
Checking in secmodt.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/secmodt.h,v  <--  secmodt.h
new revision: 1.19; previous revision: 1.18
done
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment on attachment 131634 [details] [diff] [review]
incorporate Wan-Teh's latest comments

>-
>-    /* if we want Unique certs and we already have it on our list, skip it */
>-    if ( isUnique && isOnList(certList,c) ) {
>-	return PR_SUCCESS;
>-    }
>-

I just wanted to explain this change.

This change has nothing to do with the new options for
PK11_ListCerts.  This change is an optimization.

I verified that the isOnList test is useless in NSS 3.4
or later because Stan's NSSTrustDomain_TraverseCertificates
ensures that pk11ListCertCallback is invoked on each
NSSCertificate object only once.
Comment on attachment 131634 [details] [diff] [review]
incorporate Wan-Teh's latest comments

I already verbally approved this, clean up request list.
Attachment #131634 - Flags: superreview?(rrelyea0264) → superreview+
Comment on attachment 130867 [details] [diff] [review]
add PK11CertListUserUnique and PK11CertListAll options to PK11_ListCerts

r+ already given to later patches
Attachment #130867 - Flags: review?(rrelyea0264) → review-
Attachment #131372 - Flags: review?(wchang0222)
Attachment #130867 - Flags: superreview?(wchang0222)
You need to log in before you can comment on or make changes to this bug.