Fix for peer cert verify with IP address#10169
Fix for peer cert verify with IP address#10169embhorn wants to merge 3 commits intowolfSSL:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes incorrect peer certificate verification behavior when the reference identity is an IP address by ensuring CN fallback is not used for IP checks (RFC 6125), and adds regression tests covering CN-only IP-like subjects.
Changes:
- Skip Subject CN fallback during hostname verification when the input is an IP address.
- Add regression tests ensuring CN-only certificates (including wildcard CN) are not accepted for IP verification.
- Add a sanity check ensuring CN-based matching still works for non-IP hostname verification.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/api/test_ossl_x509.c | Adds regression tests for IP verification behavior when SAN is missing and CN looks like an IP/wildcard IP. |
| src/internal.c | Updates hostname verification to avoid CN fallback when verifying IP addresses (RFC 6125 compliance). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 1 total — 1 posted, 0 skipped
Posted findings
- [Medium] No IPv6 regression test for IP-in-CN bypass —
tests/api/test_ossl_x509.c:1063-1223
Review generated by Skoll via openclaw
| ExpectIntEQ(wolfSSL_X509_check_ip_asc(NULL, "0.0.0.0", 0), 0); | ||
| ExpectIntEQ(wolfSSL_X509_check_ip_asc(empty, "127.128.0.255", 0), 0); | ||
|
|
||
| /* Regression test: a certificate with CN=<ip> and no SAN extension |
There was a problem hiding this comment.
🟡 [Medium] No IPv6 regression test for IP-in-CN bypass
💡 SUGGEST test
The regression tests cover IPv4 addresses (CN=127.0.0.1, CN=*.0.0.1) but do not include an IPv6 test case. The fix in CheckHostName applies to all IP verification equally (the isIP flag is set by CheckIPAddr regardless of address family), so functionally the fix covers IPv6. However, adding a cert with CN=::1 (or similar IPv6 literal) and verifying it is rejected by wolfSSL_X509_check_ip_asc would strengthen the regression coverage and ensure the isIP path works for both address families. Since the underlying code fix is address-family-agnostic, this is a coverage improvement rather than a blocker.
Suggestion:
| /* Regression test: a certificate with CN=<ip> and no SAN extension | |
| Consider adding a test certificate with CN="::1" (no SAN) and verifying: | |
| ExpectIntEQ(wolfSSL_X509_check_ip_asc(cn_ipv6, "::1", 0), 0); |
— Skoll automated review via openclaw
|
Jenkins retest this please |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
Issue with checking SAN with IP certs
Fixes zd21565
Testing
Added test cases to
test_wolfSSL_X509_check_ip_ascChecklist