OpenSC / libp11

PKCS#11 wrapper library
GNU Lesser General Public License v2.1
314 stars 188 forks source link

Improve key/cert search performance: use id/label to enumerate #487

Closed harshalgohel closed 1 year ago

harshalgohel commented 1 year ago

Add PKCS11enumerate*_ext functions to find keys with id or label if supplied. This change also retains previous behavior of fetching all the keys/certs if no label or id is supplied.

Add example and test for PKCS11enumerate*_ext functions.

If PKCS11enumerate*_ext functions are called with NULL template, behavior is same as before - enumerate everything.

Previous attempt to solve this was in 206af152fbcb48d4149097ed140e4840febd72d8 This improves on it and restores caching to avoid segmentation fault. And improve performance for further calls.

harshalgohel commented 1 year ago

Hi @mtrojnar

When used with OpenSSL, it was taking 10 seconds to enumerate over all keys and certs on my HSM. So this is an attempt to fix that delay, now it only takes 1 second

I would like to contribute these improvements and looking forward to see them in next libp11 release.

I would appreciate any feedback or improvement suggestions on the PR

harshalgohel commented 1 year ago

Hey @mtrojnar Can you take a look at updated PR?

Whenever we provision a certificate in SoftHSM (libp11 test), id and label of corresponding private key is same as the certificate. So I made an assumption that it will be same, and I included label and id both as part of search parameter.

This behavior is not observed in another HSM, id of certificate and private key is same but labels are different.

I could not find any requirement for labels to be same. So update the patch to remove label as search parameter from find_key(cert) and find_certificate(key) function definitions.

dwmw2 commented 1 year ago

I could not find any requirement for labels to be same. So update the patch to remove label as search parameter from find_key(cert) and find_certificate(key) function definitions.

Indeed. In http://david.woodhou.se/draft-woodhouse-cert-best-practice.html#rfc.section.8.2 I recommend "log in to the token in which the certificate was found, and perform a search using the original criteria specified in the provided PKCS#11 URI. If the key is not found and the original search was by CKA_LABEL of the certificate, then repeat the search using the CKA_ID of the certificate that was actually found, but not requiring a CKA_LABEL match."

I won't claim my recommendations there are perfect; I'd be happy to accept improvements if you feel that there is a better process to follow. But it sounds like you've just started doing it the way I recommend?

mtrojnar commented 1 year ago

This PR seems to break macOS tests. Can you take a look?

harshalgohel commented 1 year ago

This PR seems to break macOS tests. Can you take a look?

I don't have access to macOS device. I will try to setup macOS on kvm. Looks like github is using macOS 12 for "latest" tag.

Meanwhile, the code runs just fine on ubuntu and even valgrind gave no red flags.

harshalgohel commented 1 year ago

Hello @mtrojnar

I checked on macOS VM (BigSur) and macbook pro (Monterey) and observed something strange

macOS libp11 OpenSSL segfault error
BigSur libp11:master 3 no OpenSSL: error during RSA private key parsing
BigSur harshal:key_cert_perf_improv 3 no OpenSSL: error during RSA private key parsing
Monterey (12.6) libp11:master 3 yes yes: Segfault
Monterey (12.6) harshal:key_cert_perf_improv 3 no OpenSSL: error during RSA private key parsing

Now there has to be some bug within libp11 dependencies between BigSur and Monterey. I am not familiar with macOS at all. Can you help me debugging this. Standard tools like valgrind are not directly available, I followed ci.yml to install dependencies into macOS VM and not able to make any progress pin pointing source of the segfault.

mtrojnar commented 1 year ago

Well done. I like the way you implemented this feature.