305697 - Softoken needs to give on the fly access to additional databases.

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Robert Relyea]

For some management applications, it's necessary to temporarily open more than
one database at a time. Softoken needs to present these databases as additional
removable slots.

Comment 1

[Robert Relyea]

Attachment: Patch for multiple slots.

This is just the softoken portion of the patch (an NSS portion which enables
new slots is forthcoming. It's a large patch. Nelson, if you would rather have
another reviewer feel free to reassign my sr request. I would like to have at
least a couple of reviewers on a patch this size.

What is in this patch:
1) The base support for adding more than one slot. Most of this support is in
pkcs11.c, in NSC_CreateObject, and some changes to the slot
initialization/shutdown. These changes, volume wise, were relatively minor and
sufficient to allow creating new slots, but to close new slots we need to do do
more...
2) First, it was necessary to handle the problem of the kdb locks. There are
two ways the locks could be handled 1) maintain a single kdb lock structure and
keep a count of the number of kdb's that were using it, only freeing the locks
when all the kdb's go away, or 2) biting the bullet and actually having
separate locks for each key db. This patch implements the latter and accounts
for most of the changes in keydb.c.
3) Second, I could now initialize and shutdown slots, but there were race
theoretic race conditions where it was possible that another thread could be
using the slots at the time we them down. It became necessary to add reference
counts to the certificate and key database handles (the rest of the data
structures already had safe ways to removing them from the token). This
accounts for most of the volume changes withing softoken. hitting changes in
pkcs11.c pcertdb.c dbinit.c pkcs11u.c

Hopefully knowing the 3 types of changes should help in any reviews.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Add reviewers to cc list

Comment 3

[Robert Relyea]

Attachment: Incorporate Juliens comments

The following comments from julien were not addressed:
1) return on CKA_TOKEN is false and fall through: The reason was a number of
variables only used on this code path are declared in the scope of the if(). At
some point a better strategy would be to have a separate function which writes
each token object, which will simplify this code. I'll leave to to a general
refactor of softoken.

SlotReint: password not set to NULL: I've verified that SlotInit sets it to
null and SlotShutdown clears it. It seems best not to blindly set it in
SlotReinit as it should already be cleared, and if it's not, do we need to free
the memory or not.

Locks to protect the password variable. This is an existing problem. I decided
that blindly added locks would be a bad thing without more investigation. It
would serialize all key references at a higher level. It may be OK (as the
keydb is serialized at the lower level anyway), but I didn't want to add this
kind of feature into a patch this big without having any performance analysis.
(particularly verifying that such a serialization would not hurt servers).

One final note. Between this patch and some freebl and FIPS changes have landed
so there may be some problems with the mozilla patch diff feature. I'll include
a 'hand' diff to give the jist of any changes.

Comment 4

[Robert Relyea]

OK, Mozilla gives some warnings, but the patch diff seems to give a true diff
result.

Comment 5

[Robert Relyea]

Comment on attachment 193641 [details] [diff] [review]
Patch for multiple slots.

clearing obsolete review requests

Comment 6

[Robert Relyea]

Attachment: pk11 wrapper patch- give a user callable interface to create new softoken modules.

Here's the application callable interfaces to add a new database slot.

The OpenDB takes a module spec which would look something like
"tokenDescryption='Firefox' configdir='/u/home/user1/Mozilla/users/dft23954/'
flags=readOnly". It will return a PK11SlotInfo structure.

The CloseDB the name of the token, and if it's a softoken 'removable' DB,
removes it. CloseDB will not fail if passed a SlotInfo structure for a
non-internal module, and will also fail if passed one of the permanent internal
slot (crypto or internal DB slot).

Comment 7

[Julien Pierre]

Comment on attachment 196839 [details] [diff] [review]
Incorporate Juliens comments

I already gave my feedback to Bob via email. There are very minor changes.

Comment 8

[Julien Pierre]

Comment on attachment 196876 [details] [diff] [review]
pk11 wrapper patch- give a user callable interface to create new softoken modules.

1) don't you want to destroy the PKCS#11 object after creating it in
secmod_UserDBOp ?
I know it is a special object used just to trigger a slot creation or deletion,
but still it seems like a basic PKCS#11 semantic . Shouldn't we preserve it ?
This comment might apply more to the softoken patch than this one, but it was
hard to get the big picture of the softoken patch without seeing the new
interface used ...

2) Since this patch is making SECMOD_OpenUserDB a public API, I would like to
see more comments about the detailed format of the module spec that needs to be
passed in, or at least a reference to it if it's already documented somewhere
else.

3) I find the prototype for SECMOD_CloseUserDB unintuitive. I think it should
not take a tokenName. Rather, it should go hand in hand with SECMOD_OpenUserDB
and take the object returned by the first one, in this case a PK11SlotInfo*.

The search by token name really isn't necessary, and can actually add errors.
As you know, the tokenName isn't guaranteed to be unique accross modules.

If you really, really must do a search by name, at least use
PK11_FindSlotsByNames, which allows searching for a token in a particular
shared library - so you will never find tokens of the same name in other
modules than the softoken. But I think this lookup should not be needed.

Comment 9

[Robert Relyea]

Attachment: Fix comment typos (Julien's comments).

Comment 10

[Robert Relyea]

> 1) don't you want to destroy the PKCS#11 object after creating it in
> secmod_UserDBOp ?
> I know it is a special object used just to trigger a slot creation or deletion,
> but still it seems like a basic PKCS#11 semantic . Shouldn't we preserve it ?
> This comment might apply more to the softoken patch than this one, but it was
> hard to get the big picture of the softoken patch without seeing the new
> interface used ...

No because the object doesn't really exist. The handle we will get back from the
softoken is an CK_INVALID_HANDLE.

> 2) Since this patch is making SECMOD_OpenUserDB a public API, I would like to
> see more comments about the detailed format of the module spec that needs to be
> passed in, or at least a reference to it if it's already documented somewhere
else.

I think it's rather too big for the code. The current document lives at
http://wiki.mozilla.org/PKCS11_Module_Specs currently (though it will probably
move to the developer wikki.

Comment 11

[Robert Relyea]

Attachment: Update from Julien's comments.

I went ahead and added the comment on what the module spec looks like in the
source file

Comment 12

[Robert Relyea]

Note, patch 197588 replaces pk11util.c and pk11pub.h in patch 196876. nss.def is
still from patch 196876.

Comment 13

[Robert Relyea]

Comment on attachment 196839 [details] [diff] [review]
Incorporate Juliens comments

remove request on obsolete bug.

Comment 14

[Robert Relyea]

Comment on attachment 197578 [details] [diff] [review]
Fix comment typos (Julien's comments).

remove request on obsolete bug.

Comment 15

[Robert Relyea]

Comment on attachment 197578 [details] [diff] [review]
Fix comment typos (Julien's comments).

wrong patch...

Comment 16

[Julien Pierre]

Comment on attachment 197588 [details] [diff] [review]
Update from Julien's comments.

Bob,

I meant to ask you to put the information/link about the modulespec in the
header file. This is so that callers of the API know what to pass in.

It doesn't hurt for this information to be in both the header and source,
though, so this patch is r+, but please add the info to the header.

Comment 17

[Robert Relyea]

Attachment: Problems with the previous patch

There are 2 problems with the previous multiple DB open patch:
1) If you open a database, close it, and open it again, the new database doesn't show up right away (fixed in pk11util.c).
2) If you remove a database, it's slot entry does not get properly removed from the hash (fixed in pkcs11u.c).
3) If you open up a database readonly, the slot does not show up read only (fixed in pkcs11.c).

bob

Comment 18

[Robert Relyea]

Make sure it shows up on the radar.

bob

Comment 19

[Robert Relyea]

Comment on attachment 203480 [details] [diff] [review]
Problems with the previous patch

OK, now I see why I wasn't getting any reviews from julien, the review wasn't targetted to him specifically.

Comment 20

[Robert Relyea]

I wanted this for 3.11, but it missed, so I'd like to target 3.11.1

bob

Comment 21

[Robert Relyea]

Comment on attachment 197588 [details] [diff] [review]
Update from Julien's comments.

patch was checked in to NSS 3.11

Comment 22

[Robert Relyea]

Arg, previous comment should read 'checked into NSS 3.12, it still needs another review to go into 3.11 ....