Add support for id-on-bundleEID from RFC 9174.#10810
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses X.509 SubjectAltName otherName parsing for the RFC 9174 id-on-bundleEID (OID 1.3.6.1.5.5.7.8.11) by teaching the ASN.1 template decoder to accept an IA5String value for that specific otherName type-id, and adds a regression test plus certificate renewal inputs.
Changes:
- Add
BUNDLE_EID_OID(guarded byWOLFSSL_DTN) to the OID-sum table. - Extend
otherNameASN.1 template decoding to accept IA5String for id-on-bundleEID (guarded byWOLFSSL_DTN). - Add a regression test and OpenSSL renewal configuration for a bundleEID-containing DER certificate.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfssl/wolfcrypt/oid_sum.h | Adds BUNDLE_EID_OID under WOLFSSL_DTN for OID sum matching. |
| wolfcrypt/src/asn.c | Extends OtherName template and decoding logic to handle bundleEID IA5String values under WOLFSSL_DTN. |
| tests/api/test_asn.h | Registers the new regression test declaration and test list entry. |
| tests/api/test_asn.c | Adds a regression test that parses a DER cert containing bundleEID OtherName + dNSName. |
| scripts/asn1_oid_sum.pl | Documents why BUNDLE_EID_OID is maintained manually in oid_sum.h. |
| certs/renewcerts/wolfssl.cnf | Adds an OpenSSL config stanza to generate a SAN with bundleEID OtherName IA5String. |
| certs/renewcerts.sh | Adds renewal steps to generate bundle-eid-cert.der. |
| .wolfssl_known_macro_extras | Adds WOLFSSL_DTN to the known macro extras list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
Retest this please |
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-securityOverall recommendation: COMMENT
Findings: 3 total — 2 posted, 1 skipped
2 finding(s) not tied to a diff line (full detail below)
Findings not tied to a diff line
New regression test never executes: WOLFSSL_DTN not enabled by any build config
File: tests/api/test_asn.c:1508-1563; configure.ac:1307
Function: test_DecodeOtherName_bundleEID
Severity: Medium
Category: test
The entire body of test_DecodeOtherName_bundleEID is guarded by #if defined(WOLFSSL_DTN) && (defined(WOLFSSL_SEP) || defined(WOLFSSL_FPKI)) && .... A repo-wide search shows WOLFSSL_DTN is defined nowhere in the build system: it appears only in the 9 files this PR touches, and there is no --enable-dtn option or -DWOLFSSL_DTN in configure.ac (contrast WOLFSSL_FPKI, which is turned on at configure.ac:1308). As a result the new regression test always compiles to an empty EXPECT_RESULT() in CI and provides no protection against a future regression of the id-on-bundleEID decode path. The DecodeOtherHelper/otherNameASN changes in asn.c are therefore also completely unexercised by automated testing.
Recommendation: Wire WOLFSSL_DTN into the build/CI configuration so the added test exercises the new code path; otherwise the regression test is effectively dead.
Referenced code: tests/api/test_asn.c:1508-1563; configure.ac:1307-1310 (4 lines)
WOLFSSL_DTN silently requires WOLFSSL_SEP or WOLFSSL_FPKI, and is undocumented
File: wolfcrypt/src/asn.c:18716-18744; wolfcrypt/src/asn.c:18728-18731
Function: DecodeOtherName
Severity: Medium
Category: convention
The otherNameASN template (with the new BEID entry) and DecodeOtherName live inside #if defined(WOLFSSL_SEP) || defined(WOLFSSL_FPKI). If a user defines only WOLFSSL_DTN (the macro the feature is named for), none of the bundleEID handling is compiled and id-on-bundleEID OtherNames are still rejected with ASN_PARSE_E -- with no compile-time error or warning to indicate the macro had no effect. The header comment states the feature is 'Only handled when WOLFSSL_DTN is defined' but does not mention the SEP/FPKI prerequisite. Additionally, because WOLFSSL_DTN is off by default, the original report (#10694) remains unfixed for default builds; this appears intentional per the rationale comment, but it is worth confirming that gating the fix behind an off-by-default macro is the desired resolution.
Recommendation: Add documentation for the new WOLFSSL_DTN macro and its build prerequisites; confirm the off-by-default scoping is intended for issue #10694.
Referenced code: wolfcrypt/src/asn.c:18716-18744; wolfcrypt/src/asn.c:18728-18732 (5 lines)
Skipped findings
- [Low]
Generated oid_sum.h is hand-edited for BUNDLE_EID_OID
Review generated by Skoll
| /* VALUE */ { 0, ASN_CONTEXT_SPECIFIC | ASN_OTHERNAME_VALUE, 1, 1, 0 }, | ||
| /* UPN */ { 1, ASN_UTF8STRING, 0, 0, 2 }, | ||
| /* FASC-N */ { 1, ASN_OCTET_STRING, 0, 0, 2 }, | ||
| #ifdef WOLFSSL_DTN |
There was a problem hiding this comment.
Let's add WOLFSSL_DTN to the settings.h WOLFSSL_ASN_ALL so it gets tested.
| /* VALUE */ { 0, ASN_CONTEXT_SPECIFIC | ASN_OTHERNAME_VALUE, 1, 1, 0 }, | ||
| /* UPN */ { 1, ASN_UTF8STRING, 0, 0, 2 }, | ||
| /* FASC-N */ { 1, ASN_OCTET_STRING, 0, 0, 2 }, | ||
| #ifdef WOLFSSL_DTN |
There was a problem hiding this comment.
Document this new macro at top of asn.c. Thanks
Description
Fixes #10694.
Testing
Built in tests, added test, reproducer from original issue
Checklist