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
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>
There was a problem hiding this comment.
Should we consider a different solution rather than SIGN_CRL_CLEANUP(). That does avoid goto, but also seems error prone in that future devs may forget to go update that macro to free new/needed things if changing that function.
padelsbach
force-pushed
the
crl-generation
branch
14 times, most recently
from
327b4a4 to
21eb421
Compare
cconlon
left a comment
cconlon
left a comment
There was a problem hiding this comment.
Thanks, just a few more small items
|
|
||
| ret = AddSignature(NULL, tbsSz, &sigDummy, sigSz, sigType); | ||
| if (ret < 0) { | ||
| ret = tbsSz + sigSz + 64; |
There was a problem hiding this comment.
Does this + 64 have a meaning or is a known size? Maybe a small comment if so would be helpful.
src/crl.c
Outdated
| entry->lastDate, entry->lastDateFormat, | ||
| entry->nextDate, entry->nextDateFormat, | ||
| entry->certs, entry->totalCerts, | ||
| entry->crlNumberSet ? (const byte*)entry->crlNumber : NULL, |
There was a problem hiding this comment.
Please take a pass and shorten/wrap all lines longer than 80 chars, thanks
src/crl.c
Outdated
| } | ||
|
|
||
| /* Build to-be-signed (TBS) portion of the CRL buffer. | ||
| * Note that we pass the fields rather than the CRL_entry struct so |
There was a problem hiding this comment.
This and a few other comments in this file needs backing up a few spaces to line up
padelsbach
force-pushed
the
crl-generation
branch
from
21eb421 to
4aaf567
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