feat: support signing with external signature#4
feat: support signing with external signature#4sebastian-bantel wants to merge 1 commit intoembetrix:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for external signature generation, enabling use with key infrastructure systems where direct private key access is unavailable. The key enhancement is a two-step signing workflow that separates hash generation from signature application.
Key changes:
- Introduced new command-line options (
-u,-s,-d) to support external signing workflow - Refactored key loading to handle both private and public keys separately
- Added DER signature format parsing with automatic fallback to raw format
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| stm32mp-sign-tool.cpp | Core implementation of external signing support, including new functions for public key loading, DER signature parsing, and refactored signing logic |
| stm32mp-sign-tool_test.sh | Added test cases for public key hash generation and external signing workflow |
| README.md | Updated documentation with new command-line options and usage examples for two-step signing workflow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
stm32mp-sign-tool_test.sh
Outdated
|
|
||
| # Test sign process with external signing | ||
| # generate hash to sign from image | ||
| openssl ec -in private_key.pem -pubout -out public_key.pem |
There was a problem hiding this comment.
This line is duplicated from line 48. The public key file is already generated, so this command is redundant and should be removed.
| openssl ec -in private_key.pem -pubout -out public_key.pem |
There was a problem hiding this comment.
right. This line can be dropped
| return {}; | ||
| } | ||
|
|
||
| std::vector<unsigned char> r_bytes(der_sig.begin() + static_cast<std::ptrdiff_t>(idx), der_sig.begin() + static_cast<std::ptrdiff_t>(idx + r_len)); |
There was a problem hiding this comment.
[nitpick] The explicit cast to std::ptrdiff_t is unnecessary here. Vector iterators support arithmetic operations directly with size_t values.
| std::vector<unsigned char> r_bytes(der_sig.begin() + static_cast<std::ptrdiff_t>(idx), der_sig.begin() + static_cast<std::ptrdiff_t>(idx + r_len)); | |
| std::vector<unsigned char> r_bytes(der_sig.begin() + idx, der_sig.begin() + idx + r_len); |
There was a problem hiding this comment.
those casts are necessary due to sign-conversion errors
/workspaces/meta-sentron/build/workspace/sources/stm32mp-sign-tool-native/stm32mp-sign-tool.cpp:260:58: error: conversion to ‘__gnu_cxx::__normal_iterator<const unsigned char*, std::vector<unsigned char> >::difference_type’ {aka ‘long int’} from ‘size_t’ {aka ‘long unsigned int’} may change the sign of the result [-Werror=sign-conversion] 260 | std::vector<unsigned char> r_bytes(der_sig.begin() + idx, der_sig.begin() + idx + r_len); | ^~~ /workspaces/meta-sentron/build/workspace/sources/stm32mp-sign-tool-native/stm32mp-sign-tool.cpp:260:81: error: conversion to ‘__gnu_cxx::__normal_iterator<const unsigned char*, std::vector<unsigned char> >::difference_type’ {aka ‘long int’} from ‘size_t’ {aka ‘long unsigned int’} may change the sign of the result [-Werror=sign-conversion] 260 | std::vector<unsigned char> r_bytes(der_sig.begin() + idx, der_sig.begin() + idx + r_len); | ^~~ /workspaces/meta-sentron/build/workspace/sources/stm32mp-sign-tool-native/stm32mp-sign-tool.cpp:260:87: error: conversion to ‘__gnu_cxx::__normal_iterator<const unsigned char*, std::vector<unsigned char> >::difference_type’ {aka ‘long int’} from ‘size_t’ {aka ‘long unsigned int’} may change the sign of the result [-Werror=sign-conversion] 260 | std::vector<unsigned char> r_bytes(der_sig.begin() + idx, der_sig.begin() + idx + r_len);
| return {}; | ||
| } | ||
|
|
||
| std::vector<unsigned char> s_bytes(der_sig.begin() + static_cast<std::ptrdiff_t>(idx), der_sig.begin() + static_cast<std::ptrdiff_t>(idx + s_len)); |
There was a problem hiding this comment.
[nitpick] The explicit cast to std::ptrdiff_t is unnecessary here. Vector iterators support arithmetic operations directly with size_t values.
There was a problem hiding this comment.
those casts are necessary due to sign-conversion errors
| std::copy(r_bytes.begin(), r_bytes.end(), raw_sig.begin() + static_cast<std::ptrdiff_t>(32 - r_bytes.size())); | ||
|
|
||
| // Copy s (right-aligned in second 32 bytes) | ||
| std::copy(s_bytes.begin(), s_bytes.end(), raw_sig.begin() + static_cast<std::ptrdiff_t>(32 + (32 - s_bytes.size()))); |
There was a problem hiding this comment.
[nitpick] The explicit cast to std::ptrdiff_t is unnecessary here. Vector iterators support arithmetic operations directly with size_t values.
| std::copy(r_bytes.begin(), r_bytes.end(), raw_sig.begin() + static_cast<std::ptrdiff_t>(32 - r_bytes.size())); | |
| // Copy s (right-aligned in second 32 bytes) | |
| std::copy(s_bytes.begin(), s_bytes.end(), raw_sig.begin() + static_cast<std::ptrdiff_t>(32 + (32 - s_bytes.size()))); | |
| std::copy(r_bytes.begin(), r_bytes.end(), raw_sig.begin() + (32 - r_bytes.size())); | |
| // Copy s (right-aligned in second 32 bytes) | |
| std::copy(s_bytes.begin(), s_bytes.end(), raw_sig.begin() + (32 + (32 - s_bytes.size()))); |
There was a problem hiding this comment.
those casts are necessary due to sign-conversion errors
| std::copy(r_bytes.begin(), r_bytes.end(), raw_sig.begin() + static_cast<std::ptrdiff_t>(32 - r_bytes.size())); | ||
|
|
||
| // Copy s (right-aligned in second 32 bytes) | ||
| std::copy(s_bytes.begin(), s_bytes.end(), raw_sig.begin() + static_cast<std::ptrdiff_t>(32 + (32 - s_bytes.size()))); |
There was a problem hiding this comment.
[nitpick] The explicit cast to std::ptrdiff_t is unnecessary here. Vector iterators support arithmetic operations directly with size_t values.
| std::copy(s_bytes.begin(), s_bytes.end(), raw_sig.begin() + static_cast<std::ptrdiff_t>(32 + (32 - s_bytes.size()))); | |
| std::copy(s_bytes.begin(), s_bytes.end(), raw_sig.begin() + (32 + (32 - s_bytes.size()))); |
There was a problem hiding this comment.
those casts are necessary due to sign-conversion errors
stm32mp-sign-tool.cpp
Outdated
| std::cout << " -o Output signed image file" << std::endl; | ||
| std::cout << " -h Output file for public key hash" << std::endl; | ||
| std::cout << " -s Output file for hash to sign" << std::endl; | ||
| std::cout << " -d Input signature file (DER format)" << std::endl; |
There was a problem hiding this comment.
The help text states 'DER format' only, but the implementation accepts both DER and raw 64-byte signatures. Update to '-d Input signature file (DER or raw format)' for accuracy.
| std::cout << " -d Input signature file (DER format)" << std::endl; | |
| std::cout << " -d Input signature file (DER or raw format)" << std::endl; |
|
what kind of key infrastructure are you using? From my perspective this MR increases the code complexity for a very specific use case. Could you explain why the existing PKCS#11 support is not sufficient for your setup, and which requirements are still missing? As for the hash generation I’m using the combined hash-and-sign operation directly in a single step to avoid extra public key handing code: Example of usage in yocto: https://github.com/embetrix/meta-stm32mp15x/blob/scarthgap/classes/stm32mp15x-sign.bbclass#L14 |
stm32mp-sign-tool.cpp
Outdated
|
|
||
| int hash_pubkey(const char* key_desc, const char* passphrase, const std::string &output_file) { | ||
| if (!key_desc || output_file.empty()) { | ||
| int load_public_key(const char* key_desc, EC_KEY** ec_key) { |
There was a problem hiding this comment.
load_public_key
should also handle loading keys from pkcs11 token
There was a problem hiding this comment.
added pkcs11 support and tests for this use case
Thanks for the quick response! In my company we use SignClient CLI from keyfactor: https://docs.keyfactor.com/signserver/7.0/client-cli. My change decouples the hash generation and signing process and image generation. This should work with every infrastructure. I also want to sign my images with dev keys during CI build and want to be as close as possible to the real signing process. |
|
> My change decouples the hash generation and signing process and image generation. This should work with every infrastructure. => Okay if addressed in separate PR, it should however handle public key usage from pkcs11 token as well. > I also want to sign my images with dev keys during CI build and want to be as close as possible to the real signing process: =>Regarding signing images with dev keys in CI: I already have several customers doing exactly this : either by using separate key slots in the same HSM for development vs. production or by using a SoftHSM that mirrors the real signing setup. |
|
I will have a look on: pkcs11 tokens I am not 100% sure how this HSM works but in my company the HSM is stored in a different city. The HSM is indirectly called via SignClient or RestAPI calls. |
|
The existing implementation already supports Net HSM setups via PKCS#11. A typical example of this is AWS CloudHSM, which is also accessed via a PKCS#11 client library: https://aws.amazon.com/cloudhsm/ If you’d like to integrate a specific HSM setup or need help adapting this to your environment, I can gladly provide commercial support. |
53eaeac to
d4f9fe3
Compare
- add option to create hash to sign - add option to create signed image using public key and external signature file (DER format or raw) - add option to create public key hash from public key file - update tests to cover new functionality - update usage instructions
d4f9fe3 to
9baedc2
Compare
|
I got an official answer from our signing client department:
From my point of view I need the changes I made Feel free to merge this pull request or request further changes |
|
@sebastian-bantel : What I can do, however, is push your changes to a dedicated branch so you can continue to use and maintain them there ? Independently of this, I’m planning to introduce changes for signing the new STM32MP23x / STM32MP25x parts as soon as a customer needs it and I’ll keep that implementation as generic/simple as possible. |
|
@sebastian-bantel I’m also wondering how you plan to handle signing of the FIP, FITimage, Software Update and the other components of the secure boot chain: those will also need to be adapted for offline signing. |
@embetrix As far as I understand now I need to extend cert_create tool to sign the FIP image offline. When I am able to securely boot to u-boot cmd line I can reuse further internal workflows to load FIT image and rootfs securely. I'm still in discussion with our signing service. They may provide OpenSSL's PKCS#11 ENGINE |
As I use a key infrastructure it is not possible to access the private key directly. I rewrote the tool that it can generate the signed image without access to the private key.
Thank you for contributing to
stm32mp-sign-tool!By submitting this pull request, you confirm that: