Created attachment 1422911 [details] test files, SQL + DBM NSSDB Description of problem: Import into a DBM and SQL NSS DBs behave differently. With DBM, certutil -A doesn't change the nick of an existing certificate. With SQL format, an existing cert/key pair is renamed when the cert is imported a second time. FreeIPA's CA replica installer relies on the fact that the nick isn't changed when a CA cert is imported a second time. Version-Release number of selected component (if applicable): nss-3.36.0-1.0.fc28.x86_64 nss-3.36.0-1.0.fc27.x86_64 How reproducible: always Steps to Reproduce: 1. create SQL NSSDB 2. import p12 file with pk12util 3. import same public cert with certutil Actual results: The nickname of the cert/key pair is changed Expected results: Second import with certutil shouldn't change the nick Additional info: $ mkdir dbm $ certutil -d dbm:dbm -f pwdfile.txt -N $ pk12util -i cert.p12 -k pwdfile.txt -w pwdfile.txt -d dbm:dbm pk12util: PKCS12 IMPORT SUCCESSFUL $ certutil -d dbm:dbm -f pwdfile.txt -L Certificate Nickname Trust Attributes SSL,S/MIME,JAR/XPI my-ca-cert u,u,u $ certutil -d dbm:dbm -f pwdfile.txt -K certutil: Checking token "NSS Certificate DB" in slot "NSS User Private Key and Certificate Services" < 0> rsa acdf483f38fd167fe29565ab767b7a912d93aeae my-ca-cert $ certutil -d dbm:dbm -f pwdfile.txt -A -n ca-pem -t ,, -i cert.pem $ certutil -d dbm:dbm -f pwdfile.txt -L Certificate Nickname Trust Attributes SSL,S/MIME,JAR/XPI my-ca-cert u,u,u ^^^^^^^^^^ NICK STAYS THE SAME $ mkdir sql $ certutil -d sql:sql -f pwdfile.txt -N $ pk12util -i cert.p12 -k pwdfile.txt -w pwdfile.txt -d sql:sql pk12util: PKCS12 IMPORT SUCCESSFUL $ certutil -d sql:sql -f pwdfile.txt -L Certificate Nickname Trust Attributes SSL,S/MIME,JAR/XPI my-ca-cert u,u,u $ certutil -d sql:sql -f pwdfile.txt -K certutil: Checking token "NSS Certificate DB" in slot "NSS User Private Key and Certificate Services" < 0> rsa acdf483f38fd167fe29565ab767b7a912d93aeae my-ca-cert $ certutil -d sql:sql -f pwdfile.txt -A -n ca-pem -t ,, -i cert.pem $ certutil -d sql:sql -f pwdfile.txt -L Certificate Nickname Trust Attributes SSL,S/MIME,JAR/XPI ca-pem u,u,u ^^^^^^^^^^ NICK IS CHANGED
New FreeIPA upstream issue looks related to this bug, https://pagure.io/freeipa/issue/7516
I spent some time in gdb. Eventually I figured out that sdb_SetAttributeValue treats CKA_LABEL (0x3) differently for CKO_CERTIFICATE (0x1) than lg_SetAttributeValue. The sdb_SetAttributeValue() function of sqlite storage simply issues an "UPDATE nssPublic SET a3=$label WHERE id=$id;". The implementation for the legacy database is much more complicated. The call ends up in nsslowcert_AddPermNickname(). The function only changes the nickname if entry->nickname == NULL. In case the name name is already set, it calls AddNicknameToPermCert(dbhandle, cert, entry->nickname). Please note that the call does not use the nickname parameter with the new nick, but re-uses entry->nickname. Call stack: #0 nsslowcert_AddPermNickname (dbhandle=0x675730, cert=0x68e5d0, nickname=0x67e510 "ca-pem") at ../../lib/softoken/legacydb/pcertdb.c:3236 #1 0x00007ffff5cd5719 in lg_SetCertAttribute (obj=0x688e10, type=3, value=0x633450, len=6) at ../../lib/softoken/legacydb/lgattr.c:1512 #2 0x00007ffff5cd5ca5 in lg_SetSingleAttribute (obj=0x688e10, attr=0x7fffffffcc50, writePrivate=0x7fffffffcb1c) at ../../lib/softoken/legacydb/lgattr.c:1719 #3 0x00007ffff5cd5dcc in lg_SetAttributeValue (sdb=0x676e80, handle=1044714102, templ=0x7fffffffcc50, count=1) at ../../lib/softoken/legacydb/lgattr.c:1758 #4 0x00007ffff61ebc1d in sftkdb_setAttributeValue (arena=0x67e610, handle=0x63a9c0, db=0x676e80, objectID=1044714102, template=0x7fffffffcc50, count=1) at ../../lib/softoken/sftkdb.c:1090 #5 0x00007ffff61ec463 in sftkdb_SetAttributeValue (handle=0x63a9c0, object=0x691a10, template=0x7fffffffcc50, count=1) at ../../lib/softoken/sftkdb.c:1393 #6 0x00007ffff61e3d41 in sftk_forceTokenAttribute (object=0x691a10, type=3, value=0x633450, len=6) at ../../lib/softoken/pkcs11u.c:521 #7 0x00007ffff61e3e36 in sftk_forceAttribute (object=0x691a10, type=3, value=0x633450, len=6) at ../../lib/softoken/pkcs11u.c:546 #8 0x00007ffff61cf9ee in NSC_SetAttributeValue (hSession=16777219, hObject=3192197750, pTemplate=0x7fffffffcde0, ulCount=2) at ../../lib/softoken/pkcs11.c:4620 #9 0x00007ffff7baa66f in nssCKObject_SetAttributes (object=3192197750, obj_template=0x7fffffffcde0, count=2, session=0x688cb0, slot=0x684800) at ../../lib/dev/ckhelper.c:219 #10 0x00007ffff7bad15a in nssToken_ImportCertificate (tok=0x683de0, sessionOpt=0x0, certType=NSSCertificateType_PKIX, id=0x68f3d8, nickname=0x633450 "ca-pem", encoding=0x68f3e8, issuer=0x68f3f8, subject=0x68f408, serial=0x68f418, email=0x0, asTokenObject=1) at ../../lib/dev/devtoken.c:534 #11 0x00007ffff7b5c5e9 in PK11_ImportCert (slot=0x67f5b0, cert=0x68eac0, key=0, nickname=0x633450 "ca-pem", includeTrust=0) at ../../lib/pk11wrap/pk11cert.c:1089 #12 0x000000000040a7c4 in AddCert (slot=0x67f5b0, handle=0x680d60, name=0x633450 "ca-pem", trusts=0x7fffffffd90e ",,", certDER=0x7fffffffd0e0, emailcert=0, pwdata=0x7fffffffd0d0) at ../../cmd/certutil/certutil.c:129 #13 0x0000000000413f16 in certutil_main (argc=12, argv=0x7fffffffd3f8, initialize=1) at ../../cmd/certutil/certutil.c:3693 #14 0x0000000000414701 in main (argc=12, argv=0x7fffffffd3f8) at ../../cmd/certutil/certutil.c:3880
Created attachment 1428429 [details] Patch to keep existing nickname in SQL layer The attached patch fixes the issue for me. (Note, sqlite3_mprintf() does not implement positional printf arguments like %1$x. I had to hard-code a3.)
Created attachment 1428556 [details] Patch to keep existing nickname in SQL layer (2) The attached patch fixes the issue for me. Second version only pins nick names of certs. This fixes a problem with private key assignment to certs. (Note, sqlite3_mprintf() does not implement positional printf arguments like %1$x. I had to hard-code a3.)
Hmm, the sql-lite behavior is correct. DBM does not handle nicknames well. When a new nickname is written, dbm ignores it. All other PKCS #11 modules accept the new nickname. The question is what code is depending on the broken behavior?
Thinking about this a bit, is IPA using the actual pk12util program, or is the pk12util/certutil combination something that is simulating something that IPA is doing programmatically? If it's the former, I think we can add an option to pk12util that tells it to ignore the nickname supplied by the .p12 file.
Comment on attachment 1428556 [details] Patch to keep existing nickname in SQL layer (2) We don't want to change the sql-lite db behavior because that's the correct behavior. We'll need to fix this issue at a different level.
So at the very least, there's a bug in PK11_ImportCertificate (well actually in nssToken_ImportCertificate()), where we are explicitly setting the label to NULL if we are importing a new certificate with a NULL nickname (and explicitly changing the nickname to NULL if the cert already exists). I'll include a patch for that. Also certutil requires -n to be supplied when -A is specified. -E, however, functions just like -A except it doesn't require a nickname to be specified (-E is AddEmailCert, though it doesn't actually require an email certificate). Option 1: 1) At the very least nssToken_ImportCertificate needs to not set CKA_LABEL if the nickname is NULL. 2) Potentially certutil could drop the -n requirement. 3) We we just do this IPA would have to either change the certutil scripts to drop the nickname on import and if we don't do '2', then they would also have to change their '-A' to '-E'. NOTE: this means if the cert isn't already in the database, it will be imported without a nickname at all. We really need to understand the actual IPA use case to see whether this is OK or not. Option 2: 1) We change nssToken_ImportCertificate to not change the nickname when we import a certificate. NOTE: we also try to change the CKA_ID as well. This code is both very deliberate, and would have no effect on the old DBM database. It would change the semantic when importing certificates into a token. Option 3: Same as option 2 except we allow CKA_ID and nickname to be changed they are NULL entries.
Forgot to put the question for Kai. Which option do you prefer. Option 1: requires some work for the CS team. Their use case is they are importing a root cert (which doesn't need a nickname), so there shouldn't be any issues with the import without a nickname. If we choose option one, we need to decide if we wan't to remove the -n option from certutil -A or just have IPA use the certutil -E option. Option3: requires no work from the CS team. There may be some corner cases around tokens which will change their behavior. We can still recover from certs with no nickname (we can still use certutil -A to supply them nicknames), but we can't use certutil -A to change their nicknames. certutil --rename can handle the nickname rename case. In all options tokens and sql-lite DB will have the same semantics. I'll attach patches to both option 1 and option 3.
Created attachment 1428962 [details] Option 3 implemented This passes all.sh. Changing anything with CKA_ID causes OCSP to fail. Option 1 will look very much like this patch. With this patch: $ rm sql/* $ certutil -d sql:sql -f pwdfile.txt -N $ pk12util -i cert.p12 -k pwdfile.txt -w pwdfile.txt -d sql:sql pk12util: PKCS12 IMPORT SUCCESSFUL $ certutil -d sql:sql -f pwdfile.txt -L Certificate Nickname Trust Attributes SSL,S/MIME,JAR/XPI my-ca-cert u,u,u $ certutil -d sql:sql -f pwdfile.txt -K certutil: Checking token "NSS Certificate DB" in slot "NSS User Private Key and Certificate Services" < 0> rsa acdf483f38fd167fe29565ab767b7a912d93aeae my-ca-cert $ certutil -d sql:sql -f pwdfile.txt -A -n ca-pem -t ,, -i cert.pem $ certutil -d sql:sql -f pwdfile.txt -L Certificate Nickname Trust Attributes SSL,S/MIME,JAR/XPI my-ca-cert u,u,u But also: $ rm sql/* $ certutil -d sql:sql -f pwdfile.txt -N $ certutil -d sql:sql -f pwdfile.txt -E -t ,, -i cert.pem $ certutil -L -d sql:sql Certificate Nickname Trust Attributes SSL,S/MIME,JAR/XPI (NULL) ,, $ pk12util -i cert.p12 -k pwdfile.txt -w pwdfile.txt -d sql:sql $ certutil -L -d sql:sql Certificate Nickname Trust Attributes SSL,S/MIME,JAR/XPI my-ca-cert u,u,u So we are able to recover from a NULL nickname, and... $ certutil --rename -n my-ca-cert --new-n your-ca-cert -d sql:sql $ certutil -L -d sql:sql Certificate Nickname Trust Attributes SSL,S/MIME,JAR/XPI your-ca-cert u,u,u ... rename still works correctly for sql-lite databases. (NOTE: rename still does not work for dbm databases, but that's a limitation of the old dbm database). $ certutil --rename -n my-ca-cert --new-n your-ca-cert -d dbm:dbm [rrelyea@rrelyea-laptop dup_reproducer]$ certutil -L -d dbm:dbm Certificate Nickname Trust Attributes SSL,S/MIME,JAR/XPI my-ca-cert u,u,u
Created attachment 1428963 [details] Option 1 implemented The simularity of this patch to option 3 is pretty clear. The only difference is we unconditionally set the nickname if the nickname is supplied, so: $ certutil -L -d sql:sql Certificate Nickname Trust Attributes SSL,S/MIME,JAR/XPI your-ca-cert u,u,u $ certutil -d sql:sql -f pwdfile.txt -E -t ,, -i cert.pem $ certutil -L -d sql:sql Certificate Nickname Trust Attributes SSL,S/MIME,JAR/XPI your-ca-cert u,u,u ...but.... $ certutil -d sql:sql -f pwdfile.txt -A -n ca-pem -t ,, -i cert.pem $ certutil -L -d sql:sql Certificate Nickname Trust Attributes SSL,S/MIME,JAR/XPI ca-pem u,u,u With this option, the ipa scripts would need to change '-A -n {nickname}' to '-E '.
Created attachment 1428964 [details] Option to remove the '-n' requirement from AddCert (-A) This patch could be added if we select option 1. If this patch is added, only the -n {nickname} in the ipa scripts would have to change. Since we have a way of doing this '-E', it's not strictly necessary, but there is a clarity argument that says specifying 'add email cert' isn't the obvious way of importing a certificate without a nickname (though that's exactly what it is). On the other hand, certs in a hand managed database is difficult to deal with if they don't have nicknames, so removing the requirement for -n may not be desirable.
My personal recommendation is option 3.
Comment on attachment 1428962 [details] Option 3 implemented Kai if you prefer option 3, r+ this patch.
Comment on attachment 1428963 [details] Option 1 implemented Kai, if you prefer option 1, r+ this patch.
Comment on attachment 1428964 [details] Option to remove the '-n' requirement from AddCert (-A) Kai, If you select option 1, and also want certutil to be updated so that you don't need -n on the -A option, r+ this patch. NOTE: I currently don't recommend this patch.
I'm in favor of option (3), too. FreeIPA's test suite is passing with NSS build nss-3.36.1-1.0.0.option3 without additional modification.
Thanks Christian, Does nss-3.36.1-1.0.0.option1 also work if you change the -A -n cert_pem to -E in the certutil import? bob
I did not test option (1). It requires several changes to IPA and Dogtag. I simply didn't have time and resources to track down all places and change code to use -E. Option (3) makes certutil with SQL database behave certutil with DBM database. Since we are considering the behavior of certutil with shared as a regression, option (3) also resolves the regression.
Bob and Christian, thanks a lot for working on this issue. It seems you have already agreed that Option 3 is the best fix. My preference is a fix that doesn't require changes to applications. Because this isn't just about Dogtag, but other applications could run into the same issue. So a fix inside NSS, without requiring application level changes, would be the best fix IMHO. If option 3 doesn't cause any new regressions, that's fine with me. (In reply to Bob Relyea from comment #9) > Option3: requires no work from the CS team. There may be some corner cases > around tokens which will change their behavior. We can still recover from > certs with no nickname (we can still use certutil -A to supply them > nicknames), but we can't use certutil -A to change their nicknames. certutil > --rename can handle the nickname rename case. > > In all options tokens and sql-lite DB will have the same semantics. Renaming with -A seems unnecessary anyway, because we have --rename. I cannot judge the risks that Bob implies with his comment "There may be some corner cases around tokens which will change their behavior.". It sounds like you're willing to accept that risk. I'll file an upstream bug report, and Christian suggested to also add an upstream test, I'll look into that. I've started two runs for the NSS upstream test suite, and the Firefox test suite, to ensure there are no obvious regressions. https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=3360eed0cc12b17fffac49cda58687167c85a959 https://treeherder.mozilla.org/#/jobs?repo=try&revision=6053ae67cf3b659793abd7b3169c72c03d4cc741 If things look good, it might make sense to start with a Fedora 28 update that includes this as a downstream patch. This would allow us to test this change more widely, prior to including it in the upstream release or more stable releases.
Thanks Bob and Kai! Your proposal sounds like a good approach. I'm fine with a Fedora 28 update with downstream patch.
(In reply to Bob Relyea from comment #10) > Created attachment 1428962 [details] > Option 3 implemented > > This passes all.sh. Changing anything with CKA_ID causes OCSP to fail. It's not clear what this comment means. Reading this first, it sounds like you're saying that your option 3 patch introduces a patch for OCSP.
(In reply to Kai Engert (:kaie) from comment #22) > (In reply to Bob Relyea from comment #10) > > Created attachment 1428962 [details] > > Option 3 implemented > > > > This passes all.sh. Changing anything with CKA_ID causes OCSP to fail. > > It's not clear what this comment means. Reading this first, it sounds like > you're saying that your option 3 patch introduces a patch for OCSP. "introduces a regression". Does option 3 introduce a regression for OCSP ?
nss-3.36.1-1.1.fc28 has been submitted as an update to Fedora 28. https://bodhi.fedoraproject.org/updates/FEDORA-2018-8cf042000b
nss-3.36.1-1.1.fc28 has been pushed to the Fedora 28 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-8cf042000b
nss-3.36.1-1.1.fc28 has been pushed to the Fedora 28 stable repository. If problems still persist, please make note of it in this bug report.
> It's not clear what this comment means. Reading this first, it sounds like > you're saying that your option 3 patch introduces a patch for OCSP. Sorry, no. It means If I carry the same semantic for CKA_ID as I did for CKA_LABEL, then we have failurs in the OCSP tests, so I only made the change for CKA_LABEL in my patch, to reduce regressions.