One of the new issues with Shared databases is the need to not just update old databases to new ones (this is already done in the current code), but to also merge several old databases into one. Applications need a tools to help with this update.

This patch hits lots of areas to accomplish the needed task. The issues are: 1) we may need 2 passwords in the merge case (source database and target database). 2) we need to detect if we have already merged this database. The actual database merge is pretty straight forward, collecting all the information from the application and the user is where most of the complication lie.

Bob, I think you need to provide some overview here of the new "helper".

es, I do. The design for the update is documented here: http://wiki.mozilla.org/NSS_Shared_DB#Database%20Upgrade In that document, Modes 1 and 2 have already been covered. Type B applications are not relevent. So this patch implements update for Mode 3 Type A applications. To recap, Mode 3 Type A applications are applications which currently do not share any keys or certs, but want to share those keys and certs with applications that also currently have private keys and certs. That is the application is changing the directory that the NSS databases live in to a common directory, and the application already has legacy NSS databases that are private and whose data must be preservered. Most servers today have 'private' NSS databases which they share with several components. Those are Mode 2 applications and are already handled by the update case. If you wanted to take two existing servers that each have a private shared database and merge them to a single directory, then they would be Mode 3. This patch includes changes to certutil to manage an upgrade externally, so in fact these changes plus the changes to certutil would be sufficient for a user to coerce a server to move to a common shared database without any changes to that server...

How to use the code in this patch: Appropriately certutil is the first file in the patch, so you can see how the update would work. Please refer to the state diagram in the design document.. First the application opens NSS with the new NSS_InitWithMerge(). A new init function is needed in order to accept two new parameters:Update dir and Update ID. (and optionally updateTokenName). Update dir is the directory where the old database we are updating from. Throught the rest of the patch that old database is called the updateDB or the sourceDB. Update ID is a unique identifier specifying that directory. It should contain, at the very least, The name of the application ("Mozilla:Firefox" or "Sun:JavaES") it should also contain some application specifier that tells which instance it is as well (for mozilla apps this is probably the salt value used for the profile, for servers it could be some unique ID associated with the server instance). This string is used by NSS to determine if the given database has already been merged into the target instance. Update token is a localized string passed in by the application to describe the update or source DB to the user. If this string is not supplied, update ID will be used. Applications need to support automatic update of old databases will call NSS_InitWithMerge() everytime at startup. Once NSS is inited, the application can determine if an Update is needed by examining the database slot. If the database slot is removable then an Update is required*. *I'm open to added a special attribute to check for to signal this rather than overloading the 'removable' attribute. The removable attribute would still be set in order for the appropriate change of tokenname to be triggered, however. - PK11_Authenticate() - get the first password The application can prompt the user to say that and update is needed. In the certutil --merge case it always assumes and update is needed. The application can first call PK11_Authenticate(). If the source database had a password, NSS will call the applications 'get password' callback with the token name 'Update token'. Failure to provide this generates Exception A in the database design. The application can handle Exception A in any manner it wishes. Certutil handles it by aborted the update and returning. If the source database does not have a password, but the target does, NSS will call the application's 'get password' callback with the standard token name for the application. In this case the Failure to provide the password will generate Exception B. If the application needs to handle Exception B differently from Exception A, it can do so by checking the token name. In the exection A case the token name and the Update token would be the same. On success, the databases are then updated, and the token is logged in. If neither database has a password, the PK11_Authenticate call will complete with no prompts. At this point the databases are already updated. If the source database password fetch is successful, NSS will automatically 'remove and reinsert' the database slot with a new token. This token name has the normal token name associated with the application. NSS then checks to see if the target database has a null password, or a password that is identical to the source databases password. If either of these cases are true, NSS updates the database, Otherwise NSS will leave the token in a logged out state. So then end of the PK11_Authenticate(), applications handle Exception A and Exception B in the failure case and continue on the success case. -PK11_IsLoggedIn() - have we updated appropriately? On success of an update either because: 1) the source database had no password, 2) the target database had no password or 3) both databases had the same password, the application can detect this by calling PK11_IsLoggedIn() after a successful PK11_Authenticate(). If the token is logged in, the database update has completed successfully. Certutil exits with success. If the token is not logged in, this means that we still need to fetch the target database. Calling PK11_IsPresent() causes NSS to synch states appropriately (after the PK11_IsLoggedIn()), This updates the tokename for the next PK11_Authenticate() call. - PK11_Authenticate() - get the second password Now we need to get the password for the target DB. Calling PK11_Authenticate() will again cause NSS to prompt for the password using the application's password callback. This time the token name will be the normal token name for the application. Failure to get the password here generates an Exception B (always). Success means the databases are upgraded. - Other semantic notes: If the target database does not exist, then that is handled the same as if it had no password. If the update does not complete, the updateID is never written to the target database and new attempts to start the app (assuming it uses a consistant updateID) will again try to update the database until it does succeed. If the application chooses to continue after and Exception A, it will run using the old database, logged out, until such time as the user does present the password for that old database. At that time it will stay logged out, switch the token to a new name and continue to run until a new password is supplied. If the application chooses to continue after an Exception B, it will run using the old database, logged out, waiting for a new password to be supplied. If the application does not use NSS_MergeInit, none of this state machine stuff is triggered. If the shared DB exists, it will open it. If it doesn't and an old database exists, then it will do a type 2 update. If NSS_MergeInit is called without specifying a shared database as the target, none of this update code gets triggered.

the patch certutil: add the --merge option (which also requires --updateDir and --updateID). I did not add a --updateTokenName option. This patch is a pretty good example of how an application would implement a type 3 update. This patch could also be used to do offline/manual update for applications that don't want to change their NSS calls, and which don't make sense to do an automatic update for. nss.h/nssinit.c: Add the NSS_MergeInit() call which passes the new update parameters down to softoken. pk11sdr.c: This patch allows us to have multiple sdr keys in the same database and find them appropriately. Currently all sdr keys have the same CKA_ID. This means 2 keys will collide when we copy them to the same database. This patch allows sdr_Decrypt to find the correct key even if the CKA_ID is different than the one the application thought it should be. lgglue.c: When updating from 2 databases, the low level key for decrypt for the source database may be different from our target. This code allows us to read the entries in the old database as we format them for storage in the new. pkcs11.c: This code manages the token name switch depending on the state of our update. It also parses and passed the new update parameters down to the database code. Finally it makes sftk_CloseAllSessions callable by the database code (so it can signal a token removal). sdb.c: We now write meta data into the cert db (data which says whether or not the database has been updated). sftkdb.c: The primary change is implementing the update with merge. I've also put the entire update under a single transaction so we can back out the whole thing on error. This function manages the merge of 2 databases. The merge rules for most objects are: if the object exists, don't update it. There are 2 exceptions: Trust objects and SDR keys. For the latter we always update the SDR keys, but we may change the CKA_ID of the key so it doesn't colide or overwrite the old key. For Trust objects we merge the trust object according the the following rules: 1) Each trust attribute is evaluated separately. 2) If the trusts values are identical, nothing is done. 3) If one database has a value for that trust attribute and the other is either non-existant or unknown, the value wins. 4) The algorithm defines 'hard' and 'soft' trust values. 'hard' values are values that would stop a trust processing (either with a yes or no). These include CKT_NSS_TRUSTED, CKT_NSS_TRUSTED_DELEGATOR, CKT_NSS_NOT_TRUSTED. 'hard' values have preference over soft values. 5) If the databases have conflicting trust values (both hard or both soft and not equal), the target databases version of those values win. Currently the only place where these merge semantics are documented is here and in the code, though the design document talks about some options for trust merging. sftkdb.c also contains the changes to the Init code that determines whether or not we need to update and kicks off the update in those cases where we already have enough information to do so. sftkdb.h: define the helper functions that allow the rest of softoken to know our position in the update state machine. sftkpars.c: extract out the new update parameters. sftkpwd.c: implement the helper state machine helper functions. Collect the various passwords and update the state machine appropriately. When all the needed data is available, call the update function in sftkdb.c. Be sure to read the extensive comment blocks in the change password code to guide you through the different states.

comment 3 should read 'Yes I do' not 'es I do'

Comment on attachment 285308 [details] [diff] [review] monster patch to handle the issues of merging databases. Finally marking this for review.

Comment on attachment 285308 [details] [diff] [review] monster patch to handle the issues of merging databases. Bob, I didn't get very far into this review before noticing that the code to merge a second database's contents in has been structured as a flavor of NSS_Init. The implies that at most one merger can be done in the lifetime of an initialized NSS library. I question that design choice. Why limit it to only one merger per initialization? Why not allow any number of databases to be merged in while the database is open read/write? I understand that in the case of UPGRADE, wherein we are creating a new DB from an old one, where the new DB did not exist, it is reasonable to do that during initialization. Softoken needs a DB, so let it create one, when none exists, at startup. But the case of merging in contents from another database seems different in nature. It is merely the act of extracting records from a second database and adding them to the DB already in use. So, why tie it to initialization?

There are a number of issues here: The primary complication, which is probably 90% of the description of this bug, is in order to merge 2 databases you need the password for both. You can't just copy the encrypted records from one database to the other, since those records can't just be copied. In the Upgrade case, the world is a lot easier. There is only one valid database at a time (the old database if the new one doesn't exist, or the new one if it does). Applications can run against the old database until the upgrade is complete, so they can run until the user logs in in his normal course of operation. In the Merge case we have more issues. First, running against the old database means you can't see the common certs yet. If we open the shared db, we don't see the application's old certs and keys. Second, there are now 2 passwords you need, the password for the old database, and the password for the new one. It's possible that those passwords are distinct, and you can't merge until you have both. Finally the Merge case has an additional issue and deciding if it should try to merge. In the upgrade case we make the decision based on the existence or not of the new database. In the Merge case, the new database already exists, and lives in a different location from the old database. We need the following information from the application in order to complete the merge: 1) the location of the old databases, 2) some unique value that identifies these databases (so we don't try to upgrade them more than once). NSS needs this information as initialization time, or the application won't have access to it's traditional set of keys & certs. That is why it bubbles up to init. As to why one per initializations: 1) That's the 90% case. Most NSS apps have one database. When going to a new shared NSS database, they only have one database to upgrade. They need access to that old database until they merge it in. 2) Those apps that have more than one database must have special code in order to enable those databases already. That code is in the form of a SECMOD_OpenUserDB(). SECMOD_OpenUserDB contains a char* parameter to specify the database to open. That parameter can specify the same information we use in NSS_InitMerge() to tell softoken we (potentially) want to merge one database into another, so for those applications, there is no restriction "one per init", they can upgrade all of their databases. bob

Bob, based on your comment 9, I gather (for the first time) that "merge" really means "upgrade and merge"; that is, apparently you're assuming that the database whose contents are to be merged into the current DB is an "old" database (e.g. cert8 or older). That begs a question: will this code be able to merge the contents of one cert9 database into another cert9 database? I think that is a requirement. I think it would be helpful to use different nomenclature. I suspect that using the terms "old" and "new" to describe the two inputs to the merge operation has colored the design. I suppose there is standard terminology for this, but I don't know what it is, so I will suggest "source" and "target" to mean, respectively, the DB from which records will be copied and the DB into which they will be copied. It seems to me that the merger operation ought to take place between two cert9 DBs (let's not call all cert9 DBs "shared DBs" since most of them will NOT be shared). The merge operation seems to me to logically consist of initializing two tokens, each with a separate database, and then reading objects from one and writing them to the other, wrapping and unwrapping objects where necessary. Of course, objects that cannot be extracted, not even when wrapped, present an issue for that. But perhaps that merely dictates which DB is the source and which is the target. It seems to me that the merge can be done either a) outside of the tokens, in pk11wrap, using the standard PKCS#11 API to talk to the two tokens whose contents are being merged, or else b) inside of one super-token that can open two sets of DBs simultaneously. Let's call those choices the "outside" and "inside" choices. The inside choice seems to require a lot of non-standard extension to the PKCS#11 API, possible with greatly increased customizations to the string passed in during initialization. The outside choice seems much more straightforward and simpler to implement. It doesn't seem to require any further customization or embellishment of the PKCS#11 API, and it might potentially work with third-party tokens (and modules) as well as with NSS's own. It isn't so much a DB merge as a token contents merge, and that (it seems to me) is how it should be. Finally, the problem of needing multiple separate passwords seems to have the same issues whether the merge happens during init or later. I see no advantage (with respect to passwords) to doing the merge in init. Indeed, doing the merger after init potentially mitigates confusion about which database's password is being requested. How does merging in init help with the passwords?

Here are my design review comments. I will work on a true code review next.

Comment on attachment 285308 [details] [diff] [review] monster patch to handle the issues of merging databases. In light of the design review, this patch gets an r-. But I will give it a code review. Btw, the comments I attached include documentation of the steps through which an application goes to perform the merge and upgrade. Think of it as a first draft of how-to documentation for application developers.

Comment on attachment 285308 [details] [diff] [review] monster patch to handle the issues of merging databases. Code review comments. I've looked at the patch to all files except one, softoken/sftkdb.c. I'll review that separately. >Index: cmd/certutil/certutil.c >+ FPS "\t%s --merge --updateDir oldDB --updateID uniqueID -d certdir\n", >+ progName); > FPS "\t\t [-f pwfile] [-X] [-d certdir] [-P dbprefix]\n"); > FPS "\t%s -L [-n cert-name] [-X] [-d certdir] [-P dbprefix] [-r] [-a]\n", > FPS "\t%s -M -n cert-name -t trustargs [-d certdir] [-P dbprefix]\n", certutil has two usage message functions, a "short" one (hah!) and a long one. This patch added to the short one, but not to the long one. The long one needs to be updated, too. >+ /* Merge needs a update database and a update id. */ >+ if (certutil.commands[cmd_Merge].activated && >+ !(certutil.options[opt_UpdateDir].activated && >+ certutil.options[opt_UpdateID].activated)) { >+ >+printf("opt_UpdateDir = %d (%s) opt_UpdateID = %d (%s)\n", >+ certutil.options[opt_UpdateDir].activated, >+ certutil.options[opt_UpdateDir].arg, >+ certutil.options[opt_UpdateID].activated, >+ certutil.options[opt_UpdateID].arg); That seems like a debug printf that isn't intended to be left in. >- moduleSpec = PR_smprintf("name=\"%s\" parameters=\"configdir='%s' certPrefix='%s' keyPrefix='%s' secmod='%s' flags=%s %s\" NSS=\"flags=internal,moduleDB,moduleDBOnly,critical\"", >+ moduleSpec = PR_smprintf("name=\"%s\" parameters=\"configdir='%s' certPrefix='%s' keyPrefix='%s' secmod='%s' flags=%s updatedir=%s updateid=%s updateTokenDescription=%s %s\" NSS=\"flags=internal,moduleDB,moduleDBOnly,critical\"", That string really needs to be broken up into multiple concatenated strings, e.g. something like this: >+ moduleSpec = PR_smprintf("name=\"%s\" parameters=\"configdir='%s' " >+ "certPrefix='%s' keyPrefix='%s' secmod='%s' flags=%s updatedir=%s " >+ "updateid=%s updateTokenDescription=%s %s\" " >+ "NSS=\"flags=internal,moduleDB,moduleDBOnly,critical\"", >+ /* >+ * handle the case where your key indicies may have been broken This comment should tell, why this is necessary, and HOW it will handle the case. >+ /* free the list */ >+ for (testKey = keyList; testKey; testKey = nextKey) { >+ nextKey = PK11_GetNextSymKey(testKey); >+ PK11_FreeSymKey(testKey); >+ } Isn't there a function to do that? >Index: lib/softoken/pkcs11.c > static char * >-sftk_setStringName(const char *inString, char *buffer, int buffer_length) >+sftk_setStringName(const char *inString, char *buffer, int buffer_length, PRBool nullTerminate) That line got joined. Please fold it. >-static CK_RV sft_CloseAllSession(SFTKSlot *slot) >+CK_RV sftk_CloseAllSessions(SFTKSlot *slot) Oh yeah, we discussed this in the design review.. This seems like a layering violation. I'm not adamant about it, but I'd prefer if this somewhat high level function (within softoken) didn't get called from way down inside DB code. >Index: lib/softoken/sftkdb.c >@@ -1538,18 +2117,29 @@ sftk_DBInit(const char *configdir, const > if (crv2 == CKR_OK) { > if (*certDB) { > (*certDB)->update = updateCert; >+ (*certDB)->updateID = updateID && *updateID >+ ? PORT_Strdup(updateID) : NULL; > updateCert->app_private = (*certDB); > } > if (*keyDB) { > (*keyDB)->update = updateKey; >+ (*keyDB)->updateID = updateID && *updateID ? >+ PORT_Strdup(updateID) : NULL; If either of the above two PORT_Strdup calls fails, we don't detect it, and treat it as if the input argument (updateID) passed in was NULL or the string was empty. That seems wrong. Also, while not strictly necesssary for correctness, please put parentheses around "updateID && *updateID" in both places. >@@ -621,27 +729,133 @@ sftkdb_CheckPassword(SFTKDBHandle *keydb In the patch to this function we see a Lock call, that appears to be unmatched with an unlock call. The unlock call exists, but is MUCH MUCH later, in an else case after a huge block of code. >+ PZ_Lock(keydb->passwordLock); >+ if (sftkdb_NeedUpdateDBPassword(keydb)) { ... >+ keydb->updatePasswordKey = SECITEM_DupItem(&key); >+ PZ_Unlock(keydb->passwordLock); ... ... about 100 lines later ... ... >+ } else { >+ PZ_Unlock(keydb->passwordLock); >+ } The lock is not held during that huge block of code, but appears to be release afterwards. I suggest you restructure that code, using a local flag variable, so you can release the lock before the huge block of code, e.g. something like: PRBool doUpdate = PR_FALSE; ... >+ PZ_Lock(keydb->passwordLock); >+ if (sftkdb_NeedUpdateDBPassword(keydb)) { ... >+ keydb->updatePasswordKey = SECITEM_DupItem(&key); doUpdate = PR_TRUE; } >+ PZ_Unlock(keydb->passwordLock); if (doUpdate) { ... about 100 lines here ... }

Bob, as you know, I'm convinced that we need the ability to merge one cert9 DB into another. I'd really rather NOT have the method for doing that be extremely different from the method for merging a cert8 into a cert9. So, how huge a deal is it to suggest that this merger code now under review be expanded to be able to open a source DB that is a cert9.db?

Design lives at: http://wiki.mozilla.org/NSS_Shared_DB Large sections were adapted from the 'Extensive design review comments' patch attached to this bug. bob

Comment on attachment 301994 [details] Review request on design document changes. >http://wiki.mozilla.org/NSS_Shared_DB I reviewed this document, and then did some editing on it to fix formatting problems and do a little wordsmithing. I think this is in pretty fair shape now. There are a few parts about which I have questions. I think we should review them together by phone as we did before. Here are thoughts on a couple of areas that I think need more work. This draft proposes one new function not found in previous drafts, namely PK11_MergeTokens. When merging two cert9.db files, it appears to depend on the use of the function SECMOD_OpenUserDB, which (despite being over 2 years old in the tree) appears to have no documenation, no sample code, and no callers anywhere in NSS sources. So, it's clear to me that the documentation on PK11_MergeTokens needs to be accompanied by documentation and example code for SECMOD_OpenUserDB. Whatever test program is written or extended to test PK11_MergeTokens must also test SECMOD_OpenUserDB, IMO. There's a section entitled "Merge Conflicts (Mode 3A only)" which discusses a set of 4 "choices" for how to resolve merge conflicts, and the consequences of those choices. But there does not seem to be any way for the application program (or programmer) to make or express those choices. There's no function (or I missed it) by which the programmer can tell NSS "I chose choice # 3" (say). I thought there might be a callback API by which the merge code could call back to the application and tell it "Here's a conflict. Make a choice for this conflict." But I don't see any such callback definition. So, I gather that this entire section is "thinking out loud" about a choice that the implementer of this DB merge code (you, Bob :) had to make. I gather that you saw these as four alternatives from which you had to choose. And I wonder: which alternative did you choose? The document should say that, and perhaps drop the others or list them in a section about unchosen alternatives. There are some sections in this document that seem to be thinking out loud about how Mozilla applications might choose to deploy this capability. They are named "Mozilla Applications" and "Profile Issues". Since those sections are not of interest to the general population of application programmers whose applications use NSS, I suggest that they be moved into a separate document.

Comment on attachment 301994 [details] Review request on design document changes. Oh, one more thought about the new function PK11_MergeTokens. It has a single argument named "pwdata" which is documented to be a "password arg". As soon as I read that, I wondered: "for which slot?". Maybe there need to be TWO pwarg-ish arguments to this function, one for each slot.

Thanks for your review nelson! > When merging two cert9.db files, it appears to depend > on the use of the function SECMOD_OpenUserDB, which (despite being over 2 > years old in the tree) appears to have no documenation, no sample code, > and no callers anywhere in NSS sources. Documentation is available at: http://developer.mozilla.org/en/docs/NSS_PKCS11_Functions#SECMOD_OpenUserDB It's one of the few functions that were documented on the wiki about the time it was created. You are correct about it not having any samples in the NSS tree yet. That should be fixed in the implementation patch for the design (as you suggest). > There's a section entitled "Merge Conflicts (Mode 3A only)" which discusses > a set of 4 "choices" for how to resolve merge conflicts, and the > consequences of those choices. yes, I do need to flesh out those choices, and doing so may (ok will) point out some missing API's that need to be addressed. > I thought there might be a callback API by > which the merge code could call back to the application and tell it > "Here's a conflict. Make a choice for this conflict." I think we talked about this before. It's not a callback. it's an error at a specific point in the code. In the sample code it's handled by the hand wave function: handle_failure_to_get_old_DB_Password(); or handle_failure_to_get_new_DB_Password(); But currently there is no documentation on how to implement, say, choice 3. Currently the choice is up to the application about what to do. The sample code effectively fails to start. If the error is ignored, the application will start up with the old database, but won't be able to do much since it doesn't have a password. > Oh, one more thought about the new function PK11_MergeTokens. > It has a single argument named "pwdata" which is documented to be > a "password arg". As soon as I read that, I wondered: "for which slot?". > Maybe there need to be TWO pwarg-ish arguments to this function, > one for each slot. we could, but it would be totally unique among NSS API's to do so. Many NSS apis will operation on some unknown slot, so the notion that the pwarg is different for each slot is not something that NSS really supports. It happens to work in the case of an application that carefully authenticates to all the tokens it plans to use before hand. (pwarg is sometimes called wincx, since it's typically a window context). That being said Merge is already unique in that it's the only NSS API function I know of that explicitly takes 2 different slots as arguments (though some may 'implicitly' do so.... take a slot and a key which points to a different slot for instance). Adding a second pw arg would not be at all difficult in this case.

Bob, Your 4 choices are what to do about merge conflicts. Merge conflicts cannot happen until after both DB passwords have been obtained (if needed). So, your statement that > It's not a callback. it's an error at a specific point in the code. > In the sample code it's handled by the hand wave function: > handle_failure_to_get_old_DB_Password(); > or > handle_failure_to_get_new_DB_Password(); Seems to be discussing an error that would occur before any merger would begin. I don't see how password failure functions can handle merge conflicts.

Oh, merge conflicts.... Thanks for specifying that. Yes you are correct. I was thinking out loud on how we would resolve the merge conflicts, not how the application would. You are correct. I need to pick one. I'll update the document appropriately. bob

Bob, regarding the issue of one or two pwArg arguments to PK11_MergeTokens, You observe that if it had two such arguments, it would be unique among NSS API functions in that respect. But I submit that it is already unique among NSS API functions in that it explicitly addresses two tokens in a single API call. I know of no other NSS API call that explicitly addresses two tokens in a single function. So, given that this function is already unique in this respect, I think it is not odd or unexpected that it would provide a separate pwArg argument for each of the two explicitly identified tokens on which it operates.

Thanks nelson, I did meantion it's the only one that explicity references 2 slots. I'll include a patch that has both args. bob

I'd need to read more about the proposal, but adding PK11_MergeTokens to the NSS API as apparently you intend to do makes me worried. I hope applications that have made the transition to the new db format since a long time will not have to support the cruft of PK11_MergeTokens forever, even if they never call it. I think the best is to put PK11_MergeTokens in a static library and make it use only the public NSS API, so that those who don't need it don't have to carry it.

This patch includes all the changes I've made as a result of the review. I'll also include 2 other patches which is this patch broken in 2: patch 1: only those files that were included in the original patch. patch 2: changes to files not included in the original patch. the hope would be patch 1 would patch up with the previous monster patch for interdiff to work... hopefully preserving some of the previous work Nelson did in review the first patch.

Review Helper.... Most of comment 5 is still applicable. diffs between monster and subpatch 1: Global: 1) Adding the Prefix to the update DB case is the major change. Most files without comments were changes because of this. 2) Adding and exporting the PK11_IsPerm function to detect when update-merge is active. certutil: 1) renamed the parameters from hard to type names like --updateID to the more friendly --update-id. 2) updated to match the current design spec. 3) make --merge use the generic new PK11_MergeTokens function. the old merge is called --update-merge and is provided primarily as sample code and to for NSS testing purposes. 4) --merge returns an error log, so certutil now tries to print a 'somewhat' friendly output when some items fail to merge. nss.def: besides PK11_IsPerm, also added PK11_MergeTokens and functions to create and free PK11MergeLogs. lgglue.: fixed a bug where the decrypt function wasn't working properly for 'public' objects (i.e. public keys). sdb.c: Added code so the find will correctly match NULL entries. sftkdb.c: This has the most extensive changes. Mostly it was code clean up, and a little refactoring. Functionally: 1) improved the trust object code. 2) added code to detect multiple creation of objects that we only want 'one' of (the old db did this implicitly by overwriting objects automatically). 3) handled the case where a cert may already exist, but not have a label or CKA_ID when the source database does.

subpatch 2 has the following: 1) adding test cases: crlutil - changed so it will return an error if the requested crl does not exist. This allows me to use crlutil in my testcase to detect that a crl has been properly merged. sdrtest - allows the user to pass a password to sdrtest so it can be used on a database with a password. all.sh - added a new variable 'TEST_MODE' so we can do mode specific things in test case (in the case of merge.sh, it runs --update-merge instead --merge in the UPDATE_DB case). merge/merge.sh - merge several existing NSS databases and verify that they merged correctly. pk11wrap/pk11slot.c - added PK11_IsPerm. pk11wrap/* added PK11_MergeTokens. This function can merge date from one token to another, Tokens can be smart cards, sql db's, dbm db's, or a mix. Not guarrenteed to work on some tokens (where keys are not extractable) does work on tokens with sensitive keys.

Comment on attachment 306655 [details] [diff] [review] New full monster patch. nelson, I'm putting the review request on this combined patch, since that is what will be checked in. See notes in comment 5, comment 24, comment 27, and comment 28. bob

Comment on attachment 306655 [details] [diff] [review] New full monster patch. This patch is too big to review in one sitting. So, here are the results from my first day of reviewing it. There will be more review comments to follow. Let's discuss these. They may not be automatic r- issues. 1. certutil: I think this patch intended to insert some new lines into the short usage message, and not remove any existing lines, but it removes one existing line, the second line documenting the -K command. That appears to be a mistake. 2. certutil: Should that new option be --upgrade-merge instead of --update-merge? 3. in sdrtest, shouldn't -f and -p be mutually exclusive? Usage should say "[-f pwfile | -p pwd]" instead of "[-f pwfile] [-p pwd]". If both -f and -p are given, the first of those strings is leaked. The strdup'd string in pwdata.data appears to always be leaked. 4. I am not convinced that the patch to PK11_ImportPublicKey results in correct and expected behavior in all 8 cases where we're trying to import the key in some fashion (object or session) and the key already exists in some fashion (object or session) is some slot (same or different). I'm not sure what the right/expected outcome in all those 8 cases, so it is difficult to determine if this patch is correct or not. I think we need a table that specifies the expected activity and outcome for each of those 8 cases. I think this patch is trying to correct the behavior in one or more cases, but I suspect this patch makes one case wrong that was right, and doesn't fix at least one other case that seems wrong. I suspect it may be changing the behavior in at least one case that it was not intended to change. This patch changes the behavior of PK11_ImportPublicKey for three of those 8 cases, the cases where the SECKEYPublicKey is already a known token object in some slot (the "old" slot), and we're trying to import the key as either (a) a new token object in a different slot than the old slot, or (a) a new session object in a different slot than the old slot, or (c) a new token object in the same old slot. In all of those cases, the old code would have destroyed the old token object before trying to import the key. With this patch, it will NOT destroy the old token object, but will go ahead and try to import it (again). In case (c), this might fail, or might create a duplicate object. It seems to me that in case (c), we should neither destroy the old object nor try to import it a second time into the same slot. We should merely report success (it's already done) or failure (would create a duplicate). There is at least one other existing case, unchanged by this patch, where the behavior of the existing code seems wrong to me. Consider this case: - The key is already a token object in some slot, and we're trying to import it as a session object in that same slot. We merely do nothing and return success. The caller thinks he new has a handle to a newly created session object, when actually he has the handle to a pre-existing token object. If the caller then attemtps to destroy the session object, he will actually destroy the pre-existing token object. Wouldn't it be better to go ahead and create a new session object for this key in that slot, and return the new handle? --------- 5. Questions about the change to PK11_MakeIDFromPubKey. - Are CKA_IDs expected to remain the same for token objects from one run to the next? (I believe the answer is yes.) - Is it legitimate for an application to remember an object by its CKA_ID from one run to the next, and look it up in a subsequent run by the CKA_ID that it had in a previous run? (??) If the answers to those questions are "yes", then the proposed change to PK11_MakeIDFromPubKey would break programs that rely on that characteristic. I think the new code is more efficient. Too bad we didn't think of it earlier. --------- 6. This patch proposes to return CKA_OK without doing anything when the caller is trying to set (change) the value of CKA_ID or CKA_SUBJECT attributes on public key objects, or the CKA_LABEL on any object. Surely this is going to lead to bugs being filed, bugs that say "your token said it made the change, but it did not." --------- 7. Let's make the reserved fields be last in the new PK11MergeLog structures. --------- 8. The new function PK11_IsPerm is not declared in any header file, and is used undeclared in certutil. I think this will cause a compilation error on some platforms. New function PK11_IsPerm returns a result for a slot. What is a "Perm" slot? </me scratches head> I've heard of perm certs, but not perm slots. Does it mean "is capable of storing new token objects"? If so, how do we determine this for modules other than softoken? Maybe it needs a better name.

> 1. certutil: I think this patch intended to insert some new lines into > the short usage message, and not remove any existing lines, but it > removes one existing line, the second line documenting the -K command. > That appears to be a mistake. It is a mistake. > 2. certutil: Should that new option be --upgrade-merge instead of > --update-merge? I would be happy to make that change (also accept better names of --update-dir, etc... (update-id and update-token-name would probably want to change for consistancy...). > 3. in sdrtest, shouldn't -f and -p be mutually exclusive? > Usage should say "[-f pwfile | -p pwd]" > instead of "[-f pwfile] [-p pwd]". > If both -f and -p are given, the first of those strings is leaked. > The strdup'd string in pwdata.data appears to always be leaked. I can make those changes > 7. Let's make the reserved fields be last in the new PK11MergeLog structures. OK > 8. The new function PK11_IsPerm is not declared in any header file, and is > used undeclared in certutil. I think this will cause a compilation error > on some platforms. Hmm it should be. If not this is a bug. > New function PK11_IsPerm returns a result for a slot. > What is a "Perm" slot? One that isn't removable.

(In reply to comment #31) > > New function PK11_IsPerm returns a result for a slot. > > What is a "Perm" slot? > > One that isn't removable. That definitely needs to be documented via a comment in the code somewhere. I would never have guessed that particular meaning. Maybe it should be "isRemovable" instead. That is more self-documenting.

> 5. Questions about the change to PK11_MakeIDFromPubKey. > - Are CKA_IDs expected to remain the same for token objects from one run > to the next? (I believe the answer is yes.) They typically are, but it's not a requirement. On some tokens the CKA_ID > - Is it legitimate for an application to remember an object by its CKA_ID > from one run to the next, and look it up in a subsequent run by the CKA_ID > that it had in a previous run? (??) Not reliably... Importing a key on some tokens would cause all the CKA_ID's to change (in theory). The softoken doesn't change it's CKA_ID values however. > If the answers to those questions are "yes", then the proposed change to > PK11_MakeIDFromPubKey would break programs that rely on that characteristic. > I think the new code is more efficient. > Too bad we didn't think of it earlier. Actually, even if those two cases where 'yes', PK11_MakeIDFromPubKey()'s change would have not influence on the above result. The change I made only affect and application if it put a previously invalid input to PK11_MakeIDFromPubKey, that is something that wasn't a public key. The purpose of PK11_MakeIDFromPubKey is to generate an NSS CKA_ID value, mostly so that a bare key can be matched up with it's cert. This function is only called when importing or exporting keys or certs. Usually applications find a CKA_ID value by querying for it. Those applications that use PK11_MakeIDFromPubKey directly will only work if they passed in an actual public key (otherwise they will not be able to find any NSS generated public/private keys, now would NSS be able to match those keys with the appropriate certificate). What making this change allows it the ability to set those/Find those CKA_ID's directly. So the only binary incompatiblity issue that could arise is an application passed an actual CKA_ID to PK11_MakeIDFromPubKey, or a function that used it, would actually start working where before it had not.

> 6. This patch proposes to return CKA_OK without doing anything when the > caller is trying to set (change) the value of CKA_ID or CKA_SUBJECT > attributes on public key objects, or the CKA_LABEL on any object. > > Surely this is going to lead to bugs being filed, bugs that say > "your token said it made the change, but it did not." It already does it for private keys. The old DBM softoken has some foibles which means it can never be completely PKCS #11 compliant. It can't actually set the CKA_ID and CKA_SUBJECT, but if it rejects them, you will get spurious errors. In dbm public keys are not actually stored, but are implied by the existence of private keys. It's things like this that was fixed in the new shared database design. Anyway those bugs would be answered with -- used shared DB instead then.;). bob

> 4. I am not convinced that the patch to PK11_ImportPublicKey results in > correct and expected behavior in all 8 cases where we're trying to import > the key in some fashion (object or session) and the key already exists in > some fashion (object or session) is some slot (same or different). > I'm not sure what the right/expected outcome in all those 8 cases, so it > is difficult to determine if this patch is correct or not. I think we need > a table that specifies the expected activity and outcome for each of those > 8 cases. So there are really only three cases, (not 8). The SECKEYPublicKey does not point to an existing key (token or session). That's the most common case and already handled with the slot-> structure. The SECKEYPublicKey points to a session key. In this case the SECKEYPublicKey 'owns' that session key and is responsible for managing it. Since we are about to overwrite that public handle, we need to delete the underlying session key (no matter what token it exists in) because no one else will. The SECKEYPublicKey points to a token key. In that case we can simply forget the token key. We do not want the side effect of losing a token key as a result of doing an Import. The token the keys live in and the tokens you are importing into is irrelevant. For Session keys the SECKEYPublicKey structure was the only one who knew about that key, so deleting it is fine. In general it's always safe to drop out the imported session key from SECKEYPublicKey structures (as long as you delete the underlying object). If you have a token key that you are trying to import into the same token, then you will either get a duplicate or an import error (depending on the semantics of the token -- softoken would produce an error). There should be nothing to prevent the user from creating multiple copies of a public key in tokens that can support it. The input type is also irrelevant. No matter what the input type is, you still need to delete existing session objects (because no one else will) and you can't delete token objects. bob

> 8. The new function PK11_IsPerm is not declared in any header file, and is > used undeclared in certutil. I think this will cause a compilation error > on some platforms. Actually it is on Linux. PK11_IsPerm is in subpatch 2 pk11pub.h. > That definitely needs to be documented via a comment in the code somewhere. > I would never have guessed that particular meaning. > Maybe it should be "isRemovable" instead. That is more self-documenting. I can do that, though it means the external API won't match the internal 'isPerm' usage. On the other hand the exernal API is what programmers use...

Bob, your comment 35 confirms my observation in comment 30, that with this patch, if the caller tries to import a public key as a token key into a slot where it is already a token key, the function will attempt to create a duplicate key, and (with softoken) will fail. My question is: Is that the intended, desired, and expected behavior? Or should the function simply return Success without doing anything?

> My question is: Is that the intended, desired, and expected behavior? > Or should the function simply return Success without doing anything? In my mind yes. It's really a bit of a pathelogical case. If the application is making this call either the application is unaware what it is asking for, or it is purposefully trying to duplicate the public key. In the both cases returning an error if you can't duplicate the public key is the right thing to do. It also mirrors the case where the application is trying to import an public key that matches an existing token key. (you get either a duplicate or an error depending on the semantics of the underlying function). It is certainly friendlier than the current case where the existing token public key is deleted and may or may not be recreated. bob

This monster patch handles nelson's issues 1, 2, 3, 5, 7, 8. It should interdiff with "New full monster patch" I'm leaving subpatch 1 and subpatch 2 here for review aid since Nelson is still working on the review. Note on issue 3. There are still leaks in the error cases where the command line arguements were not acceptable. Also there is a leak in the FILE case that's caused by the way the password prompting code caches the password from the file. This leak exists in all of our tools, however. So fixing it would be beyond the scope of this already large patch:). bob

Comment on attachment 307155 [details] [diff] [review] V2: New full monster patch. Bob and I completed the review of this patch over the phone. There's a comment to add, and some ifdef'ed code to remove. There may also be one still-unresolved issue from a previous review, where some very low level code is calling into a high level function in softoken, a layering violation. Bob, Please fix those cosmetic things mentioned above and check this in. Then if you find that layering violation, submit a separate patch for that. That way I won't have to review another monster patch. Thanks.

Checking in cmd/certutil/certutil.c; /cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v <-- certutil.c new revision: 1.135; previous revision: 1.134 done Checking in cmd/crlutil/crlutil.c; /cvsroot/mozilla/security/nss/cmd/crlutil/crlutil.c,v <-- crlutil.c new revision: 1.30; previous revision: 1.29 done Checking in cmd/sdrtest/sdrtest.c; /cvsroot/mozilla/security/nss/cmd/sdrtest/sdrtest.c,v <-- sdrtest.c new revision: 1.14; previous revision: 1.13 done Checking in lib/nss/nss.def; /cvsroot/mozilla/security/nss/lib/nss/nss.def,v <-- nss.def new revision: 1.189; previous revision: 1.188 done Checking in lib/nss/nss.h; /cvsroot/mozilla/security/nss/lib/nss/nss.h,v <-- nss.h new revision: 1.52; previous revision: 1.51 done Checking in lib/nss/nssinit.c; /cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v <-- nssinit.c new revision: 1.92; previous revision: 1.91 done Checking in lib/pk11wrap/manifest.mn; /cvsroot/mozilla/security/nss/lib/pk11wrap/manifest.mn,v <-- manifest.mn new revision: 1.20; previous revision: 1.19 done Checking in lib/pk11wrap/pk11akey.c; /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11akey.c,v <-- pk11akey.c new revision: 1.22; previous revision: 1.21 done RCS file: /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11merge.c,v done Checking in lib/pk11wrap/pk11merge.c; /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11merge.c,v <-- pk11merge.c initial revision: 1.1 done Checking in lib/pk11wrap/pk11pub.h; /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pub.h,v <-- pk11pub.h new revision: 1.21; previous revision: 1.20 done Checking in lib/pk11wrap/pk11sdr.c; /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11sdr.c,v <-- pk11sdr.c new revision: 1.18; previous revision: 1.17 done Checking in lib/pk11wrap/pk11slot.c; /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v <-- pk11slot.c new revision: 1.92; previous revision: 1.91 done Checking in lib/pk11wrap/secmodi.h; /cvsroot/mozilla/security/nss/lib/pk11wrap/secmodi.h,v <-- secmodi.h new revision: 1.30; previous revision: 1.29 done Checking in lib/pk11wrap/secmodt.h; /cvsroot/mozilla/security/nss/lib/pk11wrap/secmodt.h,v <-- secmodt.h new revision: 1.37; previous revision: 1.36 done Checking in lib/softoken/lgglue.c; /cvsroot/mozilla/security/nss/lib/softoken/lgglue.c,v <-- lgglue.c new revision: 1.10; previous revision: 1.9 done Checking in lib/softoken/pkcs11.c; /cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v <-- pkcs11.c new revision: 1.148; previous revision: 1.147 done Checking in lib/softoken/pkcs11i.h; /cvsroot/mozilla/security/nss/lib/softoken/pkcs11i.h,v <-- pkcs11i.h new revision: 1.49; previous revision: 1.48 done Checking in lib/softoken/sdb.c; /cvsroot/mozilla/security/nss/lib/softoken/sdb.c,v <-- sdb.c new revision: 1.3.2.3; previous revision: 1.3.2.2 done Checking in lib/softoken/sftkdb.c; /cvsroot/mozilla/security/nss/lib/softoken/sftkdb.c,v <-- sftkdb.c new revision: 1.10; previous revision: 1.9 done Checking in lib/softoken/sftkdb.h; /cvsroot/mozilla/security/nss/lib/softoken/sftkdb.h,v <-- sftkdb.h new revision: 1.4; previous revision: 1.3 done Checking in lib/softoken/sftkdbti.h; /cvsroot/mozilla/security/nss/lib/softoken/sftkdbti.h,v <-- sftkdbti.h new revision: 1.3; previous revision: 1.2 done Checking in lib/softoken/sftkpars.c; /cvsroot/mozilla/security/nss/lib/softoken/sftkpars.c,v <-- sftkpars.c new revision: 1.4; previous revision: 1.3 done Checking in lib/softoken/sftkpwd.c; /cvsroot/mozilla/security/nss/lib/softoken/sftkpwd.c,v <-- sftkpwd.c new revision: 1.4; previous revision: 1.3 done Checking in lib/softoken/legacydb/lgattr.c; /cvsroot/mozilla/security/nss/lib/softoken/legacydb/lgattr.c,v <-- lgattr.c new revision: 1.5; previous revision: 1.4 done Checking in tests/all.sh; /cvsroot/mozilla/security/nss/tests/all.sh,v <-- all.sh new revision: 1.42; previous revision: 1.41 done RCS file: /cvsroot/mozilla/security/nss/tests/merge/merge.sh,v done Checking in tests/merge/merge.sh; /cvsroot/mozilla/security/nss/tests/merge/merge.sh,v <-- merge.sh initial revision: 1.1 done

This is the patch as checked int. I varies from V2 in the following areas: Addresses the review comments from Nelson (removed #ifdef notdef coded, update comment for PK11_ImportPublicKey), and merged with the current tip.

Here's the patch for the final comment from nelson -- layering issue.

Comment on attachment 308499 [details] [diff] [review] Fix tinderbox bustage... r=kengert Do you plan to reenable the merge test eventually?

Fixing the memory leak turned up a double free in FIPS mode. This patch corrects the double free.

Comment on attachment 308510 [details] [diff] [review] Preemptive correction to previous patch. r=kengert

re comment 45. Yes, I found the problem was my changes to sdb.c were checked into a branch instead of the tip. I'll check it in as soon as the tree is happily stably green again (probably tomorrow morning). bob

Comment on attachment 308476 [details] [diff] [review] Fix layering issue good. r=nelson thanks

Comment on attachment 308724 [details] [diff] [review] Fix AIX build issue oops wrong patch.

Comment on attachment 308726 [details] [diff] [review] Fix AIX build issue r=wtc.

Bob, I see two problems with recent patches related to this bug: 1. Since merge script was added to all.sh, Tinderboxes are failing: merge.sh: Decrypt - With Merged SDR Key sdrtest -d . -i /export/tinderbox/SunOS_5.10/mozilla/tests_results/security/harpsichord.1/pkix/tests.v1.11529 -t Test1 -f ../tests.pw.11529 merge.sh: #1696: Decrypt - Value 1 - FAILED This need to be fixed ASAP (fix the main reason of failure, or remove merge script from all.sh until problem is fixed), we shouldn't let Tinderbox failing. 2. We decided to use absolute path for all used tools (see bug 341584), merge.sh doesn't use this convention, so please change it to use construction ${BINDIR}/certutil instead of certutil only (for all used tools, not only for certutil).

It took some time to find reproduce this bug on my machine. Still running tests to verify that this is the problem that's plaguing tinderbox.

This patch does the following: 1) fixes the padding bug that caused the intermittent failures on the 'standard' builds. 2) fixes a bug in merge.sh to allow standalone testing of --upgrade-merge (The script used the lack of setting of NSS_DEFAULT_DB_TYPE to indicate a --upgrade-merge rather than a --merge from either dbm or sql db types. Between the time the script case created and the latest build environment, NSS_DEFAULT_DB_TYPE was set explicitly if not already set, by one the the init scripts.) 3) in dbupdate.sh, pre-require SDR keys. This is what was causing the failure of the update DB case. In my testing I ran all 4 tests at once, which guarrenteed SDR keys were created in the dbm database. When upgrade DB is ran singly (as in tinderbox), No sdr key is created in the dbm database, so there is not key to upgrade in the --upgrade-merge case. Forcing sdr to run before hand fixes this problem. 4) tools are now accessed with the ${BINDIR} variable. 5) with 1 and 3 fixed it is safe to turn the merge test back on. I'll wait until alexi fixes the pkix failure before checking in.