Closed Bug 1073330 Opened 10 years ago Closed 9 years ago

Support set nickname of imported certificates

Categories

(NSS :: Libraries, defect)

Product:
NSS
Component:
Libraries
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: chucklee, Assigned: chucklee)

References

Details

Attachments

(3 files, 3 obsolete files)

Support set nickname of imported certificates. V3. r=rrelyea
2.67 KB, patch
KaiE
: review-
Details | Diff | Splinter Review
incremental patch to fix version of new exported symbol
1.21 KB, patch
Details | Diff | Splinter Review
v5
3.10 KB, patch
KaiE
: review+
Details | Diff | Splinter Review 
I need to change nickname of imported certificates to certain prefix to support EAP-TLS for Firefox OS, so I like to suggest add support to set nickname of imported certificates.
Also, NSS already supports set nickname of private keys, I think it's OK to support change certificates' nickname.
Attachment #8495754 - Flags: review?(rrelyea) → review?(ryan.sleevi)
Comment on attachment 8495754 [details] [diff] [review]
Support set nickname of imported certificates.

I don't think this is right, but Bob is the one you should get to review this. Please don't ship reviews. Reach out to Bob if you're not hearing from him.
Attachment #8495754 - Flags: review?(ryan.sleevi) → review?(rrelyea)
Comment on attachment 8495754 [details] [diff] [review]
Support set nickname of imported certificates.

Review of attachment 8495754 [details] [diff] [review]:
-----------------------------------------------------------------

The only thing issue is that you should check to see if slot and id is actually set, if the cert is a temp cert, they may not be. If you do the check you'll get an r+

 (Sorry about the delay, I had reviewed this change a while ago and I guess I forgot to push publish).
Attachment #8495754 - Flags: review?(rrelyea) → review-
Address comment 3.
Attachment #8495754 - Attachment is obsolete: true
Attachment #8500219 - Flags: review?(rrelyea)
Chuck, would PK11_SetPublicKeyNickname and/or PK11_SetPrivateKeyNickname do what you want here?
Flags: needinfo?(chulee)
(In reply to David Keeler (:keeler) [use needinfo?] from comment #6)
> Chuck, would PK11_SetPublicKeyNickname and/or PK11_SetPrivateKeyNickname do
> what you want here?

David, do you mean to create a dummy key structure with slot and pkcs11ID of certificate, and change nickname by these function?
It do work, but does it acceptable?
Flags: needinfo?(chulee) → needinfo?(dkeeler)
Yeah, actually, I guess there's a difference between a certificate's nickname and a key's nickname, so maybe this won't work. Just a thought.
Flags: needinfo?(dkeeler)
Comment on attachment 8500219 [details] [diff] [review]
Support set nickname of imported certificates. V2

r+ this is a prefectly reasonable addition. Thanks for adding the check.

bob
Attachment #8500219 - Flags: review?(rrelyea) → review+
oops, chuck I missed the fact that you used a C++ comment. I think we still have platforms that won't accept C++ comments in .c files
(In reply to Robert Relyea from comment #10)
> oops, chuck I missed the fact that you used a C++ comment. I think we still
> have platforms that won't accept C++ comments in .c files

Oops, I didn't notice that, fix right away and thanks!
I don't know what test cases should be run while changing NSS.
so I use the normal command I use: build on all platform and run test on Firefox OS emulator.
https://tbpl.mozilla.org/?tree=Try&rev=0e07fd0991e7
Hi Kai, Wan-Teh,
  Please help to check in the patch, thanks!
Flags: needinfo?(wtc)
Flags: needinfo?(kaie)
This patch is needed for bug 1012549, please also help to land it on m-c, thanks.
Comment on attachment 8503859 [details] [diff] [review]
Support set nickname of imported certificates. V3. r=rrelyea

Sorry, I found an issue with this patch.

You're adding a new API, but you're incorrectly adding it to the section of a past version number.

It's required to add a new section for the new version number this will appear in.

Since this is a trivial change, I'll fix it for you.
Flags: needinfo?(kaie)
Attachment #8503859 - Flags: review-
Attached patch patch v4 (obsolete) — — Splinter Review 
previous patch, function exported with version 3.18
Attachment #8503859 - Attachment is obsolete: true
Attachment #8508213 - Flags: review+
Comment on attachment 8503859 [details] [diff] [review]
Support set nickname of imported certificates. V3. r=rrelyea

I had accidentally pushed this revision already.
https://hg.mozilla.org/projects/nss/rev/575919285e2b
Attachment #8503859 - Attachment is obsolete: false
Attachment #8503859 - Flags: checked-in+
Keywords: checkin-needed
Attachment #8508213 - Attachment is obsolete: true
Instead of backing out the previous revision, I've checked in a fix:
https://hg.mozilla.org/projects/nss/rev/7f496fc97e75

This attachment is what has been checked in.

Bob, can you please r+ after the fact?
Attachment #8508225 - Flags: review?(rrelyea)
Attachment #8508225 - Flags: checked-in+
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(wtc)
Resolution: --- → FIXED
Target Milestone: --- → 3.18
Kai, sorry that I didn't notice this, and thanks for your help!
I'm reopening this bug, as the implementation seems incomplete. Thanks to Wan-Teh for making us aware of this issue.

The nickname attribute of the CERTCertificate data structure doesn't get updated by the new API, but it probably should.

Also, after calling the new API, using functions that search for a certificate by nickname:
- the certificate can still be found when searching for the old nickname
- the certificate cannot be found when searching for the new nickname
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Kai Engert (:kaie) from comment #22)
> I'm reopening this bug, as the implementation seems incomplete. Thanks to
> Wan-Teh for making us aware of this issue.
> 
> The nickname attribute of the CERTCertificate data structure doesn't get
> updated by the new API, but it probably should.
> 
> Also, after calling the new API, using functions that search for a
> certificate by nickname:
> - the certificate can still be found when searching for the old nickname
> - the certificate cannot be found when searching for the new nickname

If changing CERTCertificate.nickname is the lost piece, I will look into how to do that correctly.
But I had a different test result here: I can find certificate by new nickname in keystore[1].(Didn't test if I can find cert by old nickname, though.)
And I can see the nickname of cert is being updated using 'certutil -L'.

Also, I would like to find the possibility of setting nickname while importing PKCS12 instead of change it afterward, per bug 1012549 comment 114.
I previously tried but couldn't make it happen using PKCS12 import API.
The only way I could come up with is using nickname collision callback, but it's only called for user certificates(as I can remember). I have to change the nickname of imported CA certificate, too.
It will be a great help to have discussion and get feedbacks from NSS guys!

[1] http://dxr.mozilla.org/mozilla-central/source/ipc/keystore/KeyStore.cpp#251
(In reply to Chuck Lee [:chucklee, OOO forever, will response to NI, Review, and Feedback requests] from comment #24)
> If changing CERTCertificate.nickname is the lost piece, I will look into how
> to do that correctly.

The difficulty is doing it safely in a multi-threaded process. The right locks would be required.


> But I had a different test result here: I can find certificate by new
> nickname in keystore[1].(Didn't test if I can find cert by old nickname,
> though.)
> And I can see the nickname of cert is being updated using 'certutil -L'.

What you tested is updating of the permanent store on disk.
If your test used a separate run of certutil, then you were reading from disk.

The issue is with the in-memory structures in the alive process after calling the API, which aren't updated and therefore is in an inconsistent state after the change, using the current patch.


> Also, I would like to find the possibility of setting nickname while
> importing PKCS12 instead of change it afterward, per bug 1012549 comment 114.
> I previously tried but couldn't make it happen using PKCS12 import API.
> The only way I could come up with is using nickname collision callback, but
> it's only called for user certificates(as I can remember). I have to change
> the nickname of imported CA certificate, too.
> It will be a great help to have discussion and get feedbacks from NSS guys!

Ok, we'll look into that. It might be easier for us (to make it safe) to implement an enhanced API that allows to set the desired nickname at import time.
Which P12 import APIs did you try? (Just so we know which one we should enhance.)
Attachment #8508225 - Flags: review?(rrelyea)
Attachment #8503859 - Flags: checked-in+
Attachment #8508225 - Flags: checked-in+
(In reply to Kai Engert (:kaie) from comment #26)
> Which P12 import APIs did you try? (Just so we know which one we should
> enhance.)

SEC_PKCS12DecoderIterateNext() and SEC_PKCS12DecoderImportBags(), in following way:

1. Use SEC_PKCS12DecoderIterateNext() to go through all imported bags right after calling SEC_PKCS12DecoderVerify(), before these bags are not yet being imported. Then import certs/keys to NSS on my own so I can set nickname of certs.
   I can set nickname for certificates in this method, but the problem is　decoder iterator doesn't expose private key bags, for security reason I think. So I can't import private key and relate it with user certificate, unless I modify NSS to expose safe bags for private key which is not likely possible.
   The patch for this method is [1].

2. Use SEC_PKCS12DecoderImportBags() to import certs/keys first, then use SEC_PKCS12DecoderIterateNext() the find just-imported-certs and change its nickname.
   This leads to the NSS API requirement for change certificate name.
   This is the method current patch uses [2].

[1] https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1012549&attachment=8469795, WifiCertService.cpp, ImportPkcs12Blob()
[2] https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1012549&attachment=8514811, WifiCertService.cpp, ImportPkcs12Blob()
I would like to suggest that we enhance the operations that are possible on a SEC_PKCS12DecoderContext object.

I suggest we make it possible to iterate over the elements in the SEC_PKCS12DecoderContext and allow to read and override all the nicknames that are stored in the memory structure.

With this approach, at the time you'd call SEC_PKCS12DecoderImportBags, your desired nicknames would be used for the initial import, and there isn't any need to rename.
Thanks, the concepts sounds good to me!

I have to assign nickname based on if it's a CA cert or a user cert, now determined by value of |CERTCertificate.isRoot| and |CERTCertificate.nsCertType|.
Can these information also be provided in the iterator?
There has been some additional discussion amongst NSS developers.

Just in case it wasn't clear yet, we have agreement that we recommend against using the rename nickname API in a long running process, because of the confusing effects. We might implement it anyway, to enable tools like certutil to support renaming of nicknames in persistent storage, but the API would be clearly document to advise against using it in other circumstances.

This means, we're looking for someone who's willing to implement the alternative proposal, modify the in-memory p12 data structures, after loading from p12 file, prior to importing it into the database.
I am willing to implement that, might need many feedbacks from NSS developers though.
I've opened bug 1113453 for it.
I believe this is now unnecessary for the purposes of Chuck and FFOS, because we have completed bug 1113453.
No longer blocks: 1012549
Chuck, if you wanted to help us get this into NSS, 
  not for Firefox OS, but for the purposes of certutil as Bob asked for, 

then I suggest to add a scary comment to obsolete patch v4 (will need merging, because there were other changes in nss.def. Please keep functions in the 3.18 section sorted alphabetically.)

I suggest to add the comment above BOTH the header and the implementation, to ensure that nobody will ever use it accidentally.

Ryan, Bob, would you prefer if we renamed this function to have a leading underscore, signaling it's for NSS internal use, and not for use by applications?

Or will it be sufficient to use a scary sounding comment?

For example:

/*
 * Using this API is *DANGEROUS*.
 *
 * The API will update the NSS database, but it *will NOT* update the in-memory data.
 * As a result, after calling this API, there will be INCONSISTENCY between
 * in-memory data and the database.
 *
 * Use of the API should be limited to short-lived tools, which will exit immediately
 * after using this API.
 *
 * If you ignore this warning, your process is TAINTED and will most likely misbehave.
 */
Flags: needinfo?(ryan.sleevi)
I am willing to help if this API is still needed!
I'm fine with the _ and the warning.

bob
Thanks Bob.

We already have mixed r+ from you and from me, the only issue left was to add the comment, define the recommend usage, and now the agreement to rename to include leading _.

These are trivial and obvious changes, and the patch is very small.

For the sake of simplicity, I will attach the slightly modified patch and check it in, with author being Chuck, and r=rrelyea.

I'll use your comment 35 as a r=rrelyea on the changes.
Flags: needinfo?(ryan.sleevi)
Attached patch v5 — — Splinter Review 
This has a mix of r=kaie and r=rrelyea

I used a prefix of two underscores __ because that's what all the other APIs in nss.def use.
Attachment #8569982 - Flags: review+
https://hg.mozilla.org/projects/nss/rev/629281c2e7ba
Status: REOPENED → RESOLVED
Closed: 10 years ago → 9 years ago
Resolution: --- → FIXED
Blocks: 1142209
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: