Now that intermediate preloading has landed, we could make certificate verification in Firefox more performant by exposing pk11_FindObjectsByTemplate (presumably as "PK11_FindObjectsByTemplate") and PK11_ReadAttribute from NSS (so we can directly read from the built-in roots module rather than going through CERT_CreateSubjectCertList, which can be slow).

Dana: Do you have more details about the concerns/motivations? Sorry for asking something that I'm sure is probably familiar to the folks that have been working on intermediate preloading.

I ask largely because the PK11_* interfaces 'pierce' the abstraction that the STAN/TrustDomain layer was trying to achieve. Now, it may be that the piercing is exactly what's desired, because of performance, but it does make it a bit harder to ever decide one way or the other what the 'happy' path is and whether the STAN abstraction should be nuked or finished.

My presumed understanding (and this may be entirely wrong!) is that the concern is that the NSSTrustDomain_Find* and NSSCryptoContext_Find* are perhaps inefficient, because they scan all loaded modules (... in that TrustDomain/CryptoContext, but since it was never finished, is effectively all modules). Is that correct?

Currently Firefox implements path building by repeatedly calling CERT_CreateSubjectCertList to find potential issuers. As you say, this scans all modules including the softoken, which as we found out in bug 1478148 hits the disk a ton. Now that we have intermediate preloading (bug 1535662 etc.), any certificate in the web PKI should be verifiable using just these intermediates and the roots from the built-in roots module. What I want to do is build a fast path that tries these certificates first and never even touches the softoken unless that fails. Since preloaded intermediates aren't stored in NSS, we only need to worry about querying the roots module. One option would be to re-package the roots information, but that seems like overkill if we can just use the PKCS#11 functions directly, hence this bug. (Obviously, if searching this fast path doesn't work, we would still have to fall back on CERT_CreateSubjectCertList, but the idea is most people won't ever be in that situation.)

Gotcha! Yeah, you definitely want to keep CERT_CreateSubjectCertList for the fallback path, since the preloading isn't going to cover all the cases of the existing Web PKI. A notable carveout in the policy is with respect to technically-constrained sub-CAs, which, for example, AT&T makes use of for their wifi captive portal solution. The other concern was that bypassing the trust domain has the affect of bypassing the 'temp' cert store (the in-memory one), which we don't really have any APIs that do that (the PK11_ methods today largely deal in keys).

I do worry, though, about an API exposing raw CK_OBJECT_HANDLE for certificates, since the NSS API tries to only deal in CERTCertificates for those, and those have to go through the STAN_ layer to be minted (since they hang with the TrustDomain).

The 'intent' of the NSS API had been that the CERTCertDBHandle would encapsulate the 'which slots to search', but I suspect that would still be affected by the STAN_GetDefaultCryptoContext (which was the transition API to handle the legacy 'global' behaviour of NSS) call within CERT_CreateSubjectCertList. That's the other part of why I asked - because exposing the PK11_ side adds another nail in the coffin of the CERTCertDBHandle (which we only ever expose a default one).

Wouldn't this also run into issues for something like p11kit (on Fedora/RHEL), which replaces the builtin root module with its own (and has several), which functionally look and behave similarly to the softoken DBs. Or is the idea that it would always hit the slow-path if NSS itself hadn't loaded nssckbi.[dll/so]?

Apologies if it comes off like I'm criticizing the design - just that the NSS public API has tried to limit the exposure directly of PKCS#11, and tended to layer things on (the PKCS#11 object -> STANCertificate/NSSCertificate -> CERTCertificate layer cake), so exposing something this low-level is super-powerful, and also super-error prone (in terms of object lifetime management), but also an API that would likely quickly become used and impossible to replace or refactor later.

(In reply to Ryan Sleevi from comment #3)

Gotcha! Yeah, you definitely want to keep CERT_CreateSubjectCertList for the fallback path, since the preloading isn't going to cover all the cases of the existing Web PKI. A notable carveout in the policy is with respect to technically-constrained sub-CAs, which, for example, AT&T makes use of for their wifi captive portal solution. The other concern was that bypassing the trust domain has the affect of bypassing the 'temp' cert store (the in-memory one), which we don't really have any APIs that do that (the PK11_ methods today largely deal in keys).

Right - another change I'm intending to make is to include any intermediates from the TLS handshake on this fast path, which should work for technically-constrained hierarchies (assuming the server is configured to actually send the right certificates).

I do worry, though, about an API exposing raw CK_OBJECT_HANDLE for certificates, since the NSS API tries to only deal in CERTCertificates for those, and those have to go through the STAN_ layer to be minted (since they hang with the TrustDomain).

Would it help to roll these two into one more focused API like:

SECStatus PK11_FindRawCertsWithSubject(PK11SlotInfo *slot, SECItem *derSubject, PLArenaPool *arena, SECItem **results);

... that would take a particular slot to search for certificates matching a particular subject and return the raw bytes of each?

The 'intent' of the NSS API had been that the CERTCertDBHandle would encapsulate the 'which slots to search', but I suspect that would still be affected by the STAN_GetDefaultCryptoContext (which was the transition API to handle the legacy 'global' behaviour of NSS) call within CERT_CreateSubjectCertList. That's the other part of why I asked - because exposing the PK11_ side adds another nail in the coffin of the CERTCertDBHandle (which we only ever expose a default one).

That would be great if it worked, but this part of NSS has been in this halfway state since before my first internship at Mozilla 10 years ago.

Wouldn't this also run into issues for something like p11kit (on Fedora/RHEL), which replaces the builtin root module with its own (and has several), which functionally look and behave similarly to the softoken DBs. Or is the idea that it would always hit the slow-path if NSS itself hadn't loaded nssckbi.[dll/so]?

Yes - the idea is to only use this on nssckbi.

Apologies if it comes off like I'm criticizing the design - just that the NSS public API has tried to limit the exposure directly of PKCS#11, and tended to layer things on (the PKCS#11 object -> STANCertificate/NSSCertificate -> CERTCertificate layer cake), so exposing something this low-level is super-powerful, and also super-error prone (in terms of object lifetime management), but also an API that would likely quickly become used and impossible to replace or refactor later.

I agree we shouldn't make it easy for consumers of NSS get themselves in a broken state by directly accessing PKCS#11.

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #4)

(In reply to Ryan Sleevi from comment #3)
Would it help to roll these two into one more focused API like:

SECStatus PK11_FindRawCertsWithSubject(PK11SlotInfo *slot, SECItem *derSubject, PLArenaPool *arena, SECItem **results);

... that would take a particular slot to search for certificates matching a particular subject and return the raw bytes of each?

Yeah. Apologies for not offering a more direct suggestion with my questions, that probably would have helped.

It's unclear whether being able to control the PLArena is needed. If it isn't, it might fit within the idiom to use the CERTCertificateList approach, which deals in SECItems. It requires the list owns its arena, because CERT_DestroyCertificateList just nukes the arena (much like how CERTCertList deals in CERTCertificates and cleanup nukes its arena)

If you went that route, the API is probably:

SECStatus PK11_FindRawCertsWithSubject(PK11SlotInfo *slot, SECItem *derSubject, CERTCertificateList** results);

Otherwise, the alternative API SGTM, with just a note that it might be diverging from local calling conventions in the file (... but that's never stopped NSS before ;D)

(In reply to Ryan Sleevi from comment #5)

If you went that route, the API is probably:

SECStatus PK11_FindRawCertsWithSubject(PK11SlotInfo *slot, SECItem *derSubject, CERTCertificateList** results);

Sounds good - thanks!

Ryan, I don't know if you're still reviewing NSS code, but if you are, I'd appreciate if you had a look: https://phabricator.services.mozilla.com/D32067

Set r+ on the CL, with some minor bits. Clearing the NI.

Lando mislanded on wrong branch due to lando bug 1549757

NSS_3_44_BRANCH: https://hg.mozilla.org/projects/nss/rev/3c2aceba7ae819c25fe372d9506af1e7e3a524fb
NSS_3_44_BRANCH Backout: https://hg.mozilla.org/projects/nss/rev/2b80548a8ab1

https://hg.mozilla.org/projects/nss/rev/580cc952a945a706c18b9a649cbb64f22c7467e9

A fixup is needed as this patch had abi updates that are spurious (and will be fixed by Bug 1556273). Failures in CI:

https://tools.taskcluster.net/task-inspector/#nc5VY2aZS_G-2gRS0gCRmA — clang-format @ nss-tools opt (blame: Dana Keeler)

Fixup try push: https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=cc9406680614

https://hg.mozilla.org/projects/nss/rev/b48f2c6f4adfd239c8120c9b3fb511cbcfe2fc5b