Repository navigation
Conversation
With OPENSSL_EXTRA or OPENSSL_EXTRA_X509_SMALL, every certificate parse built a WOLFSSL_X509_NAME for the issuer and subject: about 1.3 KB each plus a heap ASN1_OBJECT and ASN1_STRING per attribute. This happened for CA loads, every chain certificate and again when a verified intermediate was added as a temporary CA, but the names were only read when the certificate was copied into a WOLFSSL_X509. On a Cortex-M33 this was most of the compat layer's handshake overhead. Parsing can now skip the X509_NAMEs: DecodedCert has a noX509Name flag and keeps where each encoded name is, and GetCertX509Name() builds a name on demand. GetCertName() still checks DirectoryString lengths and fails a name with more than MAX_NAME_ENTRIES attributes when it skips building it, so the same certificates are accepted and rejected. AddCA(), the CSR2 pending signer parse, TLS peer certificate parsing, d2i_X509/X509_REQ and the X509 buffer load set the flag. The hash directory issuer and CRL lookups build the issuer name when needed. CopyDecodedToX509() adds the entries straight to the X509's names rather than building them in the DecodedCert and copying, so the leaf's names no longer exist twice at the handshake peak. A failure to build them then is reported as MEMORY_E. X509_NAME_add_entry_by_NID() sets the entry in place instead of creating a temporary entry and copying it, and RebuildFullName() uses the entry's object when it matches the NID instead of searching the OID table again for every entry on every add. X509_NAME_add_entry() now fails if the value could not be set. With OPENSSL_ALL, CopyDecodedToX509() still decoded the public key into an EVP_PKEY and created the algorithm object for every certificate, including ECC point validation, although X509CachedPubKey() and wolfSSL_X509_PUBKEY_get0_param() create them on first use. Leave them to those. STM32U585 at 160 MHz, TLS 1.3 client handshake with an ECC chain of two intermediates, overhead over a build without the compat layer: OPENSSL_EXTRA 9.0 ms -> 1.8 ms, OPENSSL_ALL 16.1 ms -> 3.0 ms, X509_SMALL 6.0 ms -> 0.2 ms. Handshake heap peak OPENSSL_EXTRA 21.4 KB -> 16.0 KB, OPENSSL_ALL 47.1 KB -> 23.2 KB.
|
Parsing that skips building the X509_NAMEs must still reject a name with more attributes than an X509_NAME holds. Add self-signed CAs whose subject and issuer have MAX_NAME_ENTRIES (16 with OPENSSL_EXTRA) and one more attributes, generated by gen-ext-certs.sh. Loading the first as an X509 gives 16 entries in each name and loading it as a CA succeeds; the second is rejected by both.
The test created a WOLFSSL_CTX from a server or client method, which the API guard check rejects as reachable in a build with neither. Loading the CAs through a WOLFSSL_CERT_MANAGER goes to AddCA() the same way and needs no TLS method.
gen-ext-certs.sh writes both DER and PEM and both are committed, so list the DER files in EXTRA_DIST too.
Contributor
|
Retest this please. (fips windows test hung or something) |
Contributor
|
Retest this please. (fips windows test hijinks) |
Member
Author
|
@claude review |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11914
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-src, wolfssl-bugs
Not reviewed (no scan target covers them): certs/test/cert-max-name-entries.cfg, certs/test/cert-max-name-entries.der, certs/test/cert-max-name-entries.pem, certs/test/cert-over-max-name-entries.cfg, certs/test/cert-over-max-name-entries.der, certs/test/cert-over-max-name-entries.pem, certs/test/gen-ext-certs.sh, certs/test/include.am, tests/api/test_ossl_x509.h
Coverage: 5 of 7 in-scope changed file(s) opened by the reviewer; not opened: src/ssl_certman.c, tests/api/test_ossl_x509.c
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
With OPENSSL_ALL, the EVP_PKEY of a certificate used to be made when the certificate was parsed: wolfSSL_EVP_PKEY_new_raw_public_key() for Ed448 and wolfSSL_d2i_PUBKEY() for key types without a case of their own. It is now made on first use by X509DecodePubKey(), which treated those keys as EC keys, so X509_get_pubkey() returned NULL for an Ed448 certificate. Import Ed448 keys in X509DecodePubKey() and decode other non-EC key types with wolfSSL_d2i_PUBKEY(), as the parse-time code did. This also makes X509_get_pubkey() work for these keys in OPENSSL_EXTRA builds without OPENSSL_ALL.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Reduces the heap use and time the OpenSSL compatibility layer adds when certificates are parsed, mostly visible in the TLS handshake and when loading CAs on embedded targets.
With
OPENSSL_EXTRAorOPENSSL_EXTRA_X509_SMALL, every certificate parse built aWOLFSSL_X509_NAMEfor the issuer and subject: about 1.3 KB each plus a heapASN1_OBJECTandASN1_STRINGper attribute. That happened for CA loads, every chain certificate, and again when a verified intermediate was added as a temporary CA. The names were only read if the certificate was copied into aWOLFSSL_X509. Building them was also quadratic: each attribute was created twice, and after every add,RebuildFullName()re-ran the linearnid2objtable search for every existing entry. The leaf's names were then built a second time when copied intossl->peerCert.Changes:
Build X509_NAMEs on demand.
DecodedCertgets anoX509Nameflag and keeps where each encoded name is.GetCertX509Name()builds a name on demand. With the flag set,GetCertName()still checks DirectoryString lengths. It also still fails a name with more thanMAX_NAME_ENTRIESattributes, the only non-OOM way building it could fail. So parsing accepts and rejects the same certificates. The flag is set by:AddCA()and the CSR2 pending-signer parse;d2i_X509/X509_REQand the X509 buffer load.The hash-directory
LoadCertByIssuerand CRL lookups build the issuer name when they need it. Externalwc_ParseCert()callers and the original (non-template) ASN parser behave as before.Add entries straight into the X509.
CopyDecodedToX509()adds the entries directly to the X509's names instead of building them in theDecodedCertand copying them. It uses the same reset andissuerSethandling aswolfSSL_X509_set_issuer_name(). A failure to build the names at that point can only be a resource failure, so it is reported asMEMORY_E.Linear name building.
X509_NAME_add_entry_by_NID()sets the entry in place instead of creating a temporary entry and copying it.RebuildFullName()reuses the entry's object when it matches the NID.X509_NAME_add_entry()now fails if the value could not be set; previously it reported success with an unset slot.OPENSSL_ALL: no parse-time public key decode.CopyDecodedToX509()still decoded every certificate's key into anEVP_PKEY(with ECC point validation) and created the algorithm object.X509CachedPubKey()andwolfSSL_X509_PUBKEY_get0_param()already create these on first use, so they are now left to those.Results
STM32U585 (Cortex-M33 at 160 MHz, newlib-nano malloc), TLS 1.3 client handshake over memory I/O, ECC P-256 chain with two intermediates. Both builds are from current master, with and without this change, and the overhead is measured against the same build without the compat layer (113.6 ms, 449 allocations, 13.2 KB peak):
OPENSSL_EXTRA_X509_SMALLOPENSSL_EXTRAOPENSSL_ALLOPENSSL_ALLoverhead goes from +29.0 ms to +7.9 ms.OPENSSL_ALLgoes from 35.4 KB to 23.3 KB.OPENSSL_EXTRA: 61.1 KB -> 57.0 KBOPENSSL_ALL: 105.5 KB -> 82.4 KBOPENSSL_EXTRA_X509_SMALL: 39.1 KB -> 34.8 KB (base 34.3 KB)Testing
test_wolfSSL_X509_max_name_entries(new) uses two new CAs fromgen-ext-certs.sh, whose subject and issuer haveMAX_NAME_ENTRIESandMAX_NAME_ENTRIES + 1attributes. It checks that the first loads with every entry, both as an X509 and as a CA, and that the second is rejected by both. It passes with and without this change, so the accepted and rejected certificates are the same.make checkpassed on macOS/clang for:--enable-opensslextra --enable-crl --enable-ocsp--enable-opensslall--enable-opensslextra=x509small--enable-all--enable-opensslextra --enable-asn=originalmake checkpassed on Linux x86_64/gcc 16 for the same configs, plus:--enable-opensslextra --enable-smallstack--enable-opensslallwith SP math, CRL, OCSP, certgen, certreq and certext--enable-allbuilt with gcc ASan + UBSan: all API tests pass except the SipHash one. SipHash fails identically on unpatched master with the same flags.--enable-allstatic): 113 passed, 0 failed.Checklist
🤖 Generated with Claude Code