-
Notifications
You must be signed in to change notification settings - Fork 923
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. |
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.
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
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4dd06e1 to
8e28ab2
Compare
|
Jenkins retest this |
|
Jenkins retest this please. |
dgarske
left a comment
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.
[check-source-text] [2 of 7] [wolfssl]
autogen.sh wolfssl... real 0m17.507s user 0m15.557s sys 0m0.446s
configure... real 0m12.829s user 0m10.049s sys 0m2.680s
trailing whitespace:
./wolfcrypt/src/asn.c:32089:/* Make signature from buffer (sz), write to sig (sigSz)·
./wolfcrypt/src/asn.c:32100:····
./wolfcrypt/src/asn.c:32119:········
./wolfcrypt/src/asn.c:32125:········
./wolfcrypt/src/asn.c:32137:········
C++-style comments:
./wolfssl/wolfcrypt/asn_public.h:269: // Perform signing using external device/HSM
./wolfssl/wolfcrypt/asn_public.h:601: // Initialize cert and set subject, etc.
./wolfssl/wolfcrypt/asn_public.h:603: // ... set cert fields ...
./wolfssl/wolfcrypt/asn_public.h:605: // Make certificate body
./wolfssl/wolfcrypt/asn_public.h:608: // Sign using callback
unescaped error code operands (missing WC_NO_ERR_TRACE()):
wolfcrypt/src/asn.c:32016: int ret = ALGO_ID_E;
|
Jenkins retest this please. |
1898d4c to
2cf03da
Compare
2cf03da to
6483a4b
Compare
6483a4b to
8b5bd3b
Compare
lealem47
left a comment
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.
Thanks Jack!
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
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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, "[email protected]", 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); | ||
|
|
||
| /* Verify the certificate was created properly */ | ||
| ExpectIntGT(derSize, 0); | ||
|
|
||
| /* Test error cases */ | ||
| ExpectIntEQ(wc_SignCert_cb(cert.bodySz, cert.sigType, der, | ||
| FOURK_BUF, ECC_TYPE, NULL, &signCtx, &rng), BAD_FUNC_ARG); | ||
|
|
||
| ret = wc_ecc_free(&key); | ||
| ExpectIntEQ(ret, 0); | ||
| ret = wc_FreeRng(&rng); | ||
| ExpectIntEQ(ret, 0); | ||
| #endif | ||
| return EXPECT_RESULT(); | ||
| } | ||
| #endif /* WOLFSSL_CERT_SIGN_CB */ |
Copilot
AI
Feb 3, 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 function test_wc_SignCert_cb only tests with ECC keys (CTC_SHA256wECDSA). Since the new API explicitly supports both RSA and ECC key types, and the mockSignCb callback implementation includes RSA support (lines 20011-20023), there should be test coverage for RSA as well to ensure the RSA code path works correctly. Consider adding an RSA test case to verify that RSA signing through the callback works properly.
|
Jenkins retest this please. |
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
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
Copilot reviewed 5 out of 7 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #ifdef WOLFSSL_CERT_SIGN_CB | ||
| int wc_SignCert_cb(int requestSz, int sType, byte* buf, word32 buffSz, | ||
| int keyType, wc_SignCertCb signCb, void* signCtx, | ||
| WC_RNG* rng) | ||
| { | ||
| int sigSz = 0; | ||
| CertSignCtx certSignCtx_lcl; | ||
| CertSignCtx* certSignCtx = &certSignCtx_lcl; | ||
|
|
||
| if (signCb == NULL || buf == NULL) { | ||
| return BAD_FUNC_ARG; | ||
| } | ||
|
|
||
| XMEMSET(certSignCtx, 0, sizeof(*certSignCtx)); | ||
|
|
||
| if (requestSz < 0) { | ||
| return requestSz; | ||
| } | ||
|
|
||
| #ifndef WOLFSSL_NO_MALLOC | ||
| certSignCtx->sig = (byte*)XMALLOC(MAX_ENCODED_SIG_SZ, NULL, | ||
| DYNAMIC_TYPE_TMP_BUFFER); | ||
| if (certSignCtx->sig == NULL) { | ||
| return MEMORY_E; | ||
| } | ||
| #endif | ||
|
|
||
| sigSz = MakeSignatureCb(certSignCtx, buf, (word32)requestSz, | ||
| certSignCtx->sig, MAX_ENCODED_SIG_SZ, sType, keyType, | ||
| signCb, signCtx, rng, NULL); | ||
|
|
||
| #ifdef WOLFSSL_ASYNC_CRYPT | ||
| if (sigSz == WC_NO_ERR_TRACE(WC_PENDING_E)) { | ||
| /* Not free'ing certSignCtx->sig here because it could still be in use | ||
| * with async operations. */ | ||
| return sigSz; | ||
| } | ||
| #endif | ||
|
|
||
| if (sigSz >= 0) { | ||
| if (requestSz + MAX_SEQ_SZ * 2 + sigSz > (int)buffSz) { | ||
| sigSz = BUFFER_E; | ||
| } | ||
| else { | ||
| sigSz = AddSignature(buf, requestSz, certSignCtx->sig, sigSz, sType); | ||
| } | ||
| } | ||
|
|
||
| #ifndef WOLFSSL_NO_MALLOC | ||
| XFREE(certSignCtx->sig, NULL, DYNAMIC_TYPE_TMP_BUFFER); | ||
| certSignCtx->sig = NULL; | ||
| #endif | ||
|
|
||
| return sigSz; | ||
| } | ||
| #endif /* WOLFSSL_CERT_SIGN_CB */ |
Copilot
AI
Feb 5, 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.
Potential memory leak in wc_SignCert_cb when WOLFSSL_ASYNC_CRYPT is enabled. When WC_PENDING_E is returned at line 34452, the function returns without freeing certSignCtx->sig (correctly, as noted in the comment). However, there is no mechanism to track this allocation across async calls, and no way for the caller to free this memory later.
In the SignCert function (lines 34103-34175), the certSignCtx is stored in the key structure (rsaKey->certSignCtx or eccKey->certSignCtx) when WOLFSSL_ASYNC_CRYPT is enabled, allowing the allocation to be tracked across async calls. But in wc_SignCert_cb, certSignCtx is a local variable that goes out of scope, so the allocated certSignCtx->sig will be lost.
Either wc_SignCert_cb needs a way to persist the certSignCtx (similar to how SignCert uses the key's embedded certSignCtx), or the API needs to document that it cannot be used with async crypto operations.
| case CERTSIGN_STATE_DO: | ||
| certSignCtx->state = CERTSIGN_STATE_DO; | ||
| outLen = sigSz; | ||
|
|
||
| /* Call the user-provided signing callback */ | ||
| #ifndef NO_RSA | ||
| if (keyType == RSA_TYPE) { | ||
| /* RSA: pass encoded digest */ | ||
| ret = signCb(certSignCtx->encSig, (word32)certSignCtx->encSigSz, | ||
| sig, &outLen, sigAlgoType, keyType, signCtx); | ||
| } | ||
| else | ||
| #endif /* !NO_RSA */ | ||
| { | ||
| /* ECC: pass raw hash */ | ||
| ret = signCb(certSignCtx->digest, (word32)digestSz, | ||
| sig, &outLen, sigAlgoType, keyType, signCtx); | ||
| } | ||
|
|
||
| if (ret == 0) { | ||
| ret = (int)outLen; | ||
| } | ||
| break; |
Copilot
AI
Feb 5, 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.
MakeSignatureCb doesn't check for or handle WC_PENDING_E return value from the user callback (signCb). At lines 34061-34070, the callback is invoked but the return value is only checked for success (ret == 0) or error (ret < 0), without any special handling for WC_PENDING_E.
If a user's callback returns WC_PENDING_E (e.g., when using async hardware), the function will not properly handle the pending state. It should check for WC_PENDING_E and return it without cleaning up state or resetting certSignCtx->state, similar to how the original MakeSignature function handled async operations.
The original implementation commented "set next state, since WC_PENDING_E reentry for these are not 'call again'" (formerly around the HashForSignature call), which suggests careful consideration was given to async state transitions. This logic needs to be preserved when the callback returns WC_PENDING_E.
| \param requestSz Size of the certificate body to sign (from Cert.bodySz). | ||
| \param sType Signature algorithm type (e.g., CTC_SHA256wRSA, | ||
| CTC_SHA256wECDSA). | ||
| \param buf Buffer containing the certificate/CSR DER data to sign. | ||
| \param buffSz Total size of the buffer (must be large enough for signature). | ||
| \param keyType Type of key used for signing (RSA_TYPE, ECC_TYPE, etc.). | ||
| \param signCb User-provided signing callback function. | ||
| \param signCtx Context pointer passed to the signing callback. | ||
| \param rng Random number generator (may be NULL if not needed). | ||
|
|
||
| \return Size of the signed certificate/CSR on success. | ||
| \return BAD_FUNC_ARG if signCb is NULL or other parameters are invalid. | ||
| \return BUFFER_E if the buffer is too small for the signed certificate. | ||
| \return MEMORY_E if memory allocation fails. | ||
| \return Negative error code on other failures. | ||
|
|
||
| _Example_ | ||
| \code | ||
| Cert cert; | ||
| byte derBuf[4096]; | ||
| int derSz; | ||
| MySignCtx myCtx; | ||
|
|
||
| wc_InitCert(&cert); | ||
|
|
||
| derSz = wc_MakeCert(&cert, derBuf, sizeof(derBuf), NULL, NULL, &rng); | ||
|
|
||
| derSz = wc_SignCert_cb(cert.bodySz, cert.sigType, derBuf, sizeof(derBuf), | ||
| RSA_TYPE, mySignCallback, &myCtx, &rng); | ||
| if (derSz > 0) { | ||
| printf("Signed certificate is %d bytes\n", derSz); | ||
| } | ||
| \endcode | ||
|
|
||
| \sa wc_SignCertCb | ||
| \sa wc_SignCert | ||
| \sa wc_SignCert_ex | ||
| \sa wc_MakeCert | ||
| \sa wc_MakeCertReq | ||
| */ | ||
| int wc_SignCert_cb(int requestSz, int sType, byte* buf, word32 buffSz, | ||
| int keyType, wc_SignCertCb signCb, void* signCtx, | ||
| WC_RNG* rng); |
Copilot
AI
Feb 5, 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 for wc_SignCert_cb does not mention support limitations for key types. The implementation in MakeSignatureCb (lines 34003-34098) only handles RSA_TYPE and ECC_TYPE, but the documentation at lines 261 and 253 says "keyType: Type of key used for signing (RSA_TYPE, ECC_TYPE, etc.)" with "etc." implying support for other types.
However, the function will silently fail or behave incorrectly for other key types like ED25519_TYPE, ED448_TYPE, or post-quantum types, since MakeSignatureCb only has conditional logic for RSA (line 34038) and falls through to ECC for everything else (line 34066-34070).
The documentation should explicitly state which key types are supported (RSA_TYPE and ECC_TYPE only) and explain that other algorithms like Ed25519, Ed448, and post-quantum schemes cannot use this callback-based API because they sign messages directly rather than hashes.
| static int test_wc_SignCert_cb(void) | ||
| { | ||
| EXPECT_DECLS; | ||
| #if defined(WOLFSSL_CERT_GEN) && !defined(NO_ASN_TIME) | ||
|
|
||
| #ifdef HAVE_ECC | ||
| /* Test with ECC key */ | ||
| { | ||
| 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, "[email protected]", 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); | ||
|
|
||
| /* Verify the certificate was created properly */ | ||
| ExpectIntGT(derSize, 0); | ||
|
|
||
| /* Test error cases */ | ||
| ExpectIntEQ(wc_SignCert_cb(cert.bodySz, cert.sigType, der, | ||
| FOURK_BUF, ECC_TYPE, NULL, &signCtx, &rng), BAD_FUNC_ARG); | ||
|
|
||
| ret = wc_ecc_free(&key); | ||
| ExpectIntEQ(ret, 0); | ||
| ret = wc_FreeRng(&rng); | ||
| ExpectIntEQ(ret, 0); | ||
| } | ||
| #endif /* HAVE_ECC */ | ||
|
|
||
| #if !defined(NO_RSA) && defined(WOLFSSL_KEY_GEN) | ||
| /* Test with RSA key */ | ||
| { | ||
| Cert cert; | ||
| byte der[FOURK_BUF]; | ||
| int derSize = 0; | ||
| WC_RNG rng; | ||
| RsaKey key; | ||
| MockSignCtx signCtx; | ||
| int ret; | ||
|
|
||
| XMEMSET(&rng, 0, sizeof(WC_RNG)); | ||
| XMEMSET(&key, 0, sizeof(RsaKey)); | ||
| XMEMSET(&cert, 0, sizeof(Cert)); | ||
| XMEMSET(&signCtx, 0, sizeof(MockSignCtx)); | ||
|
|
||
| ExpectIntEQ(wc_InitRng(&rng), 0); | ||
| ExpectIntEQ(wc_InitRsaKey(&key, NULL), 0); | ||
| ExpectIntEQ(wc_MakeRsaKey(&key, 2048, WC_RSA_EXPONENT, &rng), 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, "[email protected]", CTC_NAME_SIZE); | ||
|
|
||
| cert.selfSigned = 1; | ||
| cert.isCA = 0; | ||
| cert.sigType = CTC_SHA256wRSA; | ||
|
|
||
| /* Make cert body */ | ||
| ExpectIntGT(wc_MakeCert(&cert, der, FOURK_BUF, &key, NULL, &rng), 0); | ||
|
|
||
| /* Setup signing context with key and RNG */ | ||
| signCtx.key = &key; | ||
| signCtx.rng = &rng; | ||
|
|
||
| /* Sign using callback API with RSA */ | ||
| ExpectIntGT(derSize = wc_SignCert_cb(cert.bodySz, cert.sigType, der, | ||
| FOURK_BUF, RSA_TYPE, mockSignCb, &signCtx, &rng), 0); | ||
|
|
||
| /* Verify the certificate was created properly */ | ||
| ExpectIntGT(derSize, 0); | ||
|
|
||
| /* Test error case - NULL callback */ | ||
| ExpectIntEQ(wc_SignCert_cb(cert.bodySz, cert.sigType, der, | ||
| FOURK_BUF, RSA_TYPE, NULL, &signCtx, &rng), BAD_FUNC_ARG); | ||
|
|
||
| ret = wc_FreeRsaKey(&key); | ||
| ExpectIntEQ(ret, 0); | ||
| ret = wc_FreeRng(&rng); | ||
| ExpectIntEQ(ret, 0); | ||
| } | ||
| #endif /* !NO_RSA && WOLFSSL_KEY_GEN */ | ||
|
|
||
| #endif /* WOLFSSL_CERT_GEN && !NO_ASN_TIME */ | ||
| return EXPECT_RESULT(); | ||
| } |
Copilot
AI
Feb 5, 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.
Limited error case testing for wc_SignCert_cb. The tests only verify:
- NULL callback returns BAD_FUNC_ARG (lines 20101-20102, 20160-20161)
Missing error case tests include:
- NULL buf parameter
- Invalid keyType (e.g., unsupported key type like ED25519_TYPE)
- Buffer too small (buffSz smaller than needed)
- Negative requestSz
- Callback returning an error
- Callback returning insufficient signature data in outLen
These additional tests would help ensure the API handles error conditions correctly and returns appropriate error codes to callers.
| /* For RSA and ECC, use the callback path to eliminate duplication */ | ||
| #if (!defined(NO_RSA) && !defined(WOLFSSL_RSA_PUBLIC_ONLY) && !defined(WOLFSSL_RSA_VERIFY_ONLY)) || \ | ||
| (defined(HAVE_ECC) && defined(HAVE_ECC_SIGN)) | ||
| if (rsaKey || eccKey) { | ||
| InternalSignCtx signCtx; | ||
|
|
||
| ret = HashForSignature(buf, sz, sigAlgoType, certSignCtx->digest, | ||
| &typeH, &digestSz, 0, NULL, | ||
| INVALID_DEVID); | ||
| /* set next state, since WC_PENDING_E rentry for these are not "call again" */ | ||
| certSignCtx->state = CERTSIGN_STATE_ENCODE; | ||
| if (ret != 0) { | ||
| goto exit_ms; | ||
| } | ||
| FALL_THROUGH; | ||
| /* Setup internal signing context */ | ||
| XMEMSET(&signCtx, 0, sizeof(signCtx)); | ||
| signCtx.rng = rng; | ||
|
|
||
| case CERTSIGN_STATE_ENCODE: | ||
| #ifndef NO_RSA | ||
| /* Determine key type and set key pointer */ | ||
| if (rsaKey) { | ||
| #ifndef WOLFSSL_NO_MALLOC | ||
| certSignCtx->encSig = (byte*)XMALLOC(MAX_DER_DIGEST_SZ, heap, | ||
| DYNAMIC_TYPE_TMP_BUFFER); | ||
| if (certSignCtx->encSig == NULL) { | ||
| ret = MEMORY_E; goto exit_ms; | ||
| } | ||
| #endif | ||
|
|
||
| /* signature */ | ||
| certSignCtx->encSigSz = (int)wc_EncodeSignature(certSignCtx->encSig, | ||
| certSignCtx->digest, (word32)digestSz, typeH); | ||
| signCtx.key = rsaKey; | ||
| signCtx.keyType = RSA_TYPE; | ||
| } | ||
| #endif /* !NO_RSA */ | ||
| FALL_THROUGH; | ||
|
|
||
| case CERTSIGN_STATE_DO: | ||
| certSignCtx->state = CERTSIGN_STATE_DO; | ||
| ret = -1; /* default to error, reassigned to ALGO_ID_E below. */ | ||
|
|
||
| #if !defined(NO_RSA) && !defined(WOLFSSL_RSA_PUBLIC_ONLY) && !defined(WOLFSSL_RSA_VERIFY_ONLY) | ||
| if (rsaKey) { | ||
| /* signature */ | ||
| ret = wc_RsaSSL_Sign(certSignCtx->encSig, | ||
| (word32)certSignCtx->encSigSz, | ||
| sig, sigSz, rsaKey, rng); | ||
| else if (eccKey) { | ||
| signCtx.key = eccKey; | ||
| signCtx.keyType = ECC_TYPE; | ||
| } | ||
| #endif /* !NO_RSA */ | ||
|
|
||
| #if defined(HAVE_ECC) && defined(HAVE_ECC_SIGN) | ||
| if (!rsaKey && eccKey) { | ||
| word32 outSz = sigSz; | ||
|
|
||
| ret = wc_ecc_sign_hash(certSignCtx->digest, (word32)digestSz, | ||
| sig, &outSz, rng, eccKey); | ||
| if (ret == 0) | ||
| ret = (int)outSz; | ||
| else { | ||
| ret = BAD_FUNC_ARG; | ||
| goto exit_ms; | ||
| } | ||
| #endif /* HAVE_ECC && HAVE_ECC_SIGN */ | ||
|
|
||
| #if defined(HAVE_ED25519) && defined(HAVE_ED25519_SIGN) | ||
| if (!rsaKey && !eccKey && ed25519Key) { | ||
| word32 outSz = sigSz; | ||
| /* Use unified callback path */ | ||
| ret = MakeSignatureCb(certSignCtx, buf, sz, sig, sigSz, | ||
| (int)sigAlgoType, signCtx.keyType, | ||
| InternalSignCb, &signCtx, rng, heap); | ||
| goto exit_ms; | ||
| } | ||
| #endif |
Copilot
AI
Feb 5, 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 refactored MakeSignature function no longer handles state management for async operations (CertSignCtx state machine), which is inconsistent with the original implementation. The original MakeSignature had a state machine (CERTSIGN_STATE_BEGIN, CERTSIGN_STATE_DIGEST, CERTSIGN_STATE_ENCODE, CERTSIGN_STATE_DO) that was critical for async crypto operations.
When WOLFSSL_ASYNC_CRYPT is enabled, the CertSignCtx state needs to be preserved across calls when WC_PENDING_E is returned. The new implementation delegates to MakeSignatureCb, which does have state management, but MakeSignature itself no longer handles state transitions properly for RSA/ECC cases.
The original MakeSignature code handled the full state machine in the main function body, allowing it to return WC_PENDING_E at different stages. Now the RSA/ECC path jumps straight to MakeSignatureCb and exits, which may break async operations if they are in progress.
dgarske
left a comment
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.
@jackctj117 please review each of the copilot reviews and mark them resolved if they are solved. It looks like there might a few that need fixed still.
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