Support set nickname of certificates/private keys of imported pkcs#12 file before importing to NSS database

RESOLVED FIXED in 3.18

Status

NSS
Libraries
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: chucklee, Assigned: chucklee)

Tracking

trunk
3.18
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Per bug 1073330 comment 30, try to add or reuse API allowing set nickname of in-memory pkcs#12 certs/keys before importing.
Assignee: nobody → chuckli0706
Blocks: 1012549
An intuitive way of doing this is to make nickname collision callback of |SEC_PKCS12DecoderValidateBags()|[1] being called no matter there's a collision or not. But changing such behavior might break other application.

So I would add a new API:
typedef SECItem * (PR_CALLBACK * SEC_PKCS12NicknameUpdateCallback)(
                                  SECItem *old_nickname,
                                  PRBool *cancel,
                                  CERTCertificate *cert);
SEC_PKCS12DecoderUpdateCertNickname(SEC_PKCS12DecoderContext *p12dcx, SEC_PKCS12NicknameUpdateCallback nicknameCb)
which calls nicknameCb with every certificates in pkcs12 decoder.

As for the private key, it seems that it will be set to same nickname as corresponding certificate.
And most use cases(including mine in bug 1012549) are getting private key info by first get corresponding certificate by |CERT_FindCertByNickname()| then |PK11_FindKeyByAnyCert()|.
So I think we don't have to support change nickname of private key.

Hi Kay, can you provide some opinion?
[1] http://mxr.mozilla.org/security/source/security/nss/lib/pkcs12/p12d.c#2753
Flags: needinfo?(kaie)
I need a string as part of certificate nickname, so add a parameter to bring external data into callback function.
Also remove |old_nickname| and |cancel|, for it seems not necessary for changing certificate nickname, and cancel can be indicated by returning NULL if it doesn't want to update nickname

Proposal Update:

typedef SECItem * (PR_CALLBACK * SEC_PKCS12NicknameUpdateCallback)(
                                  CERTCertificate *cert,
                                  void *arg);
SEC_PKCS12DecoderUpdateCertNickname(SEC_PKCS12DecoderContext *p12dcx, SEC_PKCS12NicknameUpdateCallback nicknameCb, void *arg);
Created attachment 8541354 [details] [diff] [review]
(WIP) Support set certificate nickname in pkcs12 decoder context

Hi Kai,
This WIP patch is based on API proposed in comment 2, works fine in bug 1012549 with updated code.

[certutil -L]
Certificate Nickname                 Trust Attributes
                                     SSL,S/MIME,JAR/XPI

WIFI_USERCERT_test                   u,u,u
WIFI_SERVERCERT_test                 ,,

[certutil -K]
certutil: Checking token "NSS Certificate DB" in slot "NSS User Private Key and Certificate Services"
< 0> rsa      866e3630b07be49495e41b7064698ca64ad81256   WIFI_USERCERT_test
Attachment #8541354 - Flags: feedback?(kaie)
See Also: → bug 883074
Hi Kai,
  It's been a while, are you available to give some feedback for the patch, or should I ask other reviewer?

Comment 5

3 years ago
Sorry, for the delay, I'm on it now.
Flags: needinfo?(kaie)

Comment 6

3 years ago
A few initial comments:

(1)
You must export the new API in a .def file. It seems all the other SEC_PKCS12* are listed in lib/smime/smime.def
Please add a new section of the end of the file, with version 3.18


(2)
Please add comments in the header file. They should explain the parameters and the return types.

For the typedef, explain what the SECItem should contain.

(I saw the sec_pkcs12_set_nickname function expects the string in the secitem to be zero terminated, and it ignores the length element inside the secitem. Sigh.)

Comment that the secitem returned from the applications callsback function will be destroyed by the library.

Comment how the application callback should allocate the string inside the returned secitem (which allocation function).


(3)
(In reply to Chuck Lee from comment #1)
> As for the private key, it seems that it will be set to same nickname as
> corresponding certificate.

Are you sure it happens automatically? I'm not convinced yet. Looking at the code that implements the nickname collision handling in sec_pkcs12_validate_cert_nickname, the function will fail if the cert has a key assigned, but no key bag was given to the function. And the key nickname will later get updated by a call to sec_pkcs12_set_nickname_for_cert, which updates the nickname in both bags.

Unless I'm missing something, if you want the nickname of the corresponding key to be synchronized with the nickname of the cert, you'll need code to do that.

(Your code calls sec_pkcs12_set_nickname once for the cert, but the code in sec_pkcs12_set_nickname_for_cert calls sec_pkcs12_set_nickname for the key, too.)


(4)
I suggest to rename your API to SEC_PKCS12DecoderUpdateCertNicknames
(append "s") to make it clear that it will allows to update multiple/all nicknames.

Comment 7

3 years ago
Comment on attachment 8541354 [details] [diff] [review]
(WIP) Support set certificate nickname in pkcs12 decoder context

r- for the missing .def and API comments

(Also, separate from your next patch, feel free to separate attach a short subset of your application code, that shows how you call your new API, and the implementation of your callback function, it might simplify review.)
Attachment #8541354 - Flags: feedback?(kaie) → review-
Created attachment 8552265 [details] [diff] [review]
(Sample) Code using proposed API to import PKCS12 file
(In reply to Kai Engert (:kaie) from comment #6)
> (3)
> (In reply to Chuck Lee from comment #1)
> > As for the private key, it seems that it will be set to same nickname as
> > corresponding certificate.
> Are you sure it happens automatically? I'm not convinced yet. Looking at the
> code that implements the nickname collision handling in
> sec_pkcs12_validate_cert_nickname, the function will fail if the cert has a
> key assigned, but no key bag was given to the function. And the key nickname
> will later get updated by a call to sec_pkcs12_set_nickname_for_cert, which
> updates the nickname in both bags.
> 
> Unless I'm missing something, if you want the nickname of the corresponding
> key to be synchronized with the nickname of the cert, you'll need code to do
> that.
> 
> (Your code calls sec_pkcs12_set_nickname once for the cert, but the code in
> sec_pkcs12_set_nickname_for_cert calls sec_pkcs12_set_nickname for the key,
> too.)
> 

If the API is called before calling |PKCS12DecoderValidateBags()|, then the nickname of certificate will be set to corresponding key by |PKCS12DecoderValidateBags()|.

This is the limitation of my current design(as the patch in attachment 8552265 [details] [diff] [review]), or I have to copy most of the code from |sec_pkcs12_validate_bags()| [1] to find and update nickname of key for some certificate.

So the point here is if the limitation is acceptable, or the API have to work whenever being called before calling |SEC_PKCS12DecoderImportBags()|.

[1] https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/pkcs12/p12d.c#2665
Flags: needinfo?(kaie)

Comment 10

3 years ago
(In reply to Chuck Lee from comment #9)
> If the API is called before calling |PKCS12DecoderValidateBags()|, then the
> nickname of certificate will be set to corresponding key by
> |PKCS12DecoderValidateBags()|.

If you have tested it and that is sufficient for you, then it should be sufficient for us.

But I'd like to ask, please describe the situation in comments, please mention what order of function calls you suggest, to have the private key nicknames fixed later.
Flags: needinfo?(kaie)

Comment 11

3 years ago
Just an idea, because "update" is a very generic word, and the intention of your code is to rename, how about using the term "rename" in the function name?

Instead of SEC_PKCS12DecoderUpdateCertNickname
maybe call it SEC_PKCS12DecoderRenameCertNicknames ?

Comment 12

3 years ago
(In reply to Kai Engert (:kaie) from comment #6)
> For the typedef, explain what the SECItem should contain.
> 
> (I saw the sec_pkcs12_set_nickname function expects the string in the
> secitem to be zero terminated, and it ignores the length element inside the
> secitem. Sigh.)

FYI, I said that, because in other places where NSS uses SECItem, because it contains the separate length field, strings don't necessarily need to be zero terminated. It's a special requirement of this existing code, that zero termination is required. So it might save future users of your new code some headache, if you explicitly mention that requirement in the API description.
(In reply to Kai Engert (:kaie) from comment #11)
> Just an idea, because "update" is a very generic word, and the intention of
> your code is to rename, how about using the term "rename" in the function
> name?
> 
> Instead of SEC_PKCS12DecoderUpdateCertNickname
> maybe call it SEC_PKCS12DecoderRenameCertNicknames ?

This seems to be a much better name, thanks!
Also thanks for these comments with many details, I'll address them in next patch!
Created attachment 8553070 [details] [diff] [review]
Support set certificate nickname in pkcs12 decoder context.

Address reviewer's comments:
1. Rename API to PKCS12DecoderRenameCertNicknames
2. Add comments on how to use API and callback function in include file.

This patch has been tested and works as expected.
Attachment #8541354 - Attachment is obsolete: true
Attachment #8553070 - Flags: review?(kaie)
Hi Kai,
  Please help review the patch, thanks!
Flags: needinfo?(kaie)

Comment 16

3 years ago
Comment on attachment 8553070 [details] [diff] [review]
Support set certificate nickname in pkcs12 decoder context.

The .def file is incorrect, it fails to build for me on Linux. You used ;: in one place.

In the implementation of SEC_PKCS12DecoderRenameCertNicknames, you declare the variable outside the loop. You don't reset the value of the variable after you free it. You should either declare the variable inside the loop, or you should reset it to NULL after freeing it, to avoid any potential risk that it might get reused. (Yes, your code wouldn't reuse it, but I prefer safety.)

Although you don't require it in your scenario, I think it could be helpful in general to have access to the default nickname found in the p12 data, to use it as a starting point for new nicknames. I suggest to provide the default nickname in the callback API.

I think it would be nice to improve the comments a bit, to make it easier to understand the intention of the function.

You said in the comment, that the callback shouldn't change the parameters. An obvious way is to simply mark the parameters as const, and the compiler might even detect when future users of the API attempt to change them.

I'm attaching a modified version of your patch, with my changes applied.

I haven't yet tested your patch. Tomorrow I want to test if I can successfully modify p12util to use the API to rename the nicknames prior to import. If that succeeds, I will give r+. In the meantime, please check if my suggested changes make sense to you.
Flags: needinfo?(kaie)
Attachment #8553070 - Flags: review?(kaie) → review-

Comment 17

3 years ago
Created attachment 8568210 [details] [diff] [review]
suggested enhancements
Created attachment 8568523 [details] [diff] [review]
Support set certificate nickname in pkcs12 decoder context. V2

Adopt Kai's suggestion in API and comments, and change behavior to allow changing cert nickname by SEC_PKCS12DecoderRenameCertNicknames() if cert doesn't have default nickname.
This case already happened in my testing pkcs12 file and I think we should allow set nickname in such case.
Attachment #8553070 - Attachment is obsolete: true
Attachment #8568210 - Attachment is obsolete: true
Attachment #8568523 - Flags: review?(kaie)

Comment 19

3 years ago
I think it would be helpful, if the the callback code were able to signal a failure to the rename function. For example, in an out-of-memory condition, the rename function might fail to create the rename nickname, but the rename function won't notice that it should abort.

Therefore I propose we extend the API to include a SECStatus. Because functions usually return their status, we should move the result nickname to an output parameter.

Also, in the comments (all places) we should use NULL (not nullptr), because NULL is what NSS uses.

I suggest this updated callback API and comment:

/*
 * This callback is called by SEC_PKCS12DecoderRenameCertNicknames for each
 * certificate found in the p12 source data.
 *
 * cert: A decoded certificate.
 * default_nickname: The nickname as found in the source data.
 *                   Will be NULL if source data doesn't have nickname.
 * arg: The user data that was passed to SEC_PKCS12DecoderRenameCertNicknames.
 *
 * If the callback accepts that NSS will use a nickname based on the
 * default_nickname (potentially resolving conflicts), then the callback
 * must set *new_nickname to NULL.
 *
 * If the callback wishes to override the nickname, it must set *new_nickname
 * to a new SECItem which should be allocated using
 *     SECITEM_AllocItem(NULL, NULL, LENGTH_OF_NEW_NICKNAME + 1)
 * new_nickname->type should be set to siAsciiString, and new_nickname->data
 * must contain the new nickname as a zero terminated string.
 *
 * A return value of SECFailure indicates that the renaming operation failed,
 * otherwise the callback function must return SECSuccess.
 */
typedef SECStatus (PR_CALLBACK * SEC_PKCS12NicknameRenameCallback)(
                                 const CERTCertificate *cert,
                                 const SECItem *default_nickname,
                                 SECItem **new_nickname,
                                 void *arg);

Comment 20

3 years ago
Comment on attachment 8568523 [details] [diff] [review]
Support set certificate nickname in pkcs12 decoder context. V2

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

::: security/nss/lib/pkcs12/p12d.c
@@ +2811,5 @@
> +    }
> +
> +    for (i = 0; safeBag = p12dcx->safeBags[i]; i++) {
> +        SECItem *newNickname = NULL;
> +        SECItem *defaultNickname = NULL;

add
  SECStatus rename_rv;

@@ +2826,5 @@
> +            return SECFailure;
> +        }
> +
> +        defaultNickname = sec_pkcs12_get_nickname(safeBag);
> +        newNickname = (*nicknameCb)(cert, defaultNickname, arg);

change to
  rename_rv = (*nicknameCb)(cert, defaultNickname, &newNickname, arg);

@@ +2834,5 @@
> +        if (defaultNickname) {
> +            SECITEM_ZfreeItem(defaultNickname, PR_TRUE);
> +            defaultNickname = NULL;
> +        }
> +

add
        if (rename_rv != SECSuccess) {
            return rename_rv;
        }

Comment 21

3 years ago
Comment on attachment 8568523 [details] [diff] [review]
Support set certificate nickname in pkcs12 decoder context. V2

r- for the minor change I requested.

If you make the changes, then you have r=kaie
Attachment #8568523 - Flags: review?(kaie) → review-

Comment 22

3 years ago
Comment on attachment 8568523 [details] [diff] [review]
Support set certificate nickname in pkcs12 decoder context. V2

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

::: security/nss/lib/pkcs12/p12.h
@@ +53,5 @@
> + * certificate found in the p12 source data.
> + *
> + * cert: A decoded certificate.
> + * default_nickname: The nickname as found in the source data.
> + *                   Will be NULL if source data doesn't have nickname.

add
  new_nickname: Output parameter that may contain the renamed nickname
Created attachment 8569037 [details] [diff] [review]
Support set certificate nickname in pkcs12 decoder context. V3

Address reviewer's comment 19 to comment 22.
Attachment #8568523 - Attachment is obsolete: true
Attachment #8569037 - Flags: review?(kaie)

Comment 24

3 years ago
Comment on attachment 8569037 [details] [diff] [review]
Support set certificate nickname in pkcs12 decoder context. V3

Thank you for the changes, r=kaie

I'll check it in soon.

This sentence in the comment isn't clear to me:
 * Otherwise, the callback function must return SECSuccess, including use
 * default nickname as mentioned above.

What do you mean with "including use default nickname as above"?
Attachment #8569037 - Flags: review?(kaie) → review+

Comment 25

3 years ago
Checked in:
https://hg.mozilla.org/projects/nss/rev/ae29899e3619

We can do a follow-up to fix the comment, if necessary.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.18
(In reply to Kai Engert (:kaie) from comment #24)
> Comment on attachment 8569037 [details] [diff] [review]
> Support set certificate nickname in pkcs12 decoder context. V3
> 
> Thank you for the changes, r=kaie
> 
> I'll check it in soon.
> 
> This sentence in the comment isn't clear to me:
>  * Otherwise, the callback function must return SECSuccess, including use
>  * default nickname as mentioned above.
> 
> What do you mean with "including use default nickname as above"?

I try to point out that, the callback should return SECSuccess if it decides to use default_nickname and return NULL for *new_nickname.

Thanks for your help, Kai!

Updated

3 years ago
Blocks: 1137470
You need to log in before you can comment on or make changes to this bug.