Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement coalescing of fetch_all #177

Merged
merged 12 commits into from
Feb 9, 2024
Merged

Conversation

sosthene-nitrokey
Copy link
Contributor

@sosthene-nitrokey sosthene-nitrokey commented Jan 18, 2024

Fix #174, built on top of #185

Copy link

codecov bot commented Jan 19, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (629cecc) 90.36% compared to head (0a54f42) 90.59%.

Files Patch % Lines
pkcs11/src/backend/session.rs 98.79% 2 Missing ⚠️
pkcs11/src/backend/key.rs 96.77% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #177      +/-   ##
==========================================
+ Coverage   90.36%   90.59%   +0.22%     
==========================================
  Files          31       31              
  Lines        6644     6792     +148     
==========================================
+ Hits         6004     6153     +149     
+ Misses        640      639       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sosthene-nitrokey sosthene-nitrokey force-pushed the fetch-all-coalescing branch 2 times, most recently from ae66cae to b834703 Compare January 23, 2024 16:42
@sosthene-nitrokey sosthene-nitrokey marked this pull request as ready for review January 23, 2024 16:45
@sosthene-nitrokey sosthene-nitrokey force-pushed the fetch-all-coalescing branch 12 times, most recently from bb8a6ac to 84058e0 Compare February 1, 2024 15:39
@rcritten
Copy link

rcritten commented Feb 5, 2024

While PR #185 works with listing certificates uploaded using p11tool with this patchset I do not see the certificates with certutil.

@sosthene-nitrokey
Copy link
Contributor Author

Thank you for your feedback.

Can you please share your testing approach ? This patch it built on top of #185 and listing works for me.

@rcritten
Copy link

rcritten commented Feb 6, 2024

So I must have messed up something in earlier testing. Re-testing it and it does show the certificate as expected.

My methodology:

Build the module + patch and copy resulting shared library to /usr/lib64/pkcs11/nethsm-pkcs11-v1.1.0-x86_64-fedora.39.so (I haven't automated version detection yet).

Create /etc/pkcs11/modules/nethsm.conf with contents:
module: /usr/lib64/pkcs11/nethsm-pkcs11-v1.1.0-x86_64-fedora.39.so

Create an NSS database and verify the token is visible:

# mkdir nssdb
# certutil -N -d /root/nssdb --empty-password
# modutil -list -dbdir /root/nssdb/
... snip ...
  2. p11-kit-proxy
    	library name: p11-kit-proxy.so
       	uri: pkcs11:library-manufacturer=PKCS%2311%20Kit;library-description=PKCS%2311%20Kit%20Proxy%20Module;library-version=1.1
     	slots: 1 slot attached
    	status: loaded

     	slot: NetHSM
    	token: LocalHSM
      	uri: pkcs11:token=LocalHSM;manufacturer=Nitrokey%20GmbH;serial=0000000000;model=NetHSM

Run the test upload_certificate.sh (only modification is the path to the shared library).

# certutil -L -d nssdb/ -h all

@sosthene-nitrokey sosthene-nitrokey merged commit a5c3dc5 into main Feb 9, 2024
5 checks passed
@sosthene-nitrokey sosthene-nitrokey deleted the fetch-all-coalescing branch February 9, 2024 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use request coalescing for fetch_all_keys
3 participants