Preserve resumed MQTT sessions and verify TLS broker identity

[aidangarske]

F-14356, F-14383

[aidangarske]

@wolfSSL-Fenrir-bot review balanced

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate TLS, queue-handling, and test-fixture issues block approval.

Review effort: Lite
Findings: 2 · 1

Open (3)

• Test certificate validity starts too late · New
• NULL host bypasses TLS hostname verification · New
• Shared write position can delete unrelated queued messages · New

What changed in this PR

Preserves resumed MQTT session queues and adds TLS broker identity verification.

Changes:

• Adds TLS hostname/IP validation and wolfSSL version checks.
• Preserves QoS 1/2 deliveries across session takeover.
• Adds TLS, broker, build, documentation, and fixture updates.

Review findings:

• Critical (1 vote): Test certificate validity begins too late; regenerate it with an earlier start date.
• Critical (3 votes): A NULL host bypasses TLS identity verification unless explicitly using the custom-peer option.
• Moderate (3 votes): Queue-specific pending-write state is required before applying the QoS 0 partial-send rule.
• Moderate (1 vote): Normalize protocol-specific queue entries during orphan reclaim as well as live takeover.
• Moderate (1 vote): Reject 0x prefixes only after confirming the complete input is an ambiguous numeric address.
• Nit (1 vote): Clarify the certificate extension comment as “IP address.”

File	Description
wolfmqtt/​mqtt_client.h	TLS custom-peer flag and API documentation
tests/​test_mqtt_tls_host.c	TLS identity verification tests
tests/​test_broker_connect.c	Session takeover regression tests
tests/​include.am	Autotools test integration
src/​mqtt_socket.c	TLS identity validation and host classification
src/​mqtt_broker.c	Session queue transfer and retransmission handling
README.md	TLS requirements documentation
examples/​mqttexample.c	SNI identity verification
configure.ac	wolfSSL version check
CMakeLists.txt	CMake version checks and test integration
certs/​tls-host-test-cert.pem	TLS test certificate
.gitignore	Ignores generated TLS test binary

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #623

Scan targets checked: wolfmqtt-src, wolfmqtt-bugs
Coverage: 2 of 5 in-scope changed file(s) opened by the reviewer; not opened: tests/test_broker_connect.c, tests/test_mqtt_tls_host.c, wolfmqtt/mqtt_client.h

Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

Review tier: Lite

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #623

Scan targets checked: wolfmqtt-src, wolfmqtt-bugs
Coverage: 3 of 6 in-scope changed file(s) opened by the reviewer; not opened: tests/test_mqtt_tls_host.c, wolfmqtt/mqtt_broker.h, wolfmqtt/mqtt_client.h

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

Review tier: Lite

[Copilot]

Copilot review overview

🔵 Needs a closer look

Valid numeric-looking DNS names are rejected before DNS resolution.

Review effort: Lite
Findings: None

Resolved since last review (3)

• NULL host bypasses TLS hostname verification
• Test certificate validity starts too late
• Shared write position can delete unrelated queued messages

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #623

Scan targets checked: wolfmqtt-src, wolfmqtt-bugs
Coverage: 1 of 1 in-scope changed file(s) opened by the reviewer

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

Review tier: Lite

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #623

Scan targets checked: wolfmqtt-src, wolfmqtt-bugs
Coverage: 1 of 2 in-scope changed file(s) opened by the reviewer; not opened: tests/test_broker_connect.c

Fenrir result: Approved ✅

No new issues found in the changed files.

Advisory only — this automated result does not count as a GitHub approval.

Review tier: Lite

Fenrir's latest completed scan found no issues; clearing the prior automated change request.

[embhorn]

Skoll Code Review

Scan type: review

Overall recommendation: COMMENT
Findings: 9 total — 5 posted, 4 skipped
5 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [Medium] Host identity pre-check aborts TLS connections that use the library's own VERIFY_NONE context — src/mqtt_socket.c:576-601
• [Medium] broker.test TLS cases now depend on optional wolfSSL IP-SAN support; CI hides the regression — src/mqtt_socket.c:585-592
• [Medium] Peer names set by existing TLS callbacks are silently replaced unless they adopt the new flag — src/mqtt_socket.c:578-596
• [Medium] Example -S with a blank value bypasses the new IP / legacy-numeric host classification — examples/mqttexample.c:779
• [Medium] No coverage for the default (no-callback) TLS path, IPv6 acceptance, or live takeover with persistence — tests/test_mqtt_tls_host.c:275-345

Skipped findings

• [Low] Host identity rejection gives no diagnostic
• [Low] Session-resume loop duplicated between live takeover and BrokerOrphan_Reclaim
• [Low] mqtt_broker CMake target keeps source-dir-first include order, now inconsistent with libwolfmqtt
• [Low] Dropping a partially sent QoS 0 entry is no longer logged

Review generated by Skoll

[embhorn]

Skoll Code Review

Scan type: review

Overall recommendation: REQUEST_CHANGES
Findings: 4 total — 2 posted, 2 skipped
2 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [High] Moving the in-RAM KV backend out of its guard breaks make check for persist builds without v5 or retained — tests/test_broker_connect.c:9848-9967
• [Medium] Host check fails connections even when the app's own TLS context does not verify the server — src/mqtt_socket.c:577-604

Skipped findings

• [Low] Every identity-setup failure is reported as BAD_FUNC_ARG, and the single-label host limit is not in the README
• [Low] Example creates the WOLFSSL object in the callback, so a raised DH minimum set later on the CTX never reaches it

Review generated by Skoll