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

Fix certutil certificate listing #185

Merged
merged 2 commits into from
Feb 9, 2024
Merged

Fix certutil certificate listing #185

merged 2 commits into from
Feb 9, 2024

Conversation

sosthene-nitrokey
Copy link
Contributor

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

This PR contains 2 changes:

  • Add the CKA_SERIAL_NUMBER attribute to certificate objects.
  • Fix fetch_all_keys request caching a subset of all objects, using only the kind that was first requested.

This doesn't fix the certificate upload part of #183, but that cannot be fixed without support for certificates without associated private keys.

This does fix certificate listing with certutil mentionned in #187.

Copy link

codecov bot commented Jan 30, 2024

Codecov Report

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

Comparison is base (13fa492) 0.00% compared to head (1defba0) 90.36%.

Files Patch % Lines
pkcs11/src/backend/session.rs 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           main     #185       +/-   ##
=========================================
+ Coverage      0   90.36%   +90.36%     
=========================================
  Files         0       31       +31     
  Lines         0     6642     +6642     
=========================================
+ Hits          0     6002     +6002     
- Misses        0      640      +640     

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

fetch_all_keys fills a cache. This cache is expected to contain all objects,
not just the kind that was first fetched.
@rcritten
Copy link

rcritten commented Feb 5, 2024

I tested this patch and confirmed that it can list certificates uploaded using a different mechanism, and that certutil cannot add certificates.

@sosthene-nitrokey sosthene-nitrokey requested review from ansiwen and robin-nitrokey and removed request for ansiwen and robin-nitrokey February 9, 2024 08:08
@sosthene-nitrokey sosthene-nitrokey merged commit 629cecc into main Feb 9, 2024
5 checks passed
@sosthene-nitrokey sosthene-nitrokey deleted the certutil branch February 9, 2024 14:39
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.

3 participants