Skip to content

Move WOLFSSL_X509_TINY test to the unit test suite and run#10975

Open
aidangarske wants to merge 1 commit into
wolfSSL:masterfrom
aidangarske:x509-tiny-ci
Open

Move WOLFSSL_X509_TINY test to the unit test suite and run#10975
aidangarske wants to merge 1 commit into
wolfSSL:masterfrom
aidangarske:x509-tiny-ci

Conversation

@aidangarske

Copy link
Copy Markdown
Member

Description

  • Move the x509 tiny test to api.c for testing in make check.
  • Adds CI entry for x509 tiny

Copilot AI review requested due to automatic review settings July 22, 2026 18:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

Copy link
Copy Markdown

retest this please

@Frauschi Frauschi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐺 Skoll Code Review

Overall recommendation: APPROVE
Findings: 2 total — 2 posted, 0 skipped

Posted findings

  • [Low] critUnknown assertion gated on WOLFSSL_NO_ASN_STRICT, but the code does not relax ittests/api.c:3866-3872
  • [Info] Double blank line after the moved test functiontests/api.c:3887-3889

Review generated by Skoll via Claude/Codex

Comment thread tests/api.c
ExpectIntEQ(wc_ParseCert(&cert, CERT_TYPE, NO_VERIFY, NULL), 0);
wc_FreeDecodedCert(&cert);

#ifndef WOLFSSL_NO_ASN_STRICT

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [Low] critUnknown assertion gated on WOLFSSL_NO_ASN_STRICT, but the code does not relax it
💡 SUGGEST test

The new critical-unknown-extension assertion is wrapped in #ifndef WOLFSSL_NO_ASN_STRICT, implying the rejection only happens in strict builds. But the source is explicit that this is not relaxed by WOLFSSL_NO_ASN_STRICT: wolfcrypt/src/asn.c:22053-22057 comments "RFC 5280 4.2 makes this a MUST that WOLFSSL_NO_ASN_STRICT does not relax", and the default case at asn.c:22058-22061 sets ASN_CRIT_EXT_E unconditionally on a critical unknown extension. I confirmed the code returns ASN_CRIT_EXT_E regardless of the strict flag, and that it propagates out of wc_ParseCert even under NO_VERIFY (asn.c:24766-24767). So the guard needlessly drops this assertion in WOLFSSL_NO_ASN_STRICT builds and slightly contradicts the source comment. Note this is not exercised by the x509-tiny CI job (which does not define WOLFSSL_NO_ASN_STRICT), so it is a coverage/clarity issue, not a CI failure.

Suggestion:

Suggested change
#ifndef WOLFSSL_NO_ASN_STRICT
/* Unknown critical extension is rejected regardless of WOLFSSL_NO_ASN_STRICT
* (RFC 5280 4.2 MUST, see asn.c DecodeExtensionType default case). */
wc_InitDecodedCert(&cert, tinyCert_critUnknown,
(word32)sizeof(tinyCert_critUnknown), NULL);
ExpectIntEQ(wc_ParseCert(&cert, CERT_TYPE, NO_VERIFY, NULL),
WC_NO_ERR_TRACE(ASN_CRIT_EXT_E));
wc_FreeDecodedCert(&cert);

Comment thread tests/api.c
#endif
return EXPECT_RESULT();
}
#endif /* WOLFSSL_X509_TINY && HAVE_ECC && !NO_ECC256 && !NO_ECC_SECP */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚪ [Info] Double blank line after the moved test function
🔧 NIT style

The moved block ends with #endif followed by two blank lines before test_wolfSSL_CTX_load_verify_locations_ex. Single blank-line separation is the norm elsewhere in this file. Purely cosmetic.

Suggestion:

Suggested change
#endif /* WOLFSSL_X509_TINY && HAVE_ECC && !NO_ECC256 && !NO_ECC_SECP */
#endif /* WOLFSSL_X509_TINY && HAVE_ECC && !NO_ECC256 && !NO_ECC_SECP */
static int test_wolfSSL_CTX_load_verify_locations_ex(void)

@Frauschi Frauschi assigned aidangarske and unassigned wolfSSL-Bot Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants