215186 - PK11_ListCerts missing options

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Julien Pierre]

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]

How about PK11CertListAll?

Comment 2

[Julien Pierre]

Yes, PK11CertListAll would work.

Comment 3

[Julien Pierre]

Taking bug.

Comment 4

[Julien Pierre]

Attachment: 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]

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]

Attachment: 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]

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]

Attachment: one more update

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

Comment 9

[Wan-Teh Chang]

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]

Attachment: 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]

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]

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]

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]

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]

Comment on attachment 130867 [details] [diff] [review]
add PK11CertListUserUnique and PK11CertListAll options to PK11_ListCerts

r+ already given to later patches