Store in extensions the full octet string #8967
Conversation
|
Can one of the admins verify this patch? |
|
Okay to test. No approved contributor agreement yet but process started. |
|
Last pushed commit (979d732 (Add missing ext_sk to ALT_NAMES extension, 2025-07-08)) makes the LDAP tests pass.
--- a/library/spdm_crypt_lib/libspdm_crypt_cert.c
+++ b/library/spdm_crypt_lib/libspdm_crypt_cert.c
@@ -911,11 +911,13 @@ static bool libspdm_verify_leaf_cert_spdm_eku(const uint8_t *cert, size_t cert_s
req_auth_oid_find_success = false;
rsp_auth_oid_find_success = false;
+#ifndef SPDM_USING_WOLFSSL /* wolfssl returns data without sequence tag */
status = libspdm_asn1_get_tag(&ptr, eku + eku_size, &obj_len,
LIBSPDM_CRYPTO_ASN1_SEQUENCE | LIBSPDM_CRYPTO_ASN1_CONSTRUCTED);
if (!status) {
return false;
}
+#endif
while(ptr < eku + eku_size) {
status = libspdm_asn1_get_tag(&ptr, eku + eku_size, &obj_len, LIBSPDM_CRYPTO_ASN1_OID);So it is working around precisely the behavior that this PR is fixing. With the patch fixed, the |
|
Okay to test. Contributor agreement approved and on file. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
@julek-wolfssl will you please work on updating the libspdm patch and reviewing this PR? |
wolfSSL/wolfssl#8967 updates wolfSSL to return DER data in the same way as OpenSSL. When this PR goes in then wolfSSL/wolfssl#8967 needs to be re-run and merged so that other wolfSSL PR's are not blocked on libspdm test failures.
There was a problem hiding this comment.
I don't like the growing complexity of wolfSSL_X509V3_EXT_d2i. At this point I would prefer to use DecodeCertExtensions to parse the extension and then set the appropriate fields from DecodedCert. @ribes96 are you up for such a refactor or should one of us tackle this section?
julek-wolfssl
assigned
ribes96 and
julek-wolfssl and unassigned
wolfSSL-Bot and
julek-wolfssl
In fact this this function is just replicating what the following functions do: DecodeBasicCaConstraint
DecodeSubjKeyId
DecodeAuthKeyId
DecodeKeyUsageBut they couldn't be used on the one hand because they are private of So the path forward would be to add a version of these functions to the public API of I can do that |
See: https://github.com/wolfSSL/wolfssl/actions/runs/16224294061/job/45819565451?pr=8967 |
|
Consider putting in #ifdefs to have the library do the old behaviour for those that are expecting it. |
|
Retest this please: "AgentOfflineException" |
|
@ribes96 My plan was to expose |
I'm not seeing how that can be done. |
I am now working on this Any proposal for the name of the feature flag? Strictly speaking, #define ASN1_OCTET_STRING WOLFSSL_ASN1_STRINGShould we change the function signature? I'm not sure how will this affect existing callers |
|
@dgarske as I suspected, the Zephyr tests are failing again due to to commit |
|
haproxy failures have nothing to do with these commits. Repo https://github.com/wlallemand/VTest has been deleted |
Seems like the better fix is to allow more time on the test? |
Well, if it is not a problem that some workload will run considerably slower... The required time increased from over 85 secs to over 140 |
|
Jenkins retest this please: "AgentOfflineException" |
There was a problem hiding this comment.
Pull Request Overview
This PR changes how X.509 extension data is stored in WolfSSL to match OpenSSL's behavior by always storing the full OCTET STRING content in the extension value field, rather than different types of data depending on the extension type.
- Extracts extension parsing logic into reusable functions with consistent interfaces
- Modifies extension storage to always contain the complete OCTET STRING data
- Adds backwards compatibility option via
--enable-old-extdata-fmtconfigure flag
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| zephyr/samples/wolfssl_test/sample.yaml | Increases timeout for test sample from 120 to 200 seconds |
| wolfssl/wolfcrypt/asn.h | Adds function declarations for new extension decoding functions |
| wolfcrypt/src/asn.c | Refactors extension parsing into standalone functions and updates internal calls |
| tests/api.c | Adds test to verify new extension data format behavior |
| src/x509.c | Major restructuring of extension handling to store full OCTET STRING data |
| configure.ac | Adds configure option for backwards compatibility with old extension format |
Comments suppressed due to low confidence (4)
|
Jenkins retest this please: "AgentOfflineException" |
|
Jenkins retest this please: FIPS test failed with "RequestAbortedException" |
|
Jenkins retest this please: "AgentOfflineException" |
Store in WOLFSSL_X509_EXTENSION.value always the full contents of the OCTET STRING of the extension, instead of different type of data depending on the type of extension. Previously this was only done for unknown extensions.
Description
It is clear from Issue #8941 that
X509_EXTENSION_get_datahas a clear different behavior on OpenSSL than in WolfSSL. RFC8280 defines an extension as:Whereas OpenSSL always returns the full OCTET STRING when calling
X509_EXTENSION_get_data, WolfSSL returns a different type of object depending on the type of extension parsed. This can be observed in the following program:For example with this extension
Given that
wolfSSL_X509_EXTENSION_get_datais currently aliased toX509_EXTENSION_get_data, I would say the intention is for them to have the same behaviour, so this PR changes how an extension is stored in WolfSSL.Now, this is a clear change of behavior, and will probably break a lot of tests. To start, the API of the public function would need to be changed to return an
ASN1_OCTET_STRINGinstead of anASN1_STRING. But I don't really see a utility for a function that returns different type of data depending on the type of extension, other than to print it in a console.So, this PR is not really intended to be merged yet, but to trigger a discussion about how should WolfSSL behave. If it is acknowledged that they should not have the same behavior, I think the alias to OpenSSL alternatives should be removed.
Please describe the scope of the fix or feature addition.
The storage format of an X.509 extension is changed
Fixes #8941
Testing
How did you test?
No testing other than running the attached program
Checklist