337433 - Need CERT_FindCertByNicknameOrEmailAddrByUsage

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Kai Engert [:KaiE:]]

We need an exported API to search certs by usage and nickname/email at the same time.

This is required to support PKI infrastructures as described in bug 337430.

As of today, PSM uses a series of calls, trying to obtain the best email encryption certificate:
- CERT_FindCertByNicknameOrEmailAddr
- assume the result cert has the subject we want, but possibly incorrect usage
- CERT_CreateSubjectCertList
- CERT_FilterCertListByUsage

In the mentioned infrastructure, the above assumption is false.

In order to solve this issue, we could introduce a new exported function
  CERT_FindCertByNicknameOrEmailAddrByUsage
and make PSM's use it.

The new function is meant to be based on exsting CERT_FindCertByNicknameOrEmailAddr and take an additional usage parameter.
The new function should skip any certs that do no match the requested usage.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: Patch v1

This proposed implementation is based on the code of existing CERT_FindCertByNicknameOrEmailAddr.

A check was added to ensure the result of NSSCryptoContext_FindBestCertificate* really has the given usage.

This new implementation no longer calls PK11_FindCertFromNickname, but calls the multiple results function PK11_FindCertsFromNickname and filters the result list using CERT_FilterCertListByUsage.

Comment 2

[Kai Engert [:KaiE:]]

Let's try to get this into 3.11.2

Comment 3

[Wan-Teh Chang]

Comment on attachment 221626 [details] [diff] [review]
Patch v1

I suggest changing "ByUsage" to "AndUsage" or "ForUsage".
I hope we can shorten the function name but I can't think
of a better name.

Comment 4

[Julien Pierre]

I'm afraid there may be a bigger issue. Are you actually searching by nickname (ie. subject) or by email address ?

If you are looking for an email recipient cert, you may need to look in the subjectaltname extension, and not just in the subject. I don't think we have any NSS function that searches that. Our databases are not indexed by subjectaltname ...

Comment 5

[Robert Relyea]

He's actually searching by email addr.

I believe we added subject altname support for multiple email addresses for a single certificate when we went to cert8.

bob

Comment 6

[Kai Engert [:KaiE:]]

(In reply to comment #3)
> I suggest changing "ByUsage" to "AndUsage" or "ForUsage".

I like "ForUsage".

Comment 7

[Robert Relyea]

Comment on attachment 221626 [details] [diff] [review]
Patch v1

1. I agree with the need for the new function.

2. I prefer wtc's name suggestions.

3. What I would like to see is a common function for CERT_FindCertByNicknameOrEmail Addr*

Make a private function which takes an NSSUsage.

Implement CERT_FindCertByNicknameOrEmailAddr and CERT_FindCertByNicknameOrEmailAddrAndUsage
fill in the appropriate NSSUsage value and call the common function. This will limit the amount of duplicate code we have.

Other designs which combined the two functions into one are also acceptable.

bob

Comment 8

[Julien Pierre]

Retargetting all P2s to 3.11.3 .

Comment 9

[Kai Engert [:KaiE:]]

Attachment: Patch v2

Finally getting back to this bug.

Bob, I agree with your change requests:

- renamed the new function to end in "ForUsage"
- updated nss.def to add this function in 3.11.4
- converted the existing code into an internal, common function, and made both public APIs call it

Comment 10

[Robert Relyea]

Comment on attachment 240388 [details] [diff] [review]
Patch v2

r- for one bug: CERT_FilterCertListbyUsage is always filtering by certUsageEmailRecipient rather than the lookingForUsage. (r+ if you fix that.


To get this in 3.11.4, you will need a second review. Since you are adding a function, I suggest the second review before checking into the trunk (since that would affect how nss.def is set up).

bob

Comment 11

[Kai Engert [:KaiE:]]

Attachment: Patch v3

I'm marking this Patch v3 r=rrelyea - because I fixed the bug that Bob mentioned in the previous comment. Thanks a lot for catching it!

Comment 12

[Kai Engert [:KaiE:]]

Comment on attachment 240994 [details] [diff] [review]
Patch v3

Hi Julien, what do you think about adding this API to NSS 3.11? Could you please review? Thanks.

Comment 13

[Julien Pierre]

Comment on attachment 240994 [details] [diff] [review]
Patch v3

Kai,

I'm not sure about adding this one because there can still be multiple certs that match both the nickname (subject) and the usage criteria, but the API returns only one cert. How the final cert is further selected among all matches is arbitrary and is not specified.
I believe right now it will be selected by notAfter date. But there are potentially many other criteria that the application may want to consider and filter, such as key type. It should be given the opportunity.

If you change this API to return a cert list of all matches by subject and usage, instead of a single cert, I would be OK with adding this API.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Unsetting target milestone in unresolved bugs whose targets have passed.

Comment 15

[Kai Engert [:KaiE:]]

So, this bug got lost again 4 years after Julien's comment.
Summary:

Bob already gave r+, which should be sufficient for trunk checkin.

I once tried to get a second review from Julien, in order to add it to a stable branch.

Julien denied the review. He claimed that the API "is not perfect", because there might be multiple result candidates.

Today I'd say, NSS already has other functions that return a single result based on internal preference, even though there might be additional candidates.

I propose that the strive for perfection should not stop this new function. If there is desire to add an even better API, we can always do that later.


The patch no longer applies cleanly. I'll try to merge it to trunk.

Comment 16

[Kai Engert [:KaiE:]]

Attachment: Patch v3 - merged to current trunk

This still has r=relyea per comment 10.

Only difference from old patch, that was required for the merge: Changed the function signature to take "const char*" (not char*).

I'd like to add this to the NSS 3.12 branch for 3.12.8

Comment 17

[Kai Engert [:KaiE:]]

Bob didn't veto my proposals from comment 15, so I'll proceed to check it in.

Comment 18

[Kai Engert [:KaiE:]]

Checked in to NSS trunk for future NSS 3.13

Checking in lib/certdb/cert.h;
/cvsroot/mozilla/security/nss/lib/certdb/cert.h,v  <--  cert.h
new revision: 1.82; previous revision: 1.81
done
Checking in lib/certdb/stanpcertdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/stanpcertdb.c,v  <--  stanpcertdb.c
new revision: 1.85; previous revision: 1.84
done
Checking in lib/nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.208; previous revision: 1.207
done

Comment 19

[Kai Engert [:KaiE:]]

Checked in to NSS 3.12 branch for NSS 3.12.9

Checking in security/nss/lib/certdb/cert.h;
/cvsroot/mozilla/security/nss/lib/certdb/cert.h,v  <--  cert.h
new revision: 1.80.2.1; previous revision: 1.80
done
Checking in security/nss/lib/certdb/stanpcertdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/stanpcertdb.c,v  <--  stanpcertdb.c
new revision: 1.84.4.1; previous revision: 1.84
done
Checking in security/nss/lib/nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.206.2.1; previous revision: 1.206
done

Comment 20

[Kai Engert [:KaiE:]]

fixed