Skip to content

Enhance payload size detection and improve benchmark documentation#24

Open
keldonin wants to merge 4 commits intoMastercard:mainfrom
keldonin:implement_payload_fitness
Open

Enhance payload size detection and improve benchmark documentation#24
keldonin wants to merge 4 commits intoMastercard:mainfrom
keldonin:implement_payload_fitness

Conversation

@keldonin
Copy link
Contributor

This pull request introduces improved payload validation and error handling for cryptographic benchmark tests, ensuring that only supported payload sizes are processed for each encryption mode. It also adds detailed documentation for each benchmark class, clarifying requirements and testing approaches. The most important changes are grouped below.

Enhanced error handling and outcome reporting

  • Added new exception types (NotFound, AmbiguousResult, PayloadSizeNotSupported) to benchmark_result for more descriptive error reporting, and updated the operation_outcome_t variant to include them.
  • Updated the execute method in P11Benchmark to throw and handle these new exceptions, including unified error printing and outcome assignment. [1] [2] [3]
  • Modified the errorcode function to return descriptive error messages for new outcome types.

Payload validation for benchmarks

  • Introduced the is_payload_supported virtual method in P11Benchmark, allowing each benchmark to specify payload size requirements; implemented this method in all AES and DES3 benchmark subclasses to enforce block size constraints. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]

Documentation improvements

  • Added detailed block comments to the header files of each benchmark class (P11AESCBCBenchmark, P11AESECBBenchmark, P11AESGCMBenchmark, P11DES3CBCBenchmark, P11DES3ECBBenchmark) describing test case purpose, payload requirements, key attributes, options, and testing methodology. [1] [2] [3] [4] [5]

These changes collectively make the benchmarking framework more robust, user-friendly, and maintainable by enforcing correct payload sizes, providing clearer error messages, and documenting test requirements.

Copilot AI review requested due to automatic review settings February 26, 2026 08:08
@keldonin keldonin added the enhancement New feature or request label Feb 26, 2026
Copy link
Contributor

Copilot AI left a 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 pull request enhances the PKCS#11 benchmark framework with improved error handling, payload size validation, and comprehensive documentation. It introduces three new exception types for more precise error reporting, adds payload validation methods to block-cipher and RSA-OAEP benchmarks, and provides extensive documentation comments for each benchmark test case. The changes also clean up JWE benchmark initialization by removing unnecessary AES key existence checks.

Changes:

  • Added three new exception types (NotFound, AmbiguousResult, PayloadSizeNotSupported) with descriptive error messages
  • Implemented is_payload_supported() validation for AES ECB/CBC, DES3 ECB/CBC, RSA OAEP, and FindObjects benchmarks to enforce payload size constraints
  • Added detailed documentation blocks to 14 benchmark header files describing test purpose, requirements, and methodology
  • Removed redundant AES key existence checks from JWE benchmark setup since those keys are generated dynamically
  • Enhanced RSA OAEP benchmarks to retrieve modulus size dynamically and use it for buffer allocation

Reviewed changes

Copilot reviewed 32 out of 32 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/p11benchmark.hpp Added three new exception classes with message formatting and updated operation_outcome_t variant
src/p11benchmark.cpp Refactored exception handling with unified lambda, added payload validation check after prepare()
src/errorcodes.cpp Extended errorcode() visitor to handle new exception types
src/p11aescbc.hpp/cpp Added documentation and block-size validation (16 bytes)
src/p11aesecb.hpp/cpp Added documentation and block-size validation (16 bytes)
src/p11aesgcm.hpp Added comprehensive documentation (no validation needed for GCM)
src/p11des3cbc.hpp/cpp Added documentation and block-size validation (8 bytes)
src/p11des3ecb.hpp/cpp Added documentation and block-size validation (8 bytes)
src/p11oaepenc.hpp/cpp Added documentation, OAEP payload validation, and dynamic modulus size handling
src/p11oaepdec.hpp/cpp Added OAEP payload validation and dynamic modulus size handling
src/p11oaepunw.hpp/cpp Added documentation, OAEP payload validation, and dynamic modulus size handling
src/p11findobjects.hpp/cpp Added extensive documentation and configurable max-objects validation
src/p11rsasig.hpp Added RSA signature documentation
src/p11rsapss.hpp Added RSA-PSS signature documentation
src/p11ecdsasig.hpp Added ECDSA signature documentation
src/p11ecdh1derive.hpp Added ECDH key derivation documentation
src/p11hmacsha1.hpp Added HMAC-SHA1 documentation
src/p11hmacsha256.hpp Added HMAC-SHA256 documentation
src/p11hmacsha512.hpp Added HMAC-SHA512 documentation
src/p11genrandom.hpp Added random generation documentation
src/p11seedrandom.hpp Added RNG seeding documentation
src/p11xorkeydataderive.hpp Added XOR key derivation documentation
src/p11jwe.hpp Added JWE decryption documentation
src/p11perftest.cpp Removed redundant AES key checks for JWE benchmarks with explanatory comment

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…r OAEP unwrap case

Signed-off-by: Eric Devolder <eric.devolder@gmail.com>
@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
3.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Copy link

@covertmatthew covertmatthew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Verify we found exactly one object
if (found_count != 1) {
throw benchmark_result::NotFound();
throw benchmark_result::NotFound(label_data);
Copy link

@covertmatthew covertmatthew Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

label_data is a raw char* from a PKCS#11 attribute (CKA_LABEL), which is not null-terminated. The length is carried separately in ulValueLen. The NotFound(const char*) constructor passes it to std::string(label), which scans for a \0 that may not exist, risking a read past the buffer. Essentially, this passes a length-delimited buffer to a function expecting a null-terminated C string.

Using std::string(label_data, label_len) bounds the copy correctly.

Suggested change
throw benchmark_result::NotFound(label_data);
throw benchmark_result::NotFound(std::string(label_data, label_len));


auto pubk_handles = Object::search<Object>( session, pubkey_search_template.attributes() );

if( pubk_handles.size()==0 ) {
Copy link

@covertmatthew covertmatthew Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still using throw std::string for key-not-found and ambiguous-key errors. These won't match any of the typed catch handlers in execute() and will fall through to catch(...), which rethrows and crashes the program.

The fix is to replace throw std::string("Error: no public key found...") with throw benchmark_result::NotFound(label), and throw std::string("Error: more than one public key found...") with throw benchmark_result::AmbiguousResult(label). The std::cerr lines before each throw can be removed since handle_benchmark_exception already prints the error.

Now that you've implemented proper exception types, this should be:

Suggested change
if( pubk_handles.size()==0 ) {
throw benchmark_result::NotFound(label);
}
if( pubk_handles.size()>1) {
throw benchmark_result::AmbiguousResult(label);
}


auto pubk_handles = Object::search<Object>( session, pubkey_search_template.attributes() );

if( pubk_handles.size()==0 ) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still throw std::string for key-not-found and ambiguous-key errors. These won't match any of the typed catch handlers in execute() and will fall through to catch(...), which rethrows and crashes the program.

The fix is to replace throw std::string("Error: no public key found...") with throw benchmark_result::NotFound(label), and throw std::string("Error: more than one public key found...") with throw benchmark_result::AmbiguousResult(label). The std::cerr lines before each throw can be removed since handle_benchmark_exception already prints the error.

Now that you've implemented proper exception types, this should be:

Suggested change
if( pubk_handles.size()==0 ) {
throw benchmark_result::NotFound(label);
}
if( pubk_handles.size()>1) {
throw benchmark_result::AmbiguousResult(label);
}


#include "p11benchmark.hpp"

class P11OAEPDecryptBenchmark : public P11Benchmark

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to add a TEST CASE: documentation block here? The other headers appear to have one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants