Last Comment Bug 215186 - PK11_ListCerts missing options
: PK11_ListCerts missing options
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.8
: All All
: P2 enhancement (vote)
: 3.9
Assigned To: Julien Pierre
: Bishakha Banerjee
Mentors:
Depends on:
Blocks: 215214
  Show dependency treegraph
 
Reported: 2003-08-05 13:56 PDT by Julien Pierre
Modified: 2003-10-14 11:28 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
add PK11CertListUserUnique and PK11CertListAll options to PK11_ListCerts (1.78 KB, patch)
2003-09-03 15:10 PDT, Julien Pierre
rrelyea: review-
Details | Diff | Splinter Review
update (2.11 KB, patch)
2003-09-12 17:47 PDT, Julien Pierre
rrelyea: superreview-
Details | Diff | Splinter Review
one more update (2.14 KB, patch)
2003-09-16 16:20 PDT, Julien Pierre
wtc: superreview-
Details | Diff | Splinter Review
incorporate Wan-Teh's latest comments (2.92 KB, patch)
2003-09-17 16:47 PDT, Julien Pierre
wtc: review+
rrelyea: superreview+
Details | Diff | Splinter Review

Description Julien Pierre 2003-08-05 13:56:00 PDT
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 Wan-Teh Chang 2003-08-05 15:20:12 PDT
How about PK11CertListAll?
Comment 2 Julien Pierre 2003-08-05 18:53:41 PDT
Yes, PK11CertListAll would work.
Comment 3 Julien Pierre 2003-09-03 14:55:52 PDT
Taking bug.
Comment 4 Julien Pierre 2003-09-03 15:10:26 PDT
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.
Comment 5 Julien Pierre 2003-09-12 15:16:04 PDT
Wan-Teh, now that the other changes to PK11_ListCerts have been checked in,
could you please review this patch ?

Thanks.
Comment 6 Julien Pierre 2003-09-12 17:47:57 PDT
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.
Comment 7 Robert Relyea 2003-09-15 10:30:38 PDT
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
Comment 8 Julien Pierre 2003-09-16 16:20:47 PDT
Created attachment 131575 [details] [diff] [review]
one more update

- make PK11ListCAUnique a different value from PK11ListRootUnique
- add comment that PK11ListRootUnique is for legacy apps
Comment 9 Wan-Teh Chang 2003-09-16 18:12:34 PDT
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.
Comment 10 Julien Pierre 2003-09-17 16:47:34 PDT
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
Comment 11 Wan-Teh Chang 2003-09-17 17:15:55 PDT
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?
Comment 12 Julien Pierre 2003-09-17 17:23:11 PDT
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
Comment 13 Wan-Teh Chang 2003-09-17 18:40:19 PDT
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 Robert Relyea 2003-10-14 10:07:51 PDT
Comment on attachment 131634 [details] [diff] [review]
incorporate Wan-Teh's latest comments

I already verbally approved this, clean up request list.
Comment 15 Robert Relyea 2003-10-14 11:28:43 PDT
Comment on attachment 130867 [details] [diff] [review]
add PK11CertListUserUnique and PK11CertListAll options to PK11_ListCerts

r+ already given to later patches

Note You need to log in before you can comment on or make changes to this bug.