Add CRL generation code #9631
Add CRL generation code #9631
Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
padelsbach
force-pushed
the
crl-generation
branch
21 times, most recently
from
575cc9c to
42524ec
Compare
padelsbach
force-pushed
the
crl-generation
branch
4 times, most recently
from
3757923 to
4926b1b
Compare
|
Jenkins retest this please |
padelsbach
force-pushed
the
crl-generation
branch
from
4926b1b to
9d411e7
Compare
|
jenkins retest this please |
padelsbach
force-pushed
the
crl-generation
branch
from
9d411e7 to
6c973c3
Compare
|
jenkins retest this please |
src/crl.c
Outdated
| if (tbsSz < 0) { | ||
| WOLFSSL_MSG("wc_MakeCRL_ex failed"); | ||
| ret = tbsSz; | ||
| goto cleanup; |
There was a problem hiding this comment.
I suggest refactoring out goto, as per coding standards unless absolutely necessary.
There was a problem hiding this comment.
I see over 600 uses of goto from various developers in the code base, many touched/added within the last year and even a few from 2026. How rigorous should we be here?
I'm guessing do { ... } while(0); is also frowned upon?
There was a problem hiding this comment.
I interpret our coding standard rule of "Avoid gotos unless absolutely necessary" to be worded strictly. In my opinion until a day that changes, PR reviewers should be enforcing it when reviewing new code and changes to existing code.
One common practice used instead of goto for us is falling through with ret checks like so:
ret = func_call();
if (ret == 0) {
ret = func2_call();
}
if (ret == 0) {
ret = func3_call();
}
<cleanup logic>
| } | ||
| XMEMSET(entry, 0, sizeof(CRL_Entry)); | ||
|
|
||
| if (wc_InitMutex(&entry->verifyMutex) != 0) { |
There was a problem hiding this comment.
Do we need to wc_FreeMutex() in error cases below for verifyMutex?
src/crl.c
Outdated
| int tbsSz; | ||
| int totalSz; | ||
| byte* buf = NULL; | ||
| word32 bufSz = 4096; /* Working buffer for TBS + signature */ |
There was a problem hiding this comment.
Is there a chance that 4096 is not large enough for big CRLs with lots of revoked certs? For example a large CA that has 100's of revoked certs or such. Can we detect/calculate the needed size, or make this a user-overridable option via define if not?
src/crl.c
Outdated
|
|
||
| XMEMCPY(rc->serialNumber, serial, (size_t)serialSz); | ||
| rc->serialSz = serialSz; | ||
| XMEMCPY(rc->revDate, revDate, MAX_DATE_SIZE); |
There was a problem hiding this comment.
Are we sure that revDate won't be shorter than MAX_DATE_SIZE, so we don't read out of bounds here?
There was a problem hiding this comment.
Will add some checking for robustness
| #define X509_CRL_set_signature_nid wolfSSL_X509_CRL_set_signature_nid | ||
| #define X509_CRL_set_signature wolfSSL_X509_CRL_set_signature | ||
| #define X509_CRL_add_revoked wolfSSL_X509_CRL_add_revoked | ||
| #define X509_CRL_add_revoked_cert wolfSSL_X509_CRL_add_revoked_cert |
There was a problem hiding this comment.
Do we want to add an OpenSSL compat mapping for #define i2d_X509_CRL wolfSSL_i2d_X509_CRL?
| @@ -0,0 +1,92 @@ | |||
| Certificate: | |||
There was a problem hiding this comment.
Did these new certs get added to our certs/renewcerts.sh script so we can auto-regenerate them? Typically we regenerate test certs at the end of each year.
padelsbach
force-pushed
the
crl-generation
branch
from
6c973c3 to
cb4247b
Compare
4 participants
Description
Add ability to generate a certificate revocation list (CRL), in addition to the existing CRL decode logic.
Testing
New unit test in C, and new test script which uses openssl to validate the output.
Checklist