-
Notifications
You must be signed in to change notification settings - Fork 918
Add wc_SignCert_cb API for external signing callbacks #9632
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: master
Are you sure you want to change the base?
Conversation
|
Jenkins retest this please. History lost. |
| @@ -33861,6 +33805,103 @@ int wc_MakeCertReq(Cert* cert, byte* derBuffer, word32 derSz, | |||
| #endif /* WOLFSSL_CERT_REQ */ | |||
|
|
|||
|
|
|||
|
|
|||
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'd like this new feature to be wrapped in new build option to avoid code growth. Also some of this looks to be duplicate code now. Is there way way to implement such that the old code uses the callback path internally when not set. If so you can keep it always like it is.
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 pull request adds support for signing certificates and CSRs using a user-provided callback function, enabling integration with external signing devices (TPMs/HSMs) without relying on the crypto callback infrastructure. This is particularly useful for FIPS-compliant applications where offloading cryptographic operations is not acceptable.
Changes:
- Introduced new
wc_SignCert_cbAPI andwc_SignCertCbcallback type for external certificate/CSR signing - Refactored internal
MakeSignaturefunction to use newMakeSignatureCbinternally for RSA and ECC, eliminating code duplication - Added configure option
--enable-certsigncbto enable the feature
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| wolfssl/wolfcrypt/asn_public.h | Added public API declarations for the callback-based certificate signing, including typedef for wc_SignCertCb and function declaration for wc_SignCert_cb |
| wolfcrypt/src/asn.c | Implemented internal MakeSignatureCb function and refactored MakeSignature to use callback path for RSA/ECC; added wc_SignCert_cb implementation |
| tests/api.c | Added test case test_wc_SignCert_cb with mock callback to verify the new API functionality |
| configure.ac | Added configuration option --enable-certsigncb to control compilation of the new feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #if !defined(NO_RSA) && !defined(WOLFSSL_RSA_PUBLIC_ONLY) && !defined(WOLFSSL_RSA_VERIFY_ONLY) | ||
| if (keyType == RSA_TYPE && signCtx->rsaKey) { | ||
| /* For RSA, input is already encoded digest */ | ||
| ret = wc_RsaSSL_Sign(in, inLen, out, *outLen, | ||
| signCtx->rsaKey, signCtx->rng); | ||
| if (ret > 0) { | ||
| *outLen = (word32)ret; | ||
| ret = 0; | ||
| } | ||
| } | ||
| #endif /* !NO_RSA && !WOLFSSL_RSA_PUBLIC_ONLY && !WOLFSSL_RSA_VERIFY_ONLY */ | ||
|
|
||
| #if defined(HAVE_ECC) && defined(HAVE_ECC_SIGN) | ||
| if (keyType == ECC_TYPE && signCtx->eccKey) { | ||
| /* For ECC, input is the raw hash */ | ||
| ret = wc_ecc_sign_hash(in, inLen, out, outLen, | ||
| signCtx->rng, signCtx->eccKey); | ||
| } | ||
| #endif /* HAVE_ECC && HAVE_ECC_SIGN */ | ||
|
|
||
| #if defined(HAVE_ED25519) && defined(HAVE_ED25519_SIGN) | ||
| if (keyType == ED25519_TYPE && signCtx->ed25519Key) { | ||
| /* Ed25519 needs the original message, not hash */ | ||
| /* Note: For Ed25519, 'in' should be the original message buffer */ | ||
| /* This is a limitation of the refactoring - Ed25519 signs messages, not hashes */ | ||
| ret = NOT_COMPILED_IN; /* Cannot support Ed25519 through callback path */ | ||
| } | ||
| #endif /* HAVE_ED25519 && HAVE_ED25519_SIGN */ | ||
|
|
||
| #if defined(HAVE_ED448) && defined(HAVE_ED448_SIGN) | ||
| if (keyType == ED448_TYPE && signCtx->ed448Key) { | ||
| /* Ed448 needs the original message, not hash */ | ||
| ret = NOT_COMPILED_IN; /* Cannot support Ed448 through callback path */ | ||
| } | ||
| #endif /* HAVE_ED448 && HAVE_ED448_SIGN */ | ||
|
|
||
| #if defined(HAVE_FALCON) | ||
| if (keyType == FALCON_LEVEL1_TYPE || keyType == FALCON_LEVEL5_TYPE) { | ||
| if (signCtx->falconKey) { | ||
| /* Falcon needs the original message */ | ||
| ret = NOT_COMPILED_IN; /* Cannot support Falcon through callback path */ | ||
| } | ||
| } | ||
| #endif /* HAVE_FALCON */ | ||
|
|
||
| #if defined(HAVE_DILITHIUM) && !defined(WOLFSSL_DILITHIUM_NO_SIGN) | ||
| if (keyType == DILITHIUM_LEVEL2_TYPE || keyType == DILITHIUM_LEVEL3_TYPE || | ||
| keyType == DILITHIUM_LEVEL5_TYPE) { | ||
| if (signCtx->dilithiumKey) { | ||
| /* Dilithium needs the original message */ | ||
| ret = NOT_COMPILED_IN; /* Cannot support Dilithium through callback path */ | ||
| } | ||
| } | ||
| #endif /* HAVE_DILITHIUM && !WOLFSSL_DILITHIUM_NO_SIGN */ | ||
|
|
||
| #if defined(HAVE_SPHINCS) | ||
| if (keyType == SPHINCS_FAST_LEVEL1_TYPE || keyType == SPHINCS_FAST_LEVEL3_TYPE || | ||
| keyType == SPHINCS_FAST_LEVEL5_TYPE || keyType == SPHINCS_SMALL_LEVEL1_TYPE || | ||
| keyType == SPHINCS_SMALL_LEVEL3_TYPE || keyType == SPHINCS_SMALL_LEVEL5_TYPE) { | ||
| if (signCtx->sphincsKey) { | ||
| /* Sphincs needs the original message */ | ||
| ret = NOT_COMPILED_IN; /* Cannot support Sphincs through callback path */ | ||
| } | ||
| } | ||
| #endif /* HAVE_SPHINCS */ |
Copilot
AI
Jan 15, 2026
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 InternalSignCb function uses multiple independent if statements instead of else-if chains. While this works correctly in practice because MakeSignature ensures only one key type is set, it's inconsistent with the coding pattern elsewhere and could cause issues if the logic changes. The function should use else-if to make the mutual exclusivity explicit and prevent potential future bugs.
| word32 outSz = *outLen; | ||
|
|
||
| /* For RSA, input is pre-encoded digest, just sign it */ | ||
| ret = wc_RsaSSL_Sign(in, inLen, out, outSz, rsaKey, NULL); |
Copilot
AI
Jan 15, 2026
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 test mock callback passes NULL as the RNG parameter to wc_RsaSSL_Sign, but it should pass signCtx->rng to be consistent with the internal implementation and to support RSA features that require RNG (such as blinding). The RNG is available in the context and should be used.
| static int test_wc_SignCert_cb(void) | ||
| { | ||
| EXPECT_DECLS; | ||
| #if defined(WOLFSSL_CERT_GEN) && defined(HAVE_ECC) && !defined(NO_ASN_TIME) | ||
| Cert cert; | ||
| byte der[FOURK_BUF]; | ||
| int derSize = 0; | ||
| WC_RNG rng; | ||
| ecc_key key; | ||
| MockSignCtx signCtx; | ||
| int ret; | ||
|
|
||
| XMEMSET(&rng, 0, sizeof(WC_RNG)); | ||
| XMEMSET(&key, 0, sizeof(ecc_key)); | ||
| XMEMSET(&cert, 0, sizeof(Cert)); | ||
| XMEMSET(&signCtx, 0, sizeof(MockSignCtx)); | ||
|
|
||
| ExpectIntEQ(wc_InitRng(&rng), 0); | ||
| ExpectIntEQ(wc_ecc_init(&key), 0); | ||
| ExpectIntEQ(wc_ecc_make_key(&rng, 32, &key), 0); | ||
| ExpectIntEQ(wc_InitCert(&cert), 0); | ||
|
|
||
| (void)XSTRNCPY(cert.subject.country, "US", CTC_NAME_SIZE); | ||
| (void)XSTRNCPY(cert.subject.state, "state", CTC_NAME_SIZE); | ||
| (void)XSTRNCPY(cert.subject.locality, "locality", CTC_NAME_SIZE); | ||
| (void)XSTRNCPY(cert.subject.org, "org", CTC_NAME_SIZE); | ||
| (void)XSTRNCPY(cert.subject.unit, "unit", CTC_NAME_SIZE); | ||
| (void)XSTRNCPY(cert.subject.commonName, "www.example.com", | ||
| CTC_NAME_SIZE); | ||
| (void)XSTRNCPY(cert.subject.email, "test@example.com", CTC_NAME_SIZE); | ||
|
|
||
| cert.selfSigned = 1; | ||
| cert.isCA = 0; | ||
| cert.sigType = CTC_SHA256wECDSA; | ||
|
|
||
| /* Make cert body */ | ||
| ExpectIntGT(wc_MakeCert(&cert, der, FOURK_BUF, NULL, &key, &rng), 0); | ||
|
|
||
| /* Setup signing context with key and RNG */ | ||
| signCtx.key = &key; | ||
| signCtx.rng = &rng; | ||
|
|
||
| /* Sign using callback API */ | ||
| ExpectIntGT(derSize = wc_SignCert_cb(cert.bodySz, cert.sigType, der, | ||
| FOURK_BUF, ECC_TYPE, mockSignCb, &signCtx, &rng), 0); |
Copilot
AI
Jan 15, 2026
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 test only exercises the ECC code path. The mockSignCb function includes RSA support, but test_wc_SignCert_cb only tests with ECC keys. Consider adding a separate test case or expanding this test to also verify RSA certificate signing via the callback to ensure complete test coverage.
| word32 outLen; | ||
|
|
||
| (void)rng; | ||
| (void)heap; |
Copilot
AI
Jan 15, 2026
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 heap parameter is marked as unused with (void)heap on line 33871, but it is actually used on lines 33878 and 33898 for memory allocation. The (void)heap statement should be removed to avoid misleading readers and to prevent potential compiler warnings in some configurations.
| else | ||
| #endif /* !NO_RSA */ | ||
| { | ||
| /* ECC/EdDSA: pass raw hash or message */ |
Copilot
AI
Jan 15, 2026
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 comment says "ECC/EdDSA: pass raw hash or message" but EdDSA algorithms (Ed25519, Ed448) are explicitly not supported through the callback path as shown in InternalSignCb. The comment should be corrected to just say "ECC: pass raw hash" to avoid confusion.
| RsaKey* rsaKey = (RsaKey*)signCtx->key; | ||
| word32 outSz = *outLen; | ||
|
|
||
| /* For RSA, input is pre-encoded digest, just sign it */ |
Copilot
AI
Jan 15, 2026
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 comment says "input is pre-encoded digest" but should more accurately say "input is DER-encoded digest with algorithm identifier" to match the actual format produced by wc_EncodeSignature. This is consistent with the documentation issue for the callback type definition.
| \param in Data to sign. For RSA, this is the PKCS#1 v1.5 padded digest. | ||
| For ECC, this is the raw hash to sign. |
Copilot
AI
Jan 15, 2026
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 documentation states that for RSA, the callback receives "PKCS#1 v1.5 padded digest", but this is inaccurate. The callback actually receives a DER-encoded digest (DigestInfo structure) produced by wc_EncodeSignature. The PKCS#1 v1.5 padding is applied later by the signing function (e.g., wc_RsaSSL_Sign). The documentation should be corrected to say "DER-encoded digest with algorithm identifier" or "DigestInfo structure".
| \param in Data to sign. For RSA, this is the PKCS#1 v1.5 padded digest. | |
| For ECC, this is the raw hash to sign. | |
| \param in Data to sign. For RSA, this is the DER-encoded digest | |
| (DigestInfo structure with algorithm identifier). For ECC, | |
| this is the raw hash to sign. |
This pull request adds support for signing certificates and CSRs using a user-provided callback function, enabling integration with external signing devices (such as TPMs or HSMs) without relying on the crypto callback infrastructure. This is particularly useful for FIPS-compliant applications and scenarios where offloading cryptographic operations is required. The changes include new API definitions, documentation, internal implementation, and tests for the callback-based signing mechanism.
New Callback-Based Certificate Signing API
wc_SignCert_cbfunction and thewc_SignCertCbcallback type, allowing certificates and CSRs to be signed via an external callback for flexible integration with devices like TPMs/HSMs. [1] [2] [3]Internal Implementation
MakeSignatureCbfunction to handle hashing, digest encoding, and invoking the user-provided signing callback, supporting both RSA and ECC key types.Testing
Setup:
TPM simulator: swtpm running on port 2321
Built wolfSSL with: --enable-certgen --enable-certreq --enable-certext --enable-cryptocb
Built wolfTPM with: --enable-swtpm --enable-certgen --enable-debug
Tests Run:
Generated RSA and ECC test keys in TPM
Created CSRs using ./examples/csr/csr
Validated CSRs with openssl req -text -noout
Results:
wc_SignCert_cb compiled into wolfSSL
wolfTPM2_SignCertCb and CSR_MakeAndSign_Cb compiled into wolfTPM
Generated valid RSA (1228 bytes) and ECC (696 bytes) CSRs
CSRs verified successfully with OpenSSL