Closed
Bug 1073330
Opened 10 years ago
Closed 9 years ago
Support set nickname of imported certificates
Categories
(NSS :: Libraries, defect)
NSS
Libraries
Tracking
(Not tracked)
RESOLVED
FIXED
3.18
People
(Reporter: chucklee, Assigned: chucklee)
References
Details
Attachments
(3 files, 3 obsolete files)
2.67 KB,
patch
|
KaiE
:
review-
|
Details | Diff | Splinter Review |
1.21 KB,
patch
|
Details | Diff | Splinter Review | |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8495754 -
Flags: review?(rrelyea)
Assignee | ||
Updated•10 years ago
|
Attachment #8495754 -
Flags: review?(rrelyea) → review?(ryan.sleevi)
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
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-
Assignee | ||
Comment 4•10 years ago
|
||
Many thanks, Robert!
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
(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 9•10 years ago
|
||
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+
Comment 10•10 years ago
|
||
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
Assignee | ||
Comment 11•10 years ago
|
||
(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!
Assignee | ||
Comment 12•10 years ago
|
||
Fix comment style per comment 10.
Attachment #8500219 -
Attachment is obsolete: true
Assignee | ||
Comment 13•10 years ago
|
||
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
Assignee | ||
Comment 14•10 years ago
|
||
Try looks good : https://tbpl.mozilla.org/?tree=Try&rev=0e07fd0991e7
Keywords: checkin-needed
Assignee | ||
Comment 15•10 years ago
|
||
Hi Kai, Wan-Teh, Please help to check in the patch, thanks!
Flags: needinfo?(wtc)
Flags: needinfo?(kaie)
Assignee | ||
Comment 16•10 years ago
|
||
This patch is needed for bug 1012549, please also help to land it on m-c, thanks.
Comment 17•10 years ago
|
||
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-
Comment 18•10 years ago
|
||
previous patch, function exported with version 3.18
Attachment #8503859 -
Attachment is obsolete: true
Attachment #8508213 -
Flags: review+
Comment 19•10 years ago
|
||
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
Updated•10 years ago
|
Attachment #8503859 -
Attachment is obsolete: false
Attachment #8503859 -
Flags: checked-in+
Updated•10 years ago
|
Keywords: checkin-needed
Updated•10 years ago
|
Attachment #8508213 -
Attachment is obsolete: true
Comment 20•10 years ago
|
||
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+
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(wtc)
Resolution: --- → FIXED
Target Milestone: --- → 3.18
Assignee | ||
Comment 21•10 years ago
|
||
Kai, sorry that I didn't notice this, and thanks for your help!
Comment 22•9 years ago
|
||
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 → ---
Comment 23•9 years ago
|
||
I've backed out: https://hg.mozilla.org/projects/nss/rev/a08de3bd0a61 https://hg.mozilla.org/projects/nss/rev/01c19c751a74
Assignee | ||
Comment 24•9 years ago
|
||
(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
Comment 25•9 years ago
|
||
(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.
Comment 26•9 years ago
|
||
Which P12 import APIs did you try? (Just so we know which one we should enhance.)
Updated•9 years ago
|
Attachment #8508225 -
Flags: review?(rrelyea)
Updated•9 years ago
|
Attachment #8503859 -
Flags: checked-in+
Updated•9 years ago
|
Attachment #8508225 -
Flags: checked-in+
Assignee | ||
Comment 27•9 years ago
|
||
(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()
Comment 28•9 years ago
|
||
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.
Assignee | ||
Comment 29•9 years ago
|
||
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?
Comment 30•9 years ago
|
||
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.
Assignee | ||
Comment 31•9 years ago
|
||
I am willing to implement that, might need many feedbacks from NSS developers though. I've opened bug 1113453 for it.
Comment 32•9 years ago
|
||
I believe this is now unnecessary for the purposes of Chuck and FFOS, because we have completed bug 1113453.
Comment 33•9 years ago
|
||
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)
Assignee | ||
Comment 34•9 years ago
|
||
I am willing to help if this API is still needed!
Comment 35•9 years ago
|
||
I'm fine with the _ and the warning. bob
Comment 36•9 years ago
|
||
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)
Comment 37•9 years ago
|
||
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+
Comment 38•9 years ago
|
||
https://hg.mozilla.org/projects/nss/rev/629281c2e7ba
Status: REOPENED → RESOLVED
Closed: 10 years ago → 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•