refactor X509PrintSubjAltName #9238
Conversation
|
🛟 Devin Lifeguard found 1 likely issues in this PR
@effbiae |
|
Can one of the admins verify this patch? |
effbiae
force-pushed
the
X509PrintSubjAltName
branch
from
3f78890 to
5748993
Compare
|
Okay to test |
effbiae
force-pushed
the
X509PrintSubjAltName
branch
from
5748993 to
7c41395
Compare
effbiae
force-pushed
the
X509PrintSubjAltName
branch
from
7c41395 to
c3c7b11
Compare
|
Jenkins retest this please. Agentoffline and openssl.test failure |
|
Jenkins retest this please: "FIPS ERROR: Checkout failed" |
|
Merging with known Esp latest test failure. That's fixed in latest master. |
There was a problem hiding this comment.
Pull Request Overview
Refactors Subject Alternative Name (SAN) printing to remove duplicated formatting logic and reuse a single helper for both SAN and ACERT name printing, and implements actual DirName printing instead of a placeholder.
- Consolidates name-entry printing into X509_print_name_entry and updates ACERT call sites.
- Reworks X509PrintSubjAltName to delegate to the helper.
- Replaces the "DirName:" placeholder with X509PrintDirType.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (!x509 || !x509->altNames || !x509->subjAltNameSet) | ||
| return WOLFSSL_FAILURE; |
There was a problem hiding this comment.
The refactor changes behavior by returning WOLFSSL_FAILURE when no SAN is present (altNames is NULL or subjAltNameSet is false). Previously, the function treated absence of SAN as a no-op and returned success, which avoids failing overall certificate printing for a valid cert without SAN. Also, the prior implementation guarded against a NULL bio, which is no longer checked here. Suggest restoring prior semantics and input validation: return success when SAN isn't set/present, and fail early on NULL bio.
| if (!x509 || !x509->altNames || !x509->subjAltNameSet) | |
| return WOLFSSL_FAILURE; | |
| if (bio == NULL) | |
| return WOLFSSL_FAILURE; | |
| if (!x509 || !x509->altNames || !x509->subjAltNameSet) | |
| return WOLFSSL_SUCCESS; /* No SAN present, treat as no-op */ |
| len = XSNPRINTF(scratch, MAX_WIDTH, "DNS:%s", entry->name); | ||
| if (len >= MAX_WIDTH) { | ||
| ret = WOLFSSL_FAILURE; | ||
| break; | ||
| } | ||
| } | ||
| #if defined(OPENSSL_ALL) || defined(WOLFSSL_IP_ALT_NAME) | ||
| else if (entry->type == ASN_IP_TYPE) { | ||
| len = XSNPRINTF(scratch, MAX_WIDTH, "IP Address:%s", | ||
| entry->ipString); | ||
| if (len >= MAX_WIDTH) { | ||
| ret = WOLFSSL_FAILURE; | ||
| break; | ||
| } | ||
| } | ||
| #endif /* OPENSSL_ALL || WOLFSSL_IP_ALT_NAME */ | ||
| else if (entry->type == ASN_RFC822_TYPE) { | ||
| len = XSNPRINTF(scratch, MAX_WIDTH, "email:%s", | ||
| entry->name); | ||
| if (len >= MAX_WIDTH) { | ||
| ret = WOLFSSL_FAILURE; | ||
| break; | ||
| } | ||
| } | ||
| else if (entry->type == ASN_DIR_TYPE) { | ||
| len = X509PrintDirType(scratch, MAX_WIDTH, entry); | ||
| if (len >= MAX_WIDTH) { | ||
| ret = WOLFSSL_FAILURE; | ||
| break; | ||
| } | ||
| } | ||
| else if (entry->type == ASN_URI_TYPE) { | ||
| len = XSNPRINTF(scratch, MAX_WIDTH, "URI:%s", | ||
| entry->name); | ||
| if (len >= MAX_WIDTH) { | ||
| ret = WOLFSSL_FAILURE; | ||
| break; | ||
| } | ||
| } | ||
| #if defined(OPENSSL_ALL) | ||
| else if (entry->type == ASN_RID_TYPE) { | ||
| len = XSNPRINTF(scratch, MAX_WIDTH, "Registered ID:%s", | ||
| entry->ridString); | ||
| if (len >= MAX_WIDTH) { | ||
| ret = WOLFSSL_FAILURE; | ||
| break; | ||
| } | ||
| } | ||
| #endif | ||
| else if (entry->type == ASN_OTHER_TYPE) { | ||
| len = XSNPRINTF(scratch, MAX_WIDTH, | ||
| "othername <unsupported>"); |
There was a problem hiding this comment.
The previous bounds checks for truncation (len >= MAX_WIDTH) after each formatting call were removed, which silently allows truncated output. To keep behavior robust without duplicating checks, add a single post-selection check like: if (len < 0 || len >= MAX_WIDTH) { ret = WOLFSSL_FAILURE; break; } before writing to the BIO.
4 participants
Description
@TODO entry->name in ASN1 syntaxTesting
./autogen.sh && ./configure && make check && ./configure --enable-all && make checkChecklist