Implementing missing macros to openssl compatibility layer - #10520
Implementing missing macros to openssl compatibility layer #10520Roy-Carter wants to merge 11 commits into
Conversation
|
Can one of the admins verify this patch? |
|
@julek-wolfssl second part of the functions i've implemented , split the PR's for clarity of checks . |
54ef2e4 to
413e8ac
Compare
|
@dgarske errors seemed related to master diff. both tests worked for me locally , I rebased , can we re-run workflow? |
413e8ac to
62138a2
Compare
|
@julek-wolfssl @dgarske I've rebased to master , seems related to new changes added . |
|
Jenkins okay to test |
There was a problem hiding this comment.
Pull request overview
This PR expands the OpenSSL compatibility layer with aliases and implementations for several missing APIs used during migration to wolfSSL.
Changes:
- Adds compatibility mappings for
SSL_set_ciphersuites,X509_add1_ext_i2d,sk_GENERAL_NAME_new_null, andEC_KEY_oct2key. - Implements
wolfSSL_X509_add1_ext_i2dandwolfSSL_EC_KEY_oct2key. - Adds unit tests covering the new compatibility APIs.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
wolfssl/ssl.h |
Declares the new X509 extension helper. |
wolfssl/openssl/ssl.h |
Adds OpenSSL compatibility macros for SSL, X509, and GENERAL_NAME APIs. |
wolfssl/openssl/ec.h |
Declares and maps EC_KEY_oct2key. |
src/x509.c |
Implements wolfSSL_X509_add1_ext_i2d. |
src/pk_ec.c |
Implements wolfSSL_EC_KEY_oct2key. |
tests/api.c |
Adds API tests for the new compatibility additions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
julek-wolfssl
left a comment
There was a problem hiding this comment.
@Roy-Carter please address copilot review. Thanks.
62138a2 to
653ec68
Compare
|
@julek-wolfssl can we re-run workflow ? |
|
f2a562a to
99427a3
Compare
|
can we re-run workflow @julek-wolfssl also writing here about our ticket , I think it might've been closed / I lost perms to write on it even though I did send emails there it seems as if zendesk link is unavailable |
| ExpectNotNull(ctx = SSL_CTX_new(wolfSSLv23_client_method())); | ||
| ExpectNotNull(ssl = SSL_new(ctx)); | ||
|
|
||
| ExpectIntEQ(SSL_set_ciphersuites(ssl, "TLS_AES_128_GCM_SHA256"), 1); |
There was a problem hiding this comment.
With --disable-errorstrings
1309: test_wolfSSL_SSL_set_ciphersuites :
ERROR - tests/api.c line 2537 failed with:
expected: wolfSSL_set_cipher_list(ssl, "TLS_AES_128_GCM_SHA256") == 1
result: 0 != 1
failed ( 0.00002)
ERROR - tests/api.c line 36346 failed with:
expected: Test failed
result: ret 0
|
99427a3 to
76dec0a
Compare
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 13 total — 4 posted, 9 skipped
4 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] wolfSSL_EC_KEY_oct2key does not record the point conversion form —
src/pk_ec.c:4731-4775 - [Medium] wolfssl_x509_ext_is_set() silently reports 'absent' for every unrecognized NID —
src/x509.c:3601-3622 - [Medium] wolfSSL_X509_add_ext() now fails for extKeyUsage object stacks that previously returned success —
src/x509.c:1477-1488 - [Low] X509V3_ADD_SILENT has no observable effect —
src/x509.c:3740-3778
Skipped findings
- [High]
X509V3_ADD_REPLACE destroys the existing extension when the subsequent add fails - [High]
basicConstraints pathLenConstraint is taken from ASN1_INTEGER-length (DER size), not the integer value - [High]
Removing subjectAltName leaves the altName iterator permanently exhausted - [Medium]
basicConstraints test does not verify the pathlen round-trip - [Medium]
EC_KEY_oct2key test only covers the uncompressed form and hardcodes the P-256 size - [Medium]
X509V3_ADD_APPEND does not append for most NIDs - [Low]
Flag test computes dnsName/dnsLen but never asserts on them for APPEND and KEEP_EXISTING - [Low]
SSL_set_ciphersuites alias accepts TLS 1.2 cipher names - [Low]
test_san_first_dns() truncates into a static buffer while reporting the untruncated length
Review generated by Skoll
| * @return 1 on success. | ||
| * @return 0 on failure. | ||
| */ | ||
| int wolfSSL_EC_KEY_oct2key(WOLFSSL_EC_KEY *key, const unsigned char *buf, |
There was a problem hiding this comment.
🟠 [Medium] wolfSSL_EC_KEY_oct2key does not record the point conversion form
OpenSSL's EC_KEY_oct2key() saves the encoding form on the key (key->conv_form = (point_conversion_form_t)(buf[0] & ~0x01)), so that a later i2o_ECPublicKey() / EC_KEY_key2buf() re-emits the same form. The wolfSSL implementation never touches key->form, so a compressed input silently round-trips back as uncompressed.
Verified empirically on this branch:
compressed enc len=33 first=0x03
oct2key(compressed)=1 conv_form after=4 (OpenSSL sets 2)
This matters for applications that hash or compare the re-encoded public key (e.g. JWK/COSE key thumbprints, TLS raw public keys), because they will get a different byte string than OpenSSL would.
Fix: Call wolfSSL_EC_KEY_set_conv_form(key, buf[0] & ~0x01) on success, and document the behaviour in the function's doxygen block.
| return NULL; | ||
| } | ||
|
|
||
| /* Return 1 if an extension of type @nid is already present in the in-memory |
There was a problem hiding this comment.
🟠 [Medium] wolfssl_x509_ext_is_set() silently reports 'absent' for every unrecognized NID
The default: arm returns 0 for any NID outside the six recognized ones. wolfSSL_X509V3_EXT_i2d() also accepts WC_NID_issuer_alt_name and WC_NID_info_access, and with WOLFSSL_CUSTOM_OID wolfSSL_X509_add_ext() will happily store arbitrary NIDs in x509->custom_exts[]. For those NIDs the presence-dependent operations quietly do the wrong thing rather than reporting that they are unsupported:
X509V3_ADD_DEFAULTnever detects a duplicate, so repeated calls keep appending custom extensions untilNUM_CUSTOM_EXTis exhausted (thenwolfssl_x509_add_custom_ext()fails with a confusing 'Bad value for customExtCount').X509V3_ADD_KEEP_EXISTINGalways adds instead of keeping.X509V3_ADD_DELETE/X509V3_ADD_REPLACE_EXISTINGalways fail with 'No extension to delete', which is indistinguishable from the genuinely-absent case.
None of these NIDs are covered by the new tests.
Fix: Have wolfssl_x509_ext_is_set() report 'unknown NID' distinctly and make wolfSSL_X509_add1_ext_i2d() fail the presence-dependent operations (DEFAULT / KEEP_EXISTING / REPLACE_EXISTING / DELETE) for those NIDs, or at minimum document the limitation in the doxygen block alongside the extKeyUsage note. Add a test for NID_info_access / a custom OID.
| return WOLFSSL_FAILURE; | ||
| } | ||
| } | ||
| else if (ext && ext->ext_sk != NULL) { |
There was a problem hiding this comment.
🟠 [Medium] wolfSSL_X509_add_ext() now fails for extKeyUsage object stacks that previously returned success
wolfSSL_X509_add_ext(x509, ext, -1) with an extKeyUsage extension carrying an ext_sk object stack used to return WOLFSSL_SUCCESS (a silent no-op); it now returns WOLFSSL_FAILURE. Turning a silent no-op into an explicit failure is the right call, but it is a visible behaviour change for existing OPENSSL_ALL applications that build an EKU extension with X509V3_EXT_i2d(NID_ext_key_usage, ...) and then X509_add_ext() it - those calls will now start failing.
Blast radius checked: wolfSSL_X509_set_ext() (the source of extensions returned by X509_get_ext()) only sets ext_sk for AUTH_INFO_OID and ALT_NAMES_OID and always populates ext->value.data, so the common 'copy extensions from one cert to another' pattern still takes the first branch and is unaffected. tests/api/test_ossl_x509_ext.c:391 (No Data - no change -> SUCCESS) also still passes because that ext has neither data nor ext_sk.
The underlying gap remains: wolfSSL cannot consume an EKU object stack at all. The OIDs in the stack could be mapped to the EXTKEYUSE_* bits the same way ParseExtKeyUsageStr() maps names.
Fix: Either implement the OID -> EXTKEYUSE_* mapping so extKeyUsage actually works through X509_add1_ext_i2d(), or call out the behaviour change in the PR description / release notes so downstream users of X509_add_ext() are not surprised.
| op = flags & WOLFSSL_X509V3_ADD_OP_MASK; | ||
| exists = wolfssl_x509_ext_is_set(x, nid); | ||
|
|
||
| switch (op) { |
There was a problem hiding this comment.
🔵 [Low] X509V3_ADD_SILENT has no observable effect
WOLFSSL_X509V3_ADD_SILENT only gates WOLFSSL_MSG() calls, which are already compiled out unless DEBUG_WOLFSSL is defined, and wolfSSL does not push to an OpenSSL-style error queue here. The three if ((flags & WOLFSSL_X509V3_ADD_SILENT) == 0) wrappers therefore add branching and indentation for no behavioural difference. The doxygen block already states that SILENT does not change the result, so a single note would be clearer than three dead conditionals.
Fix: Either drop the three SILENT conditionals and note once that the flag is accepted-but-inert, or keep them and add a brief comment explaining they are placeholders for a future error-queue implementation.
76dec0a to
51dc78f
Compare
|
Retest this please |
1 similar comment
|
Retest this please |
cipher_names[] only holds the IANA suite name when error strings are built in, so lean builds could not match "TLS_AES_128_GCM_SHA256". Gate the test on BUILD_TLS_AES_128_GCM_SHA256 and fall back to the wolfSSL suite name. Also build the basicConstraints ASN1_OBJECT with OBJ_nid2obj() so it carries a real OID encoding, not just a hand-set type.
Removing an extension left state behind: the freed altName list was still referenced by the altNamesNext retrieval hint (use after free through X509_get_next_altname()), and the cached subject key ID string, the encoded extended key usage and the per-extension set flags kept the removed extension observable. X509V3_ADD_SILENT only suppresses error reporting in OpenSSL, so X509V3_ADD_DEFAULT on an existing extension fails either way. REPLACE and REPLACE_EXISTING now encode the new extension before dropping the old one, so a failure no longer destroys the existing extension.
OpenSSL saves the encoding form on the key so that a later i2o_ECPublicKey() re-emits it. Without this a compressed point silently round-tripped back as uncompressed.
wolfSSL_X509V3_EXT_i2d() encodes extKeyUsage as a stack of ASN1_OBJECTs, which wolfSSL_X509_add_ext() could not use at all. Map each KeyPurposeId OID to its EXTKEYUSE_* bit and store the list as DER, the layout a parsed certificate carries, so X509_get_ext_d2i() and X509_get_extended_key_usage() both read it back. OIDs wolfSSL has no bit for stay in the list, matching the certificate parser. Entries that are not a single OBJECT IDENTIFIER are rejected before anything is stored.
wolfssl_x509_ext_is_set() answered "absent" for every NID it does not know, so DELETE and REPLACE_EXISTING failed with a reason indistinguishable from a real absence, and DEFAULT and KEEP_EXISTING could not detect a duplicate. It now reports "unknown" separately and only X509V3_ADD_APPEND, which is defined as not checking, is attempted for such a NID. X509V3_ADD_SILENT only gated WOLFSSL_MSG() calls that are compiled out without DEBUG_WOLFSSL, and this API pushes nothing to an error queue. Drop the three dead conditionals and document the flag as accepted but inert.
wolfSSL_X509V3_EXT_i2d() builds the authorityKeyIdentifier object from the caller's issuer name when no key ID is given, and wolfSSL_X509_add_ext() dispatches on that object rather than on the NID that was asked for. A REPLACE therefore removed the certificate's authority key identifier and then failed, or wrote into a different extension's storage. Check the encoded NID before anything is removed.
wolfSSL_X509V3_EXT_i2d() copies the object stack without inspecting it, so a REPLACE dropped the certificate's extKeyUsage and only then discovered that the new stack could not be stored. Split the per-entry check out of wolfssl_x509_add_ext_key_usage_sk() and run it before the removal.
51dc78f to
6225adf
Compare
|
retest this please |
|
@dgarske addressed review |
|
Not sure why this got closed. Re-opened as #11375 to make it easier for me to track. |
Description
Implementing as part of migration to wolfssl
SSL_set_ciphersuites
X509_add1_ext_i2d
sk_GENERAL_NAME_new_null
EC_KEY_oct2key
Testing
unitests to each functionality added
Checklist