The default bug view has changed. See this FAQ.

PK11_ListCerts missing options

RESOLVED FIXED in 3.9

Status

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

People

(Reporter: Julien Pierre, Assigned: Julien Pierre)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

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

Comment 1

14 years ago
How about PK11CertListAll?
(Assignee)

Comment 2

14 years ago
Yes, PK11CertListAll would work.
(Assignee)

Updated

14 years ago
Blocks: 215214
(Assignee)

Comment 3

14 years ago
Taking bug.
Assignee: wchang0222 → jpierre
Severity: normal → enhancement
Priority: -- → P2
Target Milestone: --- → 3.9
(Assignee)

Comment 4

14 years ago
Created attachment 130867 [details] [diff] [review]
add PK11CertListUserUnique and PK11CertListAll options to PK11_ListCerts

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

Updated

14 years ago
Attachment #130867 - Flags: superreview?(wchang0222)
Attachment #130867 - Flags: review?(relyea)
(Assignee)

Comment 5

14 years ago
Wan-Teh, now that the other changes to PK11_ListCerts have been checked in,
could you please review this patch ?

Thanks.
(Assignee)

Comment 6

14 years ago
Created attachment 131372 [details] [diff] [review]
update

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
(Assignee)

Updated

14 years ago
Attachment #131372 - Flags: superreview?(relyea)
Attachment #131372 - Flags: review?(wchang0222)

Comment 7

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

Comment 8

14 years ago
Created attachment 131575 [details] [diff] [review]
one more update

- make PK11ListCAUnique a different value from PK11ListRootUnique
- add comment that PK11ListRootUnique is for legacy apps
(Assignee)

Updated

14 years ago
Attachment #131372 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #131575 - Flags: superreview?(wchang0222)
Attachment #131575 - Flags: review?(rrelyea0264)

Comment 9

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

Comment 10

14 years ago
Created attachment 131634 [details] [diff] [review]
incorporate Wan-Teh's latest comments

- 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
(Assignee)

Updated

14 years ago
Attachment #131634 - Flags: review?(wchang0222)

Comment 11

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

Comment 12

14 years ago
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
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 13

14 years ago
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 14

14 years ago
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 15

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

Updated

14 years ago
Attachment #131372 - Flags: review?(wchang0222)
(Assignee)

Updated

14 years ago
Attachment #130867 - Flags: superreview?(wchang0222)
You need to log in before you can comment on or make changes to this bug.