Conversation
There was a problem hiding this comment.
Pull request overview
This PR expands ECC-related integration testing in the C++ test suite and adjusts the Rust ECC pre-hashed ECDSA signing implementation to accept oversized output buffers (matching the C API’s “buffer is a capacity” contract).
Changes:
- Add extensive new C++ ECC sign/verify test coverage (binary payloads, buffer sizing sweeps, algorithm/curve matrices, and argument-validation scenarios).
- Expand C++ ECC key attestation (key report) tests with additional boundary and contract checks.
- Update Rust
HsmEccSignAlgo::sign()to treat signature buffers as “at least required size” (reject only when too small).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| api/tests/cpp/algo/ecc/sign_verify_tests.cpp | Large expansion of ECC sign/verify single-shot + streaming coverage and API contract tests. |
| api/tests/cpp/algo/ecc/keyreport_tests.cpp | Adds key report buffer/argument contract tests and additional handle-class coverage. |
| api/tests/cpp/algo/ecc/keygen_tests.cpp | Reorganizes/expands ECC keygen lifecycle tests and property-validation coverage. |
| api/lib/src/algo/ecc/sign.rs | Fixes ECC sign output-buffer sizing check to allow oversized buffers. |
| azihsm_buffer report_buf{ nullptr, 0 }; | ||
| auto over_max_err = | ||
| azihsm_generate_key_report(priv_key.get(), &over_max_report_data_buf, &report_buf); | ||
| ASSERT_NE(over_max_err, AZIHSM_STATUS_SUCCESS); |
There was a problem hiding this comment.
In report_data_boundary_cases, the oversize report_data case should assert the specific status code. The DDI implementation rejects report_data.len() > MAX_REPORT_DATA_SIZE with InvalidArgument, so this can be ASSERT_EQ(over_max_err, AZIHSM_STATUS_INVALID_ARGUMENT) instead of a generic ASSERT_NE to make the contract explicit and avoid masking regressions.
| ASSERT_NE(over_max_err, AZIHSM_STATUS_SUCCESS); | |
| ASSERT_EQ(over_max_err, AZIHSM_STATUS_INVALID_ARGUMENT); |
…g re-use of existing infra for generating key data using OpenSSL/CNG
| let Some(signature) = signature else { | ||
| return Ok(expected_len); | ||
| }; | ||
| if signature.len() != expected_len { |
There was a problem hiding this comment.
One of the tests was failing when an oversized buffer was used. So we were reporting 'too small' when really the buffer was 'too large'. From my understanding, a buffer can be larger than needed but not smaller.
| @@ -0,0 +1,61 @@ | |||
| // Copyright (c) Microsoft Corporation. | |||
There was a problem hiding this comment.
Can we not add these helpers in product code? It's ok to duplicate the helper code in C++ tests.
|
|
||
| [dependencies] | ||
| azihsm_api.workspace = true | ||
| azihsm_crypto.workspace = true |
There was a problem hiding this comment.
Why is this getting added?
| curve: AzihsmEccCurve, | ||
| der_key: *mut AzihsmBuffer, | ||
| ) -> AzihsmStatus { | ||
| abi_boundary(|| { | ||
| let der_key = deref_mut_ptr(der_key)?; |
| /// | ||
| /// @return 0 on success, or a negative error code on failure. | ||
| /// If output buffer is insufficient, required length is written to `der_key->len` | ||
| /// and the function returns AZIHSM_STATUS_INSUFFICIENT_BUFFER. |
| @@ -19,6 +20,8 @@ zerocopy = { features = ["derive"], workspace = true } | |||
| [features] | |||
| default = [] | |||
| mock = ["azihsm_api/mock"] | |||
| # Test-only feature: exports helpers consumed by C++ integration tests. | |||
| cpp-test-helpers = ["mock"] | |||
| wrapped_blob.resize(4096); | ||
| azihsm_buffer out_buf{}; | ||
| out_buf.ptr = wrapped_blob.data(); | ||
| out_buf.len = static_cast<uint32_t>(wrapped_blob.size()); | ||
|
|
||
| auto err = azihsm_crypt_encrypt(&wrap_algo, wrapping_pub_key, &in_buf, &out_buf); |
| #include "handle/part_handle.hpp" | ||
| #include "handle/part_list_handle.hpp" | ||
| #include "handle/session_handle.hpp" | ||
| #include "../aes/helpers.hpp" |
No description provided.