Skip to content

Fix server SSL context initialization - #6214

Open
binaryfire wants to merge 2 commits into
swoole:6.1from
binaryfire:fix/server-port-ssl-context-replacement
Open

binaryfire wants to merge 2 commits into
swoole:6.1from
binaryfire:fix/server-port-ssl-context-replacement

Conversation

@binaryfire

@binaryfire binaryfire commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

SNI-only server configuration skipped creation of the primary SSL context, so connections failed before SNI could select a certificate. Invalid default certificates were also reported as successfully initialized, and other creation failures left an incomplete context marked ready.

This always creates the primary context, allowing it to omit a certificate only when valid SNI certificates exist. Invalid or incomplete certificate configuration is rejected. Failed context creation now frees and clears the incomplete context so later attempts start cleanly.

The SNI-only and invalid-certificate regressions fail on unchanged 6.1 and pass with this change. Existing SNI and HTTPS server tests continue to pass.

@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 86.22%. Comparing base (a0c205f) to head (081ef7e).
⚠️ Report is 3 commits behind head on 6.1.

Files with missing lines Patch % Lines
include/swoole_server.h 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##              6.1    #6214   +/-   ##
=======================================
  Coverage   86.21%   86.22%           
=======================================
  Files         110      110           
  Lines       16790    16798    +8     
  Branches     2981     2980    -1     
=======================================
+ Hits        14476    14484    +8     
  Misses       2314     2314           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@NathanFreeman

Copy link
Copy Markdown
Member

Thank you for your PR! However, this scenario is not common in actual development. Since set() must be called before start(), all SSL configurations can be fully set up in a single call, making multiple calls unnecessary. If configuration adjustments are truly needed, you can simply modify the first set() call directly.

@binaryfire binaryfire changed the title Fix repeated server SSL context replacement Fix server SSL context initialization Sep 9, 2026
@binaryfire binaryfire changed the title Fix server SSL context initialization Fix SSL context initialization and replacement Sep 9, 2026
Server Port set skipped SSL initialization when SNI certificates were configured without a default certificate. This left the primary context absent and the SNI callback unregistered, so every handshake failed before certificate selection.

Always initialize the primary context and permit an empty primary certificate only after valid SNI contexts have been created. Continue rejecting incomplete certificate pairs.
SSLContext create reported success when a certificate could not be loaded and left a partial context behind on other failures. Callers then treated the context as ready and skipped complete initialization on retry.

Return failure for invalid certificates and free and clear every context that does not finish initialization.
@binaryfire
binaryfire force-pushed the fix/server-port-ssl-context-replacement branch from 0f23c24 to 0b657ef Compare September 9, 2026 08:34
@binaryfire binaryfire changed the title Fix SSL context initialization and replacement Fix server SSL context initialization Sep 9, 2026
@binaryfire

Copy link
Copy Markdown
Contributor Author

@NathanFreeman You’re right that repeated set() calls aren’t needed here. I’ve removed that handling. I’ve also added fixes for related SSL initialization failures found since the PR was opened: SNI-only listeners, invalid certificates, and failed context creation. Please take another look when you have a moment.

@binaryfire

Copy link
Copy Markdown
Contributor Author

Follow-up: This PR now also supports server TLS configurations that provide certificates only through SNI. It rejects invalid or incomplete certificate settings, and failed SSL context creation now clears the incomplete context so a later attempt starts cleanly. Tests cover SNI-only listeners, invalid default certificates, incomplete settings, and failed-context cleanup.

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.

2 participants