-
Notifications
You must be signed in to change notification settings - Fork 87
Accelerate fetching attributes from an object handle #322
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
base: main
Are you sure you want to change the base?
Accelerate fetching attributes from an object handle #322
Conversation
Signed-off-by: Eric Devolder <[email protected]>
…rtcard token when benchmarking Signed-off-by: Eric Devolder <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR optimizes PKCS#11 attribute retrieval by reducing the number of API calls through intelligent batching and pre-allocation. The optimization pre-allocates buffers for fixed-size attributes and batches variable-size queries to minimize round-trips to potentially slow network-based tokens.
Key changes:
- Implemented a new optimized
get_attributes()method that uses 1-2 PKCS#11 calls instead of one per attribute - Added
AttributeType::fixed_size()to identify attributes with known sizes for pre-allocation - Renamed the original implementation to
get_attributes_old()for benchmarking comparison
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| cryptoki/src/object.rs | Adds AttributeType::fixed_size() method to identify fixed-size attributes for optimization (contains critical bug with CK_BBOOL size) |
| cryptoki/src/session/object_management.rs | Implements new optimized get_attributes() and renames old version to get_attributes_old() for comparison |
| cryptoki/tests/basic.rs | Adds comprehensive test coverage for the new get_attributes() implementation |
| cryptoki/tests/common/mod.rs | Adds test utility function get_pretend_library() to simulate different library behaviors |
| cryptoki/examples/benchmark_attributes.rs | Adds benchmarking tool to compare performance between old and new implementations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Eric Devolder <[email protected]>
caa0629 to
2375a05
Compare
| } else if attr.pValue.is_null() && attr.ulValueLen > 0 { | ||
| // NULL pointer but has a length - needs fetching in pass2 | ||
| pass2_indices.push(i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will bail out on the test added in #324 where the softhsm will happily return the variable-length AllowedMechanisms with length 0. Theoretically, this could happen also with other non-fixed size attributes that might be empty, such as CKA_ID (from brief run through the pkcs11 specs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I expected, the test from #324 fails with this PR + SoftHSM so it needs some more love:
---- import_export stdout ----
thread 'import_export' (2292734) panicked at cryptoki/tests/basic.rs:1120:26:
removal index (is 0) should be < len (is 0)
|
Just out of curiosity I ran the benchmark on kryoptic, which is a bit more performance oriented: The speedup is not that huge, in comparison to softhsm, but obviously noticable: |
| AttributeType::StartDate | AttributeType::EndDate => Some(size_of::<CK_DATE>()), | ||
|
|
||
| // CK_VERSION (2 bytes: major + minor) | ||
| AttributeType::ValidationVersion => Some(size_of::<CK_VERSION>()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ValidationCountry says it should be 2 letter ISO country code so lets include it as fixed-length attribute too.
| } else if attr.pValue.is_null() && attr.ulValueLen > 0 { | ||
| // NULL pointer but has a length - needs fetching in pass2 | ||
| pass2_indices.push(i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I expected, the test from #324 fails with this PR + SoftHSM so it needs some more love:
---- import_export stdout ----
thread 'import_export' (2292734) panicked at cryptoki/tests/basic.rs:1120:26:
removal index (is 0) should be < len (is 0)
| let rv1 = unsafe { | ||
| Rv::from(get_pkcs11!(self.client(), C_GetAttributeValue)( | ||
| self.handle(), | ||
| object.handle(), | ||
| template1.as_mut_ptr(), | ||
| template1.len().try_into()?, | ||
| )) | ||
| }; | ||
|
|
||
| match rv1 { | ||
| Rv::Ok | ||
| | Rv::Error(RvError::BufferTooSmall) | ||
| | Rv::Error(RvError::AttributeSensitive) | ||
| | Rv::Error(RvError::AttributeTypeInvalid) => { | ||
| // acceptable - we'll inspect ulValueLen/pValue | ||
| } | ||
| _ => { | ||
| rv1.into_result(Function::GetAttributeValue)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rv1 is not used anywhere after this block so I would rather keep using rv instead of rv1 and rv2 as it makes it easier to read and follow.
Hello team!
this PR is all about optimizing calls when fetching attributes from a (potentially network-based, slow) token. The design goal is to minimize the number of calls to the PKCS#11 layer, using the following strategies:
C_GetAttributeValueI have tried this patch on a PKCS#11 scanning utility. In some cases, the number of invocations to PKCS#11 APIs is an order of magnitude below the original version, I found it a massive improvement in terms of performance and execution time.
A small benchmarking tool has been added to this PR - the intent is to give the ability to test it against different tokens and see how it goes. It will be removed at a later stage. The former
get_attributes()has been temporarily renamedget_attributes_old()to allow for the benchmarking.Results with SoftHSM
Note: SoftHSM is likely to be a worst-case scenario, as fetching attributes is extremely fast on a software-based token. Playing the benchmarking tool against other equipments (smart cards, HSMs) will yield better results.
Different situations have been tested below:
CKA_ENCRYPT)CKA_MODULUS)The single-variable case is typically on parity ( the stats varies from run to run between 0.95 and 1.05, which range is within standard deviation). All other cases provide better performance.
details (generated by Copilot)
Benchmarking and Developer Tooling
benchmark_attributes.rsthat benchmarks and compares the performance and correctness of the original (get_attributes_old) and optimized (get_attributes) attribute retrieval implementations. It reports statistics and speedups for various attribute types and scenarios.Attribute Retrieval Optimization
Session::get_attributesmethod that optimizes attribute retrieval by pre-allocating buffers for attributes with known fixed sizes and minimizing the number of PKCS#11 calls, while maintaining correctness and filtering out unavailable attributes.get_attributes_oldfor benchmarking and comparison purposes.Utility and Type Improvements
AttributeType::fixed_sizemethod to determine if an attribute type has a known fixed size, supporting buffer pre-allocation and optimization in the new retrieval method.c_voidand import adjustments to support the new implementation. [1] [2]