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. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors how X.509 extensions are stored in WolfSSL to always retain the full DER-encoded OCTET STRING, matching OpenSSL’s behavior, and removes per-extension type special-case storage.
- Eliminates per-OID
ASN1_STRING_setcalls and moves to a unified post-switch copy of the raw OCTET STRING intoext->value - Introduces a generic “read critical flag then copy octet string” parsing block after all known OID cases
- Updates related print and d2i routines to work with the new raw OCTET STRING storage
| objSz++; | ||
| objSz += SetLength(length, oidBuf + 1); | ||
| objSz += length; | ||
| /* The ASN1_OBJECT in the extension is set in the same way |
There was a problem hiding this comment.
All of the per-extension ext->crit = x509->*Crit assignments were removed, so known extensions no longer honor their critical flag. You’ll need to preserve the original critical values for each OID (e.g. basicConstCrit, authKeyIdCrit, etc.) before the unified parsing so that ext->crit is correctly set.
Copilot uses AI. Check for mistakes.
@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 |
|
Should I merge master to resolve the conflicts? |
|
Hi @ribes96 , please resolve merge conflicts. Thank you |
There was a problem hiding this comment.
#ifdef WOLFSSL_ASN_TEMPLATE
^
/tmp/workspace/PRB-multi-test-script/wolfssl/wolfcrypt/src/asn.c:23789:2: warning: nested redundant #ifndef; consider removing it [readability-redundant-preprocessor]
#ifndef WOLFSSL_ASN_TEMPLATE
^
/tmp/workspace/PRB-multi-test-script/wolfssl/wolfcrypt/src/asn.c:22894:2: note: previous #ifdef was here
#ifdef WOLFSSL_ASN_TEMPLATE
^
/tmp/workspace/PRB-multi-test-script/wolfssl/wolfcrypt/src/asn.c:23853:2: warning: nested redundant #ifndef; consider removing it [readability-redundant-preprocessor]
#ifndef WOLFSSL_ASN_TEMPLATE
^
/tmp/workspace/PRB-multi-test-script/wolfssl/wolfcrypt/src/asn.c:22894:2: note: previous #ifdef was here
#ifdef WOLFSSL_ASN_TEMPLATE
^
./configure --enable-jobserver=4 CC="clang -fsanitize=address,undefined -g" --enable-all --enable-sp-math-all
make check
FAIL: scripts/unit.test
There was a problem hiding this comment.
c:\jenkins\workspace\prb-windows-test-v2\src\x509.c(1971): warning C4244: 'function' : conversion from 'int' to 'byte', possible loss of data [C:\jenkins\workspace\PRB-windows-test-v2\wolfssl.vcxproj]
c:\jenkins\workspace\prb-windows-test-v2\src\x509.c(2005): warning C4244: 'function' : conversion from 'int' to 'byte', possible loss of data [C:\jenkins\workspace\PRB-windows-test-v2\wolfssl.vcxproj]
c:\jenkins\workspace\prb-windows-test-v2\src\x509.c(2056): warning C4244: 'function' : conversion from 'int' to 'byte', possible loss of data [C:\jenkins\workspace\PRB-windows-test-v2\wolfssl.vcxproj]
There was a problem hiding this comment.
Hi @ribes96 ,
This breaks some of the return code logic. For example the test_wolfSSL_CRL_duplicate_extensions we have a faketime test that pushes the clock a year out. We expect a CRL_CERT_DATE_ERR, but now we get a ASN_PARSE_E. Can you please make sure we aren't loosing some of the return code logic.
[all-check-certs-one-year] [7 of 50] [e6b39baf40] [previously fail_check]
autogen.sh e6b39baf40... real 0m4.007s user 0m2.797s sys 0m0.248s
configure... real 0m6.777s user 0m4.823s sys 0m2.537s
build... real 0m17.111s user 1m26.745s sys 0m3.905s
check[faketime]... real 0m0.055s user 0m0.046s sys 0m0.014s
configure... real 0m6.531s user 0m4.536s sys 0m2.573s
build... real 0m16.726s user 1m26.467s sys 0m4.037s
check[faketime]...FAILURES:
real 0m11.347s user 0m3.251s sys 0m0.056s
ERROR - tests/api.c line 4291 failed with:
expected: ret == ASN_PARSE_E
result: -179 != -140
ERROR - tests/api.c line 68860 failed with:
expected: Test failed
result: ret 0
ERROR - tests/api.c line 9892 failed with:
expected: wolfSSL_read_ex(ssl_s, buf, sizeof(buf), &count) == WOLFSSL_SUCCESS
result: 5 != 1
ERROR - tests/api.c line 64228 failed with:
expected: wolfSSL_shutdown(ssl_c) == 1
result: 0 != 1
FAILURES:
917: test_wolfSSL_CRL_duplicate_extensions
Here is that specific test with the logging and WOLFSSL_DEBUG_ASN_TEMPLATE set.
davidgarske@i9w680:/tmp/wolfssl_test_workdir.811078/wolfssl$ faketime -f +1y scripts/unit.test -917
starting unit tests...
Begin API Tests
wolfSSL Entering wolfSSL_Init
wolfSSL Entering wolfCrypt_Init
RNG_HEALTH_TEST_CHECK_SIZE = 128
sizeof(seedB_data) = 128
917: test_wolfSSL_CRL_duplicate_extensions :wolfSSL Entering wolfSSL_CertManagerNew
heap param is null
DYNAMIC_TYPE_CERT_MANAGER Allocating = 392 bytes
Testing CRL with duplicate Authority Key Identifier extensions
wolfSSL Entering wolfSSL_CertManagerLoadCRLBuffer
wolfSSL Entering wolfSSL_CertManagerEnableCRL
wolfSSL Entering InitCRL
wolfSSL Entering BufferLoadCRL
wolfSSL Entering PemToDer
InitDecodedCRL
ParseCRL
TEMPLATE: crlASN
wolfSSL Entering GetASN_Items
0: 0 520 + SEQUENCE
1: 4 241 + SEQUENCE
2: 7 1 INTEGER
3: 10 13 + SEQUENCE
4: 12 9 OBJECT ID
5: 23 0 NULL
7: 25 121 + SEQUENCE
8: 148 13 UTCTime
10: 163 13 UTCTime
13: 178 68 + [0]
14: 180 66 + SEQUENCE
15: 248 13 + SEQUENCE
16: 250 9 OBJECT ID
17: 261 0 NULL
19: 263 257 BIT STRING
Date AFTER check failed
CRL after date is no longer valid
wolfSSL error occurred, error = 179 line:41004 file:wolfcrypt/src/asn.c
ParseCRL error
wolfSSL Entering FreeCRL_Entry
FreeDecodedCRL
ERROR - tests/api.c line 4291 failed with:
expected: ret == ASN_PARSE_E
result: -179 != -140
wolfSSL Entering wolfSSL_CertManagerFree
wolfSSL Entering FreeCRL
failed ( 0.00010)
ERROR - tests/api.c line 68860 failed with:
expected: Test failed
result: ret 0
wolfSSL Entering wolfSSL_ERR_clear_error
wolfSSL Entering wolfSSL_Cleanup
wolfSSL Entering wolfSSL_crypto_policy_disable
wolfSSL Entering wolfCrypt_Cleanup
FAILURES:
917: test_wolfSSL_CRL_duplicate_extensions
End API Tests
Failed/Skipped/Passed/All: 1/0/0/1
davidgarske@i9w680:/tmp/wolfssl_test_workdir.811078/wolfssl$
I'm sorry, I don't understand.
The test checks that You are talking about this test, right? static int test_wolfSSL_CRL_duplicate_extensions(void)
{
EXPECT_DECLS;
#if defined(WOLFSSL_ASN_TEMPLATE) && !defined(NO_CERTS) && \
defined(HAVE_CRL) && !defined(NO_RSA) && !defined(WOLFSSL_NO_ASN_STRICT) && \
(defined(WC_ASN_RUNTIME_DATE_CHECK_CONTROL) || defined(NO_ASN_TIME_CHECK))
const unsigned char crl_duplicate_akd[] =
"-----BEGIN X509 CRL-----\n"
"MIICCDCB8QIBATANBgkqhkiG9w0BAQsFADB5MQswCQYDVQQGEwJVUzETMBEGA1UE\n"
"CAwKQ2FsaWZvcm5pYTEWMBQGA1UEBwwNU2FuIEZyYW5jaXNjbzETMBEGA1UECgwK\n"
"TXkgQ29tcGFueTETMBEGA1UEAwwKTXkgUm9vdCBDQTETMBEGA1UECwwKTXkgUm9v\n"
"dCBDQRcNMjQwOTAxMDAwMDAwWhcNMjUxMjAxMDAwMDAwWqBEMEIwHwYDVR0jBBgw\n"
"FoAU72ng99Ud5pns3G3Q9+K5XGRxgzUwHwYDVR0jBBgwFoAU72ng99Ud5pns3G3Q\n"
"9+K5XGRxgzUwDQYJKoZIhvcNAQELBQADggEBAIFVw4jrS4taSXR/9gPzqGrqFeHr\n"
"IXCnFtHJTLxqa8vUOAqSwqysvNpepVKioMVoGrLjFMjANjWQqTEiMROAnLfJ/+L8\n"
"FHZkV/mZwOKAXMhIC9MrJzifxBICwmvD028qnwQm09EP8z4ICZptD6wPdRTDzduc\n"
"KBuAX+zn8pNrJgyrheRKpPgno9KsbCzK4D/RIt1sTK2M3vVOtY+vpsN70QYUXvQ4\n"
"r2RZac3omlT43x5lddPxIlcouQpwWcVvr/K+Va770MRrjn88PBrJmvsEw/QYVBXp\n"
"Gxv2b78HFDacba80sMIm8ltRdqUCa5qIc6OATsz7izCQXEbkTEeESrcK1MA=\n"
"-----END X509 CRL-----\n";
WOLFSSL_CERT_MANAGER* cm = NULL;
int ret;
(void)wc_AsnSetSkipDateCheck(1);
cm = wolfSSL_CertManagerNew();
ExpectNotNull(cm);
/* Test loading CRL with duplicate extensions */
WOLFSSL_MSG("Testing CRL with duplicate Authority Key Identifier extensions");
ret = wolfSSL_CertManagerLoadCRLBuffer(cm, crl_duplicate_akd,
sizeof(crl_duplicate_akd),
WOLFSSL_FILETYPE_PEM);
ExpectIntEQ(ret, ASN_PARSE_E);
wolfSSL_CertManagerFree(cm);
(void)wc_AsnSetSkipDateCheck(0);
#endif
return EXPECT_RESULT();
}I am building with ./configure --enable-all CFLAGS="-DWC_ASN_RUNTIME_DATE_CHECK_CONTROL"I am using |
|
Hi @ribes96 , I was wrong, your PR is passing that Thanks, |
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.
ribes96
force-pushed
the
extension-full-value
branch
from
b07a0e0 to
3e36057
Compare
|
Jenkins retest this please. Agent Offline |
|
Hi @ribes96 , It seems like the changes in this PR are breaking some of the tests for libspdm, socat and zephyr? |
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